From ab3902af397f042c3e80296a81783246bae5ec93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Falc=C3=B3n?= Date: Tue, 11 Aug 2026 17:45:19 +0200 Subject: [PATCH] fix(budgets): keep labeled expenses out of the catch-all budget (#781) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem A catch-all budget ("Not budgeted") is supposed to absorb every expense no other budget covers. It decided that by looking at the **categories** other budgets track — labels were never considered. So for a user whose other budgets track spending **by label**, nothing was ever "claimed" and the catch-all absorbed everything, double counting it. Found in production: a user with three label-only budgets (Padel, Yearly Padel, Miami Flight) had every labeled expense sitting in their catch-all as well. Their current catch-all period read **289,136 / 170,000 (170%, over budget)** where the right figure is **75,281 / 170,000 (44%)**. 185 assignment rows are wrong across 2 users. ## Fix Precedence is now decided by the budget periods that actually match the transaction: if any budget already counts it — by category **or** by label, in a period covering its date — the catch-all stays out. The historical backfill mirrors that rule in SQL, and only treats a category or label as claimed when the claiming budget has a period overlapping the range being backfilled. That last part matters: keying purely on "some budget tracks this label" would have dropped expenses whose label budget has no period covering their date, leaving them in **no** budget at all (23 rows of one production user, ~2,204 of spend that would have silently disappeared from their budget view). Both review passes flagged it; there are now tests for it on both paths. ## Repairing existing data ```bash php artisan budgets:reassign-labeled --user= --dry-run php artisan budgets:reassign-labeled --user= ``` It re-derives every budget assignment of the labeled transactions currently sitting in a catch-all budget, with notifications suppressed — these are historical rows, so a limit email would announce a threshold crossed weeks ago. Reassignment (rather than deleting the bad rows) is deliberate: 44 of the affected transactions are not in their label budget either, so a plain delete would have left them nowhere. ## Known follow-ups (not in this PR) - **The stale state can reappear.** Catch-all membership now depends on labels, but three paths mutate labels without firing `TransactionUpdated`, so nothing reassigns: `AutomationRuleService::applyActions` (`saveQuietly` + `syncWithoutDetaching`), its bulk `applyRuleActionsToTransactions` (`LabelTransaction::insertOrIgnore`), and the `LabelTransaction` MCP tool. This is what left the 44 Miami rows out of their budget — the label was attached ~10 minutes after the transaction's last save. The web paths are fine. - Creating a label budget next to an existing catch-all does not release the catch-all's rows, and deleting one does not hand them back. - The repaired catch-all periods keep their `over_limit_notified` flag until the next expense lands in them (the flag reset lives in the notification path we skip). Self-heals on the next assignment. ## Testing `tests/Feature/CatchAllBudgetTest.php` — 11 tests: label claimed by another budget, label no budget tracks (with a *different* label claimed, so "any claim" is not enough), claiming budget with no covering period on both the per-transaction and historical paths, and the repair command end to end including `Mail::assertNothingSent()`. --- .../ReassignLabeledBudgetTransactions.php | 71 ++++++++ app/Services/BudgetTransactionService.php | 107 +++++++---- tests/Feature/CatchAllBudgetTest.php | 171 ++++++++++++++++++ 3 files changed, 310 insertions(+), 39 deletions(-) create mode 100644 app/Console/Commands/ReassignLabeledBudgetTransactions.php diff --git a/app/Console/Commands/ReassignLabeledBudgetTransactions.php b/app/Console/Commands/ReassignLabeledBudgetTransactions.php new file mode 100644 index 00000000..467b7959 --- /dev/null +++ b/app/Console/Commands/ReassignLabeledBudgetTransactions.php @@ -0,0 +1,71 @@ +option('dry-run'); + $userEmail = $this->option('user'); + $userId = null; + + if ($isDryRun) { + $this->warn('DRY RUN — no changes will be saved.'); + } + + if ($userEmail) { + $user = User::query()->where('email', $userEmail)->first(); + + if (! $user) { + $this->error("User with email '{$userEmail}' not found."); + + return self::FAILURE; + } + + $userId = $user->id; + } + + // Only labeled transactions currently absorbed by a catch-all budget can + // be wrong. Reassignment is idempotent, so the ones no other budget + // tracks are re-derived to exactly what they already are. + $query = Transaction::query() + ->when($userId !== null, fn ($q) => $q->where('user_id', $userId)) + ->whereHas('labels') + ->whereHas('budgetTransactions.budgetPeriod.budget', fn ($q) => $q->where('is_catch_all', true)); + + $checked = $query->count(); + + if ($isDryRun) { + $this->info("{$checked} labeled transaction(s) in a catch-all budget would be re-derived."); + + return self::SUCCESS; + } + + // Silently: these are historical assignments, so a budget-limit email + // would announce a threshold the user crossed weeks ago. + $query->with('labels')->chunkById(200, function (Collection $transactions) use ($service): void { + foreach ($transactions as $transaction) { + $service->assignTransaction($transaction, notify: false); + } + }); + + $moved = $checked - $query->count(); + + $this->info("{$checked} labeled transaction(s) re-derived, {$moved} moved out of a catch-all budget."); + + return self::SUCCESS; + } +} diff --git a/app/Services/BudgetTransactionService.php b/app/Services/BudgetTransactionService.php index 263966f2..6fde6d8a 100644 --- a/app/Services/BudgetTransactionService.php +++ b/app/Services/BudgetTransactionService.php @@ -7,6 +7,7 @@ use App\Models\Budget; use App\Models\BudgetPeriod; use App\Models\BudgetTransaction; use App\Models\Transaction; +use Illuminate\Database\Eloquent\Builder; use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Log; @@ -17,7 +18,11 @@ class BudgetTransactionService private readonly BudgetNotificationService $notifications = new BudgetNotificationService, ) {} - public function assignTransaction(Transaction $transaction): void + /** + * @param bool $notify set to false for bulk backfills, where the emails + * would describe budget states the user never crossed + */ + public function assignTransaction(Transaction $transaction, bool $notify = true): void { $userId = $transaction->user_id; @@ -73,10 +78,13 @@ class BudgetTransactionService } } - $matchingPeriodIds = array_merge( - $matchingPeriodIds, - $this->catchAllPeriodIds($transaction, $userId, $categoryMatchIds), - ); + // A catch-all budget only absorbs what nothing else counts. Any budget + // already tracking this transaction — by category or by label — in a + // period covering its date takes precedence, so the catch-all steps in + // only when there is no such period. + if ($matchingPeriodIds === []) { + $matchingPeriodIds = $this->catchAllPeriodIds($transaction); + } // Apply changes atomically so concurrent workers cannot leave the // transaction half-assigned and the unique index guards duplicates. @@ -116,7 +124,9 @@ class BudgetTransactionService } }, attempts: 5); - $this->notifications->handleAssignment($transaction, $matchingPeriodIds, $createdPeriodIds); + if ($notify) { + $this->notifications->handleAssignment($transaction, $matchingPeriodIds, $createdPeriodIds); + } } public function unassignTransaction(Transaction $transaction): void @@ -154,19 +164,7 @@ class BudgetTransactionService ->withoutTrashed(); if ($budget->is_catch_all) { - // A catch-all budget absorbs every expense whose category is not - // already tracked by one of the user's other budgets. - $claimedCategoryIds = $this->tree->expand( - $budget->user_id, - $this->claimedCategoryIds($budget->user_id), - ); - - $query->whereNotNull('category_id') - ->when( - $claimedCategoryIds !== [], - fn ($q) => $q->whereNotIn('category_id', $claimedCategoryIds), - ) - ->whereHas('category', fn ($q) => $q->where('type', CategoryType::Expense->value)); + $this->applyCatchAllFilters($query, $period, $budget->user_id); } else { // Filter by any tracked category OR label $query->where(function ($q) use ($categoryIds, $labelIds) { @@ -208,13 +206,37 @@ class BudgetTransactionService } /** - * Catch-all budget periods that should absorb this transaction: an expense - * whose category (or an ancestor) is not tracked by any non-catch-all budget. + * Narrow a transaction query to what a catch-all budget absorbs: expenses + * whose category and labels are not already tracked by another budget. + * + * @param Builder $query + */ + private function applyCatchAllFilters(Builder $query, BudgetPeriod $period, string $userId): void + { + $claimed = $this->claimedIds($userId, $period); + $claimedCategoryIds = $this->tree->expand($userId, $claimed['categories']); + + $query->whereNotNull('category_id') + ->when( + $claimedCategoryIds !== [], + fn ($q) => $q->whereNotIn('category_id', $claimedCategoryIds), + ) + ->when( + $claimed['labels'] !== [], + fn ($q) => $q->whereDoesntHave( + 'labels', + fn ($labelQuery) => $labelQuery->whereIn('labels.id', $claimed['labels']), + ), + ) + ->whereHas('category', fn ($q) => $q->where('type', CategoryType::Expense->value)); + } + + /** + * Catch-all budget periods that should absorb this expense. * - * @param array $categoryMatchIds the transaction category and its ancestors * @return array */ - private function catchAllPeriodIds(Transaction $transaction, string $userId, array $categoryMatchIds): array + private function catchAllPeriodIds(Transaction $transaction): array { if ($transaction->category_id === null) { return []; @@ -226,13 +248,9 @@ class BudgetTransactionService return []; } - if (array_intersect($categoryMatchIds, $this->claimedCategoryIds($userId)) !== []) { - return []; - } - return BudgetPeriod::query() - ->whereHas('budget', function ($query) use ($userId) { - $query->where('user_id', $userId)->where('is_catch_all', true); + ->whereHas('budget', function ($query) use ($transaction) { + $query->where('user_id', $transaction->user_id)->where('is_catch_all', true); }) ->where('start_date', '<=', $transaction->transaction_date) ->where('end_date', '>=', $transaction->transaction_date) @@ -241,20 +259,31 @@ class BudgetTransactionService } /** - * Category ids directly tracked by the user's non-catch-all budgets. + * Categories and labels tracked by the user's other budgets over the same + * stretch of time as the given catch-all period. * - * @return array + * A budget only claims spending it actually counts, so one whose periods do + * not reach this far back leaves its categories and labels unclaimed here — + * otherwise the catch-all would drop those expenses without any budget + * picking them up. + * + * @return array{categories: array, labels: array} */ - private function claimedCategoryIds(string $userId): array + private function claimedIds(string $userId, BudgetPeriod $period): array { - return Budget::query() + $budgets = Budget::query() ->where('user_id', $userId) ->where('is_catch_all', false) - ->with('categories:id') - ->get() - ->flatMap(fn (Budget $budget) => $budget->categories->pluck('id')) - ->unique() - ->values() - ->all(); + ->whereHas('periods', function ($query) use ($period) { + $query->where('start_date', '<=', $period->end_date) + ->where('end_date', '>=', $period->start_date); + }) + ->with('categories:id', 'labels:id') + ->get(); + + return [ + 'categories' => $budgets->flatMap(fn (Budget $budget) => $budget->categories->pluck('id'))->unique()->values()->all(), + 'labels' => $budgets->flatMap(fn (Budget $budget) => $budget->labels->pluck('id'))->unique()->values()->all(), + ]; } } diff --git a/tests/Feature/CatchAllBudgetTest.php b/tests/Feature/CatchAllBudgetTest.php index 330ac0dc..cbd02edb 100644 --- a/tests/Feature/CatchAllBudgetTest.php +++ b/tests/Feature/CatchAllBudgetTest.php @@ -5,9 +5,11 @@ use App\Models\Budget; use App\Models\BudgetPeriod; use App\Models\BudgetTransaction; use App\Models\Category; +use App\Models\Label; use App\Models\Transaction; use App\Models\User; use App\Services\BudgetTransactionService; +use Illuminate\Support\Facades\Mail; beforeEach(function () { $this->service = app(BudgetTransactionService::class); @@ -121,6 +123,175 @@ test('catch-all budget excludes a child whose parent category is tracked', funct expect(BudgetTransaction::where('transaction_id', $transaction->id)->where('budget_period_id', $catchAll->id)->exists())->toBeFalse(); }); +test('catch-all budget ignores an expense whose label is tracked by another budget', function () { + $catchAll = catchAllPeriod($this->user); + $category = Category::factory()->create([ + 'user_id' => $this->user->id, + 'type' => CategoryType::Expense, + ]); + $label = Label::factory()->create(['user_id' => $this->user->id]); + + $tracked = Budget::factory()->forLabels($label)->create(['user_id' => $this->user->id]); + $trackedPeriod = BudgetPeriod::factory()->create([ + 'budget_id' => $tracked->id, + 'start_date' => now()->subDays(30), + 'end_date' => now()->addDays(30), + ]); + + $transaction = Transaction::factory()->create([ + 'user_id' => $this->user->id, + 'category_id' => $category->id, + 'transaction_date' => now(), + 'amount' => -1000, + ]); + $transaction->labels()->attach($label); + + $this->service->assignTransaction($transaction->load('labels')); + + expect(BudgetTransaction::where('transaction_id', $transaction->id)->where('budget_period_id', $trackedPeriod->id)->exists())->toBeTrue(); + expect(BudgetTransaction::where('transaction_id', $transaction->id)->where('budget_period_id', $catchAll->id)->exists())->toBeFalse(); +}); + +test('catch-all budget still absorbs an expense carrying a label no budget tracks', function () { + $catchAll = catchAllPeriod($this->user); + $category = Category::factory()->create([ + 'user_id' => $this->user->id, + 'type' => CategoryType::Expense, + ]); + $label = Label::factory()->create(['user_id' => $this->user->id]); + + // Another budget claims a different label, so "some label is claimed" must + // not be enough to drop this expense. + $other = Budget::factory()->forLabels(Label::factory()->create(['user_id' => $this->user->id]))->create(['user_id' => $this->user->id]); + BudgetPeriod::factory()->create([ + 'budget_id' => $other->id, + 'start_date' => now()->subDays(30), + 'end_date' => now()->addDays(30), + ]); + + $transaction = Transaction::factory()->create([ + 'user_id' => $this->user->id, + 'category_id' => $category->id, + 'transaction_date' => now(), + 'amount' => -1000, + ]); + $transaction->labels()->attach($label); + + $this->service->assignTransaction($transaction->load('labels')); + + expect(BudgetTransaction::where('transaction_id', $transaction->id)->where('budget_period_id', $catchAll->id)->exists())->toBeTrue(); +}); + +test('catch-all budget keeps an expense whose label budget has no period covering it', function () { + $catchAll = catchAllPeriod($this->user); + $category = Category::factory()->create([ + 'user_id' => $this->user->id, + 'type' => CategoryType::Expense, + ]); + $label = Label::factory()->create(['user_id' => $this->user->id]); + + // The label budget was created later and only covers future dates, so it + // cannot take this expense — dropping it from the catch-all would leave it + // out of every budget. + $tracked = Budget::factory()->forLabels($label)->create(['user_id' => $this->user->id]); + BudgetPeriod::factory()->create([ + 'budget_id' => $tracked->id, + 'start_date' => now()->addDays(10), + 'end_date' => now()->addDays(40), + ]); + + $transaction = Transaction::factory()->create([ + 'user_id' => $this->user->id, + 'category_id' => $category->id, + 'transaction_date' => now(), + 'amount' => -1000, + ]); + $transaction->labels()->attach($label); + + $this->service->assignTransaction($transaction->load('labels')); + + expect(BudgetTransaction::where('transaction_id', $transaction->id)->where('budget_period_id', $catchAll->id)->exists())->toBeTrue(); +}); + +test('historical assignment keeps an expense whose label budget has no period covering it', function () { + $category = Category::factory()->create(['user_id' => $this->user->id, 'type' => CategoryType::Expense]); + $label = Label::factory()->create(['user_id' => $this->user->id]); + + $transaction = Transaction::factory()->create(['user_id' => $this->user->id, 'category_id' => $category->id, 'transaction_date' => now()->subDay(), 'amount' => -1000]); + $transaction->labels()->attach($label); + + $tracked = Budget::factory()->forLabels($label)->create(['user_id' => $this->user->id]); + BudgetPeriod::factory()->create([ + 'budget_id' => $tracked->id, + 'start_date' => now()->addDays(40), + 'end_date' => now()->addDays(70), + ]); + + $period = catchAllPeriod($this->user); + + expect($this->service->assignHistoricalTransactionsToPeriod($period))->toBe(1); + expect(BudgetTransaction::where('budget_period_id', $period->id)->where('transaction_id', $transaction->id)->exists())->toBeTrue(); +}); + +test('historical assignment skips expenses whose label another budget tracks', function () { + $category = Category::factory()->create(['user_id' => $this->user->id, 'type' => CategoryType::Expense]); + $label = Label::factory()->create(['user_id' => $this->user->id]); + + $labelled = Transaction::factory()->create(['user_id' => $this->user->id, 'category_id' => $category->id, 'transaction_date' => now()->subDay(), 'amount' => -1000]); + $labelled->labels()->attach($label); + Transaction::factory()->create(['user_id' => $this->user->id, 'category_id' => $category->id, 'transaction_date' => now()->subDay(), 'amount' => -2000]); + + $tracked = Budget::factory()->forLabels($label)->create(['user_id' => $this->user->id]); + BudgetPeriod::factory()->create([ + 'budget_id' => $tracked->id, + 'start_date' => now()->subDays(30), + 'end_date' => now()->addDays(30), + ]); + + $period = catchAllPeriod($this->user); + + expect($this->service->assignHistoricalTransactionsToPeriod($period))->toBe(1); + expect(BudgetTransaction::where('budget_period_id', $period->id)->where('transaction_id', $labelled->id)->exists())->toBeFalse(); +}); + +test('the reassign command moves a labeled transaction out of the catch-all budget', function () { + Mail::fake(); + + $catchAll = catchAllPeriod($this->user); + $category = Category::factory()->create(['user_id' => $this->user->id, 'type' => CategoryType::Expense]); + $label = Label::factory()->create(['user_id' => $this->user->id]); + + $transaction = Transaction::factory()->create([ + 'user_id' => $this->user->id, + 'category_id' => $category->id, + 'transaction_date' => now(), + 'amount' => -1000, + ]); + // The state the bug left behind: absorbed by the catch-all on creation, then + // labelled — attaching a label fires no model event, so nothing reassigned it. + expect(BudgetTransaction::where('budget_period_id', $catchAll->id)->exists())->toBeTrue(); + + $transaction->labels()->attach($label); + + $tracked = Budget::factory()->forLabels($label)->create(['user_id' => $this->user->id]); + $trackedPeriod = BudgetPeriod::factory()->create([ + 'budget_id' => $tracked->id, + 'start_date' => now()->subDays(30), + 'end_date' => now()->addDays(30), + ]); + + $this->artisan('budgets:reassign-labeled', ['--dry-run' => true])->assertSuccessful(); + expect(BudgetTransaction::where('budget_period_id', $catchAll->id)->where('transaction_id', $transaction->id)->exists())->toBeTrue(); + + $this->artisan('budgets:reassign-labeled')->assertSuccessful(); + + expect(BudgetTransaction::where('budget_period_id', $catchAll->id)->where('transaction_id', $transaction->id)->exists())->toBeFalse(); + expect(BudgetTransaction::where('budget_period_id', $trackedPeriod->id)->where('transaction_id', $transaction->id)->exists())->toBeTrue(); + + // A repair sweep must not email the user about thresholds crossed weeks ago. + Mail::assertNothingSent(); +}); + test('historical assignment backfills only unclaimed expenses into a catch-all budget', function () { $loose = Category::factory()->create(['user_id' => $this->user->id, 'type' => CategoryType::Expense]); $claimed = Category::factory()->create(['user_id' => $this->user->id, 'type' => CategoryType::Expense]);