From f4c21147f13c58ed9ad3dcac2da89cc6fd5bb4a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Falc=C3=B3n?= Date: Wed, 12 Aug 2026 12:47:43 +0200 Subject: [PATCH] feat(budgets): count shared accounts at the owner's percentage (#786) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Why #750 made a shared account count only your share of every transaction — on the dashboard and on the cashflow screen. Budgets were left out and shipped as a known gap: `budget_transactions.amount` is a snapshot written when a transaction is assigned, so a 50% joint account still spent **100%** of every expense against its budget. The same category could read €400 on the dashboard and €800 in Budgets. This closes that gap. ## What A €100 expense on an account you own 50% of now counts €50 towards your budgets. Because every budget reader funnels through `BudgetPeriod::spentAmount()` — a sum of those snapshots — weighing the snapshot covers the budget cards, the detail page, the spending chart, carry-over, the limit alert emails and the MCP tools in one move. - **Assignment writes the owner's share.** Both paths (the per-transaction listener and the historical backfill) go through one `recordSnapshot()`. - **Changing an account's share rewrites its history.** A single SQL `UPDATE` re-weighs every budget row of that account, in every period, past ones included. - **A migration re-weighs the rows written before this**, so existing shared accounts are correct on deploy instead of on the next edit. ## How The share is computed at the same two choke points #750 established: - **PHP** — `Transaction::ownerShareOf()` (extracted from `ConvertsTransactionCurrency`, which was doing the same null dance inline) feeds `BudgetTransactionService::recordSnapshot()`. - **SQL** — `BudgetTransactionService::reweighAccountSnapshots()` reuses `Transaction::OWNED_AMOUNT_SQL`, so the rounding matches what PHP would have written. A test pins both to the same answer on an uneven share. The re-weigh hangs off `Account::booted()` rather than `AccountController`, so a seeder, an artisan command or a future MCP write tool cannot silently skip it. It also clears the period's `close_to_limit_notified` / `over_limit_notified` flags, the same way a refund that drops a budget back under its limit does — otherwise a budget that fell out of "over limit" would stay claimed and never alert on the next real crossing. The owning account is eager loaded **withTrashed** everywhere the snapshot is written, because `OWNED_AMOUNT_SQL` joins `accounts` without the soft-delete scope; without it a transaction whose account was deleted would snapshot at 100% in PHP and at the real share in SQL. ## Deliberate boundaries - **Transaction rows still show the real bank amount** — #750's rule. The budget detail page is the one screen where a weighted total sits directly above its own itemised list, so it now says so in a line under the chart. The alert email had the same mismatch inside one message and now quotes your share. - **`carried_over_amount` is not re-derived.** It is a second snapshot taken when a period closes. `remainingAmount()` deliberately ignores it and the UI only types the field; it surfaces solely through MCP. Left alone rather than adding a second re-derivation path. - The migration's `down()` restores the full transaction amount, which is what those rows held — it cannot know a share an account no longer has. ## Testing `tests/Feature/SharedAccountOwnershipTest.php` gains the budget cases: assignment, the historical backfill, the re-weigh through the settings screen, PHP/SQL rounding parity on 33% of 3333, and the alert flags being cleared. `tests/Feature/WeighBudgetTransactionsMigrationTest.php` covers the backfill, including that it is idempotent and that it keeps refunds negative (the fix from `2026_02_24_193117`). Fixing that test needed the `BudgetTransaction` factory, which had been pointing at a `BudgetPeriodAllocation` model that no longer exists. Manual QA on real local data — budget "Miami Flight", account "Daily" set to 50%: | | Before | At 50% | Back at 100% | |---|---|---|---| | Miami Flight spent | €3,229.40 | **€1,955.79** | €3,229.40 | | Yearly Padel spent | €1,514.91 | €785.49 | €1,514.91 | €1,955.79 is €3,229.40 − €1,273.61, exactly half of the €2,547.24 that account had in the budget. The round trip lands back on the original figure to the cent, so the re-weigh is idempotent on real data too. ## Demo https://github.com/user-attachments/assets/1fa5b98a-2a11-4cad-b927-496b434d3295 --- .../ReassignLabeledBudgetTransactions.php | 2 +- .../Commands/ResetDemoAccountCommand.php | 2 +- .../Concerns/ConvertsTransactionCurrency.php | 2 +- app/Mail/BudgetNotificationEmail.php | 5 +- app/Models/Account.php | 16 +++ app/Models/Transaction.php | 10 ++ app/Services/BudgetTransactionService.php | 86 +++++++++---- .../factories/BudgetTransactionFactory.php | 4 +- ...dget_transactions_by_account_ownership.php | 43 +++++++ lang/es.json | 3 +- .../accounts/edit-account-dialog.tsx | 2 +- resources/js/pages/budgets/show.tsx | 40 ++++-- tests/Feature/SharedAccountOwnershipTest.php | 119 ++++++++++++++++++ .../WeighBudgetTransactionsMigrationTest.php | 62 +++++++++ 14 files changed, 357 insertions(+), 39 deletions(-) create mode 100644 database/migrations/2026_08_12_100000_weigh_budget_transactions_by_account_ownership.php create mode 100644 tests/Feature/WeighBudgetTransactionsMigrationTest.php diff --git a/app/Console/Commands/ReassignLabeledBudgetTransactions.php b/app/Console/Commands/ReassignLabeledBudgetTransactions.php index 467b7959..3dddd504 100644 --- a/app/Console/Commands/ReassignLabeledBudgetTransactions.php +++ b/app/Console/Commands/ReassignLabeledBudgetTransactions.php @@ -56,7 +56,7 @@ class ReassignLabeledBudgetTransactions extends Command // 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 { + $query->with('labels', 'account')->chunkById(200, function (Collection $transactions) use ($service): void { foreach ($transactions as $transaction) { $service->assignTransaction($transaction, notify: false); } diff --git a/app/Console/Commands/ResetDemoAccountCommand.php b/app/Console/Commands/ResetDemoAccountCommand.php index 801005d0..e6ef9aef 100644 --- a/app/Console/Commands/ResetDemoAccountCommand.php +++ b/app/Console/Commands/ResetDemoAccountCommand.php @@ -497,7 +497,7 @@ class ResetDemoAccountCommand extends Command private function assignTransactionsToBudgets(User $user): void { - $transactions = $user->transactions()->get(); + $transactions = $user->transactions()->with('account')->get(); $assignedCount = 0; $budgetAssignments = []; diff --git a/app/Http/Controllers/Api/Concerns/ConvertsTransactionCurrency.php b/app/Http/Controllers/Api/Concerns/ConvertsTransactionCurrency.php index 40841c06..cac2eb17 100644 --- a/app/Http/Controllers/Api/Concerns/ConvertsTransactionCurrency.php +++ b/app/Http/Controllers/Api/Concerns/ConvertsTransactionCurrency.php @@ -26,7 +26,7 @@ trait ConvertsTransactionCurrency $transaction->transaction_date->toDateString(), ); - return $transaction->account?->shareOfAmount($converted) ?? $converted; + return $transaction->ownerShareOf($converted); } /** diff --git a/app/Mail/BudgetNotificationEmail.php b/app/Mail/BudgetNotificationEmail.php index 576616a6..254c9c6a 100644 --- a/app/Mail/BudgetNotificationEmail.php +++ b/app/Mail/BudgetNotificationEmail.php @@ -83,8 +83,11 @@ class BudgetNotificationEmail extends Mailable implements ShouldQueue 'allocatedFormatted' => Money::format($allocated, $currency), 'spentFormatted' => Money::format($spent, $currency), 'remainingFormatted' => Money::format(abs($remaining), $currency), + // The owner's share, not the full charge: every other figure in + // this email is what the budget counted, so a shared account + // would otherwise show €80 spent against a €40 rise. 'transactionAmountFormatted' => $this->transaction - ? Money::format(abs((int) $this->transaction->amount), $currency) + ? Money::format(abs($this->transaction->ownerShareOf((int) $this->transaction->amount)), $currency) : null, // Aligns with the service: at exactly the limit we are "over". 'isOverLimit' => $hasLimit && $spent >= $allocated, diff --git a/app/Models/Account.php b/app/Models/Account.php index 5a065b58..b9914c96 100644 --- a/app/Models/Account.php +++ b/app/Models/Account.php @@ -4,6 +4,7 @@ namespace App\Models; use App\Enums\AccountType; use App\Models\Concerns\BelongsToSpace; +use App\Services\BudgetTransactionService; use Database\Factories\AccountFactory; use Illuminate\Database\Eloquent\Casts\Attribute; use Illuminate\Database\Eloquent\Concerns\HasUuids; @@ -59,6 +60,21 @@ class Account extends Model 'linked_loan_account_id', ]; + /** + * Budget amounts are snapshots taken when a transaction is assigned, so a + * new ownership share only reaches the budgets that already counted this + * account if something rewrites them. Hooked on the model rather than on + * the settings controller so every write path is covered. + */ + protected static function booted(): void + { + static::updated(function (Account $account): void { + if ($account->wasChanged('ownership_percentage')) { + app(BudgetTransactionService::class)->reweighAccountSnapshots($account); + } + }); + } + protected function casts(): array { return [ diff --git a/app/Models/Transaction.php b/app/Models/Transaction.php index 8ea35f02..280fec40 100644 --- a/app/Models/Transaction.php +++ b/app/Models/Transaction.php @@ -248,6 +248,16 @@ class Transaction extends Model return $query->whereNull('category_id')->whereNull('description_iv'); } + /** + * The owner's share of an amount held by this transaction's account, for + * the row-by-row PHP paths. Falls back to the full amount when the account + * is not loaded, so a partial select never silently zeroes the figure. + */ + public function ownerShareOf(int $amount): int + { + return $this->account?->shareOfAmount($amount) ?? $amount; + } + /** * A transaction amount reduced to the owner's share of its account, for * SQL-side aggregates. Rounds per row, matching {@see Account::shareOfAmount()} diff --git a/app/Services/BudgetTransactionService.php b/app/Services/BudgetTransactionService.php index 18b08a93..4c1847cf 100644 --- a/app/Services/BudgetTransactionService.php +++ b/app/Services/BudgetTransactionService.php @@ -3,6 +3,7 @@ namespace App\Services; use App\Enums\CategoryType; +use App\Models\Account; use App\Models\Budget; use App\Models\BudgetPeriod; use App\Models\BudgetTransaction; @@ -30,8 +31,12 @@ class BudgetTransactionService return; } - // Ensure labels are available for matching (safe if already loaded). + // Ensure labels are available for matching, and the account for the + // ownership share (both safe if already loaded). The account is loaded + // with trashed ones too, so this agrees with the SQL re-weigh, which + // ignores the soft-delete scope as well. $transaction->loadMissing('labels'); + $transaction->loadMissing(['account' => fn ($query) => $query->withTrashed()]); $matchingPeriodIds = $this->trackedPeriodIds($transaction, $userId); @@ -65,15 +70,7 @@ class BudgetTransactionService ->delete(); foreach ($matchingPeriodIds as $periodId) { - $budgetTransaction = BudgetTransaction::updateOrCreate( - [ - 'transaction_id' => $transaction->id, - 'budget_period_id' => $periodId, - ], - [ - 'amount' => -$transaction->amount, - ], - ); + $budgetTransaction = $this->recordSnapshot($transaction, $periodId); if ($budgetTransaction->wasRecentlyCreated) { $createdPeriodIds[] = $periodId; @@ -118,6 +115,10 @@ class BudgetTransactionService $query = Transaction::query() ->where('user_id', $budget->user_id) ->whereBetween('transaction_date', [$period->start_date, $period->end_date]) + // The owning account weighs every snapshot; eager loaded so a + // 500-row chunk does not turn into 500 account lookups, and with + // trashed ones so it agrees with the SQL re-weigh. + ->with(['account' => fn ($query) => $query->withTrashed()]) ->withoutTrashed(); if ($budget->is_catch_all) { @@ -143,17 +144,7 @@ class BudgetTransactionService // Process in chunks to prevent memory issues $query->chunk(500, function ($transactions) use ($period, &$assignedCount) { foreach ($transactions as $transaction) { - $budgetTransaction = BudgetTransaction::updateOrCreate( - [ - 'transaction_id' => $transaction->id, - 'budget_period_id' => $period->id, - ], - [ - 'amount' => -$transaction->amount, - ], - ); - - if ($budgetTransaction->wasRecentlyCreated) { + if ($this->recordSnapshot($transaction, $period->id)->wasRecentlyCreated) { $assignedCount++; } } @@ -162,6 +153,59 @@ class BudgetTransactionService return $assignedCount; } + /** + * Record what a transaction contributes to a budget period: the owner's + * share of it, flipped so an expense counts as positive spending. + * + * The amount is a snapshot taken here and never revisited, so a later + * change to the account's share has to go through + * {@see self::reweighAccountSnapshots()}. + */ + private function recordSnapshot(Transaction $transaction, string $budgetPeriodId): BudgetTransaction + { + return BudgetTransaction::updateOrCreate( + [ + 'transaction_id' => $transaction->id, + 'budget_period_id' => $budgetPeriodId, + ], + [ + 'amount' => -$transaction->ownerShareOf($transaction->amount), + ], + ); + } + + /** + * Re-snapshot every budget row of an account after its ownership share + * changed, in SQL so it stays one query no matter how much history the + * account has. Uses {@see Transaction::OWNED_AMOUNT_SQL} so the rounding + * matches what {@see self::recordSnapshot()} would have written. + * + * @return int the number of rows re-weighed + */ + public function reweighAccountSnapshots(Account $account): int + { + $reweighed = DB::table('budget_transactions') + ->join('transactions', 'transactions.id', '=', 'budget_transactions.transaction_id') + ->join('accounts', 'accounts.id', '=', 'transactions.account_id') + ->where('accounts.id', $account->id) + ->update(['budget_transactions.amount' => DB::raw('-('.Transaction::OWNED_AMOUNT_SQL.')')]); + + // Every affected period now holds a different total, so the limit alerts + // it already sent describe a state that no longer exists. Clearing the + // flags lets the next crossing notify again, the same way a refund that + // drops a budget back under its limit does. + if ($reweighed > 0) { + BudgetPeriod::query() + ->whereHas( + 'budgetTransactions.transaction', + fn (Builder $query) => $query->where('account_id', $account->id), + ) + ->update(['close_to_limit_notified' => false, 'over_limit_notified' => false]); + } + + return $reweighed; + } + /** * Narrow a transaction query to what a catch-all budget absorbs: expenses * whose category and labels are not already tracked by another budget. diff --git a/database/factories/BudgetTransactionFactory.php b/database/factories/BudgetTransactionFactory.php index 24540838..e88f9a89 100644 --- a/database/factories/BudgetTransactionFactory.php +++ b/database/factories/BudgetTransactionFactory.php @@ -2,7 +2,7 @@ namespace Database\Factories; -use App\Models\BudgetPeriodAllocation; +use App\Models\BudgetPeriod; use App\Models\BudgetTransaction; use App\Models\Transaction; use Illuminate\Database\Eloquent\Factories\Factory; @@ -21,7 +21,7 @@ class BudgetTransactionFactory extends Factory { return [ 'transaction_id' => Transaction::factory(), - 'budget_period_allocation_id' => BudgetPeriodAllocation::factory(), + 'budget_period_id' => BudgetPeriod::factory(), 'amount' => fake()->numberBetween(1000, 50000), ]; } diff --git a/database/migrations/2026_08_12_100000_weigh_budget_transactions_by_account_ownership.php b/database/migrations/2026_08_12_100000_weigh_budget_transactions_by_account_ownership.php new file mode 100644 index 00000000..1d751aa0 --- /dev/null +++ b/database/migrations/2026_08_12_100000_weigh_budget_transactions_by_account_ownership.php @@ -0,0 +1,43 @@ + {__( - 'For accounts you share with someone else. Income and expenses only count towards your figures by this percentage.', + 'For accounts you share with someone else. Income and expenses only count towards your figures by this percentage. Changing it also rewrites what this account has already spent in your budgets, past periods included.', )}

