diff --git a/src/app/(app)/invoicing/page.tsx b/src/app/(app)/invoicing/page.tsx index d47cdab..913f623 100644 --- a/src/app/(app)/invoicing/page.tsx +++ b/src/app/(app)/invoicing/page.tsx @@ -45,6 +45,12 @@ type RawInvoicingRow = { description?: string; item_status?: string; qb_invoice_pdf?: string | null; + /** + * What the CUSTOMER ordered. This — not `order_qty` — is what goes on + * the invoice and the delivery note. See `src/lib/order-item-qty.ts`. + */ + quantity?: number | null; + /** What we ordered from the SUPPLIER. Drives the PO, never the invoice. */ order_qty?: number | null; qty_collected?: number | null; cost_price?: number | null; @@ -58,6 +64,15 @@ type RawInvoicingRow = { */ delivery_note_number?: string | null; order_item_details?: string | Record | null; + /** + * When true this row never reaches a customer-facing document. + * Carried into `InvoicingRow.excludeFromInvoice` (see + * billableRows / buildQbInvoicePayload in view-model.ts). The PB + * projection below MUST include this field — without it, every + * extra silently bills the customer. See the + * non-invoiceable-order-extras design. + */ + exclude_from_invoice?: boolean; expand?: { purchase_order?: { id: string; @@ -81,11 +96,16 @@ const INVOICING_FIELDS = [ "description", "item_status", "qb_invoice_pdf", + // `quantity` is load-bearing: it is the customer-facing qty the + // invoice bills. Drop it from this projection and PB returns it as + // undefined, silently falling back to the supplier's `order_qty`. + "quantity", "order_qty", "qty_collected", "cost_price", "selling_price", "delivery_note_number", + "exclude_from_invoice", "order_item_details", "order", "purchase_order", @@ -303,6 +323,7 @@ function toInvoicingRow(row: RawInvoicingRow): InvoicingRow { fileName: row.qb_invoice_pdf ?? null, pocketbaseBaseUrl: POCKETBASE_FILE_BASE_URL, }), + quantity: row.quantity ?? null, orderQty: row.order_qty ?? null, qtyCollected: row.qty_collected ?? null, costPrice: row.cost_price ?? null, @@ -314,6 +335,13 @@ function toInvoicingRow(row: RawInvoicingRow): InvoicingRow { printedBy: print.printedBy, printedByName: print.printedByName, printCount: print.printCount, + // Carries the raw PB bool through to the typed InvoicingRow + // so `billableRows` / `buildQbInvoicePayload` can drop it from + // the customer-facing line list. The projection in + // INVOICING_FIELDS above must include this field — without it, + // PB returns undefined and every excluded row silently bills + // the customer. See the non-invoiceable-order-extras design. + excludeFromInvoice: row.exclude_from_invoice === true, }; } diff --git a/src/app/(app)/invoicing/view-model.test.ts b/src/app/(app)/invoicing/view-model.test.ts index 9a95ead..0f5bc42 100644 --- a/src/app/(app)/invoicing/view-model.test.ts +++ b/src/app/(app)/invoicing/view-model.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from "vitest"; import { + billableRows, buildInvoicePrintGroups, buildOrderInvoiceCards, buildQbInvoicePayload, @@ -419,6 +420,93 @@ describe("buildQbInvoicePayload", () => { ], }); }); + + // Task 5 of the non-invoiceable-order-extras plan. The split + // between `order_item_ids` (all rows, for n8n's status flip) + // and `items` (billable rows only, for the QuickBooks invoice) + // is the load-bearing piece of the whole feature. + + it("mixed set: keeps excluded ids in order_item_ids but drops them from items", () => { + const payload = buildQbInvoicePayload({ + orderNumber: "SO-1", + billingCompany: "CNL", + invoiceCompany: "Adal Engineering", + customer: { id: "qb-1", name: "Acme" }, + rows: [ + makeRow({ description: "Bolt" }), + makeRow({ id: "row-2", description: "Nut", excludeFromInvoice: true }), + makeRow({ id: "row-3", description: "Washer", excludeFromInvoice: true }), + ], + }); + + // All three ids ride along for the n8n status flip — the row + // gets flipped to invoiced alongside its siblings so the order + // closes normally. + expect(payload.order_item_ids).toEqual(["row-1", "row-2", "row-3"]); + + // Only the billable line reaches QuickBooks. + expect(payload.items).toEqual([ + { name: "Bolt", qty: 1, price: 0 }, + ]); + }); + + it("all-billable set: identical output to the no-flag case (regression)", () => { + // Passing rows where `excludeFromInvoice` is explicitly false + // must produce the same payload as the original test above — + // proves the new split doesn't accidentally drop billable rows. + const payload = buildQbInvoicePayload({ + orderNumber: "SO-1", + billingCompany: "CNL", + invoiceCompany: "Adal Engineering", + customer: { id: "qb-1", name: "Acme" }, + rows: [ + makeRow({ description: "Bolt", excludeFromInvoice: false }), + makeRow({ id: "row-2", description: "Nut", excludeFromInvoice: false }), + ], + }); + + expect(payload.order_item_ids).toEqual(["row-1", "row-2"]); + expect(payload.items).toEqual([ + { name: "Bolt", qty: 1, price: 0 }, + { name: "Nut", qty: 1, price: 0 }, + ]); + }); + + it("all-excluded set: empty items array so the UI guard (Task 6) has something to detect", () => { + const payload = buildQbInvoicePayload({ + orderNumber: "SO-1", + billingCompany: "CNL", + invoiceCompany: "Adal Engineering", + customer: { id: "qb-1", name: "Acme" }, + rows: [ + makeRow({ description: "Bolt", excludeFromInvoice: true }), + makeRow({ id: "row-2", description: "Nut", excludeFromInvoice: true }), + ], + }); + + // All rows ride along for the status flip... + expect(payload.order_item_ids).toEqual(["row-1", "row-2"]); + // ...but the invoice line list is empty — there's nothing to + // bill the customer for. + expect(payload.items).toEqual([]); + }); +}); + +describe("billableRows", () => { + it("filters out rows with excludeFromInvoice: true", () => { + const result = billableRows([ + { id: "a" }, + { id: "b", excludeFromInvoice: true }, + { id: "c", excludeFromInvoice: false }, + { id: "d" }, + ]); + expect(result.map((r) => r.id)).toEqual(["a", "c", "d"]); + }); + + it("treats undefined excludeFromInvoice as billable", () => { + const result = billableRows([{ id: "a" }, { id: "b", excludeFromInvoice: undefined }]); + expect(result).toHaveLength(2); + }); }); describe("pickClosestCustomer", () => { @@ -478,52 +566,68 @@ describe("resolveInvoicePdfUrl", () => { }); describe("resolveInvoiceLineDefaults", () => { - it("uses the customer order qty, not the supplier-collected qty (MOQ overflow scenario)", () => { - // Customer ordered 4, supplier MOQ forced an order of 10, supplier - // delivered all 10. The invoice must reflect 4 — only the customer - // qty goes on the QuickBooks payload. The remaining 6 is routed to - // stock via /api/v1/stock/surplus-book (see surplusQty below). - const row = makeRow({ orderQty: 4, qtyCollected: 10 }); + // `quantity` is what the customer ordered; `orderQty` is what we bought + // from the supplier. These tests previously conflated the two — see + // docs/superpowers/specs/2026-08-30-quantity-handling-audit.md. + + it("bills the customer quantity, not the larger supplier MOQ quantity", () => { + // Customer ordered 4. Supplier MOQ forced us to buy 10, and all 10 + // arrived. The invoice must say 4 — the other 6 goes to stock via + // /api/v1/stock/surplus-book (see surplusQty below). + const row = makeRow({ quantity: 4, orderQty: 10, qtyCollected: 10 }); expect(resolveInvoiceLineDefaults(row).qty).toBe(4); }); - it("falls back to qtyCollected when there is no customer orderQty (legacy rows)", () => { - const row = makeRow({ orderQty: null, qtyCollected: 6 }); + it("bills the customer quantity, not the smaller supplier qty on a shorted line", () => { + // Customer ordered 100; supplier A could only cover 40. Until the + // operator splits the remaining 60 onto a second line, the customer + // still ordered 100 — billing 40 would silently lose the rest. + const row = makeRow({ quantity: 100, orderQty: 40, qtyCollected: 40 }); + expect(resolveInvoiceLineDefaults(row).qty).toBe(100); + }); + + it("never bills the collected qty", () => { + const row = makeRow({ quantity: 4, orderQty: 4, qtyCollected: 10 }); + expect(resolveInvoiceLineDefaults(row).qty).toBe(4); + }); + + it("falls back to orderQty when there is no quantity (legacy rows)", () => { + const row = makeRow({ quantity: null, orderQty: 6, qtyCollected: 6 }); expect(resolveInvoiceLineDefaults(row).qty).toBe(6); }); - it("defaults to 1 when neither qty is recorded", () => { - const row = makeRow({ orderQty: null, qtyCollected: null }); + it("falls back to qtyCollected when neither quantity is recorded", () => { + const row = makeRow({ quantity: null, orderQty: null, qtyCollected: 6 }); + expect(resolveInvoiceLineDefaults(row).qty).toBe(6); + }); + + it("defaults to 1 when no qty is recorded at all", () => { + const row = makeRow({ quantity: null, orderQty: null, qtyCollected: null }); expect(resolveInvoiceLineDefaults(row).qty).toBe(1); }); - - it("uses orderQty even when it equals qtyCollected (no overflow)", () => { - const row = makeRow({ orderQty: 4, qtyCollected: 4 }); - expect(resolveInvoiceLineDefaults(row).qty).toBe(4); - }); - - it("uses orderQty when it exceeds qtyCollected (short delivery — never invoices the surplus)", () => { - const row = makeRow({ orderQty: 10, qtyCollected: 4 }); - expect(resolveInvoiceLineDefaults(row).qty).toBe(10); - }); }); describe("surplusQty", () => { - it("returns the supplier-MOQ overflow (collected - ordered)", () => { - expect(surplusQty(makeRow({ orderQty: 4, qtyCollected: 10 }))).toBe(6); + it("returns the supplier-MOQ overflow (collected - customer qty)", () => { + // The case the operator actually hits: bought 10 on a supplier MOQ, + // customer wanted 4, so 6 units are surplus and belong in stock. + // Before the audit fix this returned 0 and the surplus was unbookable. + expect(surplusQty(makeRow({ quantity: 4, orderQty: 10, qtyCollected: 10 }))).toBe(6); }); - it("returns 0 when collected equals ordered (no overflow)", () => { - expect(surplusQty(makeRow({ orderQty: 4, qtyCollected: 4 }))).toBe(0); + it("returns 0 when collected equals the customer qty (no overflow)", () => { + expect(surplusQty(makeRow({ quantity: 4, orderQty: 4, qtyCollected: 4 }))).toBe(0); }); - it("returns 0 when short delivery (collected < ordered)", () => { - expect(surplusQty(makeRow({ orderQty: 10, qtyCollected: 4 }))).toBe(0); + it("returns 0 on a short delivery (collected < customer qty)", () => { + expect(surplusQty(makeRow({ quantity: 10, orderQty: 10, qtyCollected: 4 }))).toBe(0); }); - it("returns 0 when either side is missing", () => { - expect(surplusQty(makeRow({ orderQty: null, qtyCollected: 10 }))).toBe(0); - expect(surplusQty(makeRow({ orderQty: 4, qtyCollected: null }))).toBe(0); - expect(surplusQty(makeRow({ orderQty: null, qtyCollected: null }))).toBe(0); + it("returns 0 when the collected qty is missing", () => { + expect(surplusQty(makeRow({ quantity: 4, orderQty: 4, qtyCollected: null }))).toBe(0); + }); + + it("returns 0 when no customer qty is known", () => { + expect(surplusQty(makeRow({ quantity: null, orderQty: null, qtyCollected: 10 }))).toBe(0); }); }); diff --git a/src/app/(app)/invoicing/view-model.ts b/src/app/(app)/invoicing/view-model.ts index 98d3b26..9e724f7 100644 --- a/src/app/(app)/invoicing/view-model.ts +++ b/src/app/(app)/invoicing/view-model.ts @@ -16,6 +16,17 @@ export type InvoicingRow = { invoiceDate?: string | null; invoiceNumber?: string | null; qb_invoice_pdf?: string | null; + /** + * `order_items.quantity` — what the CUSTOMER ordered. This is the + * number that goes on the invoice and the delivery note. + */ + quantity?: number | null; + /** + * `order_items.order_qty` — what WE ordered from the supplier. Differs + * from `quantity` on supplier-MOQ lines (order_qty larger) and shorted + * lines (order_qty smaller). Never bill this. See + * `src/lib/order-item-qty.ts`. + */ orderQty?: number | null; qtyCollected?: number | null; costPrice?: number | null; @@ -33,6 +44,15 @@ export type InvoicingRow = { printedBy?: string | null; printedByName?: string | null; printCount?: number | null; + /** + * When true this row never reaches a customer-facing document. The + * invoice / delivery-note payload builders drop it from `items` + * (see `billableRows` / `buildQbInvoicePayload` below) but keep it + * in `order_item_ids` so n8n's status-flip writes the row to + * `invoiced` with the batch. See the non-invoiceable-order-extras + * design (docs/superpowers/specs/2026-08-30-...). + */ + excludeFromInvoice?: boolean; }; export type PurchaseOrderGroup = { @@ -360,20 +380,30 @@ export type InvoiceLineOverride = { }; export function resolveInvoiceLineDefaults(row: InvoicingRow): { qty: number; price: number } { - // The invoice qty must reflect what the CUSTOMER ordered (`orderQty`), - // never what was `qtyCollected` from the supplier. When a supplier - // MOQ forces us to buy more than the customer wanted (e.g. ordered 4, - // received 10), the operator routes the surplus into stock via - // /api/v1/stock/surplus-book — only the customer-facing 4 goes on the - // invoice / delivery note. Fall back to `qtyCollected` only when no - // explicit customer qty was ever recorded (legacy rows / quotes not - // yet promoted into a real order). + // The invoice qty must be what the CUSTOMER ordered (`quantity`) — + // never `orderQty` (what we bought from the supplier) and never + // `qtyCollected` (what physically arrived). + // + // This used to read `orderQty`, which is wrong in both directions: + // a supplier MOQ (customer 4, we buy 10) over-billed by 6, and a + // shorted line (customer 100, supplier A covers 40) under-billed by + // 60 whenever the operator hadn't split the remainder yet. See + // docs/superpowers/specs/2026-08-30-quantity-handling-audit.md. + // + // The MOQ overflow is routed into stock via /api/v1/stock/surplus-book + // instead — see `surplusQty` below. + // + // Fall back through `orderQty` to `qtyCollected` only for rows that + // never recorded a customer qty (legacy rows / quotes not yet promoted + // into a real order), where the two were written identically anyway. const qty = - typeof row.orderQty === "number" - ? row.orderQty - : typeof row.qtyCollected === "number" - ? row.qtyCollected - : 1; + typeof row.quantity === "number" + ? row.quantity + : typeof row.orderQty === "number" + ? row.orderQty + : typeof row.qtyCollected === "number" + ? row.qtyCollected + : 1; const price = typeof row.sellingPrice === "number" ? row.sellingPrice : 0; return { qty, price }; } @@ -382,12 +412,24 @@ export function resolveInvoiceLineDefaults(row: InvoicingRow): { qty: number; pr * How many units of `row` were collected from the supplier that do NOT * belong on the customer's invoice — i.e. the supplier-MOQ overflow * that should be booked into stock. Returns 0 when there's no - * headroom (collected ≤ ordered) or when either side is unknown. + * headroom (collected ≤ customer qty) or when either side is unknown. + * + * The subtrahend is the CUSTOMER quantity, not `orderQty`. Measuring + * against `orderQty` made this return 0 for the exact case it exists to + * catch: we buy 10 on a supplier MOQ, the customer wanted 4, all 10 + * arrive — `collected - orderQty` is 0, so the 6 surplus units were + * unbookable and silently vanished from inventory. */ export function surplusQty(row: InvoicingRow): number { if (typeof row.qtyCollected !== "number") return 0; - if (typeof row.orderQty !== "number") return 0; - return Math.max(0, row.qtyCollected - row.orderQty); + const customer = + typeof row.quantity === "number" + ? row.quantity + : typeof row.orderQty === "number" + ? row.orderQty + : null; + if (customer === null) return 0; + return Math.max(0, row.qtyCollected - customer); } export function buildQbInvoicePayload(input: { @@ -402,6 +444,24 @@ export function buildQbInvoicePayload(input: { sentByName?: string; }): QbInvoicePayload { const overrideMap = new Map(input.lineOverrides?.map((line) => [line.rowId, line]) ?? []); + // Task 5 of the non-invoiceable-order-extras plan. The split below + // is the load-bearing piece of the whole feature — it relies on + // a property of the existing n8n pipeline that was verified + // before this plan was written: + // + // `order_item_ids` drives n8n's "Build PocketBase Invoice Update" + // node, which patches every row's `item_status` to `invoiced` and + // stamps the invoice number for audit. We want excluded rows to + // follow that path so they close the order like every sibling. + // + // `items` drives the QuickBooks invoice itself. Excluded rows + // must NOT appear here or QuickBooks would bill the customer for + // a line they didn't approve. + // + // So `order_item_ids` is built from ALL rows, and `items` is built + // from BILLABLE rows only. Excluded rows ride along for the status + // flip but never reach QuickBooks. + const billable = billableRows(input.rows); return { order_number: input.orderNumberOverride?.trim() || input.orderNumber, @@ -409,10 +469,15 @@ export function buildQbInvoicePayload(input: { invoice_company: input.invoiceCompany, customer_id: input.customer.id, customer_name: input.customer.name, + // All rows — including excluded — ride along for the n8n status + // flip. See the docblock above for the rationale. order_item_ids: input.rows.map((row) => row.id), sent_by_id: input.sentById ?? "", sent_by_name: input.sentByName ?? "", - items: input.rows.map((row) => { + // Billable rows only — excluded rows drop out here so they never + // reach the customer invoice. The flag is authoritative: a row + // with excludeFromInvoice=true is excluded regardless of price. + items: billable.map((row) => { const line = overrideMap.get(row.id); const defaults = resolveInvoiceLineDefaults(row); return { @@ -424,6 +489,21 @@ export function buildQbInvoicePayload(input: { }; } +/** + * Filter to rows that should appear on the customer-facing invoice / + * delivery note. Excludes every row with `excludeFromInvoice: true` + * — the flag is authoritative, the price column is not consulted. + * + * Exported so the invoice UI (Task 6) and the delivery-note route + * (Task 7) share one definition of "billable" with this payload + * builder — three callers, one truth. + */ +export function billableRows( + rows: ReadonlyArray, +): T[] { + return rows.filter((row) => row.excludeFromInvoice !== true); +} + export type QbCustomer = { id: string; name: string }; export function nameSimilarity(selectedName: string, expectedName: string): number {