fix(orders): close cross-line quote_item collision, pin pairing's field projections
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.
This commit is contained in:
parent
b4ad67680a
commit
0a5f352784
2 changed files with 119 additions and 3 deletions
|
|
@ -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");
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<LineRow & { quote_item?: string }>({
|
||||
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),
|
||||
};
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue