From fa4b420cd9cb8bdb55e76e32cc4e2b7d023e72be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vi=CC=81ctor=20Falco=CC=81n?= Date: Wed, 12 Aug 2026 12:09:11 +0200 Subject: [PATCH] fix(budgets): re-derive budget membership when labels are attached without a model event MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Catch-all budget membership depends on labels since #781, but three paths attach labels through pivot writes or mass updates, which fire no model event. AssignTransactionToBudget never ran for them, so the transaction stayed in whatever budget it was in — typically the catch-all, even once a budget tracked its new label. Dispatch a dedicated ReassignTransactionsToBudgets job from those paths instead of re-broadcasting TransactionUpdated, which would also re-run the automation rules that dispatched it. The bulk apply queues one job for the whole batch rather than one per transaction. Extract the three action branches of applyRuleActionsToTransactions so the added dispatch keeps the method under the complexity threshold. --- app/Jobs/ReassignTransactionsToBudgets.php | 46 +++++ app/Mcp/Tools/LabelTransaction.php | 5 + app/Services/AutomationRuleService.php | 186 ++++++++++++------ .../Feature/AutomationRuleApplicationTest.php | 7 +- tests/Feature/LabelBudgetReassignmentTest.php | 126 ++++++++++++ 5 files changed, 303 insertions(+), 67 deletions(-) create mode 100644 app/Jobs/ReassignTransactionsToBudgets.php create mode 100644 tests/Feature/LabelBudgetReassignmentTest.php diff --git a/app/Jobs/ReassignTransactionsToBudgets.php b/app/Jobs/ReassignTransactionsToBudgets.php new file mode 100644 index 00000000..35ef64ae --- /dev/null +++ b/app/Jobs/ReassignTransactionsToBudgets.php @@ -0,0 +1,46 @@ + $transactionIds + * @param bool $notify set to false for bulk applies, where the budget + * emails would describe limits crossed long ago + */ + public function __construct( + public array $transactionIds, + public bool $notify = true, + ) {} + + public function handle(BudgetTransactionService $service): void + { + Transaction::query() + ->whereIn('id', $this->transactionIds) + ->with('labels') + ->chunkById(200, function (Collection $transactions) use ($service): void { + foreach ($transactions as $transaction) { + $service->assignTransaction($transaction, notify: $this->notify); + } + }); + } +} diff --git a/app/Mcp/Tools/LabelTransaction.php b/app/Mcp/Tools/LabelTransaction.php index 1c087b66..38058b13 100644 --- a/app/Mcp/Tools/LabelTransaction.php +++ b/app/Mcp/Tools/LabelTransaction.php @@ -2,6 +2,7 @@ namespace App\Mcp\Tools; +use App\Jobs\ReassignTransactionsToBudgets; use App\Models\User; use Illuminate\Contracts\JsonSchema\JsonSchema; use Laravel\Mcp\Request; @@ -44,6 +45,10 @@ class LabelTransaction extends WriteTool $transaction->labels()->detach($remove->pluck('id')->all()); } + // Pivot writes fire no model event, so nothing would re-derive which + // budgets now track this transaction by label. + ReassignTransactionsToBudgets::dispatch([$transaction->id]); + return $this->json(['transaction' => $this->presentTransaction($transaction->refresh())]); } } diff --git a/app/Services/AutomationRuleService.php b/app/Services/AutomationRuleService.php index 9a31d76e..11c84ca9 100644 --- a/app/Services/AutomationRuleService.php +++ b/app/Services/AutomationRuleService.php @@ -3,6 +3,7 @@ namespace App\Services; use App\Enums\CategorySource; +use App\Jobs\ReassignTransactionsToBudgets; use App\Models\AutomationRule; use App\Models\LabelTransaction; use App\Models\Transaction; @@ -40,8 +41,8 @@ class AutomationRuleService $transactionData = $this->prepareTransactionData($transaction); $matchedRule = $this->evaluateRules($rules, $transactionData); - if ($matchedRule) { - $this->applyActions($transaction, $matchedRule); + if ($matchedRule && $this->applyActions($transaction, $matchedRule)) { + ReassignTransactionsToBudgets::dispatch([$transaction->id]); } } @@ -98,75 +99,130 @@ class AutomationRuleService } } - $changedTransactionIds = []; + $changedTransactionIds = array_values(array_unique(array_merge( + $this->applyCategoryInBulk($transactions, $rule), + $this->applyNoteInBulk($transactions, $rule), + $this->applyLabelsInBulk($transactions, $rule), + ))); - if ($rule->action_category_id !== null) { - $categoryTransactionIds = $transactions - ->filter(fn (Transaction $transaction): bool => $transaction->category_id !== $rule->action_category_id) - ->pluck('id') - ->all(); - - if ($categoryTransactionIds !== []) { - Transaction::query() - ->whereIn('id', $categoryTransactionIds) - ->update([ - 'category_id' => $rule->action_category_id, - 'category_source' => CategorySource::Rule->value, - 'categorized_by_rule_id' => $rule->id, - 'updated_at' => now(), - ]); - - foreach ($categoryTransactionIds as $transactionId) { - $changedTransactionIds[$transactionId] = true; - } - } - } - - if ($rule->action_note && $rule->action_note_iv === null) { - foreach ($transactions as $transaction) { - $existingNotes = $transaction->notes ?? ''; - - if ($this->noteAlreadyPresent($existingNotes, $rule->action_note)) { - continue; - } - - $transaction->notes = $existingNotes - ? $existingNotes."\n".$rule->action_note - : $rule->action_note; - $transaction->saveQuietly(); - $changedTransactionIds[$transaction->id] = true; - } - } - - $labelIds = $rule->labels->pluck('id')->all(); - if ($labelIds !== []) { - $now = now(); - $labelTransactionRows = []; - - foreach ($transactions as $transaction) { - $transactionLabelIds = $transaction->labels->pluck('id')->all(); - $missingLabelIds = array_diff($labelIds, $transactionLabelIds); - - foreach ($missingLabelIds as $labelId) { - $labelTransactionRows[] = [ - 'id' => (string) Str::uuid(), - 'label_id' => $labelId, - 'transaction_id' => $transaction->id, - 'created_at' => $now, - 'updated_at' => $now, - ]; - $changedTransactionIds[$transaction->id] = true; - } - } - - if ($labelTransactionRows !== []) { - LabelTransaction::query()->insertOrIgnore($labelTransactionRows); - } + if ($changedTransactionIds !== []) { + // One job for the whole batch: the mass update and the pivot insert + // above fire no model event, so nothing else re-derives which budgets + // now track these transactions — and dispatching per row would queue a + // job for every transaction the rule touched. + ReassignTransactionsToBudgets::dispatch($changedTransactionIds, notify: false); } return count($changedTransactionIds); } + /** + * Set the rule's category on every transaction that does not already carry + * it, in a single mass update. + * + * @param EloquentCollection $transactions + * @return array ids of the transactions changed + */ + private function applyCategoryInBulk(EloquentCollection $transactions, AutomationRule $rule): array + { + if ($rule->action_category_id === null) { + return []; + } + + $transactionIds = $transactions + ->filter(fn (Transaction $transaction): bool => $transaction->category_id !== $rule->action_category_id) + ->pluck('id') + ->all(); + + if ($transactionIds === []) { + return []; + } + + Transaction::query() + ->whereIn('id', $transactionIds) + ->update([ + 'category_id' => $rule->action_category_id, + 'category_source' => CategorySource::Rule->value, + 'categorized_by_rule_id' => $rule->id, + 'updated_at' => now(), + ]); + + return $transactionIds; + } + + /** + * Append the rule's note where it is not already present. Encrypted notes are + * skipped because applying them requires the user's key. + * + * @param EloquentCollection $transactions + * @return array ids of the transactions changed + */ + private function applyNoteInBulk(EloquentCollection $transactions, AutomationRule $rule): array + { + if (! $rule->action_note || $rule->action_note_iv !== null) { + return []; + } + + $transactionIds = []; + + foreach ($transactions as $transaction) { + $existingNotes = $transaction->notes ?? ''; + + if ($this->noteAlreadyPresent($existingNotes, $rule->action_note)) { + continue; + } + + $transaction->notes = $existingNotes + ? $existingNotes."\n".$rule->action_note + : $rule->action_note; + $transaction->saveQuietly(); + $transactionIds[] = $transaction->id; + } + + return $transactionIds; + } + + /** + * Attach the rule's labels in a single pivot insert, skipping the pairs that + * already exist. + * + * @param EloquentCollection $transactions + * @return array ids of the transactions changed + */ + private function applyLabelsInBulk(EloquentCollection $transactions, AutomationRule $rule): array + { + $labelIds = $rule->labels->pluck('id')->all(); + + if ($labelIds === []) { + return []; + } + + $now = now(); + $labelTransactionRows = []; + $transactionIds = []; + + foreach ($transactions as $transaction) { + $missingLabelIds = array_diff($labelIds, $transaction->labels->pluck('id')->all()); + + foreach ($missingLabelIds as $labelId) { + $labelTransactionRows[] = [ + 'id' => (string) Str::uuid(), + 'label_id' => $labelId, + 'transaction_id' => $transaction->id, + 'created_at' => $now, + 'updated_at' => $now, + ]; + $transactionIds[] = $transaction->id; + } + } + + if ($labelTransactionRows !== []) { + LabelTransaction::query()->insertOrIgnore($labelTransactionRows); + } + + return $transactionIds; + } + /** * Whether a transaction should be skipped when "only uncategorized" is on. * diff --git a/tests/Feature/AutomationRuleApplicationTest.php b/tests/Feature/AutomationRuleApplicationTest.php index 65fd5f5c..882d3d58 100644 --- a/tests/Feature/AutomationRuleApplicationTest.php +++ b/tests/Feature/AutomationRuleApplicationTest.php @@ -3,6 +3,7 @@ use App\Events\TransactionCreated; use App\Events\TransactionUpdated; use App\Jobs\ApplySingleAutomationRuleJob; +use App\Jobs\ReassignTransactionsToBudgets; use App\Models\Account; use App\Models\AutomationRule; use App\Models\Bank; @@ -212,7 +213,9 @@ test('apply endpoint runs synchronously when matches are below threshold', funct ->assertJsonPath('updated', 3) ->assertJsonPath('total', 3); - Queue::assertNothingPushed(); + Queue::assertNotPushed(ApplySingleAutomationRuleJob::class); + // One reassignment job for the whole batch, not one per transaction. + Queue::assertPushed(ReassignTransactionsToBudgets::class, 1); expect( Transaction::where('user_id', $this->user->id) @@ -263,7 +266,7 @@ test('apply endpoint batches category and label writes', function () { expect($transactionUpdateQueries)->toHaveCount(1) ->and($perTransactionPivotLookupQueries)->toHaveCount(0); - Queue::assertNothingPushed(); + Queue::assertNotPushed(ApplySingleAutomationRuleJob::class); $this->assertDatabaseCount('label_transaction', 5); }); diff --git a/tests/Feature/LabelBudgetReassignmentTest.php b/tests/Feature/LabelBudgetReassignmentTest.php new file mode 100644 index 00000000..ea49848b --- /dev/null +++ b/tests/Feature/LabelBudgetReassignmentTest.php @@ -0,0 +1,126 @@ +create([ + 'budget_id' => $budget->id, + 'start_date' => now()->subDays(30), + 'end_date' => now()->addDays(30), + ]); +} + +function budgetsOf(Transaction $transaction): array +{ + return BudgetTransaction::query() + ->where('transaction_id', $transaction->id) + ->pluck('budget_period_id') + ->all(); +} + +beforeEach(function () { + $this->user = User::factory()->create(); + $this->label = Label::factory()->create(['user_id' => $this->user->id, 'name' => 'Miami 26']); + + $category = Category::factory()->create([ + 'user_id' => $this->user->id, + 'type' => CategoryType::Expense, + ]); + + $this->catchAllPeriod = periodCovering(Budget::factory()->catchAll()->create(['user_id' => $this->user->id])); + $this->labelPeriod = periodCovering(Budget::factory()->forLabels($this->label)->create(['user_id' => $this->user->id])); + + // Created before it carries the label, so it lands in the catch-all — the + // exact state each of the three label paths used to leave behind. + $this->transaction = Transaction::factory()->plaintext()->create([ + 'user_id' => $this->user->id, + 'account_id' => Account::factory()->create(['user_id' => $this->user->id])->id, + 'category_id' => $category->id, + 'description' => 'Hotel Miami', + 'transaction_date' => now(), + 'amount' => -1000, + ]); + + expect(budgetsOf($this->transaction))->toBe([$this->catchAllPeriod->id]); +}); + +function labelRule(User $user, Label $label): AutomationRule +{ + $rule = AutomationRule::factory()->create([ + 'user_id' => $user->id, + 'priority' => 1, + 'rules_json' => ['in' => ['hotel', ['var' => 'description']]], + 'action_category_id' => null, + ]); + + $rule->labels()->attach($label->id); + + return $rule->load('labels'); +} + +test('a rule labelling a single transaction moves it into the label budget', function () { + labelRule($this->user, $this->label); + + app(AutomationRuleService::class)->applyRules($this->transaction); + + expect(budgetsOf($this->transaction))->toBe([$this->labelPeriod->id]); +}); + +test('applying a rule in bulk moves every labelled transaction into the label budget', function () { + $rule = labelRule($this->user, $this->label); + + $transactions = Transaction::query()->whereKey($this->transaction->id)->with('labels')->get(); + + app(AutomationRuleService::class)->applyRuleActionsToTransactions($transactions, $rule); + + expect(budgetsOf($this->transaction))->toBe([$this->labelPeriod->id]); +}); + +test('the bulk apply reassigns transactions whose category it mass-updated', function () { + $tracked = Category::factory()->create(['user_id' => $this->user->id, 'type' => CategoryType::Expense]); + $categoryPeriod = periodCovering(Budget::factory()->forCategories($tracked)->create(['user_id' => $this->user->id])); + + $rule = AutomationRule::factory()->create([ + 'user_id' => $this->user->id, + 'priority' => 1, + 'rules_json' => ['in' => ['hotel', ['var' => 'description']]], + 'action_category_id' => $tracked->id, + ]); + + $transactions = Transaction::query()->whereKey($this->transaction->id)->with('labels')->get(); + + app(AutomationRuleService::class)->applyRuleActionsToTransactions($transactions, $rule); + + expect(budgetsOf($this->transaction))->toBe([$categoryPeriod->id]); +}); + +test('the MCP tool reassigns the transaction when it attaches and when it removes a label', function () { + $this->user->withAccessToken($this->user->createToken('mcp', ['mcp:read', 'mcp:write'])->accessToken); + + WhisperMoneyServer::actingAs($this->user)->tool(LabelTransaction::class, [ + 'transaction_id' => $this->transaction->id, + 'add_label_ids' => [$this->label->id], + ])->assertOk(); + + expect(budgetsOf($this->transaction))->toBe([$this->labelPeriod->id]); + + WhisperMoneyServer::actingAs($this->user)->tool(LabelTransaction::class, [ + 'transaction_id' => $this->transaction->id, + 'remove_label_ids' => [$this->label->id], + ])->assertOk(); + + expect(budgetsOf($this->transaction))->toBe([$this->catchAllPeriod->id]); +});