transactions: let a gift card be a payment method you can pick
ci / lint-test (push) Successful in 43s
ci / lint-test (push) Successful in 43s
`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).
This commit is contained in:
@@ -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);
|
||||
});
|
||||
});
|
||||
@@ -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(
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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<number[]>([]);
|
||||
const [splits, setSplits] = useState<{ participant_id: number; share_percent: number }[]>(
|
||||
@@ -163,21 +169,23 @@ export function AddTransactionModal({
|
||||
</div>
|
||||
|
||||
<div>
|
||||
<label className="block text-xs text-zinc-500 mb-1">Paid by</label>
|
||||
{/* "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. */}
|
||||
<label className="block text-xs text-zinc-500 mb-1">Payment method</label>
|
||||
<select
|
||||
value={paymentMethod}
|
||||
onChange={(e) => setPaymentMethod(e.target.value)}
|
||||
className="w-full bg-zinc-800 border border-zinc-700 rounded px-2 py-1.5 text-sm"
|
||||
>
|
||||
<option value="">— unknown —</option>
|
||||
<option value="card">Card</option>
|
||||
<option value="cash">Cash</option>
|
||||
<option value="bank_transfer">Bank transfer</option>
|
||||
<option value="other">Other</option>
|
||||
{PAYMENT_METHODS.map((m) => (
|
||||
<option key={m} value={m}>{PAYMENT_METHOD_LABELS[m]}</option>
|
||||
))}
|
||||
</select>
|
||||
{paymentMethod === "cash" && (
|
||||
{NON_RECONCILING_REASON[paymentMethod] && (
|
||||
<p className="text-[11px] text-zinc-500 mt-1">
|
||||
Cash won't be offered for reconciliation — it never appears on a statement.
|
||||
{NON_RECONCILING_REASON[paymentMethod]}
|
||||
</p>
|
||||
)}
|
||||
</div>
|
||||
|
||||
@@ -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<number | null>(transaction.trip_id ?? null);
|
||||
const [ownerId, setOwnerId] = useState<number | null>(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({
|
||||
<InlineTags transactionId={transaction.id} initialTags={transaction.tags ?? []} />
|
||||
</div>
|
||||
|
||||
{/* 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 && (
|
||||
<div>
|
||||
<label className="block text-xs text-zinc-500 mb-1">Payment method</label>
|
||||
<select
|
||||
value={paymentMethod}
|
||||
onChange={(e) => setPaymentMethod(e.target.value)}
|
||||
className="w-full bg-zinc-800 border border-zinc-700 rounded px-2 py-1.5 text-sm"
|
||||
>
|
||||
<option value="">— unknown —</option>
|
||||
{PAYMENT_METHODS.map((m) => (
|
||||
<option key={m} value={m}>{PAYMENT_METHOD_LABELS[m]}</option>
|
||||
))}
|
||||
</select>
|
||||
{NON_RECONCILING_REASON[paymentMethod] && (
|
||||
<p className="text-[11px] text-zinc-500 mt-1">
|
||||
{NON_RECONCILING_REASON[paymentMethod]}
|
||||
</p>
|
||||
)}
|
||||
</div>
|
||||
)}
|
||||
|
||||
{/* Paid by — sits next to Splits deliberately: together they are
|
||||
whose money went out and whose share it was. */}
|
||||
<div>
|
||||
|
||||
@@ -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<PaymentMethod, string> = {
|
||||
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<string, string> = {
|
||||
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);
|
||||
}
|
||||
Reference in New Issue
Block a user