From 9f1be7433017ced6da58c13d8b9499a1335541fc Mon Sep 17 00:00:00 2001 From: siddharthd Date: Mon, 17 Aug 2026 18:14:58 +1000 Subject: [PATCH] transactions: let a gift card be a payment method you can pick MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `credits` has been a valid payment_method since migration 0018, which added it so the order pipeline could file gift-card-funded orders. It reached the database, `needsCardMatch()` (which holds those rows out of reconciliation) and `bankLabel()` (which renders them "Gift Card"). It never reached the two places a human touches it. So entering a gift-card purchase by hand was impossible: the dropdown stopped at "Other", the row took the NULL default, and NULL is *reconcilable* — it joined the pending queue to be offered card matches within 3 days and 1% that cannot exist, because the balance already paid the bill. Accepting one is the destructive case the cash exclusion documents: reconciled manual rows are filtered out of every query, so the spend vanishes while an unrelated card row claims to be it. - `src/lib/payment-methods.ts` — one canonical list, mirroring the `statement-types.ts` pattern, plus the TS mirror of `needsCardMatch()`. - `PATCH /api/transactions/[id]` — the `VALID_METHODS` literal omitted `credits`, so the API answered 400 for a value the CHECK constraint admits and 372 live rows already carry. It validates against the list now. - `POST /api/transactions` — validated at all, so a bad value is a 400 naming the field rather than a 500 carrying a Postgres error. - Add modal — offers every method, and explains the consequence when you pick one that suppresses reconciliation. Relabelled "Paid by" → "Payment method", which the edit modal uses for the *owner*; they are different questions. - Edit modal — had no payment-method field at all, so an existing row wrongly in the queue could not be corrected. Manual rows only: the queue is `statement_id IS NULL`, so it decides nothing on a statement row. The test asserts the list against `pg_get_constraintdef` and evaluates `needsCardMatch()` as real SQL rather than re-implementing it, so the next migration that widens the constraint fails the suite instead of drifting for two months. Mutation-tested by dropping `credits`. Unit 130 pass, integration 206 pass (the integration DB needed `npm run test:setup` first — `.env.test` still named a stale container IP). --- CLAUDE.md | 38 +++++++++-- .../integration/payment-methods.test.ts | 63 +++++++++++++++++++ src/app/api/transactions/[id]/route.ts | 8 ++- src/app/api/transactions/route.ts | 7 +++ src/components/add-transaction-modal.tsx | 24 ++++--- src/components/edit-transaction-modal.tsx | 36 +++++++++++ src/lib/payment-methods.ts | 56 +++++++++++++++++ 7 files changed, 217 insertions(+), 15 deletions(-) create mode 100644 src/__tests__/integration/payment-methods.test.ts create mode 100644 src/lib/payment-methods.ts diff --git a/CLAUDE.md b/CLAUDE.md index 1af8267..c95d318 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -131,21 +131,41 @@ docker exec postgres-personal psql -U personal -d personal < prisma/migrations/< ### Key Tables - `statements` — one row per billing period per bank account -- `transactions` — line items; `statement_id` is nullable (NULL = manual entry); `reconciled_with_id` links a manual tx to its matched statement tx; `payment_method` (migration 0016) is `card | cash | bank_transfer | other`, NULL = unknown; `source` / `source_ref` / `source_account` (migration 0028) identify a row that came from an account feed rather than a statement or a hand entry — `source_ref` is the provider's own id and carries a partial unique index, which is the only thing making a re-import idempotent +- `transactions` — line items; `statement_id` is nullable (NULL = manual entry); `reconciled_with_id` links a manual tx to its matched statement tx; `payment_method` (migration 0016, `credits` added by 0018) is `card | cash | bank_transfer | credits | other`, NULL = unknown, and the TypeScript mirror is `src/lib/payment-methods.ts`; `source` / `source_ref` / `source_account` (migration 0028) identify a row that came from an account feed rather than a statement or a hand entry — `source_ref` is the provider's own id and carries a partial unique index, which is the only thing making a re-import idempotent ### Cash and reconciliation -`payment_method = 'cash'` excludes a transaction from reconciliation via the -`notCash()` fragment in `queries.ts`. Cash never appears on a statement, so +`payment_method` in (`cash`, `credits`) excludes a transaction from +reconciliation via the `needsCardMatch()` fragment in `queries.ts` (named +`notCash()` when it only knew about cash). Cash never appears on a statement, so without it a cash entry sits in the pending queue forever being offered matches within 3 days and 1% on amount — and accepting one is silently destructive: reconciled manual rows are filtered out of every query, so the cash spend disappears while the card transaction it matched claims to be that same spend. -Only cash is excluded on payment method. Bank transfers *do* appear on a +`credits` is the same shape for a different reason: a gift card or store balance +already paid the bill, so no card leg is ever coming. `bankLabel()` therefore +renders those rows **"Gift Card"** rather than "Manual" — "Manual" reads as +"still awaiting a card line", which is wrong here. + +Only those two are excluded on payment method. Bank transfers *do* appear on a statement now that transaction accounts are imported, and NULL means unknown — both stay candidates, preserving the behaviour of every pre-existing row. +**The vocabulary lives in three places and drifted between them (fixed +2026-08-17).** Migration 0018 admitted `credits` to the CHECK constraint for the +order pipeline; the `VALID_METHODS` literal in `PATCH /api/transactions/[id]` +and the add-transaction dropdown were never updated. So the database held 372 +gift-card rows carrying a value the UI could not offer and the API answered 400 +for — a hand-entered gift-card purchase silently took the NULL default, and NULL +*is* reconcilable, so it joined the queue to be offered matches that cannot +exist. `src/lib/payment-methods.ts` is now the single list (mirroring +`statement-types.ts`), and both routes validate against it. +`src/__tests__/integration/payment-methods.test.ts` asserts it against +`pg_get_constraintdef` and evaluates `needsCardMatch()` in real SQL, so the next +migration that widens the constraint fails the suite instead of drifting +quietly. + **Account-feed rows are NOT excluded, and the removed exclusion is worth knowing about.** There was an `awaitsStatementLine()` predicate here, added with the Frollo importer on the premise that a feed row is the account's own ledger @@ -1112,7 +1132,15 @@ See `README.md` → **Known Gaps / TODOs** for full details. drone, camera). Correct in that they are excluded from spend, but it mixes asset disposals into the income line alongside salary. - **`payment_method` is not shown in the transactions list** — settable on create - and edit only. Worth a column or filter if cash becomes routine. + and edit only. Worth a column or filter if cash becomes routine. A `credits` + row is at least visible indirectly: `bankLabel()` renders it "Gift Card" + rather than "Manual", and the bank filter has a chip for it. +- **219 manual rows carry a NULL `payment_method`**, so they sit in the pending + queue being offered card matches. NULL means *unknown*, not *unreconcilable* — + that is deliberate — but any of them paid by gift card or cash is being + offered a match that cannot exist. No bulk fix, because guessing the method + from a description would be a guess written into a column that suppresses + reconciliation. - **Pantry receipts still land as yours.** `processReceiptIngestion` hardcodes `DEFAULT_OWNER_ID`; the ingest route takes no owner. Correctable per row now (see "Changing who paid"), but a "Paid by" step at capture time would stop the diff --git a/src/__tests__/integration/payment-methods.test.ts b/src/__tests__/integration/payment-methods.test.ts new file mode 100644 index 0000000..ded52da --- /dev/null +++ b/src/__tests__/integration/payment-methods.test.ts @@ -0,0 +1,63 @@ +import { describe, it, expect } from "vitest"; +import { queryRaw, queryRow } from "../../lib/db"; +import { needsCardMatch } from "../../lib/queries"; +import { + PAYMENT_METHODS, + NON_RECONCILING, + reconcilesAgainstStatement, +} from "../../lib/payment-methods"; + +/** + * These two lists drifted, and the drift was silent for two months. + * + * Migration 0018 admitted 'credits' to `transactions_payment_method_check` so + * the order pipeline could file gift-card-funded orders. Nothing propagated it + * to the hand-written allowlist in `PATCH /api/transactions/[id]` or to the + * add-transaction dropdown, so the database held 372 rows carrying a value the + * UI could not enter and the API refused to set. + * + * The failure mode is invisible on screen — a gift-card purchase entered by + * hand takes the NULL default, and NULL is *reconcilable*, so it joins the + * pending queue and is offered card matches that cannot exist. Accepting one is + * destructive: reconciled manual rows are filtered out of every query, so the + * spend vanishes while an unrelated card row claims to be it. + * + * So: assert against the database, not against a second copy of the list. + */ +describe("payment methods mirror the database", () => { + it("PAYMENT_METHODS equals the CHECK constraint's vocabulary", async () => { + const row = await queryRow<{ def: string }>( + `SELECT pg_get_constraintdef(oid) AS def FROM pg_constraint + WHERE conname = 'transactions_payment_method_check'` + ); + expect(row, "constraint transactions_payment_method_check is missing").not.toBeNull(); + + const inConstraint = [...row!.def.matchAll(/'([a-z_]+)'::text/g)].map((m) => m[1]); + expect(inConstraint.length).toBeGreaterThan(0); // guard against the regex silently matching nothing + expect([...inConstraint].sort()).toEqual([...PAYMENT_METHODS].sort()); + }); + + it("reconcilesAgainstStatement() agrees with needsCardMatch() for every method", async () => { + // Evaluate the real SQL predicate rather than re-implementing it: this is + // the TypeScript mirror, so the SQL is the thing being mirrored. + const rows = await queryRaw<{ pm: string | null; sql_reconciles: boolean }>( + `SELECT v.payment_method AS pm, ${needsCardMatch("v")} AS sql_reconciles + FROM (SELECT unnest($1::text[]) AS payment_method) v`, + [[...PAYMENT_METHODS, null]] + ); + expect(rows).toHaveLength(PAYMENT_METHODS.length + 1); + + for (const { pm, sql_reconciles } of rows) { + expect(reconcilesAgainstStatement(pm), `payment_method = ${pm ?? "NULL"}`) + .toBe(sql_reconciles); + } + }); + + it("holds cash and credits out of the queue, and nothing else", () => { + expect([...NON_RECONCILING].sort()).toEqual(["cash", "credits"]); + // NULL means *unknown*, never "no card leg is coming" — it must stay a + // candidate or every pre-existing row would drop out of reconciliation. + expect(reconcilesAgainstStatement(null)).toBe(true); + expect(reconcilesAgainstStatement(undefined)).toBe(true); + }); +}); diff --git a/src/app/api/transactions/[id]/route.ts b/src/app/api/transactions/[id]/route.ts index 91aabec..ef02621 100644 --- a/src/app/api/transactions/[id]/route.ts +++ b/src/app/api/transactions/[id]/route.ts @@ -3,6 +3,7 @@ import { getTransactionById, canAccessTransactions } from "@/lib/queries"; import { getCurrentUser } from "@/lib/auth"; import { prisma } from "@/lib/db"; import { queryRaw } from "@/lib/db"; +import { isPaymentMethod } from "@/lib/payment-methods"; const VALID_TYPES = ["debit", "credit", "payment", "refund", "fee", "interest", "transfer"]; @@ -90,9 +91,12 @@ export async function PATCH( // payment_method is a property of the transaction itself, not a user override // of extracted data, so it lives on the transactions table. + // + // The allowlist is PAYMENT_METHODS, not a local literal: this one omitted + // 'credits' from migration 0018 until 2026-08-17, so the DB admitted a + // gift-card row that this route then refused to set or correct. if (payment_method !== undefined) { - const VALID_METHODS = ["card", "cash", "bank_transfer", "other"]; - if (payment_method !== null && !VALID_METHODS.includes(payment_method)) { + if (payment_method !== null && !isPaymentMethod(payment_method)) { return NextResponse.json({ error: "Invalid payment_method" }, { status: 400 }); } await queryRaw( diff --git a/src/app/api/transactions/route.ts b/src/app/api/transactions/route.ts index 5e7bf64..ac6c725 100644 --- a/src/app/api/transactions/route.ts +++ b/src/app/api/transactions/route.ts @@ -3,6 +3,7 @@ import { getCurrentUser } from "@/lib/auth"; import { getTransactions } from "@/lib/queries"; import { queryRaw } from "@/lib/db"; import { completeSplit } from "@/lib/splits"; +import { isPaymentMethod } from "@/lib/payment-methods"; export async function GET(req: NextRequest) { const user = await getCurrentUser(req); @@ -53,6 +54,12 @@ export async function POST(req: NextRequest) { return NextResponse.json({ error: "date, description, amount are required" }, { status: 400 }); } + // Validated here rather than left to the CHECK constraint, which surfaces as + // a 500 with a Postgres error rather than a 400 naming the field. + if (body.payment_method && !isPaymentMethod(body.payment_method)) { + return NextResponse.json({ error: "Invalid payment_method" }, { status: 400 }); + } + // Insert manual transaction with no statement (statement_id = NULL, owner_id set directly) const txRows = await queryRaw<{ id: number }>( `INSERT INTO transactions (statement_id, owner_id, transaction_date, description, amount, transaction_type, merchant_normalized, category, payment_method, row_index) diff --git a/src/components/add-transaction-modal.tsx b/src/components/add-transaction-modal.tsx index 60afaff..bc4a956 100644 --- a/src/components/add-transaction-modal.tsx +++ b/src/components/add-transaction-modal.tsx @@ -3,6 +3,11 @@ import { useState } from "react"; import { useCreateTransaction, useParticipants, useTags } from "@/lib/hooks"; import { CATEGORIES, formatCategory } from "@/lib/categories"; +import { + PAYMENT_METHODS, + PAYMENT_METHOD_LABELS, + NON_RECONCILING_REASON, +} from "@/lib/payment-methods"; const TRANSACTION_TYPES = ["debit", "credit", "payment", "refund", "fee", "interest", "transfer"]; @@ -35,7 +40,8 @@ export function AddTransactionModal({ const [type, setType] = useState(prefill?.transaction_type ?? "debit"); const [merchant, setMerchant] = useState(prefill?.merchant_normalized ?? ""); const [category, setCategory] = useState(prefill?.category ?? ""); - // Cash is excluded from reconciliation — it will never appear on a statement. + // Cash and gift-card credits are excluded from reconciliation — neither will + // ever produce a card line. See needsCardMatch() in queries.ts. const [paymentMethod, setPaymentMethod] = useState(""); const [selectedTagIds, setSelectedTagIds] = useState([]); const [splits, setSplits] = useState<{ participant_id: number; share_percent: number }[]>( @@ -163,21 +169,23 @@ export function AddTransactionModal({
- + {/* "Payment method", not "Paid by" — the edit modal uses that label + for the owner, and they are different questions: which instrument + the money left by, versus whose money it was. */} + - {paymentMethod === "cash" && ( + {NON_RECONCILING_REASON[paymentMethod] && (

- Cash won't be offered for reconciliation — it never appears on a statement. + {NON_RECONCILING_REASON[paymentMethod]}

)}
diff --git a/src/components/edit-transaction-modal.tsx b/src/components/edit-transaction-modal.tsx index 5fab619..3035544 100644 --- a/src/components/edit-transaction-modal.tsx +++ b/src/components/edit-transaction-modal.tsx @@ -14,6 +14,11 @@ import { import { SplitModal } from "./split-modal"; import { OrderDetails } from "./order-details"; import { CATEGORIES, formatCategory } from "@/lib/categories"; +import { + PAYMENT_METHODS, + PAYMENT_METHOD_LABELS, + NON_RECONCILING_REASON, +} from "@/lib/payment-methods"; import type { TransactionRow, TagRow } from "@/lib/queries"; const TRANSACTION_TYPES = ["debit", "credit", "payment", "refund", "fee", "interest", "transfer"]; @@ -111,6 +116,9 @@ export function EditTransactionModal({ const [date, setDate] = useState(transaction.transaction_date?.slice(0, 10) ?? ""); const [description, setDescription] = useState(transaction.description); const [amount, setAmount] = useState(String(transaction.amount)); + // Manual-only, because the reconciliation queue is `statement_id IS NULL`: + // this is the only place payment_method changes what happens to a row. + const [paymentMethod, setPaymentMethod] = useState(transaction.payment_method ?? ""); const [tripId, setTripId] = useState(transaction.trip_id ?? null); const [ownerId, setOwnerId] = useState(transaction.owner_id ?? null); @@ -153,6 +161,10 @@ export function EditTransactionModal({ patch.description = description; if (parseFloat(amount) !== transaction.amount) patch.amount = parseFloat(amount); + // "" means unknown, which is NULL — not the empty string, which the + // CHECK constraint would reject. + if (paymentMethod !== (transaction.payment_method ?? "")) + patch.payment_method = paymentMethod || null; } if (tripId !== (transaction.trip_id ?? null)) @@ -307,6 +319,30 @@ export function EditTransactionModal({ + {/* Payment method — manual rows only. A statement row is already on + a statement, so the field decides nothing there; on a manual row + it decides whether reconciliation will ever offer it a match. */} + {isManual && ( +
+ + + {NON_RECONCILING_REASON[paymentMethod] && ( +

+ {NON_RECONCILING_REASON[paymentMethod]} +

+ )} +
+ )} + {/* Paid by — sits next to Splits deliberately: together they are whose money went out and whose share it was. */}
diff --git a/src/lib/payment-methods.ts b/src/lib/payment-methods.ts new file mode 100644 index 0000000..644e2c8 --- /dev/null +++ b/src/lib/payment-methods.ts @@ -0,0 +1,56 @@ +// Canonical payment methods. Mirrors `transactions_payment_method_check` +// (migration 0018, which admitted 'credits') — keep them in sync. +// +// This list exists because the drift already happened: the CHECK constraint +// admitted 'credits' from 0018 onward while the PATCH route's hand-written +// allowlist and the add-transaction dropdown both still stopped at 'other'. +// A gift-card row could be written by the order pipeline but neither entered +// nor corrected by hand. +export const PAYMENT_METHODS = [ + "card", + "cash", + "bank_transfer", + "credits", + "other", +] as const; + +export type PaymentMethod = (typeof PAYMENT_METHODS)[number]; + +export const PAYMENT_METHOD_LABELS: Record = { + card: "Card", + cash: "Cash", + bank_transfer: "Bank transfer", + credits: "Gift card / credits", + other: "Other", +}; + +export function isPaymentMethod(v: string | null | undefined): v is PaymentMethod { + return !!v && (PAYMENT_METHODS as readonly string[]).includes(v); +} + +export function formatPaymentMethod(v: string | null | undefined): string { + return isPaymentMethod(v) ? PAYMENT_METHOD_LABELS[v] : "Unknown"; +} + +/** + * Methods held out of the reconciliation queue. The TypeScript mirror of + * `needsCardMatch()` in `queries.ts` — if one changes, change both. + * + * NULL means *unknown*, not "no card leg", so it stays a candidate. + */ +export const NON_RECONCILING: readonly PaymentMethod[] = ["cash", "credits"]; + +/** + * Why a method never reaches the queue. Shown at the point of choosing it, + * because the consequence is invisible afterwards: the row simply never + * appears in pending reconciliation, which reads identically to a bug. + */ +export const NON_RECONCILING_REASON: Record = { + cash: "Cash won't be offered for reconciliation — it never appears on a statement.", + credits: + "A gift-card payment won't be offered for reconciliation — the balance already paid it, so no card line is coming. It shows as “Gift Card” rather than “Manual” in the bank column.", +}; + +export function reconcilesAgainstStatement(v: string | null | undefined): boolean { + return !isPaymentMethod(v) || !NON_RECONCILING.includes(v); +}