RomanNum3ral
/
whisper-money
mirror of https://github.com/whisper-money/whisper-money.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fix(budgets): keep labeled expenses out of the catch-all budget (#781) 

## 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=<email> --dry-run
php artisan budgets:reassign-labeled --user=<email>
```

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()`.
This commit is contained in:
Víctor Falcón 2026-08-11 17:45:19 +02:00 committed by GitHub
parent b2ff1664e2
commit ab3902af39
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 310 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

71
app/Console/Commands/ReassignLabeledBudgetTransactions.php Normal file
View File

@ -0,0 +1,71 @@
<?php
namespace App\Console\Commands;
use App\Models\Transaction;
use App\Models\User;
use App\Services\BudgetTransactionService;
use Illuminate\Console\Command;
use Illuminate\Database\Eloquent\Collection;
class ReassignLabeledBudgetTransactions extends Command
{
protected $signature = 'budgets:reassign-labeled
{--user= : Filter by user email address}
{--dry-run : Preview what would be reassigned without making changes}';
protected $description = 'Re-derive every budget assignment of the labeled transactions sitting in a catch-all budget, which used to absorb them even when another budget tracked their label';
public function handle(BudgetTransactionService $service): int
{
$isDryRun = (bool) $this->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;
}
}

107
app/Services/BudgetTransactionService.php
View File

@ -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<Transaction> $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<int, string> $categoryMatchIds the transaction category and its ancestors
* @return array<int, string>
*/
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<int, string>
* 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<int, string>, labels: array<int, string>}
*/
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(),
];
}
}

171
tests/Feature/CatchAllBudgetTest.php
View File

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