diff --git a/src/app/(app)/collection-confirm/view-model.test.ts b/src/app/(app)/collection-confirm/view-model.test.ts index bf22493..14fd41d 100644 --- a/src/app/(app)/collection-confirm/view-model.test.ts +++ b/src/app/(app)/collection-confirm/view-model.test.ts @@ -9,6 +9,7 @@ import { buildConfirmPatch, buildReturnPatch, canConfirmRow, + formatNoteBody, groupRowsByPurchaseOrder, nextItemStatusForConfirm, parseNotesFeed, @@ -443,6 +444,52 @@ describe("appendCostNote", () => { body: { prevCost: 45, newCost: 50 }, }); }); + + it("writes a cost note when prevCost is null and newCost is 0", () => { + // Regression: the old code lossy-fell-back `null → 0` to `0 → 0`, + // which short-circuited inside appendCostNote and silently skipped + // the audit note. The fix threads null through so the equality + // check `null === 0` is false and the note IS recorded. + const result = appendCostNote(null, { id: "u-1", name: "sj" }, null, 0); + const parsed = JSON.parse(result) as Array>; + expect(parsed).toHaveLength(1); + expect(parsed[0]).toMatchObject({ + category: "cost", + body: { prevCost: null, newCost: 0 }, + }); + }); + + it("writes a cost note when prevCost is null and newCost is a positive number", () => { + const result = appendCostNote("", { id: "u-1", name: "sj" }, null, 50); + const parsed = JSON.parse(result) as Array>; + expect(parsed).toHaveLength(1); + expect(parsed[0]).toMatchObject({ + category: "cost", + body: { prevCost: null, newCost: 50 }, + }); + }); +}); + +describe("formatNoteBody", () => { + it("renders cost transitions as 'Cost changed: prev → next'", () => { + expect(formatNoteBody({ prevCost: 45, newCost: 50 })).toBe("Cost changed: 45 → 50"); + }); + + it("renders a null prevCost (no prior cost) as 'none'", () => { + // Diagnostic UX: when the audit entry records a transition from + // "no prior cost" to a value, the operator sees "none → 50" + // rather than a raw JSON dump. + expect(formatNoteBody({ prevCost: null, newCost: 50 })).toBe( + "Cost changed: none → 50", + ); + expect(formatNoteBody({ prevCost: null, newCost: 0 })).toBe( + "Cost changed: none → 0", + ); + }); + + it("passes string bodies through unchanged", () => { + expect(formatNoteBody("hello world")).toBe("hello world"); + }); }); describe("dedupeAttachments", () => { diff --git a/src/app/(app)/collection-confirm/view-model.ts b/src/app/(app)/collection-confirm/view-model.ts index 7cc4fd6..b20f360 100644 --- a/src/app/(app)/collection-confirm/view-model.ts +++ b/src/app/(app)/collection-confirm/view-model.ts @@ -1,6 +1,6 @@ export type NoteCategory = "collection" | "note" | "problem" | "quantity" | "confirm" | "cost"; -export type NoteBody = string | { prevCost?: number; newCost?: number; [k: string]: unknown }; +export type NoteBody = string | { prevCost?: number | null; newCost?: number; [k: string]: unknown }; export type NoteEntry = { ts: string; @@ -292,7 +292,7 @@ export function buildReturnPatch(input: ReturnPatchInput): ReturnPatch { export function appendCostNote( rawNotes: string | null | undefined, actor: ConfirmActor, - prevCost: number, + prevCost: number | null, newCost: number, ): string { if (prevCost === newCost) return rawNotes ?? ""; @@ -337,10 +337,11 @@ export function formatNoteBody(body: NoteBody): string { if (typeof body === "string") return body; if (body && typeof body === "object") { if ("prevCost" in body || "newCost" in body) { - const prev = (body as { prevCost?: number }).prevCost; + const prev = (body as { prevCost?: number | null }).prevCost; const next = (body as { newCost?: number }).newCost; - if (typeof prev === "number" && typeof next === "number") { - return `Cost changed: ${prev} → ${next}`; + if (typeof next === "number") { + const prevLabel = typeof prev === "number" ? String(prev) : "none"; + return `Cost changed: ${prevLabel} → ${next}`; } } return JSON.stringify(body); diff --git a/src/lib/server/collection-confirm-service.test.ts b/src/lib/server/collection-confirm-service.test.ts index f1148ac..b048dd1 100644 --- a/src/lib/server/collection-confirm-service.test.ts +++ b/src/lib/server/collection-confirm-service.test.ts @@ -432,6 +432,62 @@ describe("updateOrderItemCost", () => { expect(failingGetOne).toHaveBeenCalledWith("row-1"); expect(dummyUpdate).not.toHaveBeenCalled(); }); + + it("writes cost_price and appends a cost note when going from null to 0", async () => { + // Regression: the lossy fallback `prevCost ?? 0` collapsed a true + // `null → 0` transition into `0 → 0` inside appendCostNote, which + // short-circuited and silently dropped the audit note. The fix + // threads the original `null` through; this test pins that + // behaviour. + const existing = { id: "row-1", cost_price: null, notes: null }; + const { admin, update } = makeAdminForUpdate({ existing }); + vi.mocked(getAdminPocketBase).mockResolvedValue(admin as never); + + const result = await updateOrderItemCost("row-1", { + cost_price: 0, + actor: { id: "u-1", name: "sj" }, + }); + + expect(update).toHaveBeenCalledTimes(1); + const [, patch] = update.mock.calls[0] as [ + string, + { cost_price: number; notes: string }, + ]; + expect(patch.cost_price).toBe(0); + const parsed = JSON.parse(patch.notes) as Array>; + expect(parsed).toHaveLength(1); + expect(parsed[0]).toMatchObject({ + category: "cost", + actorId: "u-1", + actorName: "sj", + body: { prevCost: null, newCost: 0 }, + }); + + expect(result.cost_price).toBe(0); + }); + + it("writes cost_price and appends a cost note when going from null to a positive number", async () => { + const existing = { id: "row-1", cost_price: null, notes: null }; + const { admin, update } = makeAdminForUpdate({ existing }); + vi.mocked(getAdminPocketBase).mockResolvedValue(admin as never); + + await updateOrderItemCost("row-1", { + cost_price: 50, + actor: { id: "u-1", name: "sj" }, + }); + + const [, patch] = update.mock.calls[0] as [ + string, + { cost_price: number; notes: string }, + ]; + expect(patch.cost_price).toBe(50); + const parsed = JSON.parse(patch.notes) as Array>; + expect(parsed).toHaveLength(1); + expect(parsed[0]).toMatchObject({ + category: "cost", + body: { prevCost: null, newCost: 50 }, + }); + }); }); describe("confirmOrderItemsForInvoicing", () => { @@ -607,4 +663,36 @@ describe("confirmOrderItemsForInvoicing", () => { const notes = JSON.parse(patch.notes as string) as Array>; expect(notes.map((entry) => entry.category)).toEqual(["confirm"]); }); + + it("writes cost_price and appends a cost note when overriding from null to 0", async () => { + // Regression for the lossy `prevCost ?? 0` fallback that + // collapsed a `null → 0` transition into `0 → 0` and skipped the + // cost note. The fix threads null through; this test pins it. + const rows = [ + { + id: "row-1", + item_status: "collected", + cost_price: null, + notes: null, + order_item_details: null, + }, + ]; + const { admin, update } = makeAdminForConfirm({ rows }); + vi.mocked(getAdminPocketBase).mockResolvedValue(admin as never); + + await confirmOrderItemsForInvoicing(["row-1"], { + invoice: { date: "2026-08-03", number: "INV-1" }, + costOverrides: { "row-1": 0 }, + actor: { id: "u-1", name: "sj" }, + }); + + expect(update).toHaveBeenCalledTimes(1); + const patch = update.mock.calls[0]?.[1] as Record; + expect(patch.cost_price).toBe(0); + const notes = JSON.parse(patch.notes as string) as Array>; + // The confirm note (from buildConfirmPatch) is appended first, + // then the cost note — same shape as any other override. + expect(notes.map((entry) => entry.category)).toEqual(["confirm", "cost"]); + expect(notes[1]?.body).toEqual({ prevCost: null, newCost: 0 }); + }); }); diff --git a/src/lib/server/collection-confirm-service.ts b/src/lib/server/collection-confirm-service.ts index 56a7ece..77cef10 100644 --- a/src/lib/server/collection-confirm-service.ts +++ b/src/lib/server/collection-confirm-service.ts @@ -217,14 +217,11 @@ export async function listCollectedItems(): Promise { * newly-appended notes feed (preserving any prior entries) in a single * round-trip. * - * `prevCost` is read from the existing row's `cost_price`. When the row - * has no prior cost (null), `appendCostNote` receives `0` as the - * previous cost — the early-return check still treats `null → 0` as a - * change (since `null === 0` is `false`), and the note records the - * transition as `prevCost: 0`. For `null → X > 0` the note correctly - * captures the move; for `null → 0` the `appendCostNote` internal - * short-circuit returns the existing notes unchanged, so the only - * side-effect is the `cost_price: 0` write. + * `prevCost` is read from the existing row's `cost_price` and threaded + * through to `appendCostNote` verbatim (including `null` when the row + * has no prior cost). The `null === 0` check in `appendCostNote` is + * `false`, so `null → 0` is treated as a real change and a cost note + * IS appended with `{ prevCost: null, newCost: 0 }`. */ export async function updateOrderItemCost( id: string, @@ -240,7 +237,7 @@ export async function updateOrderItemCost( const newNotes = appendCostNote( (existing as { notes?: string | null }).notes ?? null, args.actor, - prevCost ?? 0, + prevCost, args.cost_price, ); const updated = await pb @@ -296,7 +293,7 @@ export async function confirmOrderItemsForInvoicing( notes = appendCostNote( confirmPatch.notes, args.actor, - currentCost ?? 0, + currentCost, costOverride, ); }