From 0a5f35278497a40c099ff147c0bcafd2bf705a87 Mon Sep 17 00:00:00 2001 From: Dave Date: Sat, 29 Aug 2026 14:01:13 +0200 Subject: [PATCH] fix(orders): close cross-line quote_item collision, pin pairing's field projections MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pairOrderLines' no-overwrite guard only checked whether an order line's own row already carried a quote_item, not whether the quote line it was about to claim was already held by a different order line. A description drift (or a hand-confirmed link) on one line let a fresh confident match on another line silently double-assign the same quote_item, so two parts would render the same estimator's note and attachments — the exact failure this feature exists to prevent, reached from the other direction. Fixed by seeding a claimed-quote-item set from the fetched order rows before the write loop, and skipping any confident pair that targets an already-claimed quote line. unmatchedOrderItemIds now also excludes any line that already carries a link, even when this run's pairing pass didn't re-propose it. Also names the two field projections (PAIR_ORDER_ITEM_FIELDS, PAIR_QUOTE_ITEM_FIELDS) next to the existing MATCHER_QUOTE_ITEM_FIELDS constant, and adds a test that asserts on the actual `fields` string passed to each getFullList call. Previously nothing distinguished `fields: "id,description"` from `fields: "id,description,quote_item"` at the order-items query — a one-token trim silently disabled the no-overwrite guard (every row read as unlinked) with the full suite staying green, since the existing behavioral tests supply `quote_item` values directly in their fixtures rather than through the query. Verified by re-running that exact mutation against the new test: it fails, and neither pre-existing behavioral test catches it on its own. --- .../server/order-quote-link-service.test.ts | 80 +++++++++++++++++++ src/lib/server/order-quote-link-service.ts | 42 +++++++++- 2 files changed, 119 insertions(+), 3 deletions(-) diff --git a/src/lib/server/order-quote-link-service.test.ts b/src/lib/server/order-quote-link-service.test.ts index c5b205b..87794aa 100644 --- a/src/lib/server/order-quote-link-service.test.ts +++ b/src/lib/server/order-quote-link-service.test.ts @@ -396,4 +396,84 @@ describe("pairOrderLines", () => { // ...but the already-set relation is never clobbered. expect(update).not.toHaveBeenCalled(); }); + + it("does not hand a quote line to a second order line when another line already claims it", async () => { + // oi_A's DB row already points at qi_X, but its description has + // since drifted — it no longer matches qi_X (or anything else), + // so this run's algorithm proposes no pair for it at all. oi_B's + // description is now a confident match for qi_X. The guard must + // stop oi_B's write, or both lines would end up rendering qi_X's + // note and attachments. + const update = vi.fn().mockResolvedValue({}); + mockAdminPb( + makePb({ + [ORDER_ITEMS]: { + getFullList: vi.fn().mockResolvedValue([ + { + id: "oi_A", + description: "silicone sealant clear cartridge", + quote_item: "qi_X", + }, + { id: "oi_B", description: "M20 HEX BOLT GR8.8 ZINC" }, + ]), + update, + }, + [QUOTE_ITEMS]: { + getFullList: vi.fn().mockResolvedValue([ + { id: "qi_X", quote: "q1", description: "Hex Bolt M20 Gr8.8 Zinc" }, + ]), + }, + }), + ); + + const res = await pairOrderLines("ord1", "q1"); + + // oi_B's confident match is reported for the UI... + expect(res.confident).toEqual([ + expect.objectContaining({ orderItemId: "oi_B", quoteItemId: "qi_X" }), + ]); + // ...but never written, because qi_X is already claimed. + expect(update).not.toHaveBeenCalled(); + // oi_A still carries a link in the DB, even though this run's + // pairing pass didn't (re-)propose it — it must not be reported + // as needing attention. + expect(res.unmatchedOrderItemIds).toEqual([]); + }); + + it("projects the fields the pairing and its no-overwrite guard actually need", async () => { + // Regression coverage for the `fields` string literals: a + // one-token edit to either (e.g. dropping `quote_item` off the + // order-items projection) silently disables the no-overwrite + // guard or blanks every description, and nothing else in this + // suite would catch it since the mocks return fixed data + // regardless of what `fields` asked for. + const orderItemsGetFullList = vi.fn().mockResolvedValue([ + { id: "oi1", description: "hex bolt m20 gr8.8 zinc" }, + ]); + const quoteItemsGetFullList = vi.fn().mockResolvedValue([ + { id: "qi1", quote: "q1", description: "hex bolt m20 gr8.8 zinc" }, + ]); + mockAdminPb( + makePb({ + [ORDER_ITEMS]: { + getFullList: orderItemsGetFullList, + update: vi.fn().mockResolvedValue({}), + }, + [QUOTE_ITEMS]: { getFullList: quoteItemsGetFullList }, + }), + ); + + await pairOrderLines("ord1", "q1"); + + const orderItemsOpts = orderItemsGetFullList.mock.calls[0][0] as { + fields: string; + }; + expect(orderItemsOpts.fields).toContain("description"); + expect(orderItemsOpts.fields).toContain("quote_item"); + + const quoteItemsOpts = quoteItemsGetFullList.mock.calls[0][0] as { + fields: string; + }; + expect(quoteItemsOpts.fields).toContain("description"); + }); }); diff --git a/src/lib/server/order-quote-link-service.ts b/src/lib/server/order-quote-link-service.ts index 0a68f02..f6e2527 100644 --- a/src/lib/server/order-quote-link-service.ts +++ b/src/lib/server/order-quote-link-service.ts @@ -111,6 +111,22 @@ const CANDIDATE_QUOTE_FIELDS = "id,quote_number,company,customer,created"; * and start scoring every candidate near zero. */ const MATCHER_QUOTE_ITEM_FIELDS = "id,quote,description,selling_price,quantity"; +/** What `pairOrderLines` needs off each order line: enough to pair by + * description, plus the existing `quote_item` so the write loop can + * detect an already-set link and never clobber it. If this ever + * drops `quote_item`, that guard goes silently blind — every line + * reads as unlinked and a human-confirmed pairing can be + * overwritten. */ +const PAIR_ORDER_ITEM_FIELDS = "id,description,quote_item"; + +/** What `pairOrderLines` needs off each quote line — just enough to + * pair by description. Passed explicitly to + * `fetchQuoteItemsForQuotesAdmin` for the same reason as + * `MATCHER_QUOTE_ITEM_FIELDS` above: its default projection belongs + * to the duplicate-quote scan, a different caller with different + * needs, and must not silently drift under this one. */ +const PAIR_QUOTE_ITEM_FIELDS = "id,quote,description"; + function esc(s: string): string { return (s || "").replace(/"/g, '\\"'); } @@ -382,14 +398,14 @@ export async function pairOrderLines( .collection(ORDER_ITEMS_COLLECTION) .getFullList({ filter: `order = "${esc(orderId)}"`, - fields: "id,description,quote_item", + fields: PAIR_ORDER_ITEM_FIELDS, batch: 1000, }), // Reuses the same helper the fuzzy pass uses for quote_items — it // already handles PB's IN(...) filter and the chunked-OR-chain // fallback, so there is one code path in this file that knows how // to fetch quote_items rather than two. - fetchQuoteItemsForQuotesAdmin(pb, [quoteId], "id,quote,description"), + fetchQuoteItemsForQuotesAdmin(pb, [quoteId], PAIR_QUOTE_ITEM_FIELDS), ]); // `pairQuoteLines` returns indices into exactly the arrays passed to @@ -419,6 +435,20 @@ export async function pairOrderLines( else possible.push(entry); } + // Quote lines already claimed by an order line's existing DB value. + // Seeded up front, before any write — a stale link (the line's + // description drifted since it was set, or an operator hand-picked + // it) still occupies that quote line. Without this, a fresh + // confident pair on a *different* order line can point a second + // line at the same quote_item: both would then render the same + // estimator's note and attachments, which is exactly the failure + // this feature exists to prevent, just reached from the other + // direction (`row?.quote_item` below only guards the pair's own + // order line, not the quote line it targets). + const claimedQuoteItemIds = new Set( + orderItems.map((r) => r.quote_item).filter((v): v is string => Boolean(v)), + ); + // Persist confident pairs only, and never clobber an existing link // (a line paired by createOrderFromQuote, or confirmed by hand). // @@ -430,6 +460,7 @@ export async function pairOrderLines( for (const c of confident) { const row = orderItems.find((r) => r.id === c.orderItemId); if (row?.quote_item) continue; + if (claimedQuoteItemIds.has(c.quoteItemId)) continue; await pb.collection(ORDER_ITEMS_COLLECTION).update(c.orderItemId, { quote_item: c.quoteItemId, }); @@ -438,8 +469,13 @@ export async function pairOrderLines( return { confident, possible, + // A line already carrying a `quote_item` in the DB is matched + // even if this run's algorithm didn't (re-)propose a pair for + // it — e.g. its description drifted away from its original + // counterpart. Only a line with neither an existing link nor a + // fresh pair counts as genuinely unmatched. unmatchedOrderItemIds: orderItems - .filter((_, idx) => !pairedOrderIdx.has(idx)) + .filter((r, idx) => !pairedOrderIdx.has(idx) && !r.quote_item) .map((r) => r.id), }; }