From 4930405e791b505a100bcc0f51ecb95ebdfe89a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vi=CC=81ctor=20Falco=CC=81n?= Date: Tue, 11 Aug 2026 17:10:30 +0200 Subject: [PATCH] fix(budgets): keep labelled expenses out of the catch-all budget A catch-all budget only treated categories as "claimed" by other budgets, so every expense tracked by a label-only budget was absorbed by it as well. Also adds budgets:reassign-labelled to re-derive the assignments the bug already wrote. --- .../ReassignLabelledBudgetTransactions.php | 65 +++++++++++ app/Services/BudgetTransactionService.php | 88 ++++++++++----- tests/Feature/CatchAllBudgetTest.php | 105 ++++++++++++++++++ 3 files changed, 229 insertions(+), 29 deletions(-) create mode 100644 app/Console/Commands/ReassignLabelledBudgetTransactions.php diff --git a/app/Console/Commands/ReassignLabelledBudgetTransactions.php b/app/Console/Commands/ReassignLabelledBudgetTransactions.php new file mode 100644 index 00000000..ee4469cc --- /dev/null +++ b/app/Console/Commands/ReassignLabelledBudgetTransactions.php @@ -0,0 +1,65 @@ +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 labelled transactions currently absorbed by a catch-all budget can + // be wrong. Reassignment is idempotent, so the ones whose label no 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)); + + $count = $query->count(); + + if (! $isDryRun) { + // 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 ($transactions) use ($service) { + foreach ($transactions as $transaction) { + $service->assignTransaction($transaction, notify: false); + } + }); + } + + $verb = $isDryRun ? 'would be reassigned' : 'reassigned'; + $this->info("{$count} labelled transaction(s) {$verb}."); + + return self::SUCCESS; + } +} diff --git a/app/Services/BudgetTransactionService.php b/app/Services/BudgetTransactionService.php index 263966f2..2e1b1296 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; @@ -75,7 +80,7 @@ class BudgetTransactionService $matchingPeriodIds = array_merge( $matchingPeriodIds, - $this->catchAllPeriodIds($transaction, $userId, $categoryMatchIds), + $this->catchAllPeriodIds($transaction, $userId, $categoryMatchIds, $transactionLabelIds->all()), ); // Apply changes atomically so concurrent workers cannot leave the @@ -116,7 +121,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 +161,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, $budget->user_id); } else { // Filter by any tracked category OR label $query->where(function ($q) use ($categoryIds, $labelIds) { @@ -207,14 +202,42 @@ class BudgetTransactionService return $assignedCount; } + /** + * 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, string $userId): void + { + $claimed = $this->claimedIds($userId); + $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 transaction: an expense - * whose category (or an ancestor) is not tracked by any non-catch-all budget. + * whose category (or an ancestor) and labels are not tracked by any + * non-catch-all budget. * * @param array $categoryMatchIds the transaction category and its ancestors + * @param array $transactionLabelIds * @return array */ - private function catchAllPeriodIds(Transaction $transaction, string $userId, array $categoryMatchIds): array + private function catchAllPeriodIds(Transaction $transaction, string $userId, array $categoryMatchIds, array $transactionLabelIds): array { if ($transaction->category_id === null) { return []; @@ -226,7 +249,13 @@ class BudgetTransactionService return []; } - if (array_intersect($categoryMatchIds, $this->claimedCategoryIds($userId)) !== []) { + $claimed = $this->claimedIds($userId); + + if (array_intersect($categoryMatchIds, $claimed['categories']) !== []) { + return []; + } + + if (array_intersect($transactionLabelIds, $claimed['labels']) !== []) { return []; } @@ -241,20 +270,21 @@ class BudgetTransactionService } /** - * Category ids directly tracked by the user's non-catch-all budgets. + * Categories and labels directly tracked by the user's non-catch-all budgets. * - * @return array + * @return array{categories: array, labels: array} */ - private function claimedCategoryIds(string $userId): array + private function claimedIds(string $userId): 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(); + ->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..36bce7ff 100644 --- a/tests/Feature/CatchAllBudgetTest.php +++ b/tests/Feature/CatchAllBudgetTest.php @@ -5,6 +5,7 @@ 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; @@ -121,6 +122,110 @@ 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 whose 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]); + + $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 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 labelled transaction out of the catch-all 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]); + + $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-labelled', ['--dry-run' => true])->assertSuccessful(); + expect(BudgetTransaction::where('budget_period_id', $catchAll->id)->exists())->toBeTrue(); + + $this->artisan('budgets:reassign-labelled')->assertSuccessful(); + + expect(BudgetTransaction::where('budget_period_id', $catchAll->id)->exists())->toBeFalse(); + expect(BudgetTransaction::where('budget_period_id', $trackedPeriod->id)->where('transaction_id', $transaction->id)->exists())->toBeTrue(); +}); + 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]);