From f2a7f955e67465bb415685d9c17ecab213f7decf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Falc=C3=B3n?= Date: Tue, 24 Feb 2026 21:12:36 +0100 Subject: [PATCH] fix(budgets): handle refunds correctly in budget spending calculations (#152) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 --- app/Services/BudgetPeriodService.php | 2 +- app/Services/BudgetTransactionService.php | 4 +- ..._fix_budget_transaction_refund_amounts.php | 34 ++++++ .../Feature/BudgetTransactionServiceTest.php | 102 +++++++++++++++++- 4 files changed, 135 insertions(+), 7 deletions(-) create mode 100644 database/migrations/2026_02_24_193117_fix_budget_transaction_refund_amounts.php diff --git a/app/Services/BudgetPeriodService.php b/app/Services/BudgetPeriodService.php index 589a5cb6..8caf03c2 100644 --- a/app/Services/BudgetPeriodService.php +++ b/app/Services/BudgetPeriodService.php @@ -40,7 +40,7 @@ class BudgetPeriodService if ($budget->rollover_type->value === 'carry_over') { $totalSpent = $period->budgetTransactions()->sum('amount'); - $remaining = $period->allocated_amount - abs($totalSpent); + $remaining = $period->allocated_amount - $totalSpent; if ($remaining > 0) { $carriedOverAmount = $remaining; diff --git a/app/Services/BudgetTransactionService.php b/app/Services/BudgetTransactionService.php index cc117e1b..2d9d2214 100644 --- a/app/Services/BudgetTransactionService.php +++ b/app/Services/BudgetTransactionService.php @@ -62,7 +62,7 @@ class BudgetTransactionService BudgetTransaction::create([ 'transaction_id' => $transaction->id, 'budget_period_id' => $period->id, - 'amount' => abs($transaction->amount), + 'amount' => -$transaction->amount, ]); } } @@ -126,7 +126,7 @@ class BudgetTransactionService BudgetTransaction::create([ 'transaction_id' => $transaction->id, 'budget_period_id' => $period->id, - 'amount' => abs($transaction->amount), + 'amount' => -$transaction->amount, ]); $assignedCount++; diff --git a/database/migrations/2026_02_24_193117_fix_budget_transaction_refund_amounts.php b/database/migrations/2026_02_24_193117_fix_budget_transaction_refund_amounts.php new file mode 100644 index 00000000..8dda3039 --- /dev/null +++ b/database/migrations/2026_02_24_193117_fix_budget_transaction_refund_amounts.php @@ -0,0 +1,34 @@ + 0 + ) + '); + } + + /** + * Revert: make all budget_transaction amounts positive again (original behavior). + */ + public function down(): void + { + DB::statement(' + UPDATE budget_transactions + SET amount = ABS(amount) + '); + } +}; diff --git a/tests/Feature/BudgetTransactionServiceTest.php b/tests/Feature/BudgetTransactionServiceTest.php index d69dc9eb..28dbd791 100644 --- a/tests/Feature/BudgetTransactionServiceTest.php +++ b/tests/Feature/BudgetTransactionServiceTest.php @@ -240,14 +240,14 @@ test('assignHistoricalTransactionsToPeriod works with transactions having multip 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]); - $transaction = Transaction::factory()->create([ + Transaction::factory()->create([ 'user_id' => $this->user->id, 'category_id' => $category->id, 'transaction_date' => now()->subDays(5), - 'amount' => -5000, // Negative amount + 'amount' => -5000, // Expense (negative) ]); $budget = Budget::factory()->create([ @@ -265,7 +265,101 @@ test('assignHistoricalTransactionsToPeriod stores absolute value of amount', fun $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 () {