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); +});