fix(budgets): handle refunds correctly in budget spending calculations (#152)
## Why ### Problem When a refund (positive transaction amount) is assigned to a budget, it incorrectly **increases** the cumulative spending instead of **reducing** it. This causes: - The spending chart line to go **up** on refunds instead of down - The "Spent" amount to be inflated - The "Remaining" amount to be understated - Period rollover calculations to carry over incorrect amounts ### Root Cause `BudgetTransactionService` uses `abs($transaction->amount)` when creating budget transactions, which forces all amounts to be positive — including refunds. Since expenses are stored as negative in the `transactions` table and refunds as positive, `abs()` treats both as spending. ## What ### Changes - Replace `abs($transaction->amount)` with `-$transaction->amount` in both `assignTransaction()` and `assignHistoricalTransactionsToPeriod()` — expenses (`-5000`) become positive spending (`5000`), refunds (`+1000`) become negative spending (`-1000`) - Remove redundant `abs()` in `BudgetPeriodService::closePeriod()` rollover calculation - Add data migration to fix existing `budget_transactions` rows where the original transaction was a refund - No frontend changes needed — the chart and budget card already sum `t.amount` directly ## Verification ### Tests - Updated `assignHistoricalTransactionsToPeriod stores negated transaction amount for expenses` — verifies expense sign is preserved - Added `assignHistoricalTransactionsToPeriod stores refund as negative amount` — verifies refunds reduce spending - Added `budget spending correctly reflects mix of expenses and refunds` — verifies net spending (expense - refund) - Added `assignTransaction stores refund as negative budget transaction amount` — verifies real-time assignment handles refunds
This commit is contained in:
parent
545cc66024
commit
f2a7f955e6
|
|
@ -40,7 +40,7 @@ class BudgetPeriodService
|
||||||
|
|
||||||
if ($budget->rollover_type->value === 'carry_over') {
|
if ($budget->rollover_type->value === 'carry_over') {
|
||||||
$totalSpent = $period->budgetTransactions()->sum('amount');
|
$totalSpent = $period->budgetTransactions()->sum('amount');
|
||||||
$remaining = $period->allocated_amount - abs($totalSpent);
|
$remaining = $period->allocated_amount - $totalSpent;
|
||||||
|
|
||||||
if ($remaining > 0) {
|
if ($remaining > 0) {
|
||||||
$carriedOverAmount = $remaining;
|
$carriedOverAmount = $remaining;
|
||||||
|
|
|
||||||
|
|
@ -62,7 +62,7 @@ class BudgetTransactionService
|
||||||
BudgetTransaction::create([
|
BudgetTransaction::create([
|
||||||
'transaction_id' => $transaction->id,
|
'transaction_id' => $transaction->id,
|
||||||
'budget_period_id' => $period->id,
|
'budget_period_id' => $period->id,
|
||||||
'amount' => abs($transaction->amount),
|
'amount' => -$transaction->amount,
|
||||||
]);
|
]);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
@ -126,7 +126,7 @@ class BudgetTransactionService
|
||||||
BudgetTransaction::create([
|
BudgetTransaction::create([
|
||||||
'transaction_id' => $transaction->id,
|
'transaction_id' => $transaction->id,
|
||||||
'budget_period_id' => $period->id,
|
'budget_period_id' => $period->id,
|
||||||
'amount' => abs($transaction->amount),
|
'amount' => -$transaction->amount,
|
||||||
]);
|
]);
|
||||||
|
|
||||||
$assignedCount++;
|
$assignedCount++;
|
||||||
|
|
|
||||||
|
|
@ -0,0 +1,34 @@
|
||||||
|
<?php
|
||||||
|
|
||||||
|
use Illuminate\Database\Migrations\Migration;
|
||||||
|
use Illuminate\Support\Facades\DB;
|
||||||
|
|
||||||
|
return new class extends Migration
|
||||||
|
{
|
||||||
|
/**
|
||||||
|
* Fix budget_transactions where the original transaction was a refund (positive amount).
|
||||||
|
* These were incorrectly stored as positive via abs() — they should be negative
|
||||||
|
* to correctly reduce the budget's cumulative spending.
|
||||||
|
*/
|
||||||
|
public function up(): void
|
||||||
|
{
|
||||||
|
DB::statement('
|
||||||
|
UPDATE budget_transactions
|
||||||
|
SET amount = -amount
|
||||||
|
WHERE transaction_id IN (
|
||||||
|
SELECT id FROM transactions WHERE amount > 0
|
||||||
|
)
|
||||||
|
');
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Revert: make all budget_transaction amounts positive again (original behavior).
|
||||||
|
*/
|
||||||
|
public function down(): void
|
||||||
|
{
|
||||||
|
DB::statement('
|
||||||
|
UPDATE budget_transactions
|
||||||
|
SET amount = ABS(amount)
|
||||||
|
');
|
||||||
|
}
|
||||||
|
};
|
||||||
|
|
@ -240,14 +240,14 @@ test('assignHistoricalTransactionsToPeriod works with transactions having multip
|
||||||
expect($count)->toBe(1);
|
expect($count)->toBe(1);
|
||||||
});
|
});
|
||||||
|
|
||||||
test('assignHistoricalTransactionsToPeriod stores absolute value of amount', function () {
|
test('assignHistoricalTransactionsToPeriod stores negated transaction amount for expenses', function () {
|
||||||
$category = Category::factory()->create(['user_id' => $this->user->id]);
|
$category = Category::factory()->create(['user_id' => $this->user->id]);
|
||||||
|
|
||||||
$transaction = Transaction::factory()->create([
|
Transaction::factory()->create([
|
||||||
'user_id' => $this->user->id,
|
'user_id' => $this->user->id,
|
||||||
'category_id' => $category->id,
|
'category_id' => $category->id,
|
||||||
'transaction_date' => now()->subDays(5),
|
'transaction_date' => now()->subDays(5),
|
||||||
'amount' => -5000, // Negative amount
|
'amount' => -5000, // Expense (negative)
|
||||||
]);
|
]);
|
||||||
|
|
||||||
$budget = Budget::factory()->create([
|
$budget = Budget::factory()->create([
|
||||||
|
|
@ -265,7 +265,101 @@ test('assignHistoricalTransactionsToPeriod stores absolute value of amount', fun
|
||||||
|
|
||||||
$budgetTransaction = $period->budgetTransactions()->first();
|
$budgetTransaction = $period->budgetTransactions()->first();
|
||||||
|
|
||||||
expect($budgetTransaction->amount)->toBe(5000); // Should be positive
|
expect($budgetTransaction->amount)->toBe(5000); // -(-5000) = 5000, adds to spending
|
||||||
|
});
|
||||||
|
|
||||||
|
test('assignHistoricalTransactionsToPeriod stores refund as negative amount', function () {
|
||||||
|
$category = Category::factory()->create(['user_id' => $this->user->id]);
|
||||||
|
|
||||||
|
Transaction::factory()->create([
|
||||||
|
'user_id' => $this->user->id,
|
||||||
|
'category_id' => $category->id,
|
||||||
|
'transaction_date' => now()->subDays(5),
|
||||||
|
'amount' => 1000, // Refund (positive)
|
||||||
|
]);
|
||||||
|
|
||||||
|
$budget = Budget::factory()->create([
|
||||||
|
'user_id' => $this->user->id,
|
||||||
|
'category_id' => $category->id,
|
||||||
|
]);
|
||||||
|
|
||||||
|
$period = BudgetPeriod::factory()->create([
|
||||||
|
'budget_id' => $budget->id,
|
||||||
|
'start_date' => now()->subDays(30),
|
||||||
|
'end_date' => now()->addDays(30),
|
||||||
|
]);
|
||||||
|
|
||||||
|
$this->service->assignHistoricalTransactionsToPeriod($period);
|
||||||
|
|
||||||
|
$budgetTransaction = $period->budgetTransactions()->first();
|
||||||
|
|
||||||
|
expect($budgetTransaction->amount)->toBe(-1000); // -(+1000) = -1000, reduces spending
|
||||||
|
});
|
||||||
|
|
||||||
|
test('budget spending correctly reflects mix of expenses and refunds', function () {
|
||||||
|
$category = Category::factory()->create(['user_id' => $this->user->id]);
|
||||||
|
|
||||||
|
// $50 expense
|
||||||
|
Transaction::factory()->create([
|
||||||
|
'user_id' => $this->user->id,
|
||||||
|
'category_id' => $category->id,
|
||||||
|
'transaction_date' => now()->subDays(5),
|
||||||
|
'amount' => -5000,
|
||||||
|
]);
|
||||||
|
|
||||||
|
// $10 refund
|
||||||
|
Transaction::factory()->create([
|
||||||
|
'user_id' => $this->user->id,
|
||||||
|
'category_id' => $category->id,
|
||||||
|
'transaction_date' => now()->subDays(3),
|
||||||
|
'amount' => 1000,
|
||||||
|
]);
|
||||||
|
|
||||||
|
$budget = Budget::factory()->create([
|
||||||
|
'user_id' => $this->user->id,
|
||||||
|
'category_id' => $category->id,
|
||||||
|
]);
|
||||||
|
|
||||||
|
$period = BudgetPeriod::factory()->create([
|
||||||
|
'budget_id' => $budget->id,
|
||||||
|
'start_date' => now()->subDays(30),
|
||||||
|
'end_date' => now()->addDays(30),
|
||||||
|
'allocated_amount' => 10000,
|
||||||
|
]);
|
||||||
|
|
||||||
|
$this->service->assignHistoricalTransactionsToPeriod($period);
|
||||||
|
|
||||||
|
// Net spending should be $40 (5000 - 1000 = 4000)
|
||||||
|
$totalSpent = (int) $period->budgetTransactions()->sum('amount');
|
||||||
|
expect($totalSpent)->toBe(4000);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('assignTransaction stores refund as negative budget transaction amount', function () {
|
||||||
|
$category = Category::factory()->create(['user_id' => $this->user->id]);
|
||||||
|
|
||||||
|
$budget = Budget::factory()->create([
|
||||||
|
'user_id' => $this->user->id,
|
||||||
|
'category_id' => $category->id,
|
||||||
|
]);
|
||||||
|
|
||||||
|
$period = BudgetPeriod::factory()->create([
|
||||||
|
'budget_id' => $budget->id,
|
||||||
|
'start_date' => now()->subDays(30),
|
||||||
|
'end_date' => now()->addDays(30),
|
||||||
|
]);
|
||||||
|
|
||||||
|
// Create a refund transaction
|
||||||
|
$refund = Transaction::factory()->create([
|
||||||
|
'user_id' => $this->user->id,
|
||||||
|
'category_id' => $category->id,
|
||||||
|
'transaction_date' => now()->subDays(2),
|
||||||
|
'amount' => 2000, // positive = refund
|
||||||
|
]);
|
||||||
|
|
||||||
|
$this->service->assignTransaction($refund);
|
||||||
|
|
||||||
|
$budgetTransaction = $period->budgetTransactions()->first();
|
||||||
|
expect($budgetTransaction->amount)->toBe(-2000);
|
||||||
});
|
});
|
||||||
|
|
||||||
test('assignHistoricalTransactionsToPeriod only assigns to correct user', function () {
|
test('assignHistoricalTransactionsToPeriod only assigns to correct user', function () {
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue