From 49fb62d94db27d4d20e941afb8bd0d2446935880 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vi=CC=81ctor=20Falco=CC=81n?= Date: Sun, 19 Jul 2026 14:49:18 +0200 Subject: [PATCH] fix(transactions): address split feature review findings - keep editing an already-split transaction working when the flag is off: only resend splits when the split content actually changed or was undone - make the dashboard cashflow summary and the Transaction Analysis screen split-aware (per-line category/type + tag attribution) - skip split transactions on bulk label assignment (labels live on lines), matching the existing bulk-category skip - reject splitting a zero-amount transaction - split editor no longer shows a green 'Remaining: 0' with a dead Save button when a line is still zero - add isSplit() helper, UUID split-line keys, clarify allocation docblocks --- .../Api/TransactionAnalysisController.php | 12 ++- app/Http/Controllers/DashboardController.php | 10 ++- .../Controllers/TransactionController.php | 30 ++++--- .../Requests/UpdateTransactionRequest.php | 9 ++- app/Services/TransactionAllocations.php | 25 ++++-- lang/en.json | 4 +- lang/es.json | 4 +- .../components/transactions/category-cell.tsx | 3 +- .../transactions/edit-transaction-dialog.tsx | 32 +++++++- .../transactions/split-editor.test.tsx | 4 + .../components/transactions/split-editor.tsx | 28 ++++--- resources/js/pages/transactions/index.tsx | 10 +-- resources/js/types/transaction.ts | 5 ++ tests/Feature/TransactionSplitTest.php | 80 +++++++++++++++++++ 14 files changed, 205 insertions(+), 51 deletions(-) diff --git a/app/Http/Controllers/Api/TransactionAnalysisController.php b/app/Http/Controllers/Api/TransactionAnalysisController.php index ed325060..516bc7c6 100644 --- a/app/Http/Controllers/Api/TransactionAnalysisController.php +++ b/app/Http/Controllers/Api/TransactionAnalysisController.php @@ -9,6 +9,7 @@ use App\Models\Label; use App\Models\Transaction; use App\Services\CategoryTree; use App\Services\ExchangeRateService; +use App\Services\TransactionAllocations; use Carbon\Carbon; use Illuminate\Http\JsonResponse; use Illuminate\Support\Collection; @@ -56,14 +57,19 @@ class TransactionAnalysisController extends Controller $transactions = Transaction::query() ->where('user_id', $user->id) - ->with(['account.bank', 'category', 'labels']) + ->with(['account.bank', 'category', 'labels', 'splits.category', 'splits.labels']) ->applyFilters($filters) ->get(); $this->preloadExchangeRates($transactions, $currency); - $byCategory = $this->categoryBreakdown($transactions, $currency, $user->id); - $byTag = $this->tagBreakdown($transactions, $currency); + // Category and tag breakdowns are per-line for split transactions; payee, + // account and the totals stay at the transaction level (a split doesn't + // change who was paid or the amount that moved). + $allocations = TransactionAllocations::expand($transactions); + + $byCategory = $this->categoryBreakdown($allocations, $currency, $user->id); + $byTag = $this->tagBreakdown($allocations, $currency); $byPayee = $this->payeeBreakdown($transactions, $currency); $byAccount = $this->accountBreakdown($transactions, $currency); diff --git a/app/Http/Controllers/DashboardController.php b/app/Http/Controllers/DashboardController.php index f85ba628..3841a037 100644 --- a/app/Http/Controllers/DashboardController.php +++ b/app/Http/Controllers/DashboardController.php @@ -4,11 +4,11 @@ namespace App\Http\Controllers; use App\Enums\CategoryType; use App\Models\Account; -use App\Models\Transaction; use App\Services\AccountMetricsService; use App\Services\CashflowSummaryService; use App\Services\CategorySpendingService; use App\Services\PeriodComparator; +use App\Services\TransactionAllocations; use Carbon\Carbon; use Illuminate\Http\Request; use Illuminate\Support\Facades\DB; @@ -110,9 +110,11 @@ class DashboardController extends Controller private function getTransactionSum(string $userId, Carbon $from, Carbon $to, CategoryType $type): int { - return Transaction::query() - ->where('transactions.user_id', $userId) - ->whereBetween('transactions.transaction_date', [$from, $to]) + // Read effective allocations so a split's lines are classified by their + // own category type (a savings/transfer line no longer counts as a plain + // expense just because the parent row has no category). + return DB::query() + ->fromSub(TransactionAllocations::query($userId, $from, $to), 'transactions') ->where(function ($q) use ($type) { $q->whereExists(function ($sub) use ($type) { $sub->select(DB::raw(1)) diff --git a/app/Http/Controllers/TransactionController.php b/app/Http/Controllers/TransactionController.php index fe97b311..7c4b8ced 100644 --- a/app/Http/Controllers/TransactionController.php +++ b/app/Http/Controllers/TransactionController.php @@ -353,16 +353,23 @@ class TransactionController extends Controller $transactions = $query->get(); } - // Split transactions carry no single category, so a bulk category - // assignment skips them (it would silently destroy their lines) and we - // report how many were left untouched. - $omittedSplitIds = collect(); - $updateData = []; - if ($request->has('category_id')) { - $newCategoryId = $request->input('category_id'); + $labelIds = $request->input('label_ids'); + $hasLabelUpdate = $request->has('label_ids'); + $hasCategoryUpdate = $request->has('category_id'); + // A bulk category or label assignment skips split transactions: setting a + // single category would destroy their lines, and labels live on the lines, + // not the row. Notes still apply (they are transaction-level). We report + // how many splits were left untouched. + $omittedSplitIds = collect(); + if ($hasCategoryUpdate || $hasLabelUpdate) { $transactions->loadMissing('splits:id,transaction_id'); $omittedSplitIds = $transactions->filter->isSplit()->pluck('id'); + } + + $updateData = []; + if ($hasCategoryUpdate) { + $newCategoryId = $request->input('category_id'); $overrideHandler = app(CategoryOverrideHandler::class); @@ -386,9 +393,6 @@ class TransactionController extends Controller $updateData['notes_iv'] = $request->input('notes_iv'); } - $labelIds = $request->input('label_ids'); - $hasLabelUpdate = $request->has('label_ids'); - if (empty($updateData) && ! $hasLabelUpdate) { return response()->json([ 'message' => 'No update data provided.', @@ -402,7 +406,7 @@ class TransactionController extends Controller } elseif ($filters !== null) { $updateQuery->applyFilters($filters); } - if ($omittedSplitIds->isNotEmpty()) { + if ($hasCategoryUpdate && $omittedSplitIds->isNotEmpty()) { $updateQuery->whereNotIn('id', $omittedSplitIds->all()); } $updateQuery->update($updateData); @@ -410,6 +414,10 @@ class TransactionController extends Controller if ($hasLabelUpdate) { foreach ($transactions as $transaction) { + if ($transaction->isSplit()) { + continue; + } + $transaction->labels()->sync($labelIds ?? []); $transaction->save(); } diff --git a/app/Http/Requests/UpdateTransactionRequest.php b/app/Http/Requests/UpdateTransactionRequest.php index e339c9f0..489b82fa 100644 --- a/app/Http/Requests/UpdateTransactionRequest.php +++ b/app/Http/Requests/UpdateTransactionRequest.php @@ -75,6 +75,13 @@ class UpdateTransactionRequest extends FormRequest } $total = $this->effectiveAmount(); + + if ($total === 0) { + $validator->errors()->add('splits', __("You can't split a transaction with no amount.")); + + return; + } + $sum = 0; foreach ($lines as $index => $line) { @@ -83,7 +90,7 @@ class UpdateTransactionRequest extends FormRequest if ($amount === 0) { $validator->errors()->add("splits.{$index}.amount", __('Each split line must be a non-zero amount.')); - } elseif ($total !== 0 && ($amount > 0) !== ($total > 0)) { + } elseif (($amount > 0) !== ($total > 0)) { $validator->errors()->add("splits.{$index}.amount", __('Split lines must have the same sign as the transaction.')); } } diff --git a/app/Services/TransactionAllocations.php b/app/Services/TransactionAllocations.php index ed3d77b1..65b6cc3f 100644 --- a/app/Services/TransactionAllocations.php +++ b/app/Services/TransactionAllocations.php @@ -10,10 +10,13 @@ use Illuminate\Support\Collection; use Illuminate\Support\Facades\DB; /** - * The single source of truth for "effective category attribution": an unsplit - * transaction is attributed to its own category, a split transaction to its - * lines. Every category breakdown reads through here so a split never gets - * double-counted or dropped. + * Effective category attribution: an unsplit transaction is attributed to its + * own category, a split transaction to its lines. The category breakdowns read + * through here — either the SQL {@see self::query()} or the collection + * {@see self::expand()} — so a split is never double-counted or dropped. The two + * encarnations share the "iterate lines vs the row itself" rule but not code: + * SQL sums raw rows, expand() needs hydrated models (account/category/labels) + * for currency conversion and side classification. */ class TransactionAllocations { @@ -50,10 +53,14 @@ class TransactionAllocations /** * Expand a loaded transaction collection into effective allocations: a split * transaction becomes one synthetic Transaction per line (carrying the line's - * amount + category while keeping the parent's account/currency/date for FX - * and side classification); unsplit transactions pass through unchanged. + * amount + category + labels while keeping the parent's account/currency/date + * for FX and side classification); unsplit transactions pass through + * unchanged. * - * Callers must eager-load `splits.category` (and `account` for FX). + * Callers must eager-load `splits.category` (and `splits.labels`/`account` + * where they read those). Synthetic line instances deliberately keep the + * parent's `id`, so consumers must aggregate by `category_id`/labels, never + * `keyBy('id')`/`unique('id')` (that would collapse a split's lines). * * @param Collection $transactions * @return Collection @@ -76,6 +83,10 @@ class TransactionAllocations $allocation->setRelation('account', $transaction->relationLoaded('account') ? $transaction->account : null); $allocation->setRelation('category', $line->relationLoaded('category') ? $line->category : null); + if ($line->relationLoaded('labels')) { + $allocation->setRelation('labels', $line->labels); + } + return $allocation; })->all(); })->values(); diff --git a/lang/en.json b/lang/en.json index 14251de3..5f8647df 100644 --- a/lang/en.json +++ b/lang/en.json @@ -6,9 +6,11 @@ "Remove line": "Remove line", "Undo split": "Undo split", "Remaining: :amount": "Remaining: :amount", - "Skipped :count split transactions": "Skipped :count split transactions", + "Each line needs a non-zero amount": "Each line needs a non-zero amount", + "Split transactions skipped: :count": "Split transactions skipped: :count", "A split needs at least two lines.": "A split needs at least two lines.", "The split lines must add up to the transaction amount.": "The split lines must add up to the transaction amount.", "Each split line must be a non-zero amount.": "Each split line must be a non-zero amount.", + "You can't split a transaction with no amount.": "You can't split a transaction with no amount.", "Split lines must have the same sign as the transaction.": "Split lines must have the same sign as the transaction." } diff --git a/lang/es.json b/lang/es.json index 5deef2fe..a12af9a2 100644 --- a/lang/es.json +++ b/lang/es.json @@ -2298,9 +2298,11 @@ "Remove line": "Eliminar línea", "Undo split": "Deshacer división", "Remaining: :amount": "Restante: :amount", - "Skipped :count split transactions": "Se omitieron :count transacciones divididas", + "Each line needs a non-zero amount": "Cada línea necesita un importe distinto de cero", + "Split transactions skipped: :count": "Transacciones divididas omitidas: :count", "A split needs at least two lines.": "Una división necesita al menos dos líneas.", "The split lines must add up to the transaction amount.": "Las líneas de la división deben sumar el importe de la transacción.", "Each split line must be a non-zero amount.": "Cada línea de la división debe tener un importe distinto de cero.", + "You can't split a transaction with no amount.": "No puedes dividir una transacción sin importe.", "Split lines must have the same sign as the transaction.": "Las líneas de la división deben tener el mismo signo que la transacción." } diff --git a/resources/js/components/transactions/category-cell.tsx b/resources/js/components/transactions/category-cell.tsx index f29e04f1..ddf2c51b 100644 --- a/resources/js/components/transactions/category-cell.tsx +++ b/resources/js/components/transactions/category-cell.tsx @@ -18,6 +18,7 @@ import { type SharedData } from '@/types'; import { type Account, type Bank } from '@/types/account'; import { getCategoryColorClasses, type Category } from '@/types/category'; import { + isSplit, type DecryptedTransaction, type TransactionSplit, } from '@/types/transaction'; @@ -67,7 +68,7 @@ export function CategoryCell({ // A split transaction shows its per-line breakdown instead of a single, // editable category. Editing happens in the modal (click the row). - if (transaction.splits && transaction.splits.length > 0) { + if (isSplit(transaction)) { return ( ().props.features.transactionSplitting; const isSplitting = splitLines.length > 0; - const wasSplit = (transaction?.splits?.length ?? 0) > 0; + const wasSplit = transaction ? isSplit(transaction) : false; function startSplit() { // Seed with the current category as the first line at the full amount, @@ -496,8 +496,32 @@ export function EditTransactionDialog({ // A split change turns one row into several (or back). Persist it // and let the server-rendered list re-fetch rather than patch the - // grouped rows by hand. - if (isSplitting || wasSplit) { + // grouped rows by hand. Only send `splits` when the split content + // actually changed (or was undone) — editing another field of an + // already-split transaction must not re-trigger the gated split + // write, so it keeps working even if the feature is rolled back. + const originalSplitSignature = JSON.stringify( + (transaction.splits ?? []).map((split) => ({ + category_id: split.category_id, + amount: split.amount, + label_ids: [ + ...(split.labels?.map((label) => label.id) ?? []), + ].sort(), + })), + ); + const currentSplitSignature = JSON.stringify( + splitLines.map((line) => ({ + category_id: line.category_id, + amount: line.amount, + label_ids: [...line.label_ids].sort(), + })), + ); + const splitDirty = + (isSplitting && + currentSplitSignature !== originalSplitSignature) || + (!isSplitting && wasSplit); + + if (splitDirty) { if (isSplitting) { if (!isSplitValid(amount, splitLines)) { toast.error( diff --git a/resources/js/components/transactions/split-editor.test.tsx b/resources/js/components/transactions/split-editor.test.tsx index 4f8c6ddd..bf7d729a 100644 --- a/resources/js/components/transactions/split-editor.test.tsx +++ b/resources/js/components/transactions/split-editor.test.tsx @@ -37,4 +37,8 @@ describe('isSplitValid', () => { it('rejects a line whose sign differs from the total', () => { expect(isSplitValid(-10000, lines(-12500, 2500))).toBe(false); }); + + it('rejects splitting a zero-total transaction', () => { + expect(isSplitValid(0, lines(50, -50))).toBe(false); + }); }); diff --git a/resources/js/components/transactions/split-editor.tsx b/resources/js/components/transactions/split-editor.tsx index 85f1183a..5bd8e03c 100644 --- a/resources/js/components/transactions/split-editor.tsx +++ b/resources/js/components/transactions/split-editor.tsx @@ -26,15 +26,11 @@ interface SplitEditorProps { onLabelCreated?: (label: Label) => void; } -let keyCounter = 0; - export function newSplitLine( partial?: Partial, ): SplitLineDraft { - keyCounter += 1; - return { - key: `split-${keyCounter}`, + key: crypto.randomUUID(), category_id: null, amount: 0, label_ids: [], @@ -48,17 +44,16 @@ export function splitRemaining(total: number, lines: SplitLineDraft[]): number { /** * A split is valid once it has at least two lines that each carry a non-zero - * amount on the same side as the transaction and together reconcile to its - * total to the cent. + * amount on the same side as the (non-zero) transaction total and together + * reconcile to it to the cent. */ export function isSplitValid(total: number, lines: SplitLineDraft[]): boolean { - if (lines.length < 2 || splitRemaining(total, lines) !== 0) { + if (total === 0 || lines.length < 2 || splitRemaining(total, lines) !== 0) { return false; } return lines.every( - (line) => - line.amount !== 0 && (total === 0 || line.amount > 0 === total > 0), + (line) => line.amount !== 0 && line.amount > 0 === total > 0, ); } @@ -88,6 +83,11 @@ export function SplitEditor({ const addLine = () => onChange([...lines, newSplitLine({ amount: remaining })]); + const valid = isSplitValid(total, lines); + // Remaining can be 0 while the split is still invalid (e.g. a seeded + // zero-amount line), so key the styling off validity, not just the balance. + const hasZeroLine = lines.some((line) => line.amount === 0); + const formattedRemaining = new Intl.NumberFormat(locale, { style: 'currency', currency: currencyCode, @@ -168,13 +168,17 @@ export function SplitEditor({ - {__('Remaining: :amount', { amount: formattedRemaining })} + {remaining === 0 && hasZeroLine + ? __('Each line needs a non-zero amount') + : __('Remaining: :amount', { + amount: formattedRemaining, + })} diff --git a/resources/js/pages/transactions/index.tsx b/resources/js/pages/transactions/index.tsx index 94fd842c..7bdc1e04 100644 --- a/resources/js/pages/transactions/index.tsx +++ b/resources/js/pages/transactions/index.tsx @@ -104,6 +104,7 @@ import { import { type Category } from '@/types/category'; import { type Label } from '@/types/label'; import { + isSplit, type DecryptedTransaction, type TransactionFilters as Filters, type ServerTransaction, @@ -1139,7 +1140,7 @@ export default function Transactions({ ); if (response.data.omitted_splits > 0) { toast.info( - __('Skipped :count split transactions', { + __('Split transactions skipped: :count', { count: response.data.omitted_splits, }), ); @@ -1159,10 +1160,7 @@ export default function Transactions({ // (it would wipe their lines), so skip them and report how many. const splitIds = new Set( allTransactions - .filter( - (transaction) => - (transaction.splits?.length ?? 0) > 0, - ) + .filter((transaction) => isSplit(transaction)) .map((transaction) => transaction.id.toString()), ); const targetIds = selectedIds.filter((id) => !splitIds.has(id)); @@ -1195,7 +1193,7 @@ export default function Transactions({ if (omitted > 0) { toast.info( - __('Skipped :count split transactions', { + __('Split transactions skipped: :count', { count: omitted, }), ); diff --git a/resources/js/types/transaction.ts b/resources/js/types/transaction.ts index 9d41a96d..7a5d6ff0 100644 --- a/resources/js/types/transaction.ts +++ b/resources/js/types/transaction.ts @@ -52,6 +52,11 @@ export interface Transaction { updated_at: string; } +/** Whether a transaction is split into per-category lines. */ +export function isSplit(transaction: Pick): boolean { + return (transaction.splits?.length ?? 0) > 0; +} + export interface ServerTransaction extends Transaction { account?: Account; category?: Category | null; diff --git a/tests/Feature/TransactionSplitTest.php b/tests/Feature/TransactionSplitTest.php index 2c73cf63..fc49e474 100644 --- a/tests/Feature/TransactionSplitTest.php +++ b/tests/Feature/TransactionSplitTest.php @@ -299,6 +299,86 @@ it('does not treat a fully-categorized split as uncategorized', function () { expect($matches->all())->not->toContain($split->id); }); +it('rejects splitting a zero-amount transaction', function () { + $category = expenseCategory($this->user); + $transaction = splittableTransaction($this->user, 0); + + actingAs($this->user) + ->patchJson(route('transactions.update', $transaction), [ + 'splits' => [ + ['category_id' => $category->id, 'amount' => 50], + ['category_id' => $category->id, 'amount' => -50], + ], + ]) + ->assertJsonValidationErrors(['splits']); +}); + +it('classifies split lines by their own category type in the dashboard cash flow', function () { + $expense = expenseCategory($this->user, 'Groceries'); + $savings = Category::factory()->for($this->user)->create([ + 'name' => 'Savings', + 'type' => CategoryType::Savings, + 'cashflow_direction' => CategoryCashflowDirection::Outflow, + ]); + $transaction = splittableTransaction($this->user, -10000); + $transaction->update(['transaction_date' => now()]); + + app(TransactionSplitter::class)->apply($transaction, [ + ['category_id' => $expense->id, 'amount' => -3000], + ['category_id' => $savings->id, 'amount' => -7000], + ]); + + $response = actingAs($this->user)->getJson( + '/api/dashboard/cash-flow?from='.now()->startOfMonth()->toDateString().'&to='.now()->endOfMonth()->toDateString(), + )->assertOk(); + + // Only the expense line counts as expense; the savings line is excluded. + expect($response->json('current.expense'))->toBe(3000); +}); + +it('attributes split lines per category on the analysis screen', function () { + $restaurant = expenseCategory($this->user, 'Restaurant'); + $travel = expenseCategory($this->user, 'Travel'); + $transaction = splittableTransaction($this->user, -10000); + $transaction->update(['transaction_date' => now()]); + + app(TransactionSplitter::class)->apply($transaction, [ + ['category_id' => $restaurant->id, 'amount' => -2500], + ['category_id' => $travel->id, 'amount' => -7500], + ]); + + $response = actingAs($this->user)->getJson( + '/api/transactions/analysis?date_from='.now()->subDay()->toDateString().'&date_to='.now()->addDay()->toDateString(), + )->assertOk(); + + $byCategory = collect($response->json('by_category'))->keyBy('category_id'); + + expect($byCategory[$restaurant->id]['amount'])->toBe(2500); + expect($byCategory[$travel->id]['amount'])->toBe(7500); +}); + +it('skips split transactions on a bulk label assignment', function () { + $category = expenseCategory($this->user); + $label = Label::factory()->for($this->user)->create(); + $split = splittableTransaction($this->user, -10000); + $plain = splittableTransaction($this->user, -5000); + + app(TransactionSplitter::class)->apply($split, [ + ['category_id' => $category->id, 'amount' => -2500], + ['category_id' => $category->id, 'amount' => -7500], + ]); + + actingAs($this->user) + ->patchJson(route('transactions.bulk-update'), [ + 'transaction_ids' => [$split->id, $plain->id], + 'label_ids' => [$label->id], + ]) + ->assertOk(); + + expect($split->fresh()->labels)->toHaveCount(0); + expect($plain->fresh()->labels->pluck('id')->all())->toContain($label->id); +}); + it('skips split transactions on a bulk category assignment', function () { $category = expenseCategory($this->user); $newCategory = expenseCategory($this->user, 'New');