From 7dd70577a724519822a53cf5f34d853766ba93bd Mon Sep 17 00:00:00 2001 From: Dave Date: Thu, 5 Nov 2026 22:28:18 +0200 Subject: [PATCH] fix(stock): a catalogue mirror error no longer aborts the batch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The price_list mirror could reject — a bad filter, a 400, a network blip — and the rejection escaped setStockQuantitiesBulk. The quantities had already been written and their ledger rows were in place, so the caller got an exception instead of a summary: the remaining lines were never reported, and a retry re-applied the quantities and wrote a second set of ledger rows, double-counting the audit trail. The mirror is now guarded. The quantity landed, so the row stays "applied" — reporting it as "failed" would say the quantity did not change, which is false. The row carries price_list_rows_touched: 0 and a message saying the catalogue snapshot failed and why. The batch always returns its summary and results. Also: "ok" now means what it says — summary.failed === 0 — instead of always being true. The price-list page is unbounded by design at this data volume (about 2,900 catalogue rows over about 2,700 SKUs), and the comment now says so, with the caveat that the filter's raw-SKU LIKE clause treats % and _ as wildcards, so the page is sized by substring noise rather than by the SKU's own rows. --- .../stock-quantities-bulk-service.test.ts | 85 +++++++++++++++++-- .../server/stock-quantities-bulk-service.ts | 36 ++++++-- 2 files changed, 107 insertions(+), 14 deletions(-) diff --git a/src/lib/server/stock-quantities-bulk-service.test.ts b/src/lib/server/stock-quantities-bulk-service.test.ts index c640e1c..4dc9485 100644 --- a/src/lib/server/stock-quantities-bulk-service.test.ts +++ b/src/lib/server/stock-quantities-bulk-service.test.ts @@ -35,7 +35,7 @@ const STOCK_ITEMS = [ function stubPb( priceListRows: Array> = [], - opts: { failUpdateIds?: string[] } = {}, + opts: { failUpdateIds?: string[]; failPriceListForSku?: string } = {}, ) { const calls: string[] = []; const ledgerCreates: Array> = []; @@ -82,10 +82,26 @@ function stubPb( } if (name === "price_list") { return { - getList: vi.fn().mockResolvedValue({ - items: priceListRows, - totalItems: priceListRows.length, - }), + getList: vi.fn().mockImplementation( + async ( + _page: number, + _perPage: number, + query?: Record, + ) => { + // One SKU's catalogue query blows up — a rejected + // filter, a 400, a network blip. + if ( + opts.failPriceListForSku && + String(query?.filter ?? "").includes(opts.failPriceListForSku) + ) { + throw new Error("400: invalid filter"); + } + return { + items: priceListRows, + totalItems: priceListRows.length, + }; + }, + ), update: vi.fn().mockImplementation(async (id: string, patch: Record) => { priceListUpdates.push({ id, patch }); return { id, ...patch }; @@ -131,6 +147,7 @@ describe("setStockQuantitiesBulk — applying", () => { actor: { id: "u1", name: "Dave" }, }); expect(res.summary.applied).toBe(1); + expect(res.ok).toBe(true); expect(calls[0]).toBe("stock_ledger.create"); expect(calls[1]).toBe("stock_items.update"); expect(ledgerCreates[0].change_qty).toBe(38); @@ -207,7 +224,7 @@ describe("setStockQuantitiesBulk — applying", () => { }); it("keeps the rest of the batch going when one write fails", async () => { - const { pb } = stubPb([], { failUpdateIds: ["id-def"] }); + const { pb, ledgerCreates } = stubPb([], { failUpdateIds: ["id-def"] }); const res = await setStockQuantitiesBulk({ pb, lines: [ @@ -228,6 +245,20 @@ describe("setStockQuantitiesBulk — applying", () => { expect(res.summary.unmatched).toBe(0); expect(res.results[1].status).toBe("failed"); expect(res.results[1].message).toContain("Failed to update"); + // The ledger row for the failed line is compensated, so the two + // rows net to zero — the orphan cannot double-count on a retry. + const failedLineRows = ledgerCreates.filter( + (r) => r.stock_item === "id-def", + ); + expect(failedLineRows.map((r) => r.change_qty)).toEqual([4, -4]); + expect( + failedLineRows.reduce((sum, r) => sum + Number(r.change_qty), 0), + ).toBe(0); + // The failed row says the ledger was compensated, so a retry is + // safe. + expect(res.results[1].message).toContain("reversed"); + // A failed write makes the batch a partial success. + expect(res.ok).toBe(false); // Every line is counted once, in exactly one bucket. const { applied, unchanged, unmatched, ambiguous, failed } = res.summary; expect(res.summary.total).toBe( @@ -293,6 +324,48 @@ describe("setStockQuantitiesBulk — summary", () => { }); }); +describe("setStockQuantitiesBulk — catalogue failure", () => { + it("keeps the batch going when the catalogue mirror fails", async () => { + const { pb, ledgerCreates } = stubPb( + [{ id: "pl1", sku: "M16-FLAT-WASHER", sku_normalised: "M16FLATWASHER" }], + { failPriceListForSku: "M16-FLAT-WASHER" }, + ); + const res = await setStockQuantitiesBulk({ + pb, + lines: [ + { sku: "M16-FLAT-WASHER", qty: 50 }, + { sku: "BOLT-M6-40", qty: 9 }, + ], + mode: "set", + dryRun: false, + actor: { id: "u1", name: "Dave" }, + }); + + // The call returns normally — no rejection, no lost rows. + expect(res.results).toHaveLength(2); + expect(res.summary.applied).toBe(2); + expect(res.summary.failed).toBe(0); + expect(res.ok).toBe(true); + + // The row whose mirror failed still applied — the quantity DID + // move — and the row says why the catalogue did not follow. + expect(res.results[0].status).toBe("applied"); + expect(res.results[0].new_qty).toBe(50); + expect(res.results[0].price_list_rows_touched).toBe(0); + expect(res.results[0].message).toContain( + "price-list catalogue snapshot failed", + ); + expect(res.results[0].message).toContain("invalid filter"); + + // The next line was still processed. + expect(res.results[1].status).toBe("applied"); + expect(res.results[1].new_qty).toBe(9); + + // The quantities really landed. + expect(ledgerCreates).toHaveLength(2); + }); +}); + describe("setStockQuantitiesBulk — price_list mirror", () => { it("mirrors the new quantity onto every catalogue row for that SKU", async () => { const { pb, priceListUpdates } = stubPb([ diff --git a/src/lib/server/stock-quantities-bulk-service.ts b/src/lib/server/stock-quantities-bulk-service.ts index 51186c2..57e591d 100644 --- a/src/lib/server/stock-quantities-bulk-service.ts +++ b/src/lib/server/stock-quantities-bulk-service.ts @@ -193,8 +193,13 @@ async function mirrorPriceListQty( const norm = normaliseSku(rawSku); if (!norm) return 0; - // One page is enough at the catalogue's size: a SKU has one - // row per supplier, so a handful at most. + // One page is unbounded by design at this data volume: the + // catalogue holds about 2,900 rows over about 2,700 SKUs, so a SKU + // has one row per supplier and a handful at most. + // + // Caveat: the filter carries a raw-SKU LIKE clause, and PocketBase + // treats `%` and `_` inside a LIKE value as wildcards — so the page + // is sized by substring noise rather than by the SKU's own rows. const page = await pb .collection("price_list") .getList(1, 200, { @@ -404,12 +409,22 @@ export async function setStockQuantitiesBulk( summary.applied += 1; let priceListRowsTouched = 0; + let message: string | undefined; if (line.item.sku) { - priceListRowsTouched = await mirrorPriceListQty( - pb, - line.item.sku, - p.target, - ); + try { + priceListRowsTouched = await mirrorPriceListQty( + pb, + line.item.sku, + p.target, + ); + } catch (err) { + // The quantity is already written and its ledger row is in + // place. The catalogue row is a snapshot of our quantity, not + // the quantity itself, so a catalogue error must not fail the + // row — and must not abort the batch. The operator still gets + // the whole summary, and the reason is on the row. + message = `The quantity was applied but the price-list catalogue snapshot failed: ${err instanceof Error ? err.message : String(err)}`; + } } results.push({ @@ -422,11 +437,16 @@ export async function setStockQuantitiesBulk( new_qty: p.target, delta, price_list_rows_touched: priceListRowsTouched, + ...(message ? { message } : {}), }); } return { - ok: true, + // A failed write makes the batch a partial success: the rows that + // applied are applied, and the rows that failed are on their own. + // Read `summary` and `results[]` — this flag only says whether + // anything failed. + ok: summary.failed === 0, mode, dry_run: dryRun, summary,