From 756597068e96f2bc0c750b3d18c7adc52a370880 Mon Sep 17 00:00:00 2001 From: Dave Date: Wed, 12 Aug 2026 15:36:31 +0200 Subject: [PATCH] fix(delivery-notes): correct DN cancellation and filter issues Update the cancel delivery note dialog description to correctly state items are marked as cancelled Fix PocketBase expand field whitelisting for delivery items to avoid missing related entity data Change order item status to "cancelled" instead of "completed" when cancelling a delivery note, with updated JSDoc explaining the choice of status Update the buyer tasks PO sent filter and its regression test to cover all current ItemStatus values --- src/app/(app)/delivery-notes/[id]/page.tsx | 2 +- src/lib/server/buyer-tasks-service.test.ts | 53 ++++++++++++++++++---- src/lib/server/buyer-tasks-service.ts | 16 ++++++- src/lib/server/deliveries-service.ts | 12 ++++- src/lib/server/delivery-notes-service.ts | 16 +++++-- 5 files changed, 83 insertions(+), 16 deletions(-) diff --git a/src/app/(app)/delivery-notes/[id]/page.tsx b/src/app/(app)/delivery-notes/[id]/page.tsx index 04a1ddc..9cdc329 100644 --- a/src/app/(app)/delivery-notes/[id]/page.tsx +++ b/src/app/(app)/delivery-notes/[id]/page.tsx @@ -1016,7 +1016,7 @@ export default function DeliveryNoteDetailPage({ params }: PageProps) { Cancel delivery note {deliveryNote.delivery_note_number}? - The items will be marked as completed and stamped with a note that this delivery note was cancelled. The original delivery note record is preserved for audit. + The items will be marked as cancelled and stamped with a note that this delivery note was cancelled. The original delivery note record is preserved for audit.
diff --git a/src/lib/server/buyer-tasks-service.test.ts b/src/lib/server/buyer-tasks-service.test.ts index 6af7095..c2f6045 100644 --- a/src/lib/server/buyer-tasks-service.test.ts +++ b/src/lib/server/buyer-tasks-service.test.ts @@ -478,21 +478,54 @@ describe("listBuyerTasks", () => { expect(groups[0].supplierEmail).toBeNull(); }); - it("excludes 'collected' and 'ready_invoice' from the po_sent tab", async () => { - // The buyer's responsibility ends once the goods leave the supplier. - // Once the driver sets item_status='collected' the driver owns it; - // once collection-confirm flips it to 'ready_invoice' the - // invoicing/operator workflow owns it. In both cases the buyer - // shouldn't see the line on the PO Sent tab anymore — only true - // partial collections (still awaiting driver pickup) should remain. + it("excludes every non-po_generated ItemStatus from the po_sent tab", async () => { + // Regression guard. The po_sent filter is a negative list — it + // must exclude every ItemStatus union member that isn't + // `po_generated` (the only legitimate state for a PO that's been + // emailed but the buyer still owns). Adding a new status to the + // union without updating this list leaks the item back into the + // tab — the bug that brought 27 `delivery_note` items back onto + // PO Sent in production (operator reported: "I invoiced those, + // why are they on PO Sent?"). + // + // Source of truth: the ItemStatus union in src/lib/types.ts. + // Keep the two lists in sync; this test will fail if anyone + // adds a new status to the union and forgets to update the + // filter below. + const ITEM_STATUSES_INCLUSIVE = [ + // Active (buyer-owned) — should NOT appear in the exclude list. + "po_generated", + // Downstream — every one MUST appear in the exclude list. + "ready_delivery", + "collection_pool", + "assigned", + "collected", + "delivery_note", + "ready_invoice", + "invoiced", + "stock_booked", + "with_driver", + "checked_in", + "completed", + "delivered", + "cancelled", + "removed", + ]; const { admin, getFullList } = makeAdmin([]); vi.mocked(getAdminPocketBase).mockResolvedValue(admin as never); await listBuyerTasks("po_sent"); - const filter = (getFullList.mock.calls[0]?.[0] as { filter: string } | undefined)?.filter; - expect(filter).toContain(`item_status != "collected"`); - expect(filter).toContain(`item_status != "ready_invoice"`); + const filter = (getFullList.mock.calls[0]?.[0] as { filter: string } | undefined)?.filter ?? ""; + for (const status of ITEM_STATUSES_INCLUSIVE) { + if (status === "po_generated") continue; + expect( + filter, + `po_sent filter must exclude item_status "${status}" — ` + + `add it to the exclude list in buyer-tasks-service.ts ` + + `(and to this test if it's a brand-new status).`, + ).toContain(`item_status != "${status}"`); + } }); it("accepts a PO-sent item in a non-excluded status on po_sent", async () => { diff --git a/src/lib/server/buyer-tasks-service.ts b/src/lib/server/buyer-tasks-service.ts index 1281823..3721eca 100644 --- a/src/lib/server/buyer-tasks-service.ts +++ b/src/lib/server/buyer-tasks-service.ts @@ -87,7 +87,21 @@ export async function listBuyerTasks(tab: BuyerTaskTab): Promise`), PB silently drops the + // whole `expand` object. Without `expand.order` here, every + // row comes back with `r.expand` undefined and the page + // falls into its "(NO ORDER) / Counter pickup" bucket for + // all items. Verified live against PB 2026-08-12. expand: "order.customer,order.company,checked_in_by", fields: "id,description,quantity,selling_price,cost_price,item_status,delivery_reference," + - "loaded_at,checked_in_at,checked_in_by,order,updated,order_item_details", + "loaded_at,checked_in_at,checked_in_by,order,updated,order_item_details," + + "expand.order,expand.order.customer,expand.order.company,expand.checked_in_by", }); type Row = { diff --git a/src/lib/server/delivery-notes-service.ts b/src/lib/server/delivery-notes-service.ts index 8a46553..70d83c6 100644 --- a/src/lib/server/delivery-notes-service.ts +++ b/src/lib/server/delivery-notes-service.ts @@ -530,10 +530,20 @@ export interface CancelDeliveryNoteInput { } /** - * Cancel a delivery note. Items are flipped back to "completed" + * Cancel a delivery note. Items are flipped to "cancelled" * (no longer "delivery_note") so they fall off the Invoicing * "ready for delivery note" list, with a note stamping who - * cancelled it, when, and why. + * cancelled it, when, and why. The DN header itself also goes + * to "cancelled" — both ends match. + * + * Why "cancelled" on the order_items, not "completed": the + * `order_items.item_status` select field on PB does not include + * "completed" (only the `delivery_notes.status` field does — the + * mark-invoiced flow on the DN uses it as the terminal state). + * We mirror the DN's own "cancelled" status on the items so the + * order_items enum is satisfied without needing a PB admin + * schema change, and so the audit trail reads consistently + * (cancelled DN → cancelled items). * * Allowed from draft / sent / po_received / invoiced. From * "completed" you must not cancel — terminal. From "cancelled" @@ -569,7 +579,7 @@ export async function cancelDeliveryNote( await Promise.all( items.map((it) => pb.collection(ORDER_ITEMS).update(it.id, { - item_status: "completed", + item_status: "cancelled", notes: it.notes ? `${it.notes}\n${banner}` : banner, }), ),