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]);