From dd5bf92fc19e97d7e2e7d5532533771a337f51b7 Mon Sep 17 00:00:00 2001 From: null Date: Sun, 5 Jul 2026 11:51:06 -0500 Subject: [PATCH] fix(money): manual payment path flags suspected dupes (409) w/o losing legit ones (Track A) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The deliberate manual POST /payments had no dedupe guard (double-submit made a second payment), but silently deduping it like the auto paths would lose a legitimately-identical same-day payment — a money bug. So the server returns 409 DUPLICATE_SUSPECTED (existing payment attached), and a shared client helper (client/lib/paymentActions.ts, used by the tracker inline cell, partial-payment dialog, and bill modal) turns it into an 'add anyway?' toast that resends with allow_duplicate. Automated one-click paths keep deduping silently (a repeat there is a retry). Dropped a now-unused api import in PaymentLedgerDialog (Track E sweep). Server suite 188 green; typecheck/lint/build clean; probe 17/17. Co-Authored-By: Claude Opus 4.8 --- HISTORY.md | 3 +- client/components/BillModal.tsx | 20 ++++++--- client/components/tracker/EditableCell.tsx | 18 +++++--- .../tracker/PaymentLedgerDialog.tsx | 22 +++++----- client/lib/paymentActions.ts | 43 +++++++++++++++++++ routes/payments.js | 22 +++++++++- tests/paymentsRoute.test.js | 23 ++++++++++ 7 files changed, 124 insertions(+), 27 deletions(-) create mode 100644 client/lib/paymentActions.ts diff --git a/HISTORY.md b/HISTORY.md index 6265bdf..78e23e4 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -3,7 +3,8 @@ ### 💰 Payment atomicity audit (Track A) — every balance mutation is now transactional -- **[Money integrity] Wrapped the four remaining non-atomic payment paths in `db.transaction()`.** Quick-pay and bulk-pay were already atomic (X1), but the audit found the INSERT/UPDATE + `applyBalanceDelta` pair split across two un-transactional statements on **manual `POST /payments`**, **autopay-suggestion confirm**, **`DELETE` (unpay)**, and **`POST /:id/restore`** — plus **`PUT` (edit)**, where the bill-balance write and the payment `UPDATE` weren't atomic. A crash/constraint failure between the two statements could leave a payment without its balance adjustment (or a balance reversed without the row deleted). Each is now a single transaction: the row write and the balance mutation land together or neither. Behavior-preserving — `tests/paymentsRoute.test.js` pins the create → delete → restore → edit balance round-trip and the double-restore no-op; full server suite green (186 tests). (Dedupe-guard work for the manual path follows as a separate change.) +- **[Money integrity] Wrapped the four remaining non-atomic payment paths in `db.transaction()`.** Quick-pay and bulk-pay were already atomic (X1), but the audit found the INSERT/UPDATE + `applyBalanceDelta` pair split across two un-transactional statements on **manual `POST /payments`**, **autopay-suggestion confirm**, **`DELETE` (unpay)**, and **`POST /:id/restore`** — plus **`PUT` (edit)**, where the bill-balance write and the payment `UPDATE` weren't atomic. A crash/constraint failure between the two statements could leave a payment without its balance adjustment (or a balance reversed without the row deleted). Each is now a single transaction: the row write and the balance mutation land together or neither. Behavior-preserving — `tests/paymentsRoute.test.js` pins the create → delete → restore → edit balance round-trip and the double-restore no-op. +- **[Money integrity] Manual `POST /payments` now flags a suspected duplicate instead of blindly creating one — without silently dropping a legitimate one.** The deliberate manual-add path had *no* dedupe guard, so a double-submit created a second payment. But unlike the automated one-click paths (`/quick`, `/bulk`, autopay-confirm — which dedupe *silently* because a repeat there is a retry), the manual path may legitimately record two identical payments on the same day, so silently deduping would *lose a real payment* — itself a money bug. Resolution: the server returns **`409 DUPLICATE_SUSPECTED`** (with the existing payment attached) on a same `bill+date+amount`; a shared client helper (`client/lib/paymentActions.ts`, used by the tracker inline cell, the partial-payment ledger dialog, and the bill modal) turns that into an **"add anyway?"** toast that resends with `allow_duplicate`. Best of both: retry-safety on the auto paths, no lost-write on the deliberate one. (A DB `UNIQUE` index was rejected — it would forbid legitimately-identical payments; an idempotency-key header is the documented upgrade if this ever goes multi-process.) Full server suite 188 green; e2e probe 17/17. (Track A) ### 🔷 TypeScript foundation + branded money types diff --git a/client/components/BillModal.tsx b/client/components/BillModal.tsx index 87b85a0..fad81e5 100644 --- a/client/components/BillModal.tsx +++ b/client/components/BillModal.tsx @@ -16,6 +16,7 @@ import { Select, SelectTrigger, SelectValue, SelectContent, SelectItem, } from '@/components/ui/select'; import { api } from '@/api'; +import { createPaymentOrConfirm } from '@/lib/paymentActions'; import { cn, todayStr, errMessage } from '@/lib/utils'; import DebtDetailsSection from '@/components/bill-modal/DebtDetailsSection'; import AutopayTrustIndicator from '@/components/bill-modal/AutopayTrustIndicator'; @@ -368,17 +369,22 @@ export default function BillModal({ bill, initialBill, categories, onClose, onSa notes: paymentNotes || null, payment_source: 'manual', }; + const finishPaymentSave = async (successMessage: string) => { + toast.success(successMessage); + resetPaymentForm(); + setPaymentFormOpen(false); + await loadPayments(); + onSave?.(); + }; if (editingPayment) { await api.updatePayment(editingPayment.id, payload); - toast.success('Payment updated'); + await finishPaymentSave('Payment updated'); } else { - await api.createPayment({ ...payload, bill_id: bill?.id }); - toast.success('Payment added'); + await createPaymentOrConfirm( + { ...payload, bill_id: bill?.id }, + () => finishPaymentSave('Payment added'), + ); } - resetPaymentForm(); - setPaymentFormOpen(false); - await loadPayments(); - onSave?.(); } catch (err) { toast.error(errMessage(err, 'Payment could not be saved.')); } finally { diff --git a/client/components/tracker/EditableCell.tsx b/client/components/tracker/EditableCell.tsx index 4a0d7a7..b77d3a2 100644 --- a/client/components/tracker/EditableCell.tsx +++ b/client/components/tracker/EditableCell.tsx @@ -1,6 +1,7 @@ import { useState, useRef } from 'react'; import { toast } from 'sonner'; import { api } from '@/api'; +import { createPaymentOrConfirm } from '@/lib/paymentActions'; import { cn, fmt, fmtDate, errMessage } from '@/lib/utils'; import { Input } from '@/components/ui/input'; import type { Dollars } from '@/lib/money'; @@ -47,15 +48,18 @@ export function EditableCell({ row, field, threshold, defaultPaymentDate, refres if (field === 'amount') update.amount = parseFloat(val); if (field === 'date') update.paid_date = val; await api.updatePayment(row.payments[0]!.id, update); + toast.success('Saved'); + refresh(); } else { - await api.createPayment({ - bill_id: row.id, - amount: field === 'amount' ? parseFloat(val) : threshold, - paid_date: field === 'date' ? val : defaultPaymentDate, - }); + await createPaymentOrConfirm( + { + bill_id: row.id, + amount: field === 'amount' ? parseFloat(val) : threshold, + paid_date: field === 'date' ? val : defaultPaymentDate, + }, + () => { toast.success('Saved'); refresh(); }, + ); } - toast.success('Saved'); - refresh(); } catch (err) { toast.error(errMessage(err)); } diff --git a/client/components/tracker/PaymentLedgerDialog.tsx b/client/components/tracker/PaymentLedgerDialog.tsx index 7391181..99626f8 100644 --- a/client/components/tracker/PaymentLedgerDialog.tsx +++ b/client/components/tracker/PaymentLedgerDialog.tsx @@ -1,7 +1,7 @@ import { useState } from 'react'; import { Plus } from 'lucide-react'; import { toast } from 'sonner'; -import { api } from '@/api'; +import { createPaymentOrConfirm } from '@/lib/paymentActions'; import { fmt, fmtDate, errMessage } from '@/lib/utils'; import { METHOD_NONE, paymentSummary } from '@/lib/trackerUtils'; import { Button } from '@/components/ui/button'; @@ -53,16 +53,16 @@ export function PaymentLedgerDialog({ row, year, month, threshold, defaultPaymen setBusy(true); try { - await api.createPayment({ - bill_id: row.id, - amount: parsedAmount, - paid_date: date, - method: method === METHOD_NONE ? null : method, - notes: notes || null, - }); - toast.success('Partial payment added'); - onSaved?.(); - onClose?.(); + await createPaymentOrConfirm( + { + bill_id: row.id, + amount: parsedAmount, + paid_date: date, + method: method === METHOD_NONE ? null : method, + notes: notes || null, + }, + () => { toast.success('Partial payment added'); onSaved?.(); onClose?.(); }, + ); } catch (err) { toast.error(errMessage(err, 'Failed to add payment')); } finally { diff --git a/client/lib/paymentActions.ts b/client/lib/paymentActions.ts new file mode 100644 index 0000000..4baacf6 --- /dev/null +++ b/client/lib/paymentActions.ts @@ -0,0 +1,43 @@ +import { api, type ApiError } from '@/api'; +import { toast } from 'sonner'; +import { errMessage } from '@/lib/utils'; + +/** + * Create a manual payment, treating the server's "duplicate suspected" (409) as a + * question rather than an error. The deliberate manual-add path must never + * *silently* drop a legitimately-identical second payment (same bill, date, + * amount) — losing a real payment is itself a money-integrity bug. So the server + * returns `409 DUPLICATE_SUSPECTED` instead of silently deduping, and here we + * surface an "add anyway?" toast; confirming resends with `allow_duplicate`. + * + * `onSuccess` runs after any *real* creation — immediate OR confirmed-retry — so + * callers put their toast/refresh/close there. Any non-duplicate error re-throws + * for the caller's existing `catch`. + */ +export async function createPaymentOrConfirm( + payload: Record, + onSuccess: () => void | Promise, +): Promise { + const create = async (allowDuplicate: boolean) => { + await api.createPayment(allowDuplicate ? { ...payload, allow_duplicate: true } : payload); + await onSuccess(); + }; + try { + await create(false); + } catch (err) { + const e = err as ApiError; + if (e?.status === 409 && e?.code === 'DUPLICATE_SUSPECTED') { + toast.warning('A matching payment already exists for this bill, date, and amount.', { + description: 'Add it anyway?', + action: { + label: 'Add anyway', + onClick: () => { + create(true).catch(retryErr => toast.error(errMessage(retryErr, 'Failed to add payment'))); + }, + }, + }); + return; // handled — not an error the caller should re-toast + } + throw err; + } +} diff --git a/routes/payments.js b/routes/payments.js index b022ee4..321710e 100644 --- a/routes/payments.js +++ b/routes/payments.js @@ -190,7 +190,7 @@ router.post('/:id/undo-auto', (req, res) => { // POST /api/payments — create single payment router.post('/', (req, res) => { const db = getDb(); - const { bill_id, amount, paid_date, method, notes, payment_source, autopay_failure } = req.body; + const { bill_id, amount, paid_date, method, notes, payment_source, autopay_failure, allow_duplicate } = req.body; const validation = validatePaymentInput({ bill_id, amount, paid_date, payment_source: payment_source ?? 'manual' }); if (validation.error) { @@ -202,6 +202,26 @@ router.post('/', (req, res) => { if (!bill) return res.status(404).json(standardizeError('Bill not found', 'NOT_FOUND', 'bill_id')); + // The deliberate manual add must not *silently* drop a same-key payment — a user + // may legitimately record two identical payments on the same day, and dropping + // one is itself a money-integrity bug. Surface a 409 the client turns into an + // "add anyway?" confirm; a resend with allow_duplicate bypasses this. (The + // automated one-click paths — /quick, /bulk, autopay-confirm — dedupe silently + // instead, because a repeat there is a retry, not a second intent.) + if (!allow_duplicate) { + const existingDuplicate = db.prepare( + `SELECT p.* FROM payments p + WHERE p.bill_id = ? AND p.paid_date = ? AND p.amount = ? AND p.${SQL_NOT_DELETED} + ORDER BY p.id DESC LIMIT 1` + ).get(bill.id, payment.paid_date, payment.amount); + if (existingDuplicate) { + return res.status(409).json({ + ...standardizeError('A matching payment already exists for this bill, date, and amount', 'DUPLICATE_SUSPECTED', 'amount'), + existing: serializePayment(existingDuplicate), + }); + } + } + const balCalc = computeBalanceDelta(bill, payment.amount); const failureFlag = autopay_failure ? 1 : 0; diff --git a/tests/paymentsRoute.test.js b/tests/paymentsRoute.test.js index 0678646..cab1532 100644 --- a/tests/paymentsRoute.test.js +++ b/tests/paymentsRoute.test.js @@ -98,3 +98,26 @@ test('PUT edit reverses the old amount and applies the new one atomically', asyn // reverse $100 (→ 1000.00) then apply $50 (→ 950.00) assert.equal(balanceOf(billId), 95000, '1000.00 − 50.00 = 950.00 (cents)'); }); + +test('manual POST of a same bill+date+amount is a suspected duplicate (409, no new row, balance unchanged)', async () => { + const before = balanceOf(billId); + const { status, data } = await call('post', '/', { + userId, body: { bill_id: billId, amount: 50, paid_date: '2026-07-01' }, + }); + assert.equal(status, 409); + assert.equal(data.code, 'DUPLICATE_SUSPECTED'); + assert.ok(data.existing?.id, 'returns the existing payment so the client can show it'); + assert.equal(balanceOf(billId), before, 'balance not touched by a rejected duplicate'); + const active = getDb().prepare('SELECT COUNT(*) c FROM payments WHERE bill_id = ? AND deleted_at IS NULL').get(billId).c; + assert.equal(active, 1, 'still one payment'); +}); + +test('manual POST with allow_duplicate bypasses the guard and records the second payment', async () => { + const { status } = await call('post', '/', { + userId, body: { bill_id: billId, amount: 50, paid_date: '2026-07-01', allow_duplicate: true }, + }); + assert.equal(status, 201); + assert.equal(balanceOf(billId), 90000, '950.00 − 50.00 = 900.00 (cents)'); + const active = getDb().prepare('SELECT COUNT(*) c FROM payments WHERE bill_id = ? AND deleted_at IS NULL').get(billId).c; + assert.equal(active, 2, 'a legitimately-identical second payment is recorded'); +});