From 690b8a2c26d96e045af35dd53ed7de9f7e5b4b5c Mon Sep 17 00:00:00 2001 From: SJCoetz Date: Mon, 3 Aug 2026 15:47:01 +0200 Subject: [PATCH] fix(po-send): reject sends for POs without a generated PDF MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dashboard's send-email route fires the n8n webhook unconditionally once pre-send validation passes. If a PO has no generated PDF (legacy state — `documents` empty + `documents_history: [null]`), n8n tries to download from PB with an empty filename, gets a 404, and the webhook returns HTTP 500. The operator sees "Webhook returned HTTP 500. Status reverted to mail_failed." with no indication of the actual cause. Add a PDF-existence guard in `sendPurchaseOrderEmail` using the same `documents` → `documents_history` fallback chain the route uses to compute `latestFilename`. When both are empty / unusable, return a `no_pdf` failure that flips the PO to `mail_failed` with a clear operator-facing message BEFORE the webhook fires. While here, rename the misleading `code: "no_pdf"` on the line-items check to `code: "no_line_items"` — the old code name was wrong (it checked for linked items, not PDFs) and would have collided with the new `no_pdf` guard. The new guard also filters null entries out of `documents_history` before calling `.find()` / `.file` — the legacy `[null]` shape was making the old route code throw. Co-Authored-By: Claude Co-authored-by: Orca --- .../server/purchase-orders-service.test.ts | 190 +++++++++++++++++- src/lib/server/purchase-orders-service.ts | 43 +++- 2 files changed, 231 insertions(+), 2 deletions(-) diff --git a/src/lib/server/purchase-orders-service.test.ts b/src/lib/server/purchase-orders-service.test.ts index dc90d33..efdd4f6 100644 --- a/src/lib/server/purchase-orders-service.test.ts +++ b/src/lib/server/purchase-orders-service.test.ts @@ -5,7 +5,10 @@ vi.mock("@/lib/server/pocketbase-admin", () => ({ })); import { getAdminPocketBase } from "./pocketbase-admin"; -import { recordPurchaseOrderSendOutcome } from "./purchase-orders-service"; +import { + recordPurchaseOrderSendOutcome, + sendPurchaseOrderEmail, +} from "./purchase-orders-service"; /** * Build a mock admin client wired for the outcome-recording flow: @@ -67,6 +70,78 @@ beforeEach(() => { vi.clearAllMocks(); }); +/** + * Build a mock admin client wired for the *pre-send* validation flow: + * - purchase_orders.getOne → the PO (overridable) + * - purchase_orders.update → captures the status flip + * - suppliers.getOne → the supplier row + * - order_items.getList → controls `totalItems` so we can simulate + * "PO has no linked items" + */ +const makeSendAdmin = ({ + poOverrides = {}, + linkedItemsTotal = 1, +}: { + poOverrides?: Record; + linkedItemsTotal?: number; +} = {}) => { + const po = { + id: "po-1", + po_number: "PO-TEST-001", + status: "draft", + supplier: "sup-1", + documents: ["po_test_001_v1.pdf"], + documents_history: [], + additional_attachments: [], + email_payload: undefined, + ...poOverrides, + }; + + const poUpdate = vi + .fn() + .mockImplementation((_id: string, payload: Record) => + Promise.resolve({ ...po, ...payload }), + ); + const supplierGetOne = vi.fn().mockResolvedValue({ + id: "sup-1", + email: "sup@example.com", + suppliername: "Acme", + }); + const orderItemsGetList = vi.fn().mockResolvedValue({ + items: [], + totalItems: linkedItemsTotal, + page: 1, + perPage: 1, + totalPages: 1, + }); + + const admin = { + authStore: { token: "test-pb-token" }, + collection: vi.fn().mockImplementation((name: string) => { + if (name === "purchase_orders") { + return { + getOne: vi.fn().mockResolvedValue(po), + update: poUpdate, + getFullList: vi.fn().mockResolvedValue([]), + }; + } + if (name === "suppliers") { + return { + getOne: supplierGetOne, + }; + } + if (name === "order_items") { + return { + getList: orderItemsGetList, + }; + } + return { getOne: vi.fn(), update: vi.fn(), getFullList: vi.fn(), getList: vi.fn() }; + }), + }; + + return { admin, po, poUpdate, supplierGetOne, orderItemsGetList }; +}; + describe("recordPurchaseOrderSendOutcome", () => { it("does NOT auto-flip linked order_items.item_status to collection_pool on success (operator must mark them ready manually)", async () => { const { admin, orderItemUpdate } = makeAdmin({ @@ -152,4 +227,117 @@ describe("recordPurchaseOrderSendOutcome", () => { email_error: "[smtp] SMTP 550 mailbox unavailable", }); }); +}); + +describe("sendPurchaseOrderEmail", () => { + const VALID_PAYLOAD = { + cc: "", + bcc: "", + subject: "Purchase Order PO-TEST-001", + message: "", + }; + + it("returns failure with code 'no_line_items' when PO has no linked order items", async () => { + const { admin, poUpdate } = makeSendAdmin({ linkedItemsTotal: 0 }); + vi.mocked(getAdminPocketBase).mockResolvedValue(admin as never); + + const result = await sendPurchaseOrderEmail("po-1", VALID_PAYLOAD); + + expect(result.failure?.code).toBe("no_line_items"); + expect(result.failure?.message).toMatch(/line items/i); + // Status was flipped to mail_failed via markPreSendFailure. + expect(poUpdate).toHaveBeenCalled(); + const payload = poUpdate.mock.calls[0]![1] as Record; + expect(payload).toMatchObject({ + status: "mail_failed", + email_error: expect.stringMatching(/line items/i) as unknown as string, + }); + }); + + it("returns failure with code 'no_pdf' when PO has empty documents AND empty documents_history", async () => { + const { admin, poUpdate } = makeSendAdmin({ + poOverrides: { + documents: [], + documents_history: [], + }, + }); + vi.mocked(getAdminPocketBase).mockResolvedValue(admin as never); + + const result = await sendPurchaseOrderEmail("po-1", VALID_PAYLOAD); + + expect(result.failure?.code).toBe("no_pdf"); + expect(result.failure?.message).toMatch(/PDF/i); + // Status flipped to mail_failed BEFORE the webhook would have fired. + expect(poUpdate).toHaveBeenCalled(); + const payload = poUpdate.mock.calls[0]![1] as Record; + expect(payload.status).toBe("mail_failed"); + expect(payload.email_error as string).toMatch(/PDF/i); + }); + + it("returns failure with code 'no_pdf' when documents is empty AND documents_history is [null] (legacy broken state)", async () => { + // PO-2026-07-006 hits this exact shape: documents: "" + documents_history: [null]. + // The old route would happily fire the webhook and n8n would 404 on the + // missing filename — surfacing as "Webhook returned HTTP 500" to the operator. + const { admin, poUpdate } = makeSendAdmin({ + poOverrides: { + documents: "", + documents_history: [null] as never, + }, + }); + vi.mocked(getAdminPocketBase).mockResolvedValue(admin as never); + + const result = await sendPurchaseOrderEmail("po-1", VALID_PAYLOAD); + + expect(result.failure?.code).toBe("no_pdf"); + expect(poUpdate).toHaveBeenCalled(); + const payload = poUpdate.mock.calls[0]![1] as Record; + expect(payload.status).toBe("mail_failed"); + }); + + it("accepts the send when documents has the latest PDF token", async () => { + const { admin, poUpdate } = makeSendAdmin({ + poOverrides: { + documents: ["po_test_001_v1.pdf"], + documents_history: [], + }, + }); + vi.mocked(getAdminPocketBase).mockResolvedValue(admin as never); + + const result = await sendPurchaseOrderEmail("po-1", VALID_PAYLOAD); + + expect(result.failure).toBeNull(); + // Normal happy path: status flipped to "sending". + expect(poUpdate).toHaveBeenCalled(); + const payload = poUpdate.mock.calls[0]![1] as Record; + expect(payload.status).toBe("sending"); + }); + + it("accepts the send when documents is empty BUT documents_history has a 'current' entry", async () => { + // Pre-migration POs only have documents_history (no documents array yet). + // The fallback chain should treat the current history entry as the PDF. + const { admin, poUpdate } = makeSendAdmin({ + poOverrides: { + documents: [], + documents_history: [ + { + version: 1, + file: "po_test_001_v1.pdf", + token: "po_test_001_v1.pdf", + generatedAt: "2026-08-01T00:00:00.000Z", + generatedBy: "", + reason: "", + status: "current", + }, + ], + }, + }); + vi.mocked(getAdminPocketBase).mockResolvedValue(admin as never); + + const result = await sendPurchaseOrderEmail("po-1", VALID_PAYLOAD); + + expect(result.failure).toBeNull(); + expect(poUpdate).toHaveBeenCalled(); + const payload = poUpdate.mock.calls[0]![1] as Record; + expect(payload.status).toBe("sending"); + }); }); \ No newline at end of file diff --git a/src/lib/server/purchase-orders-service.ts b/src/lib/server/purchase-orders-service.ts index 3b9c8ed..371b9f3 100644 --- a/src/lib/server/purchase-orders-service.ts +++ b/src/lib/server/purchase-orders-service.ts @@ -52,6 +52,7 @@ export interface SendPurchaseOrderEmailFailure { | "no_recipient" | "invalid_recipient" | "no_pdf" + | "no_line_items" | "invalid_status" | "purchase_order_not_found" | "supplier_not_found" @@ -173,12 +174,52 @@ export async function sendPurchaseOrderEmail( }); if (linkedItems.totalItems === 0) { return markPreSendFailure(pb, po, { - code: "no_pdf", + code: "no_line_items", message: "This PO has no line items attached. Re-link items on the order detail page before sending.", }); } + // ── 5b. Validate the PO has a generated PDF attached. ───────────── + // The route fires the n8n webhook which downloads the PO PDF + // from PB by filename. If neither `documents` nor + // `documents_history` resolves to a usable filename, the + // download 404s and the workflow surfaces to the operator as + // "Webhook returned HTTP 500". Reject the send here instead + // — clearer error, no wasted webhook round-trip. + // + // The fallback chain matches `send-email/route.ts`'s + // `latestFilename` computation so the service and the route + // agree on what counts as "the latest PDF". Mirrors the + // service-layer → route-layer duplication pattern already + // used in stock-purchases + dispatch. + // + // `documents_history` can contain legacy `[null]` entries + // (PO was created before the array was filled in); filter + // those out before calling `.find()` / accessing `.file` — + // otherwise the whole send throws on a malformed row. + const documentsField = po.documents; + const documentsList: string[] = Array.isArray(documentsField) + ? documentsField + : typeof documentsField === "string" && documentsField + ? [documentsField] + : []; + const documentsHistory = (po.documents_history ?? []).filter( + (h): h is NonNullable => h != null, + ); + const latestFilename = + documentsList[documentsList.length - 1] ?? + (documentsHistory.find((h) => h.status === "current") ?? + documentsHistory[documentsHistory.length - 1])?.file ?? + ""; + if (!latestFilename) { + return markPreSendFailure(pb, po, { + code: "no_pdf", + message: + "This PO has no generated PDF attached. Open the order detail page and regenerate the document before sending.", + }); + } + // ── 5b. Generic body default ─────────────────────────────── // When the operator leaves the message blank, substitute a // professional supplier-personalised template. SMTP relays