fix(po-send): reject sends for POs without a generated PDF
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 <noreply@anthropic.com> Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
SJCoetz
parent
18e1d97b6c
commit
690b8a2c26
2 changed files with 231 additions and 2 deletions
|
|
@ -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<string, unknown>;
|
||||
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<string, unknown>) =>
|
||||
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<string, unknown>;
|
||||
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<string, unknown>;
|
||||
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<string, unknown>;
|
||||
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<string, unknown>;
|
||||
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<string, unknown>;
|
||||
expect(payload.status).toBe("sending");
|
||||
});
|
||||
});
|
||||
|
|
@ -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<typeof h> => 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
|
||||
|
|
|
|||
Loading…
Reference in a new issue