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 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.
This commit is contained in:
Víctor Falcón 2026-08-11 17:10:30 +02:00
parent b2ff1664e2
commit 4930405e79
3 changed files with 229 additions and 29 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

65
app/Console/Commands/ReassignLabelledBudgetTransactions.php Normal file
View File

@ -0,0 +1,65 @@
<?php
namespace App\Console\Commands;
use App\Models\Transaction;
use App\Models\User;
use App\Services\BudgetTransactionService;
use Illuminate\Console\Command;
class ReassignLabelledBudgetTransactions extends Command
{
protected $signature = 'budgets:reassign-labelled
{--user= : Filter by user email address}
{--dry-run : Preview what would be reassigned without making changes}';
protected $description = 'Re-run budget assignment for labelled 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 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;
}
}

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

105
tests/Feature/CatchAllBudgetTest.php
View File

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