fix(budgets): re-derive budget membership when labels are attached without a model event
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.
This commit is contained in:
Víctor Falcón
parent
8af9b74fd3
commit
fa4b420cd9
|
|
@ -0,0 +1,46 @@
|
|||
<?php
|
||||
|
||||
namespace App\Jobs;
|
||||
|
||||
use App\Models\Transaction;
|
||||
use App\Services\BudgetTransactionService;
|
||||
use Illuminate\Contracts\Queue\ShouldQueue;
|
||||
use Illuminate\Database\Eloquent\Collection;
|
||||
use Illuminate\Foundation\Queue\Queueable;
|
||||
|
||||
/**
|
||||
* Re-derive budget membership for transactions whose category or labels changed
|
||||
* without a model event.
|
||||
*
|
||||
* Pivot writes and mass updates fire nothing, so `AssignTransactionToBudget`
|
||||
* never runs for them and the transaction stays in whatever budget it was in —
|
||||
* typically the catch-all, even once a budget tracks its new label. This job
|
||||
* stands in for the missing event without re-broadcasting `TransactionUpdated`,
|
||||
* which would also re-run the automation rules that dispatched it.
|
||||
*/
|
||||
class ReassignTransactionsToBudgets implements ShouldQueue
|
||||
{
|
||||
use Queueable;
|
||||
|
||||
/**
|
||||
* @param array<int, string> $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);
|
||||
}
|
||||
});
|
||||
}
|
||||
}
|
||||
|
|
@ -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())]);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<int, Transaction> $transactions
|
||||
* @return array<int, string> 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<int, Transaction> $transactions
|
||||
* @return array<int, string> 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<int, Transaction> $transactions
|
||||
* @return array<int, string> 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.
|
||||
*
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,126 @@
|
|||
<?php
|
||||
|
||||
use App\Enums\CategoryType;
|
||||
use App\Mcp\Servers\WhisperMoneyServer;
|
||||
use App\Mcp\Tools\LabelTransaction;
|
||||
use App\Models\Account;
|
||||
use App\Models\AutomationRule;
|
||||
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\AutomationRuleService;
|
||||
|
||||
function periodCovering(Budget $budget): BudgetPeriod
|
||||
{
|
||||
return BudgetPeriod::factory()->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]);
|
||||
});
|
||||
Loading…
Reference in New Issue