fix(orders): pin matcher's field deps, project queries, and stop swallowing real errors
Six review findings on suggestQuoteForOrder, all bite hardest once
order intake and the backlog sweep start calling it:
1. fetchQuoteItemsForQuotesAdmin's `fields` string was a hardcoded
constant owned by the duplicate-quote scan. The matcher now passes
its own `fields` (an added, defaulted parameter) instead of
depending on that string never being trimmed out from under it —
losing `description`/`selling_price` would have collapsed every
confidence score toward zero with no exception and no failing
test.
2. The candidate-quote query in fuzzyPass had no `fields`, so it
pulled full quote records (email_body, ai_insights,
migration_meta) for up to 200 candidates, twice per order when the
window widens, to read five scalars. Added
`fields: "id,quote_number,company,customer,created"`.
3. findExactMatch's bare `catch {}` swallowed everything, not just
the "field doesn't exist on an older PB schema" case it claimed to
guard (all four fields exist per pb_schema-2.json). An expired
admin token or a PB blip during the sweep would silently fall
through to the fuzzy path, and writeSuggestion would then persist
quote_matching_attempted: true / quote_match_method: "no_match" -
permanently hiding an order that had a valid citation from the
sweep's retry filter. Narrowed to rethrow anything that isn't a
ClientResponseError with status 400.
4. Exact-identifier lookups (quote_reference_text, rfq_number are
customer free text) weren't scoped to the order's company, so a
reference-number collision across two customers could return a
confidence-1.0 suggestion pointing at the wrong company's quote.
Added `company = "..."` to the filter when the order has one.
5. SuggestQuoteResult.window used the literal "90d", hardcoding
FUZZY_WINDOW_DAYS. Renamed to "initial" so retuning the constant
can't make the sweep's reporting lie.
6. order-quote-link-service.test.ts's makePb forced every test to
redeclare the full collection map. Gave it a default map merged
with a per-test override and renamed useP to mockAdminPb, since
two more tasks add tests to this file.
Adds tests for #1 (matcher's quote_items fields include description
and selling_price), #2 (candidate query's fields), and #4 (a
same-reference quote belonging to another company is not returned).
This commit is contained in:
parent
cefa52c3e3
commit
8e990e8a57
3 changed files with 164 additions and 52 deletions
|
|
@ -18,23 +18,6 @@ interface CollectionMock {
|
|||
update?: ReturnType<typeof vi.fn>;
|
||||
}
|
||||
|
||||
function makePb(collections: Record<string, CollectionMock>) {
|
||||
return {
|
||||
autoCancellation: vi.fn(),
|
||||
collection: (name: string) => {
|
||||
const c = collections[name];
|
||||
if (!c) throw new Error(`unexpected collection: ${name}`);
|
||||
return c;
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
function useP(pb: ReturnType<typeof makePb>) {
|
||||
vi.mocked(getAdminPocketBase).mockResolvedValue(
|
||||
pb as unknown as Awaited<ReturnType<typeof getAdminPocketBase>>,
|
||||
);
|
||||
}
|
||||
|
||||
const ORDER = {
|
||||
id: "ord1",
|
||||
created: "2026-08-29T10:00:00.000Z",
|
||||
|
|
@ -44,22 +27,57 @@ const ORDER = {
|
|||
quote_reference_text: "",
|
||||
};
|
||||
|
||||
/**
|
||||
* Sensible defaults for every collection this service touches, merged
|
||||
* with a per-test override map. Two more tasks (order intake, the
|
||||
* backlog sweep) add tests to this file — without a default map, each
|
||||
* test has to redeclare the full collection set even when it only
|
||||
* cares about one of them, and an undeclared *method* on a
|
||||
* hand-rolled mock fails with an opaque "not a function" rather than
|
||||
* the collection map's clear "unexpected collection" error.
|
||||
*/
|
||||
function makePb(overrides: Record<string, CollectionMock> = {}) {
|
||||
const defaults: Record<string, CollectionMock> = {
|
||||
[ORDERS]: {
|
||||
getOne: vi.fn().mockResolvedValue(ORDER),
|
||||
update: vi.fn().mockResolvedValue({}),
|
||||
},
|
||||
[QUOTES]: { getList: vi.fn().mockResolvedValue({ items: [] }) },
|
||||
[ORDER_ITEMS]: { getFullList: vi.fn().mockResolvedValue([]) },
|
||||
[QUOTE_ITEMS]: { getFullList: vi.fn().mockResolvedValue([]) },
|
||||
};
|
||||
const merged: Record<string, CollectionMock> = { ...defaults, ...overrides };
|
||||
return {
|
||||
autoCancellation: vi.fn(),
|
||||
collection: (name: string) => {
|
||||
const c = merged[name];
|
||||
if (!c) throw new Error(`unexpected collection: ${name}`);
|
||||
return c;
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
function mockAdminPb(pb: ReturnType<typeof makePb>) {
|
||||
vi.mocked(getAdminPocketBase).mockResolvedValue(
|
||||
pb as unknown as Awaited<ReturnType<typeof getAdminPocketBase>>,
|
||||
);
|
||||
}
|
||||
|
||||
describe("suggestQuoteForOrder", () => {
|
||||
beforeEach(() => vi.clearAllMocks());
|
||||
|
||||
it("resolves an exact quote_number citation regardless of the date window", async () => {
|
||||
const update = vi.fn().mockResolvedValue({});
|
||||
const quotesGetList = vi.fn().mockResolvedValue({
|
||||
items: [{ id: "q_old", quote_number: "Q-2024-0847", created: "2024-01-05T00:00:00.000Z" }],
|
||||
items: [{ id: "q_old", quote_number: "Q-2024-0847", company: "co1", created: "2024-01-05T00:00:00.000Z" }],
|
||||
});
|
||||
useP(
|
||||
mockAdminPb(
|
||||
makePb({
|
||||
[ORDERS]: {
|
||||
getOne: vi.fn().mockResolvedValue({ ...ORDER, quote_reference_text: "Q-2024-0847" }),
|
||||
update,
|
||||
},
|
||||
[QUOTES]: { getList: quotesGetList },
|
||||
[ORDER_ITEMS]: { getFullList: vi.fn().mockResolvedValue([]) },
|
||||
}),
|
||||
);
|
||||
|
||||
|
|
@ -76,26 +94,66 @@ describe("suggestQuoteForOrder", () => {
|
|||
expect(update.mock.calls[0][1]).not.toHaveProperty("quote");
|
||||
});
|
||||
|
||||
it("constrains fuzzy candidates to the 90-day window before the order date", async () => {
|
||||
const quotesGetList = vi.fn().mockResolvedValue({ items: [] });
|
||||
useP(
|
||||
it("scopes exact-identifier lookups to the order's own company", async () => {
|
||||
// Two quotes share the same customer-supplied reference text but
|
||||
// belong to different companies. The fixture is deliberately
|
||||
// ordered so an UNscoped lookup would return the wrong one
|
||||
// (q_other_co, listed first) — only a company-scoped filter
|
||||
// narrows it down to q_co1.
|
||||
const quoteRows = [
|
||||
{ id: "q_other_co", quote_reference: "SHARED-REF", company: "co2", created: "2026-01-05T00:00:00.000Z" },
|
||||
{ id: "q_co1", quote_reference: "SHARED-REF", company: "co1", created: "2026-01-01T00:00:00.000Z" },
|
||||
];
|
||||
const quotesGetList = vi.fn().mockImplementation(
|
||||
(_page: number, _perPage: number, opts: { filter: string }) => {
|
||||
const conditions = opts.filter.split(" && ").map((clause) => {
|
||||
const m = clause.trim().match(/^(\w+) = "(.*)"$/);
|
||||
return m ? { field: m[1], value: m[2] } : null;
|
||||
});
|
||||
const items = quoteRows.filter((row) =>
|
||||
conditions.every(
|
||||
(c) => c === null || (row as unknown as Record<string, string>)[c.field] === c.value,
|
||||
),
|
||||
);
|
||||
return Promise.resolve({ items });
|
||||
},
|
||||
);
|
||||
mockAdminPb(
|
||||
makePb({
|
||||
[ORDERS]: {
|
||||
getOne: vi.fn().mockResolvedValue({ ...ORDER, quote_reference_text: "SHARED-REF" }),
|
||||
update: vi.fn().mockResolvedValue({}),
|
||||
},
|
||||
[QUOTES]: { getList: quotesGetList },
|
||||
}),
|
||||
);
|
||||
|
||||
const res = await suggestQuoteForOrder("ord1");
|
||||
|
||||
expect(res.best?.quoteId).toBe("q_co1");
|
||||
});
|
||||
|
||||
it("constrains fuzzy candidates to the 90-day window before the order date, and projects only scalar fields", async () => {
|
||||
const quotesGetList = vi.fn().mockResolvedValue({ items: [] });
|
||||
mockAdminPb(
|
||||
makePb({
|
||||
[ORDERS]: { getOne: vi.fn().mockResolvedValue(ORDER), update: vi.fn().mockResolvedValue({}) },
|
||||
[QUOTES]: { getList: quotesGetList },
|
||||
[ORDER_ITEMS]: {
|
||||
getFullList: vi.fn().mockResolvedValue([
|
||||
{ id: "oi1", description: "hex bolt m20 gr8.8 zinc", selling_price: 10, quantity: 5 },
|
||||
]),
|
||||
},
|
||||
[QUOTE_ITEMS]: { getFullList: vi.fn().mockResolvedValue([]) },
|
||||
}),
|
||||
);
|
||||
|
||||
await suggestQuoteForOrder("ord1");
|
||||
|
||||
const filter = quotesGetList.mock.calls[0][2].filter as string;
|
||||
expect(filter).toContain('created >= "2026-05-31T10:00:00.000Z"');
|
||||
expect(filter).toContain('company = "co1"');
|
||||
const opts = quotesGetList.mock.calls[0][2] as { filter: string; fields?: string };
|
||||
expect(opts.filter).toContain('created >= "2026-05-31T10:00:00.000Z"');
|
||||
expect(opts.filter).toContain('company = "co1"');
|
||||
// `quotes` also carries email_body / ai_insights / migration_meta —
|
||||
// this query must not pull those to read five scalars.
|
||||
expect(opts.fields).toBe("id,quote_number,company,customer,created");
|
||||
});
|
||||
|
||||
it("widens to 12 months when the 90-day window yields nothing above threshold", async () => {
|
||||
|
|
@ -105,20 +163,18 @@ describe("suggestQuoteForOrder", () => {
|
|||
.mockResolvedValueOnce({
|
||||
items: [{ id: "q1", quote_number: "Q-1", company: "co1", customer: "cust1", created: "2025-11-01T00:00:00.000Z" }],
|
||||
});
|
||||
useP(
|
||||
const quoteItemsGetFullList = vi.fn().mockResolvedValue([
|
||||
{ id: "qi1", quote: "q1", description: "hex bolt m20 gr8.8 zinc", selling_price: 10, quantity: 5 },
|
||||
]);
|
||||
mockAdminPb(
|
||||
makePb({
|
||||
[ORDERS]: { getOne: vi.fn().mockResolvedValue(ORDER), update: vi.fn().mockResolvedValue({}) },
|
||||
[QUOTES]: { getList: quotesGetList },
|
||||
[ORDER_ITEMS]: {
|
||||
getFullList: vi.fn().mockResolvedValue([
|
||||
{ id: "oi1", description: "hex bolt m20 gr8.8 zinc", selling_price: 10, quantity: 5 },
|
||||
]),
|
||||
},
|
||||
[QUOTE_ITEMS]: {
|
||||
getFullList: vi.fn().mockResolvedValue([
|
||||
{ id: "qi1", quote: "q1", description: "hex bolt m20 gr8.8 zinc", selling_price: 10, quantity: 5 },
|
||||
]),
|
||||
},
|
||||
[QUOTE_ITEMS]: { getFullList: quoteItemsGetFullList },
|
||||
}),
|
||||
);
|
||||
|
||||
|
|
@ -127,6 +183,14 @@ describe("suggestQuoteForOrder", () => {
|
|||
expect(quotesGetList).toHaveBeenCalledTimes(2);
|
||||
expect(res.window).toBe("widened");
|
||||
expect(res.best?.quoteId).toBe("q1");
|
||||
|
||||
// The matcher's quote_items projection must include `description`
|
||||
// and `selling_price` — fetchQuoteItemsForQuotesAdmin's own default
|
||||
// fields string belongs to a different caller (the duplicate-quote
|
||||
// scan) and could be trimmed independently of the matcher's needs.
|
||||
const itemsOpts = quoteItemsGetFullList.mock.calls[0][0] as { fields: string };
|
||||
expect(itemsOpts.fields).toContain("description");
|
||||
expect(itemsOpts.fields).toContain("selling_price");
|
||||
});
|
||||
|
||||
it("chunks the quote_items fetch instead of issuing one oversized OR-chain filter", async () => {
|
||||
|
|
@ -156,9 +220,8 @@ describe("suggestQuoteForOrder", () => {
|
|||
return Promise.resolve([]);
|
||||
});
|
||||
|
||||
useP(
|
||||
mockAdminPb(
|
||||
makePb({
|
||||
[ORDERS]: { getOne: vi.fn().mockResolvedValue(ORDER), update: vi.fn().mockResolvedValue({}) },
|
||||
[QUOTES]: { getList: quotesGetList },
|
||||
[ORDER_ITEMS]: {
|
||||
getFullList: vi.fn().mockResolvedValue([
|
||||
|
|
@ -194,16 +257,14 @@ describe("suggestQuoteForOrder", () => {
|
|||
|
||||
it("records no_match and still stamps quote_matching_attempted", async () => {
|
||||
const update = vi.fn().mockResolvedValue({});
|
||||
useP(
|
||||
mockAdminPb(
|
||||
makePb({
|
||||
[ORDERS]: { getOne: vi.fn().mockResolvedValue(ORDER), update },
|
||||
[QUOTES]: { getList: vi.fn().mockResolvedValue({ items: [] }) },
|
||||
[ORDER_ITEMS]: {
|
||||
getFullList: vi.fn().mockResolvedValue([
|
||||
{ id: "oi1", description: "widget", selling_price: 1, quantity: 1 },
|
||||
]),
|
||||
},
|
||||
[QUOTE_ITEMS]: { getFullList: vi.fn().mockResolvedValue([]) },
|
||||
}),
|
||||
);
|
||||
|
||||
|
|
@ -220,7 +281,7 @@ describe("suggestQuoteForOrder", () => {
|
|||
|
||||
it("skips an order that already has a hard quote link", async () => {
|
||||
const update = vi.fn();
|
||||
useP(
|
||||
mockAdminPb(
|
||||
makePb({
|
||||
[ORDERS]: { getOne: vi.fn().mockResolvedValue({ ...ORDER, quote: "already" }), update },
|
||||
}),
|
||||
|
|
|
|||
|
|
@ -18,6 +18,7 @@
|
|||
* `setOrderQuote` in `orders-service.ts`.
|
||||
*/
|
||||
|
||||
import { ClientResponseError } from "pocketbase";
|
||||
import { getAdminPocketBase } from "@/lib/server/pocketbase-admin";
|
||||
import { fetchQuoteItemsForQuotesAdmin } from "@/lib/server/quotes-service";
|
||||
import type { QuoteItem } from "@/lib/types";
|
||||
|
|
@ -54,8 +55,10 @@ export interface SuggestQuoteResult {
|
|||
best: QuoteMatchCandidate | null;
|
||||
alternatives: QuoteMatchCandidate[];
|
||||
method: QuoteMatchMethod;
|
||||
/** Which pass produced the result — for the sweep's reporting. */
|
||||
window: "exact" | "90d" | "widened" | "none";
|
||||
/** Which pass produced the result — for the sweep's reporting.
|
||||
* "initial" is the FUZZY_WINDOW_DAYS pass, not a fixed "90d" label
|
||||
* — retuning that constant must not make this label lie. */
|
||||
window: "exact" | "initial" | "widened" | "none";
|
||||
/** Days from the matched quote's creation to the order's. null when unmatched. */
|
||||
ageGapDays: number | null;
|
||||
/** Set when the order was deliberately not processed. */
|
||||
|
|
@ -91,6 +94,22 @@ const ORDER_COLLECTION = "orders";
|
|||
const QUOTE_COLLECTION = "quotes";
|
||||
const ORDER_ITEMS_COLLECTION = "order_items";
|
||||
|
||||
/** The scalars `fuzzyPass` reads off each candidate quote. Explicit
|
||||
* `fields` on the query — `quotes` also carries `email_body`,
|
||||
* `ai_insights`, `migration_meta`, etc., and pulling those for up to
|
||||
* FUZZY_CANDIDATE_CAP (200) candidates (twice, if the window widens)
|
||||
* to read five scalars would dominate the whole operation's cost at
|
||||
* intake volume. */
|
||||
const CANDIDATE_QUOTE_FIELDS = "id,quote_number,company,customer,created";
|
||||
|
||||
/** What `scoreOrderVsQuote` needs off each quote line. Passed
|
||||
* explicitly to `fetchQuoteItemsForQuotesAdmin` — its default
|
||||
* projection belongs to the duplicate-quote scan, a different
|
||||
* stakeholder with different needs. If that default is ever trimmed,
|
||||
* this call must not silently lose `description` or `selling_price`
|
||||
* and start scoring every candidate near zero. */
|
||||
const MATCHER_QUOTE_ITEM_FIELDS = "id,quote,description,selling_price,quantity";
|
||||
|
||||
function esc(s: string): string {
|
||||
return (s || "").replace(/"/g, '\\"');
|
||||
}
|
||||
|
|
@ -103,11 +122,20 @@ function project(rows: ReadonlyArray<LineRow>) {
|
|||
}));
|
||||
}
|
||||
|
||||
/** Try the four exact-identifier fields, in priority order. Never date
|
||||
* limited — a citation is evidence regardless of age. */
|
||||
/**
|
||||
* Try the four exact-identifier fields, in priority order. Never date
|
||||
* limited — a citation is evidence regardless of age.
|
||||
*
|
||||
* Scoped to the order's own company when known: `quote_reference_text`
|
||||
* and `rfq_number` are customer-supplied free text, and without this
|
||||
* clause a reference-number collision across two customers would
|
||||
* return a confidence-1.0 suggestion pointing at a different
|
||||
* company's quote.
|
||||
*/
|
||||
async function findExactMatch(
|
||||
pb: Awaited<ReturnType<typeof getAdminPocketBase>>,
|
||||
ref: string,
|
||||
companyId?: string,
|
||||
): Promise<{ quote: QuoteRow; method: QuoteMatchCandidate["method"] } | null> {
|
||||
const trimmed = (ref || "").trim();
|
||||
if (!trimmed) return null;
|
||||
|
|
@ -118,13 +146,27 @@ async function findExactMatch(
|
|||
["external_number", "exact_external_number"],
|
||||
] as const;
|
||||
for (const [field, method] of fields) {
|
||||
const clauses = [`${field} = "${esc(trimmed)}"`];
|
||||
if (companyId) clauses.push(`company = "${esc(companyId)}"`);
|
||||
try {
|
||||
const list = await pb
|
||||
.collection(QUOTE_COLLECTION)
|
||||
.getList<QuoteRow>(1, 5, { filter: `${field} = "${esc(trimmed)}"` });
|
||||
.getList<QuoteRow>(1, 5, { filter: clauses.join(" && ") });
|
||||
if (list.items.length > 0) return { quote: list.items[0], method };
|
||||
} catch {
|
||||
// Field may not exist on older PB deploys — try the next.
|
||||
} catch (err) {
|
||||
// A field this deploy's PB schema doesn't have 400s — try the
|
||||
// next one; all four fields exist per pb-exports/pb_schema-2.json
|
||||
// so this is a defensive fallback, not the expected path.
|
||||
// Anything else (expired admin token, connection reset, PB
|
||||
// restart mid-sweep) is a real failure and must propagate:
|
||||
// swallowing it here would let the fuzzy path silently run
|
||||
// instead, and writeSuggestion would then persist
|
||||
// `quote_matching_attempted: true` / `quote_match_method:
|
||||
// "no_match"` for an order that actually had a valid citation —
|
||||
// permanently hiding it from the sweep's "haven't tried yet"
|
||||
// filter, since nothing revisits an order once that flag is set.
|
||||
if (err instanceof ClientResponseError && err.status === 400) continue;
|
||||
throw err;
|
||||
}
|
||||
}
|
||||
return null;
|
||||
|
|
@ -155,6 +197,7 @@ async function fuzzyPass(
|
|||
.getList<QuoteRow>(1, FUZZY_CANDIDATE_CAP, {
|
||||
filter: filters.join(" && "),
|
||||
sort: "-created",
|
||||
fields: CANDIDATE_QUOTE_FIELDS,
|
||||
});
|
||||
if (candidates.items.length === 0) return [];
|
||||
|
||||
|
|
@ -167,7 +210,7 @@ async function fuzzyPass(
|
|||
// `quotes-service.ts`, which already has this exact chunking logic
|
||||
// for its duplicate-quote scan.
|
||||
const quoteIds = candidates.items.map((q) => q.id);
|
||||
const quoteItems = await fetchQuoteItemsForQuotesAdmin(pb, quoteIds);
|
||||
const quoteItems = await fetchQuoteItemsForQuotesAdmin(pb, quoteIds, MATCHER_QUOTE_ITEM_FIELDS);
|
||||
|
||||
const byQuote = new Map<string, QuoteItem[]>();
|
||||
for (const it of quoteItems) {
|
||||
|
|
@ -230,7 +273,7 @@ export async function suggestQuoteForOrder(
|
|||
|
||||
// 1. Exact identifier citation, if the order carries reference text.
|
||||
const ref = (order.quote_reference_text || order.rfq_number || "").trim();
|
||||
const exact = await findExactMatch(pb, ref);
|
||||
const exact = await findExactMatch(pb, ref, order.company);
|
||||
if (exact) {
|
||||
const best: QuoteMatchCandidate = {
|
||||
quoteId: exact.quote.id,
|
||||
|
|
@ -260,7 +303,7 @@ export async function suggestQuoteForOrder(
|
|||
});
|
||||
|
||||
let scored = await fuzzyPass(pb, order, orderItems, FUZZY_WINDOW_DAYS);
|
||||
let window: SuggestQuoteResult["window"] = "90d";
|
||||
let window: SuggestQuoteResult["window"] = "initial";
|
||||
if (scored.length === 0) {
|
||||
scored = await fuzzyPass(pb, order, orderItems, FUZZY_WINDOW_WIDEN_DAYS);
|
||||
window = "widened";
|
||||
|
|
|
|||
|
|
@ -3232,15 +3232,23 @@ export async function findDuplicateQuotes(
|
|||
* fuzzy matcher's quote-items fetch instead of building its own
|
||||
* unchunked OR-chain (which blows the PB filter length limit past
|
||||
* ~50 candidate quotes).
|
||||
*
|
||||
* `fields` defaults to this module's own projection but is a caller
|
||||
* parameter, not a shared constant, because a second stakeholder now
|
||||
* depends on specific columns being present: the matcher needs
|
||||
* `description` and `selling_price` for its scoring, which the
|
||||
* duplicate-quote scan (the original caller) has no reason to know
|
||||
* about. Trimming the default for one caller must not silently starve
|
||||
* the other.
|
||||
*/
|
||||
export async function fetchQuoteItemsForQuotesAdmin(
|
||||
pb: Awaited<ReturnType<typeof getAdminPocketBase>>,
|
||||
quoteIds: string[],
|
||||
fields: string = "id,quote,description,sku,quantity,selling_price",
|
||||
): Promise<QuoteItem[]> {
|
||||
if (quoteIds.length === 0) return [];
|
||||
const escape = (id: string) => id.replace(/"/g, '\\"');
|
||||
const safeIds = quoteIds.map(escape);
|
||||
const fields = "id,quote,description,sku,quantity,selling_price";
|
||||
try {
|
||||
return await pb.collection(QUOTE_ITEMS_COLLECTION).getFullList<QuoteItem>({
|
||||
filter: `quote IN (${safeIds.map((id) => `"${id}"`).join(",")})`,
|
||||
|
|
|
|||
Loading…
Reference in a new issue