+ periodTransactions.some( + (transaction) => + (transaction.account?.ownership_percentage ?? 100) < 100, + ), + [periodTransactions], + ); + return ( ) : ( - + <> + {hasSharedAccountTransactions && ( +

+ {__( + 'Each row shows the full amount charged by the bank. The totals above only count your share of the accounts you share with someone else.', + )} +

+ )} + + )} diff --git a/tests/Feature/SharedAccountOwnershipTest.php b/tests/Feature/SharedAccountOwnershipTest.php index 90ee7923..2b2dfa93 100644 --- a/tests/Feature/SharedAccountOwnershipTest.php +++ b/tests/Feature/SharedAccountOwnershipTest.php @@ -5,9 +5,12 @@ use App\Enums\CategoryType; use App\Models\Account; use App\Models\AccountBalance; use App\Models\Bank; +use App\Models\Budget; +use App\Models\BudgetPeriod; use App\Models\Category; use App\Models\Transaction; use App\Models\User; +use App\Services\BudgetTransactionService; use Illuminate\Support\Facades\Http; beforeEach(function () { @@ -255,6 +258,122 @@ test('updating an account persists its ownership settings', function () { ->and($account->fresh()->ownership_applies_to_balance)->toBeTrue(); }); +/** + * A budget tracking one expense category over the current month, plus a shared + * account holding a single expense in that category. The transaction is not + * assigned yet, so each test decides which assignment path writes the snapshot. + * + * @return array{account: Account, period: BudgetPeriod, transaction: Transaction} + */ +function sharedAccountExpenseAndBudget(User $user, int $percentage, int $amount = -80000): array +{ + $account = Account::factory()->create([ + 'user_id' => $user->id, + 'type' => AccountType::Checking, + 'currency_code' => 'USD', + 'ownership_percentage' => $percentage, + ]); + + $category = Category::factory()->create([ + 'user_id' => $user->id, + 'type' => CategoryType::Expense, + ]); + + $budget = Budget::factory()->forCategories($category)->create([ + 'user_id' => $user->id, + ]); + + $period = BudgetPeriod::factory()->create([ + 'budget_id' => $budget->id, + 'start_date' => now()->startOfMonth(), + 'end_date' => now()->endOfMonth(), + 'allocated_amount' => 100000, + ]); + + $transaction = Transaction::factory()->create([ + 'user_id' => $user->id, + 'account_id' => $account->id, + 'category_id' => $category->id, + 'amount' => $amount, + 'currency_code' => 'USD', + 'transaction_date' => now(), + ]); + + return ['account' => $account, 'period' => $period, 'transaction' => $transaction]; +} + +test('a budget counts only the owner share of a transaction on a shared account', function () { + ['period' => $period, 'transaction' => $transaction] = sharedAccountExpenseAndBudget($this->user, 50); + + app(BudgetTransactionService::class)->assignTransaction($transaction); + + expect($period->spentAmount())->toBe(40000) + ->and($period->remainingAmount())->toBe(60000); +}); + +test('a fully owned account still counts in full towards a budget', function () { + ['period' => $period, 'transaction' => $transaction] = sharedAccountExpenseAndBudget($this->user, 100); + + app(BudgetTransactionService::class)->assignTransaction($transaction); + + expect($period->spentAmount())->toBe(80000); +}); + +test('the historical backfill counts only the owner share of a shared account', function () { + ['period' => $period] = sharedAccountExpenseAndBudget($this->user, 50); + + app(BudgetTransactionService::class)->assignHistoricalTransactionsToPeriod($period); + + expect($period->spentAmount())->toBe(40000); +}); + +test('changing an account share re-weighs the budget amounts already recorded', function () { + ['account' => $account, 'period' => $period, 'transaction' => $transaction] = sharedAccountExpenseAndBudget($this->user, 100); + + app(BudgetTransactionService::class)->assignTransaction($transaction); + expect($period->spentAmount())->toBe(80000); + + $this->patch(route('accounts.update', $account), [ + 'name' => $account->name, + 'type' => AccountType::Checking->value, + 'currency_code' => 'USD', + 'ownership_percentage' => 50, + ])->assertRedirect(); + + expect($period->spentAmount())->toBe(40000); +}); + +/** + * Budget amounts are written in PHP on assignment and rewritten in SQL when the + * share changes. An amount that does not divide evenly is the only thing that + * catches the two rounding rules drifting apart. + */ +test('the PHP and SQL budget paths round an uneven share identically', function () { + ['account' => $account, 'period' => $period, 'transaction' => $transaction] = sharedAccountExpenseAndBudget($this->user, 33, amount: -3333); + + app(BudgetTransactionService::class)->assignTransaction($transaction); + expect($period->spentAmount())->toBe(1100); + + // Round-trip through 100% and back so the SQL path recomputes the same 33%. + $account->update(['ownership_percentage' => 100]); + expect($period->spentAmount())->toBe(3333); + + $account->update(['ownership_percentage' => 33]); + expect($period->spentAmount())->toBe(1100); +}); + +test('a re-weigh lets a budget notify again on the next crossing', function () { + ['account' => $account, 'period' => $period, 'transaction' => $transaction] = sharedAccountExpenseAndBudget($this->user, 100, amount: -120000); + + app(BudgetTransactionService::class)->assignTransaction($transaction); + $period->update(['over_limit_notified' => true, 'close_to_limit_notified' => true]); + + $account->update(['ownership_percentage' => 50]); + + expect($period->fresh()->over_limit_notified)->toBeFalse() + ->and($period->fresh()->close_to_limit_notified)->toBeFalse(); +}); + test('the ownership percentage must stay between 1 and 100', function () { $bank = Bank::factory()->create(); $account = Account::factory()->create([ diff --git a/tests/Feature/WeighBudgetTransactionsMigrationTest.php b/tests/Feature/WeighBudgetTransactionsMigrationTest.php new file mode 100644 index 00000000..c7bbf3b0 --- /dev/null +++ b/tests/Feature/WeighBudgetTransactionsMigrationTest.php @@ -0,0 +1,62 @@ +up(); +} + +/** + * A budget row holding the full transaction amount, the way every row written + * before the ownership weighting looks. + */ +function unweighedBudgetRow(User $user, int $percentage, int $amount): BudgetTransaction +{ + $account = Account::factory()->create([ + 'user_id' => $user->id, + 'ownership_percentage' => $percentage, + ]); + + $transaction = Transaction::factory()->create([ + 'user_id' => $user->id, + 'account_id' => $account->id, + 'amount' => $amount, + ]); + + return BudgetTransaction::factory()->create([ + 'transaction_id' => $transaction->id, + 'budget_period_id' => BudgetPeriod::factory()->create()->id, + 'amount' => -$amount, + ]); +} + +it('re-weighs only the rows of shared accounts', function () { + $user = User::factory()->create(); + + $shared = unweighedBudgetRow($user, 50, -80000); + $refundOnShared = unweighedBudgetRow($user, 50, 20000); + $fullyOwned = unweighedBudgetRow($user, 100, -80000); + + runWeighBudgetTransactionsMigration(); + + expect($shared->fresh()->amount)->toBe(40000) + // A refund stays negative, keeping the fix from the 2026_02_24 migration. + ->and($refundOnShared->fresh()->amount)->toBe(-10000) + ->and($fullyOwned->fresh()->amount)->toBe(80000); +}); + +it('is idempotent, so a second run does not halve the amounts again', function () { + $user = User::factory()->create(); + $shared = unweighedBudgetRow($user, 50, -80000); + + runWeighBudgetTransactionsMigration(); + runWeighBudgetTransactionsMigration(); + + expect($shared->fresh()->amount)->toBe(40000); +});