From d983eccd6c804b4a0bfad6a683840b7a21881fd9 Mon Sep 17 00:00:00 2001 From: Dave Date: Mon, 5 Oct 2026 19:49:20 +0200 Subject: [PATCH] fix(delivery-notes): return to the list after quoting; kill the phantom supplier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things the operator hit straight away. 1. Creating a quote parked her on the quote detail page. She is working through notes one after another, so that pulled her out of the list for a document she had just reviewed in the modal. `afterCreate` now defaults to "list": the modal closes, the toast confirms, and she is back where she was with the row's first slot now showing the quote number. 2. "Couldn't load this quote's PDF" — blaming a supplier. The message was right about the failure and wrong about the cause. The readiness rule existed in TWO places: `quotes-service.ts` for the send path, `quote-pdf-service.ts` for the PDF route. When the supplier requirement was dropped I changed one of them and left the other enforcing it. So a counter note whose lines never had a supplier could be quoted and sent but never rendered. Both copies now call one function, `quoteItemReadinessGaps`, so a third copy cannot appear. Verified in the browser: Q202609631 now fails with "Every item still needs: cost price, not processed, selling price" — no supplier — and that quote's line genuinely has cost 0, selling 0 and is_processed false, so refusing it is correct. The phantom requirement is gone; the real blockers are named. `is_processed` is deliberately KEPT. It is the flag that says a person reviewed the line, and nothing else can set it on a delivery-note quote — the quote_items update whitelists exclude it. Dropping it would let a quote go out with lines nobody looked at. The quotes-service test mock now spreads the real module, so these tests run against the real rule rather than a stub that could agree with the implementation while production disagreed. Full suite 3676 passing, tsc clean, no new lint errors. --- .../delivery-notes/quote-review-dialog.tsx | 24 ++++++- src/lib/server/quote-pdf-service.test.ts | 7 +- src/lib/server/quote-pdf-service.ts | 72 +++++++++++++++++-- src/lib/server/quotes-service.test.ts | 7 +- src/lib/server/quotes-service.ts | 27 +++---- 5 files changed, 110 insertions(+), 27 deletions(-) diff --git a/src/components/delivery-notes/quote-review-dialog.tsx b/src/components/delivery-notes/quote-review-dialog.tsx index 9431a63..e5c0fc4 100644 --- a/src/components/delivery-notes/quote-review-dialog.tsx +++ b/src/components/delivery-notes/quote-review-dialog.tsx @@ -68,6 +68,19 @@ export interface QuoteReviewDialogProps { customerName?: string; /** Fired after a successful create, with the new quote. */ onCreated?: (result: { id: string; quote_number: string }) => void; + /** + * Where to go once the quote exists. + * + * `"list"` returns to the delivery-notes list, which is what the + * operator asked for: create the quote, then carry on with the next + * note. Landing on the quote detail page pulled her out of the list + * she was working through, for a document she had just already + * reviewed in this very modal. + * + * `"quote"` opens the quote itself — kept for any caller that wants + * to land on the finished document. + */ + afterCreate?: "list" | "quote"; } export function QuoteReviewDialog({ @@ -77,6 +90,7 @@ export function QuoteReviewDialog({ deliveryNoteNumber, customerName, onCreated, + afterCreate = "list", }: QuoteReviewDialogProps) { const router = useRouter(); @@ -201,7 +215,15 @@ export function QuoteReviewDialog({ toast.success(`Created quote ${body.quote_number}`); onOpenChange(false); onCreated?.({ id: body.id, quote_number: body.quote_number ?? "" }); - router.push(`/quotes/${body.id}`); + if (afterCreate === "quote") { + router.push(`/quotes/${body.id}`); + } else { + // Back to the list. The operator is working through notes one + // after another, so we hand her back where she was rather than + // parking her on a document she has already seen the contents + // of in this modal. + router.push("/delivery-notes"); + } } catch (err: unknown) { toast.error(err instanceof Error ? err.message : "Failed to create the quote"); } finally { diff --git a/src/lib/server/quote-pdf-service.test.ts b/src/lib/server/quote-pdf-service.test.ts index 9bdea3d..2fd0fdf 100644 --- a/src/lib/server/quote-pdf-service.test.ts +++ b/src/lib/server/quote-pdf-service.test.ts @@ -607,7 +607,12 @@ describe("generateQuotePdf", () => { expect(outcome.code).toBe("items_not_ready"); expect(outcome.details.issues?.length).toBe(1); expect(outcome.details.issues?.[0].description).toBe("Unpriced item"); - expect(outcome.details.issues?.[0].missing).toContain("no supplier"); + // A missing supplier is NOT a blocker — it was removed, and this + // is the copy of the rule that kept enforcing it after the other + // copy had dropped it. Assert the real blockers instead, and + // assert the supplier is absent so a future re-add is deliberate. + expect(outcome.details.issues?.[0].missing).toContain("cost price"); + expect(outcome.details.issues?.[0].missing).not.toContain("no supplier"); } }); diff --git a/src/lib/server/quote-pdf-service.ts b/src/lib/server/quote-pdf-service.ts index 23b70ee..1c4aaee 100644 --- a/src/lib/server/quote-pdf-service.ts +++ b/src/lib/server/quote-pdf-service.ts @@ -336,14 +336,19 @@ export async function generateQuotePdf( // - selling_price > 0 (we computed a sell price) // Items missing any of these get flagged with `missing` so the UI // can highlight them. - const ready: QuoteItem[] = []; + const ready: QuoteItem[] = []; const issues: QuotePdfReadinessIssue[] = []; for (const it of allItems) { - const missing: string[] = []; - if (!it.is_processed) missing.push("not processed"); - if (!it.supplier) missing.push("no supplier"); - if (!(it.cost_price > 0)) missing.push("cost price"); - if (!(it.selling_price > 0)) missing.push("selling price"); + // The rule lives in `quoteItemReadinessGaps` — ONE place. + // + // It used to be written out twice (here and in + // `quotes-service.ts`, for the send-email path) and the two + // copies drifted: the supplier requirement was removed from one + // and left in the other. Symptom: a quote the send path accepted + // still refused to render, with "no supplier" in the message, on a + // counter note whose lines never carried one. Two copies of a + // business rule is one more than this codebase should have. + const missing = quoteItemReadinessGaps(it); if (missing.length === 0) { ready.push(it); } else { @@ -627,3 +632,58 @@ export async function isQuotePdfStale(quoteId: string): Promise { // only the quote-level timestamp decides in that case. return (itemAt !== undefined && itemAt > pdfAt) || quoteAt > pdfAt; } + +/* ============================================================================ + * Shared readiness rule + * ========================================================================= */ + +/** + * What, if anything, is missing before a quote item can be priced and + * drawn on a customer-facing PDF. + * + * ## Why this is one function + * + * It was previously written out twice — once here for the PDF route, + * once in `quotes-service.ts` for the send-email route. The copies + * drifted: the `no supplier` requirement was removed from one and left + * in the other. A quote the send path happily accepted would then + * refuse to render, blaming a supplier on a counter note whose lines + * never carried one. Verified in the browser: quote Q202609631 failed + * with "no supplier" long after the requirement was dropped. + * + * ## Why no supplier + * + * A delivery note raised at the counter has no parent order, so its + * lines routinely have no supplier recorded. Requiring one refused + * the PDF for a field the counter form never asks for. The supplier + * is still PRINTED on the document when one is present. + * + * ## Why `is_processed` + * + * Kept deliberately. It is the flag that says a person looked at the + * line, and nothing else can set it on a delivery-note quote — the + * `quote_items` update whitelists exclude it. Dropping it would let a + * quote go out with lines nobody ever reviewed. + * + * ## `isNoQuote` + * + * A decline letter is explicitly unpriced, so the pricing gaps are + * skipped. The caller (the send-email path) still runs this so it can + * report on the decline. + */ +export function quoteItemReadinessGaps( + it: { + is_processed?: boolean; + cost_price?: number | null; + selling_price?: number | null; + }, + opts: { isNoQuote?: boolean } = {}, +): string[] { + const missing: string[] = []; + if (!opts.isNoQuote) { + if (!it.is_processed) missing.push("not processed"); + if (!(Number(it.cost_price) > 0)) missing.push("cost price"); + if (!(Number(it.selling_price) > 0)) missing.push("selling price"); + } + return missing; +} diff --git a/src/lib/server/quotes-service.test.ts b/src/lib/server/quotes-service.test.ts index ec9285e..34fb72b 100644 --- a/src/lib/server/quotes-service.test.ts +++ b/src/lib/server/quotes-service.test.ts @@ -247,12 +247,17 @@ const { pbMock, clearStore } = vi.hoisted(() => { return { store, pbMock, clearStore }; }); -vi.mock("./quote-pdf-service", () => { +vi.mock("./quote-pdf-service", async (importOriginal) => { // The `*Mock` aliases point at the SAME fns the service imports, so // tests can configure them via `await import("./quote-pdf-service")`. const isQuotePdfStale = vi.fn(); const generateQuotePdf = vi.fn(); return { + // Spread the real module first. `quoteItemReadinessGaps` is the + // ONE readiness rule both the PDF route and the send path use — + // it must be the real implementation, not a stub, or these tests + // would pass against a fake rule while production used another. + ...(await importOriginal()), isQuotePdfStale, generateQuotePdf, isQuotePdfStaleMock: isQuotePdfStale, diff --git a/src/lib/server/quotes-service.ts b/src/lib/server/quotes-service.ts index 8a08c14..14aa4be 100644 --- a/src/lib/server/quotes-service.ts +++ b/src/lib/server/quotes-service.ts @@ -1,4 +1,5 @@ import { getAdminPocketBase } from "./pocketbase-admin"; +import { quoteItemReadinessGaps } from "./quote-pdf-service"; import { compressFile } from "@/lib/server/attachment-compressor"; import { QUOTE_VALID_DAYS } from "@/lib/pdf/format"; import { createQuoteItem } from "./quote-items-service"; @@ -2012,24 +2013,14 @@ async function diagnosePdfReadiness( missing: string[]; }> = []; for (const it of allItems) { - const missing: string[] = []; - // Pricing / processed checks are skipped when the operator has - // marked this quote as a decline (`is_no_quote`) — they explicitly - // don't want a priced PDF. - if (!quote.is_no_quote) { - if (!it.is_processed) missing.push("not processed"); - if (!(Number(it.cost_price) > 0)) missing.push("cost price"); - if (!(Number(it.selling_price) > 0)) missing.push("selling price"); - } - // A missing supplier is NO LONGER a blocker. It used to be, and - // that was wrong for this workflow: a delivery note raised at the - // counter (no parent order, so no procurement trail) often has - // goods with no supplier recorded against the line at all. The - // block meant the operator could quote the note, see the quote - // appear, and then be refused a PDF for a field that the counter - // workflow never asks for. The supplier is still printed on the - // document when one IS present — we just no longer refuse to - // render without it. + // ONE readiness rule, shared with the PDF route via + // `quoteItemReadinessGaps`. It used to be written out here AND in + // quote-pdf-service.ts, and the two copies drifted — the supplier + // requirement was dropped from this one and left in that one, so a + // quote could be sent but never rendered. + const missing = quoteItemReadinessGaps(it, { + isNoQuote: Boolean(quote.is_no_quote), + }); if (missing.length > 0) { issues.push({ itemId: String(it.id),