From ae0c34fce7ede8b8f4fdf1c8fa621c2bb5d3fc5b Mon Sep 17 00:00:00 2001 From: siddharthd Date: Mon, 27 Jul 2026 10:41:10 +1000 Subject: [PATCH] fix(orders): two defects the backfill exposed that tests could not MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both were found by looking at the data after the live backfill, not by the suite — 105 tests were green while 85 rows were invisible and 4 were double counted. owner_id was NULL on every ingested order. Analytics scope on COALESCE(t.owner_id, s.owner_id), and an ingested order carries no statement, so the coalesce resolved to NULL and matched no owner. The rows existed in `transactions` and appeared in no view in the app. Ingestion now sets DEFAULT_OWNER_ID, and a regression test asserts the row survives the same COALESCE scoping the UI uses. [Family] orders are card-settled, not credits-funded. Their receipts name the payer ("Payments Siddharth LKR 3,783.20") and no instrument, which an earlier version read as credits. The card statement carries all four of them (CBA ...3893, exact foreign_currency_amount matches), so creating a transaction duplicated spend already recorded — the double-count I5 exists to prevent. They now record provenance only; the statement line is the transaction and is what carries the `family` tag that keeps them out of budgets. Production data corrected separately by smarthome:docker/scripts/fix-order-backfill-2026-07-27.sql. Also: reconciliation tests no longer assert global row counts. reconcilePendingOrders() scans every pending row, so leftovers from other files moved the totals — the source of an intermittent failure that only appeared on the first run after a source edit. --- .../integration/order-ingestion.test.ts | 83 ++++++++++++------- src/__tests__/unit/order-ingestion.test.ts | 1 - src/lib/order-ingestion.ts | 23 +++-- src/lib/order-parse.ts | 27 ++---- 4 files changed, 78 insertions(+), 56 deletions(-) diff --git a/src/__tests__/integration/order-ingestion.test.ts b/src/__tests__/integration/order-ingestion.test.ts index 09a6c98..03713b7 100644 --- a/src/__tests__/integration/order-ingestion.test.ts +++ b/src/__tests__/integration/order-ingestion.test.ts @@ -9,7 +9,6 @@ import { reconcilePendingOrders, parseOrderAmendment, applyOrderAmendment, - OrderParseError, NotAReceiptError, type MessageMeta, } from "../../lib/order-ingestion"; @@ -181,14 +180,22 @@ describe("Order ingestion — invariants", () => { ).rejects.toThrow(); }); - it("I11: a [Family] order is imported, tagged, and excluded from spend", async () => { + it("I11: a [Family] order records provenance and creates no transaction", async () => { + // Requirement was "import them but tag so they're excluded from budgets". + // Correct mechanism: the CARD statement line is the transaction and carries + // the family tag. Creating a second, credits-flavoured row duplicated it. const p = parseOrderHTML( html("ue-04"), meta({ subject: "[Family] Your Tuesday evening order with Uber Eats", sender: "uber.com", receivedAt: "2026-07-07T10:08:00Z" }) ); const res = await processOrderIngestion(p); - expect(res.transactionId).not.toBeNull(); - expect(res.flags).toContain("family_payment_assumed_credits"); + expect(res.transactionId).toBeNull(); + expect(res.flags).toContain("family_card_settled_no_transaction"); + + const meta_ = await queryRow<{ currency: string }>( + `SELECT currency FROM expense_metadata WHERE id = $1`, [res.metadataId] + ); + expect(meta_!.currency).toBe("LKR"); }); it("a foreign-currency order records the original amount and code", async () => { @@ -215,6 +222,12 @@ describe("Order ingestion — invariants", () => { expect(res.transactionId).toBeNull(); expect(res.flags).toContain("awaiting_card_statement"); + // Repeat runs would otherwise accumulate identical candidate charges. + await queryRaw( + `DELETE FROM transactions WHERE description = 'DD *DOORDASH WOOLWORTHS MELBOURNE AUS'` + ); + await queryRaw(`DELETE FROM statements WHERE filename = 'test-westpac-2026-03.pdf'`); + // Statement arrives: card 8032 took 40.93 of the 60.93 order. const st = await queryRow<{ id: number }>( `INSERT INTO statements (bank_name, account_number, filename) @@ -227,8 +240,11 @@ describe("Order ingestion — invariants", () => { ); const out = await reconcilePendingOrders(); - expect(out.resolved).toBeGreaterThanOrEqual(1); - expect(out.created).toBeGreaterThanOrEqual(1); + // Deliberately not asserting global counts: reconcilePendingOrders() scans + // every pending row in the database, so another test's leftovers change the + // totals. Assert on THIS order's outcome instead — that is what the test is + // actually about, and it does not depend on what else is in the table. + expect(out.examined).toBeGreaterThanOrEqual(1); const credits = await queryRow<{ amount: string }>( `SELECT t.amount::text FROM transactions t @@ -240,9 +256,12 @@ describe("Order ingestion — invariants", () => { }); it("reconciliation is idempotent — a second pass creates nothing", async () => { - const before = await queryRow<{ c: string }>(`SELECT count(*)::text c FROM transactions`); + // A first pass has already run above; a second must add nothing. Scoped to + // 'Order - %' rows so unrelated fixtures cannot move the number. + const q = `SELECT count(*)::text c FROM transactions WHERE description LIKE 'Order - %'`; + const before = await queryRow<{ c: string }>(q); const out = await reconcilePendingOrders(); - const after = await queryRow<{ c: string }>(`SELECT count(*)::text c FROM transactions`); + const after = await queryRow<{ c: string }>(q); expect(out.created).toBe(0); expect(after!.c).toBe(before!.c); }); @@ -320,39 +339,43 @@ describe("Refund amendments", () => { }); }); -describe("[Family] orders import rather than park", () => { - it("records a family order as credits and tags it", async () => { +describe("[Family] orders are card-settled, not credits", () => { + it("creates provenance but NO transaction — the statement line is the transaction", async () => { + // Regression: these were assumed credits-funded because the receipt names + // the payer and no instrument. The card statement carries all four (CBA + // ...3893, exact LKR matches), so creating a transaction double-counted + // spend already recorded. const p = parseOrderHTML( html("ue-04"), meta({ subject: "[Family] Your Tuesday evening order with Uber Eats", sender: "uber.com", receivedAt: "2026-07-07T10:08:00Z" }) ); - expect(p.flags).toContain("family_payment_assumed_credits"); + expect(p.flags).toContain("family_card_settled_no_transaction"); + expect(p.payment.credits_amount).toBeNull(); const res = await processOrderIngestion(p); - expect(res.transactionId).not.toBeNull(); + expect(res.transactionId).toBeNull(); + expect(res.metadataId).not.toBeNull(); + }); +}); - const tag = await queryRow<{ name: string }>( - `SELECT tg.name FROM transaction_tags tt JOIN tags tg ON tg.id = tt.tag_id - WHERE tt.transaction_id = $1`, +describe("owner scoping", () => { + it("sets owner_id so the row is visible to the app", async () => { + // Regression: analytics scope on COALESCE(t.owner_id, s.owner_id). An + // ingested order has no statement, so a NULL owner_id made all 85 + // backfilled rows invisible in every view while sitting in the table. + const p = parseOrderHTML(html("dd-01"), meta({ messageId: `owner-${Date.now()}` })); + const res = await processOrderIngestion(p); + const row = await queryRow<{ owner_id: number | null }>( + `SELECT owner_id FROM transactions WHERE id = $1`, [res.transactionId] ); - expect(tag!.name).toBe("family"); + expect(row!.owner_id).not.toBeNull(); - // LKR is preserved, and amount_aud stays NULL — no FX rate is available. - const txn = await queryRow<{ foreign_currency_code: string; amount_aud: string | null }>( - `SELECT foreign_currency_code, amount_aud::text FROM transactions WHERE id = $1`, - [res.transactionId] - ); - expect(txn!.foreign_currency_code).toBe("LKR"); - expect(txn!.amount_aud).toBeNull(); - - // And it must not reach spend. const visible = await queryRaw( - `SELECT t.id FROM transactions t - LEFT JOIN transaction_overrides o ON o.transaction_id = t.id - WHERE t.id = $1 AND (${EXCLUDE_NON_SPEND})`, - [res.transactionId] + `SELECT t.id FROM transactions t LEFT JOIN statements s ON s.id = t.statement_id + WHERE t.id = $1 AND COALESCE(t.owner_id, s.owner_id) = $2`, + [res.transactionId, row!.owner_id] ); - expect(visible).toHaveLength(0); + expect(visible).toHaveLength(1); }); }); diff --git a/src/__tests__/unit/order-ingestion.test.ts b/src/__tests__/unit/order-ingestion.test.ts index a617d9a..6fc0e39 100644 --- a/src/__tests__/unit/order-ingestion.test.ts +++ b/src/__tests__/unit/order-ingestion.test.ts @@ -5,7 +5,6 @@ import { parseOrderHTML, validateOrderTotals, resolveCategory, - OrderParseError, NotAReceiptError, type MessageMeta, type ParsedOrder, diff --git a/src/lib/order-ingestion.ts b/src/lib/order-ingestion.ts index 3b2267f..947656f 100644 --- a/src/lib/order-ingestion.ts +++ b/src/lib/order-ingestion.ts @@ -5,6 +5,13 @@ export * from "./order-parse"; export const CUTOVER_DATE = "2026-01-09"; +/** + * Owner for ingested orders. Analytics scope on COALESCE(t.owner_id, + * s.owner_id); an ingested order has no statement, so leaving owner_id NULL + * hides it from every view in the app while it sits in the table. + */ +export const DEFAULT_OWNER_ID = 1; + export interface IngestResult { transactionId: number | null; metadataId: number | null; @@ -86,7 +93,7 @@ async function ensureTag(name: string): Promise { */ export async function processOrderIngestion( order: ParsedOrder, - options: { messageId?: string; backfillMode?: boolean } = {} + options: { messageId?: string; backfillMode?: boolean; ownerId?: number } = {} ): Promise { const flags = [...order.flags]; const day = order.order_datetime.slice(0, 10); @@ -108,7 +115,6 @@ export async function processOrderIngestion( // ---- resolve the credits portion ---------------------------------------- let creditsAmount: number | null = null; - let cardAmount: number | null = order.payment.card_amount; if (order.payment.ambiguous) { const { cardAmount: reconciled } = await reconcileCardLeg(order); @@ -122,9 +128,7 @@ export async function processOrderIngestion( // resolves it once the statement lands. Backfill hits the same path and // resolves immediately, because those statements are already imported. flags.push("awaiting_card_statement"); - cardAmount = null; } else { - cardAmount = reconciled; const remainder = Number((order.totals.total_charged - reconciled).toFixed(2)); if (remainder > 0.02) { creditsAmount = remainder; @@ -151,7 +155,7 @@ export async function processOrderIngestion( transaction_date, description, amount, amount_aud, category, payment_method, merchant_name, merchant_normalized, transaction_type, foreign_currency_amount, foreign_currency_code, owner_id - ) VALUES ($1,$2,$3,$4,$5,'credits',$6,$6,'debit',$7,$8,NULL) + ) VALUES ($1,$2,$3,$4,$5,'credits',$6,$6,'debit',$7,$8,$9) RETURNING id`, [ day, @@ -164,6 +168,11 @@ export async function processOrderIngestion( order.merchant_name, isAud ? null : creditsAmount, isAud ? null : order.currency, + // Owner scoping is COALESCE(t.owner_id, s.owner_id). These rows carry + // no statement, so a NULL owner_id makes them invisible in every view + // in the app — present in the table, absent from the UI. The backfill + // inserted 85 rows nobody could see. + options.ownerId ?? DEFAULT_OWNER_ID, ] ); transactionId = txn!.id; @@ -280,9 +289,9 @@ export async function reconcilePendingOrders(): Promise<{ `INSERT INTO transactions ( transaction_date, description, amount, amount_aud, category, payment_method, merchant_name, merchant_normalized, transaction_type, owner_id - ) VALUES ($1,$2,$3,$3,$5,'credits',$4,$4,'debit',NULL) + ) VALUES ($1,$2,$3,$3,$5,'credits',$4,$4,'debit',$6) RETURNING id`, - [row.transaction_date, `Order - ${row.merchant_normalized}`, remainder, row.merchant_normalized, category] + [row.transaction_date, `Order - ${row.merchant_normalized}`, remainder, row.merchant_normalized, category, DEFAULT_OWNER_ID] ); txnId = txn!.id; created++; diff --git a/src/lib/order-parse.ts b/src/lib/order-parse.ts index dfd9c8d..e2b6d65 100644 --- a/src/lib/order-parse.ts +++ b/src/lib/order-parse.ts @@ -98,15 +98,6 @@ const decodeEntities = (s: string) => const collapse = (s: string) => s.replace(/\s+/g, " ").trim(); -/** URL-decodes without throwing on malformed percent-escapes. */ -function safeDecode(s: string): string { - try { - return decodeURIComponent(s.replace(/%(?![0-9a-f]{2})/gi, "%25")); - } catch { - return s; - } -} - const money = (raw: string): number => Math.abs(parseFloat(raw.replace(/[$,]/g, ""))); /** @@ -181,7 +172,6 @@ function parseMerchant(platform: string, meta: MessageMeta, text: string): strin } function parsePayment(platform: string, html: string, text: string): PaymentBreakdown { - // eslint-disable-next-line no-param-reassign const out: PaymentBreakdown = { credits_amount: null, card_amount: null, @@ -385,14 +375,15 @@ export function parseOrderHTML(html: string, meta: MessageMeta): ParsedOrder { const payment = parsePayment(platform, clean, text); if (payment.ambiguous && is_family) { // [Family] receipts name the payer, not an instrument ("Payments Siddharth - // LKR 3,783.20"), so no split is recoverable and there is no card leg to - // reconcile against — parking them would mean never importing them, which - // fails the actual requirement (import, tag, exclude from budgets). - // Treated as credits so the order is recorded and tagged. Safe because the - // family tag removes it from every budget regardless of instrument. - payment.ambiguous = false; - payment.credits_amount = totals.total_charged; - flags.push("family_payment_assumed_credits"); + // LKR 3,783.20"). An earlier version read that as credits-funded. It is + // not: the card statement carries all four of them (CBA ...3893, exact + // foreign_currency_amount matches), so creating a transaction duplicated + // spend that was already recorded — precisely the double-count I5 exists to + // prevent. + // + // Treated as card-settled: provenance only, no transaction. The statement + // line IS the transaction, and it is what should carry the `family` tag. + flags.push("family_card_settled_no_transaction"); } else if (payment.ambiguous) { // Split not stated and resolvable from the card statement — left for the // ingestion runner to reconcile, not guessed here.