fix(orders): stop an order matching its own delivery note; include draft/loaded
Two coupled corrections to delivery-note repeat detection. Self-match: delivery notes have no line-item collection -- items are order_items rows tagged with delivery_note_number. The Path A flow 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. isExactItemMatch compared them against themselves and returned true unconditionally. 149 live rows are in that state. Guarded both by DN id and, for combined DNs whose `order` is empty, by inspecting the DN rows' own order relation. Candidate statuses: `draft` and `loaded` are real deliveries here. The owner confirmed drafts simply never get advanced in the system, and `loaded` is reachable directly from draft, so goods can go out without ever passing through `sent`. Excluding both left a candidate pool of 3 DNs out of 116, making the feature inert. Ordering matters: the self-match guard had to land first, because the DN that exposed the bug is itself a draft. Cancelled stays excluded pending confirmation -- it is 84 of 116 and may have been used to clear superseded delivery orders rather than to mark abandonment. Tests: a prior review showed by mutation that the existing tests assert output shape but pin no scoping decision -- company-vs-customer, the status filter and the tie-break could all be inverted with the suite green. Added filter-inspecting and self-match tests, each verified by mutation to fail when its guard is removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
parent
5a869c5f41
commit
eb874bfd93
2 changed files with 224 additions and 6 deletions
|
|
@ -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",
|
||||
}));
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<DnCandidate>({
|
||||
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,
|
||||
|
|
|
|||
Loading…
Reference in a new issue