diff --git a/src/lib/server/order-delivery-note-link-service.test.ts b/src/lib/server/order-delivery-note-link-service.test.ts index a9b7995..a3a6171 100644 --- a/src/lib/server/order-delivery-note-link-service.test.ts +++ b/src/lib/server/order-delivery-note-link-service.test.ts @@ -225,3 +225,175 @@ describe("suggestDeliveryNoteForOrder", () => { expect(res.best?.deliveryNoteId).toBe("dn2"); }); }); + +/* ============================================================================ + * Scoping guards. + * + * A prior code review demonstrated by mutation that the tests above + * assert on OUTPUT SHAPE but pin no scoping DECISION: swapping + * `company` for `customer`, deleting the status filter, and reversing + * the recency tie-break all left the suite green. These tests inspect + * the actual PB `filter` strings and the self-match guard instead. + * ========================================================================== */ +describe("suggestDeliveryNoteForOrder — scoping", () => { + beforeEach(() => vi.clearAllMocks()); + + it("scopes candidate delivery notes by COMPANY, never by customer", async () => { + // The domain rule this protects: a different person at the same + // company can place the repeat order, so customer would miss most + // real cases. + const dnGetFullList = vi.fn().mockResolvedValue([]); + mockAdminPb( + makePb({ + [ORDERS]: { + getOne: vi.fn().mockResolvedValue({ id: "ord1", company: "co1", customer: "cust1" }), + update: vi.fn().mockResolvedValue({}), + }, + [ORDER_ITEMS]: { + getFullList: vi.fn().mockResolvedValue([ + { id: "oi1", description: "Widget", quantity: 1 }, + ]), + }, + [DELIVERY_NOTES]: { getFullList: dnGetFullList }, + }), + ); + + await suggestDeliveryNoteForOrder("ord1"); + + const filter = dnGetFullList.mock.calls[0][0].filter as string; + expect(filter).toContain('company = "co1"'); + expect(filter).not.toContain("customer"); + }); + + it("restricts candidates to statuses whose goods actually went out", async () => { + const dnGetFullList = vi.fn().mockResolvedValue([]); + mockAdminPb( + makePb({ + [ORDER_ITEMS]: { + getFullList: vi.fn().mockResolvedValue([ + { id: "oi1", description: "Widget", quantity: 1 }, + ]), + }, + [DELIVERY_NOTES]: { getFullList: dnGetFullList }, + }), + ); + + await suggestDeliveryNoteForOrder("ord1"); + + const filter = dnGetFullList.mock.calls[0][0].filter as string; + // draft + loaded are real deliveries here — confirmed with the + // business owner. Excluding them made the feature inert (3 of 116). + for (const s of ["draft", "loaded", "sent", "po_received", "invoiced", "completed"]) { + expect(filter).toContain(`status = "${s}"`); + } + // Cancelled is the one deliberate exclusion. + expect(filter).not.toContain('status = "cancelled"'); + }); + + it("never flags an order as a repeat of its OWN delivery note", async () => { + // Path A stamps `delivery_note_number` onto rows that keep their + // `order` relation, so an order and its own DN are backed by the + // SAME rows and compare exactly equal. Without the guard this + // returns a match every time. + const update = vi.fn().mockResolvedValue({}); + const sameRows = [{ id: "oi1", order: "ord1", description: "Widget", quantity: 2 }]; + const itemsGetFullList = vi.fn().mockResolvedValue(sameRows); + mockAdminPb( + makePb({ + [ORDERS]: { + getOne: vi.fn().mockResolvedValue({ id: "ord1", company: "co1" }), + update, + }, + [ORDER_ITEMS]: { getFullList: itemsGetFullList }, + [DELIVERY_NOTES]: { + getFullList: vi.fn().mockResolvedValue([ + { + id: "dn_own", delivery_note_number: "DN1", company: "co1", + status: "draft", created: "2026-08-01T00:00:00Z", order: "ord1", + }, + ]), + }, + }), + ); + + const res = await suggestDeliveryNoteForOrder("ord1"); + + expect(res.method).toBe("no_match"); + expect(res.best).toBeNull(); + // The `dn.order === orderId` short-circuit must fire BEFORE the + // per-DN item fetch. Without this assertion the test still passes + // via the combined-DN fallback below, leaving the id guard + // unpinned — verified by mutation. Exactly one call = the order's + // own lines; a second call would mean we paid for a fetch we can + // skip on every processed order. + expect(itemsGetFullList).toHaveBeenCalledTimes(1); + }); + + it("also skips a COMBINED delivery note that contains this order's lines", async () => { + // A combined DN has `dn.order` empty because its items span + // several orders, so the id check can't catch it — the fallback + // inspects the DN rows' own `order` relation. + const sameRows = [{ id: "oi1", order: "ord1", description: "Widget", quantity: 2 }]; + mockAdminPb( + makePb({ + [ORDERS]: { + getOne: vi.fn().mockResolvedValue({ id: "ord1", company: "co1" }), + update: vi.fn().mockResolvedValue({}), + }, + [ORDER_ITEMS]: { getFullList: vi.fn().mockResolvedValue(sameRows) }, + [DELIVERY_NOTES]: { + getFullList: vi.fn().mockResolvedValue([ + { + id: "dn_combined", delivery_note_number: "DN9", company: "co1", + status: "sent", created: "2026-08-01T00:00:00Z", order: "", + }, + ]), + }, + }), + ); + + const res = await suggestDeliveryNoteForOrder("ord1"); + + expect(res.method).toBe("no_match"); + expect(res.best).toBeNull(); + }); + + it("still matches a genuine repeat: another order's delivery note with identical lines", async () => { + // The positive control for the two guards above — they must not + // suppress real matches. + const update = vi.fn().mockResolvedValue({}); + mockAdminPb( + makePb({ + [ORDERS]: { + getOne: vi.fn().mockResolvedValue({ id: "ord1", company: "co1" }), + update, + }, + [ORDER_ITEMS]: { + getFullList: vi + .fn() + // the order's own lines + .mockResolvedValueOnce([{ id: "oi1", order: "ord1", description: "Widget", quantity: 2 }]) + // the candidate DN's lines — different rows, different order + .mockResolvedValueOnce([{ id: "oi_other", order: "ord_other", description: "widget", quantity: 2 }]), + }, + [DELIVERY_NOTES]: { + getFullList: vi.fn().mockResolvedValue([ + { + id: "dn_other", delivery_note_number: "DN7", company: "co1", + status: "draft", created: "2026-08-01T00:00:00Z", order: "ord_other", + }, + ]), + }, + }), + ); + + const res = await suggestDeliveryNoteForOrder("ord1"); + + expect(res.method).toBe("exact_items"); + expect(res.best).toEqual({ deliveryNoteId: "dn_other", deliveryNoteNumber: "DN7" }); + expect(update).toHaveBeenCalledWith("ord1", expect.objectContaining({ + potential_delivery_note: "dn_other", + potential_delivery_note_number: "DN7", + })); + }); +}); diff --git a/src/lib/server/order-delivery-note-link-service.ts b/src/lib/server/order-delivery-note-link-service.ts index 6c9032c..70422a9 100644 --- a/src/lib/server/order-delivery-note-link-service.ts +++ b/src/lib/server/order-delivery-note-link-service.ts @@ -34,9 +34,30 @@ const DELIVERY_NOTES_COLLECTION = "delivery_notes"; const QUOTES_COLLECTION = "quotes"; const QUOTE_ITEMS_COLLECTION = "quote_items"; -/** DNs the customer has actually seen. A draft was never sent, a - * cancelled one was abandoned — neither can be "repeated." */ -const CANDIDATE_STATUSES = ["sent", "po_received", "invoiced", "completed"]; +/** DNs representing goods that actually went out to the customer. + * + * `draft` and `loaded` are INCLUDED, corrected from the original spec: + * - `draft` — confirmed with the business owner (2026-08-30) that + * drafts are real deliveries; the status simply never gets + * advanced in the system. Excluding them left a candidate pool of + * 3 DNs out of 116, making the whole feature inert. + * - `loaded` — reachable directly from `draft` + * (`deliveries-service.ts`), so a DN can go out on the truck + * without ever passing through `sent`. Goods physically left. + * + * `cancelled` remains excluded: nominally "abandoned, kept for audit". + * NOTE it is 84 of 116 live DNs, and the owner has said cancel was + * also used to clear out superseded delivery orders — so some of + * those may be real deliveries. Revisit once that is confirmed; + * including them is a one-line change here. */ +const CANDIDATE_STATUSES = [ + "draft", + "loaded", + "sent", + "po_received", + "invoiced", + "completed", +]; function esc(s: string): string { return (s || "").replace(/"/g, '\\"'); @@ -48,6 +69,11 @@ interface DnCandidate { company?: string; status: string; created: string; + /** The order this DN's items originated from (Path A). Empty for + * ad-hoc counter DNs and for combined DNs spanning several orders. + * Read solely to skip an order's own delivery note — see the guard + * in the candidate loop. */ + order?: string; } export async function suggestDeliveryNoteForOrder( @@ -91,19 +117,39 @@ export async function suggestDeliveryNoteForOrder( .collection(DELIVERY_NOTES_COLLECTION) .getFullList({ filter: `company = "${esc(order.company)}" && (${CANDIDATE_STATUSES.map((s) => `status = "${s}"`).join(" || ")})`, - fields: "id,delivery_note_number,company,status,created", + fields: "id,delivery_note_number,company,status,created,order", batch: 1000, }); const matched: DnCandidate[] = []; for (const dn of candidates) { + // An order is never a "repeat" of its OWN delivery note. + // + // This guard is load-bearing, not defensive. Delivery notes have no + // line-item collection — items are `order_items` rows tagged with + // `delivery_note_number`. The Path A flow (invoicing → "mark as + // delivery note") stamps that number onto rows that KEEP their + // `order` relation, so a processed order and its own DN are backed + // by the *same database rows*. Comparing them comes out exactly + // equal every time, and the order would be flagged as a repeat of + // the delivery it originated. 149 live rows are in that state. + if (dn.order && dn.order === orderId) continue; + const dnItemRows = await pb .collection(ORDER_ITEMS_COLLECTION) - .getFullList<{ description?: string; quantity?: number }>({ + .getFullList<{ order?: string; description?: string; quantity?: number }>({ filter: `delivery_note_number = "${esc(dn.delivery_note_number)}"`, - fields: "description,quantity", + fields: "order,description,quantity", batch: 1000, }); + + // Second half of the same guard: a combined DN (see + // `combineDeliveryNotes`) has `dn.order` empty because its items + // span several orders. The id check above can't catch those, so + // fall back to checking whether any of the DN's rows belong to the + // order we're matching. + if (dnItemRows.some((r) => r.order && r.order === orderId)) continue; + const dnLines: DeliveryLine[] = dnItemRows.map((r) => ({ description: r.description || "", quantity: Number(r.quantity) || 0,