From 3b926570c7d95e5893d30c269cbb62b33b8719b9 Mon Sep 17 00:00:00 2001
From: Dave
Company
-{quote.expand?.company?.company || "—"}
-Created
{formatDate(quote.created)}
diff --git a/src/app/(app)/quotes/[id]/process/page.tsx b/src/app/(app)/quotes/[id]/process/page.tsx index 12d26bb..19ce100 100644 --- a/src/app/(app)/quotes/[id]/process/page.tsx +++ b/src/app/(app)/quotes/[id]/process/page.tsx @@ -843,20 +843,43 @@ export default function QuoteProcessPage({ params }: { params: Promise<{ id: str /** * Swap the customer on this quote. Called by the `(id, { - expand: "customer", + expand: "customer,customer.company", }); setQuote(fresh); } catch (err) { diff --git a/src/app/(app)/quotes/approvals/[id]/page.tsx b/src/app/(app)/quotes/approvals/[id]/page.tsx index e036693..97143be 100644 --- a/src/app/(app)/quotes/approvals/[id]/page.tsx +++ b/src/app/(app)/quotes/approvals/[id]/page.tsx @@ -626,9 +626,14 @@ export default function ApprovalReviewPage() { } const customerName = quote.expand?.customer?.customer ?? "—"; - const companyName = quote.expand?.customer?.expand?.company?.company - ?? quote.expand?.company?.company - ?? ""; + // Company comes from the CUSTOMER, never from the quote's own + // `company` relation. That relation is a denormalised copy which + // used to go stale whenever the quote's customer was changed + // (Q202610315 showed "Mre" while its customer sat at Harmony). + // The customer cascade now keeps them in step, but falling back to + // the stale copy would just re-hide the same class of bug — and a + // missing company is a harmless blank, a wrong one is not. + const companyName = quote.expand?.customer?.expand?.company?.company ?? ""; const repName = quote.expand?.customer?.expand?.rep?.name ?? ""; // Frozen at fetch time so the value is stable across re-renders // (React's purity rule: no Date.now() during render). The rep sees diff --git a/src/app/api/v1/quotes/[id]/pdf/route.test.ts b/src/app/api/v1/quotes/[id]/pdf/route.test.ts new file mode 100644 index 0000000..fa1e3f5 --- /dev/null +++ b/src/app/api/v1/quotes/[id]/pdf/route.test.ts @@ -0,0 +1,138 @@ +import { describe, it, expect, beforeEach, vi } from "vitest"; +import { NextRequest } from "next/server"; + +/** + * Tests for `PATCH /api/v1/quotes/[id]/pdf` — the customer-swap branch. + * + * The route exists so the process page can replace a placeholder + * customer with the real one. It also owns the `quotes.company` + * cascade: `quotes.company` is a denormalised copy of the customer's + * company and PB does not re-cascade relations, so writing only + * `customer` leaves the quote pointing at the previous customer's + * company forever (see quote Q202610315, which held `company = Mre` + * while its customer sat at Harmony). + */ +const { getAdminPocketBase } = vi.hoisted(() => ({ + getAdminPocketBase: vi.fn(), +})); + +vi.mock("@/lib/pocketbase", () => ({ + default: vi.fn(), +})); + +vi.mock("@/lib/server/pocketbase-admin", () => ({ + getAdminPocketBase: (...args: unknown[]) => getAdminPocketBase(...args), +})); + +vi.mock("@/lib/server/quote-pdf-service", () => ({ + generateQuotePdf: vi.fn(), + isQuotePdfStale: vi.fn(), + QuotePdfError: class QuotePdfError extends Error {}, +})); + +import { PATCH } from "./route"; + +type Rec = Record; + +/** Minimal in-memory PB stand-in: customers + quotes collections. */ +function makePb(customers: Rec[], quotes: Rec[]) { + const update = vi.fn(async (id: string, data: Rec) => { + const row = quotes.find((q) => q.id === id); + if (!row) throw new Error("not found"); + Object.assign(row, data); + return { ...row }; + }); + const getOne = vi.fn(async (id: string) => { + const c = customers.find((x) => x.id === id); + if (!c) throw new Error("not found"); + return { ...c }; + }); + return { + collection: vi.fn((name: string) => + name === "customers" ? { getOne } : { update } + ), + __update: update, + }; +} + +function makeReq(id: string, body: unknown) { + return new NextRequest(`http://localhost/api/v1/quotes/${id}/pdf`, { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }); +} + +const CUSTOMER_WITH_COMPANY = { + id: "cust-harmony", + customer: "Pierre Visser", + email: "pierre.visser@harmony.co.za", + company: "comp-harmony", +}; +const CUSTOMER_NO_COMPANY = { + id: "cust-solo", + customer: "Walk-in Buyer", + email: "walkin@buyer.co.za", + company: "", +}; + +describe("PATCH /api/v1/quotes/[id]/pdf — customer swap", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("cascades quotes.company to the new customer's company", async () => { + const quote = { id: "q1", customer: "cust-old", company: "comp-mre" }; + const pb = makePb([CUSTOMER_WITH_COMPANY], [quote]); + getAdminPocketBase.mockResolvedValue(pb); + + const res = await PATCH(makeReq("q1", { customer: "cust-harmony" }), { + params: Promise.resolve({ id: "q1" }), + }); + + expect(res.status).toBe(200); + // The whole point — both relations move together. + expect(pb.__update).toHaveBeenCalledWith("q1", { + customer: "cust-harmony", + company: "comp-harmony", + }); + expect(quote.company).toBe("comp-harmony"); + }); + + it("clears the company when the new customer has none", async () => { + // Must not leave the PREVIOUS customer's company dangling. + const quote = { id: "q1", customer: "cust-old", company: "comp-mre" }; + const pb = makePb([CUSTOMER_NO_COMPANY], [quote]); + getAdminPocketBase.mockResolvedValue(pb); + + await PATCH(makeReq("q1", { customer: "cust-solo" }), { + params: Promise.resolve({ id: "q1" }), + }); + + expect(pb.__update).toHaveBeenCalledWith("q1", { + customer: "cust-solo", + company: "", + }); + expect(quote.company).toBe(""); + }); + + it("422s when no customer id is supplied", async () => { + const res = await PATCH(makeReq("q1", {}), { + params: Promise.resolve({ id: "q1" }), + }); + expect(res.status).toBe(422); + }); + + it("422s and writes nothing when the customer has no email", async () => { + const noEmail = { ...CUSTOMER_WITH_COMPANY, email: "" }; + const pb = makePb([noEmail], [{ id: "q1" }]); + getAdminPocketBase.mockResolvedValue(pb); + + const res = await PATCH(makeReq("q1", { customer: "cust-harmony" }), { + params: Promise.resolve({ id: "q1" }), + }); + + expect(res.status).toBe(422); + expect(pb.__update).not.toHaveBeenCalled(); + }); +}); \ No newline at end of file diff --git a/src/app/api/v1/quotes/[id]/pdf/route.ts b/src/app/api/v1/quotes/[id]/pdf/route.ts index 564e920..1f2fa8a 100644 --- a/src/app/api/v1/quotes/[id]/pdf/route.ts +++ b/src/app/api/v1/quotes/[id]/pdf/route.ts @@ -197,7 +197,7 @@ export async function PATCH( const pb = await getAdminPocketBase(); const customer = await pb .collection("customers") - .getOne<{ id: string; customer: string; email?: string }>(newCustomerId); + .getOne<{ id: string; customer: string; email?: string; company?: string }>(newCustomerId); // Same placeholder block as the PDF route. We also reject if // the new customer has no email — the operator would hit @@ -218,8 +218,23 @@ export async function PATCH( ); } + // Reassign BOTH relations together. + // + // `quotes.company` is a denormalised copy of the customer's + // company, and nothing in PB re-cascades it. Writing only + // `customer` left the quote pointing at the OLD customer's + // company forever: quote Q202610315 (changed to Pierre Visser at + // Harmony) kept `company = Mre`, so one quote held two + // contradictory companies. The customer-facing PDF reads + // `customer.expand.company` so the customer never saw it, which + // is exactly why it went unnoticed — but any consumer reading the + // quote's own `company` got the wrong answer. + // + // Empty company (customer has none) clears the relation rather + // than leaving the previous one dangling. const updated = await pb.collection("quotes").update(quoteId, { customer: customer.id, + company: customer.company ?? "", }); return NextResponse.json({ ok: true, quote: updated, customer }); } catch (err) { diff --git a/src/lib/server/delivery-notes-service.test.ts b/src/lib/server/delivery-notes-service.test.ts index c5fa4ca..4aaa348 100644 --- a/src/lib/server/delivery-notes-service.test.ts +++ b/src/lib/server/delivery-notes-service.test.ts @@ -244,7 +244,9 @@ describe("createQuoteFromDeliveryNote", () => { const quotes = Array.from(getOrCreate("quotes").values()); expect(quotes).toHaveLength(1); const q = quotes[0] as Record ; - expect(q.status).toBe("delivery_note"); + // Born APPROVED — a DN quote is never sent on its own, it goes out + // with the note. It must never enter the rep approval queue. + expect(q.status).toBe("approved"); expect(q.delivery_note).toBe(dnId); expect(q.customer).toBe("customer-1"); expect(q.company).toBe("company-1"); @@ -412,6 +414,26 @@ describe("createQuoteFromDeliveryNote", () => { ); }); + it("still blocks a duplicate once the existing quote has been sent", async () => { + // Regression pin. The guard used to also require + // `status = "delivery_note"`, so a quote that had already gone out + // (`emailed`) fell straight through it — a second "Create quote" + // click would then raise a duplicate invoice for the same note. + // The guard is now relation-only and strictly broader than the UI. + const dnId = seedDeliveryNote(); + seedOrderItem("DN26071401"); + getOrCreate("quotes").set("already-sent", { + id: "already-sent", + quote_number: "Q202607002", + status: "emailed", + delivery_note: dnId, + }); + + await expect(createQuoteFromDeliveryNote(dnId)).rejects.toThrow( + /already exists/, + ); + }); + it("throws when the delivery note has no order_items", async () => { const dnId = seedDeliveryNote(); // No seedOrderItem call → the items lookup returns []. diff --git a/src/lib/server/delivery-notes-service.ts b/src/lib/server/delivery-notes-service.ts index 528b14d..ae60249 100644 --- a/src/lib/server/delivery-notes-service.ts +++ b/src/lib/server/delivery-notes-service.ts @@ -258,11 +258,21 @@ export async function createQuoteFromDeliveryNote( // status filter is a belt-and-braces check — the operator could // in theory have transitioned the previous quote to "sent" or // beyond, in which case the relationship is no longer "active". - // We look for the *primary* quote (status="delivery_note" with - // delivery_note=dn.id) — a quote that has been sent and is now - // in flight doesn't count. + // Duplicate guard: has THIS note already got a quote? + // + // Match on the relation alone. The old filter also pinned + // `status = "delivery_note"`, which quietly scoped the guard to + // quotes that had never been sent — so once the quote went + // `emailed`, this returned nothing and a second quote could be + // raised for the same note (duplicate invoice to the customer). + // It also broke the moment DN quotes stopped being created in + // `delivery_note` status. + // + // The UI already hides "Create quote" when `hasQuote`, so this + // server-side check is the real backstop — it must be strictly + // broader than the UI gate, not narrower. const existing = await pb.collection(QUOTES).getList (1, 1, { - filter: `delivery_note = "${deliveryNoteId}" && status = "delivery_note"`, + filter: `delivery_note = "${deliveryNoteId}"`, }); if (existing.items.length > 0) { const q = existing.items[0]; @@ -289,7 +299,27 @@ export async function createQuoteFromDeliveryNote( // Create the quote header. const quoteCreate: Record= { quote_number: quoteNumber, - status: "delivery_note", + // Created APPROVED — the quote skips the approval queue. + // + // Why: a delivery-note quote is never sent on its own. The office + // creates it off the note ("Create quote"), prices it in the same + // dialog, and then sends DN + quote together with the note's own + // "Send" button. There is no separate quote-send step and no rep + // in the loop — the goods have already physically gone out with + // the driver, so an approval round-trip is pure friction on an + // invoice the customer is already liable for. + // + // This used to create as `delivery_note`, which was then silently + // overwritten to `approval_pending` by the PDF render + // (`generateQuotePdf` flips every non-terminal quote). That's how + // DN quotes ended up clogging the rep's Pending Approvals queue. + // The quote is now born approved, and the PDF render leaves + // `approved` alone — so the quote never enters the approval queue. + // + // `approved` is in `RESENDABLE_STATUSES`, so the note's Send button + // (which POSTs `/api/v1/quotes/[id]/send-email`) can send it + // straight through to `sending` → `emailed`. + status: "approved", delivery_note: dn.id, // customer/company are required on the Quote collection schema. // The delivery note has both, so we just copy through. diff --git a/src/lib/server/order-delivery-note-link-service.ts b/src/lib/server/order-delivery-note-link-service.ts index 1ef4622..bf09008 100644 --- a/src/lib/server/order-delivery-note-link-service.ts +++ b/src/lib/server/order-delivery-note-link-service.ts @@ -205,7 +205,7 @@ function mostRecent(candidates: DnCandidate[]): DnCandidate { } /** Among tied exact-item-match candidates, prefer the one whose priced - * quote (status="delivery_note") also matches the order's line + * quote (the quote linked to that note) also matches the order's line * prices. Returns null if no candidate's quote prices match — caller * falls back to most-recent. */ async function breakTieByQuotePrice( @@ -222,10 +222,13 @@ async function breakTieByQuotePrice( } for (const dn of candidates) { + // Status-agnostic on purpose: we want the note's quote whatever + // stage it's at. It used to pin `status = "delivery_note"`, which + // stopped matching as soon as the quote was approved or sent. const quotes = await pb .collection(QUOTES_COLLECTION) .getFullList<{ id: string; delivery_note: string }>({ - filter: `delivery_note = "${esc(dn.id)}" && status = "delivery_note"`, + filter: `delivery_note = "${esc(dn.id)}"`, fields: "id,delivery_note", batch: 5, }); diff --git a/src/lib/server/quote-pdf-service.test.ts b/src/lib/server/quote-pdf-service.test.ts index 2fd0fdf..781e9cf 100644 --- a/src/lib/server/quote-pdf-service.test.ts +++ b/src/lib/server/quote-pdf-service.test.ts @@ -893,6 +893,117 @@ describe("generateQuotePdf", () => { }); }); +// ───────────────────────────────────────────────────────────────────────────── +// generateQuotePdf — delivery-note quotes skip the approval queue +// +// Regression pin for the 2026-10-16 bug. The render used to auto-flip EVERY +// non-terminal quote to `approval_pending`, which forced every DN quote into +// the rep's approval queue before it could be sent — defeating the +// skip-approval path in `sendQuoteEmail` (which gates on the quote still +// being `delivery_note` / `claimed` / `processed`). +// ───────────────────────────────────────────────────────────────────────────── + +describe("generateQuotePdf — delivery-note exemption", () => { + /** + * A DN quote mid-flow: real customer, a fully priced line, and the + * `delivery_note` relation set. `status` is what the assertion pins. + */ + async function seedDnQuote(status: "delivery_note" | "claimed" | "processed") { + const customer = await pbMock.collection("customers").create({ + customer: "Pierre Visser", + email: "pierre.visser@harmony.co.za", + company: "Harmony", + dateadded: "", + lastvisiteddate: "", + rep: "", + coordinates: "", + description: "", + alternative_email: "", + buyer_email: "", + number: "", + }); + const quote = await pbMock.collection("quotes").create({ + quote_number: "Q202610315", + customer: customer.id, + status, + delivery_note: "dn-abc123", + notes: "", + email_sender: "", + email_subject: "", + email_body: "", + has_attachments: false, + company_letterhead: "ISHERWOOD", + }); + await pbMock.collection("quote_items").create({ + quote: quote.id, + description: "M16 hex bolt", + quantity: 1, + cost_price: 2.39, + selling_price: 3.11, + markup: 30, + supplier: "sup-acme", + is_processed: true, + moved_to_quote: null, + }); + return quote; + } + + it("does NOT flip a delivery_note quote into approval_pending", async () => { + const quote = await seedDnQuote("delivery_note"); + + const outcome = await generateQuotePdf(quote.id, { letterhead: "ISHERWOOD" }); + expect(outcome.kind).toBe("ok"); + if (outcome.kind !== "ok") throw new Error("expected ok outcome"); + + // The whole point: the status survives the render so the operator's + // send button hits the DN branch in `sendQuoteEmail`. + expect(outcome.status).toBe("delivery_note"); + + const after = await pbMock + .collection("quotes") + .getOne<{ status: string; pdf_generated_at: string }>(quote.id); + expect(after.status).toBe("delivery_note"); + // The PDF timestamp still gets stamped — `sendQuoteEmail`'s staleness + // check depends on it being fresh. + expect(after.pdf_generated_at).toBeTruthy(); + }); + + it("keeps a claimed delivery-note quote claimed (no approval flip)", async () => { + const quote = await seedDnQuote("claimed"); + const outcome = await generateQuotePdf(quote.id, { letterhead: "CNL" }); + expect(outcome.kind).toBe("ok"); + if (outcome.kind !== "ok") throw new Error("expected ok outcome"); + expect(outcome.status).toBe("claimed"); + + const after = await pbMock.collection("quotes").getOne<{ status: string }>(quote.id); + expect(after.status).toBe("claimed"); + }); + + it("still flips a REGULAR quote to approval_pending", async () => { + // Guard against over-correcting: the exemption must be scoped to DN + // quotes. A normal quote with no `delivery_note` keeps the old + // auto-submit behaviour. + const { quote } = await seedQuoteWithPdf(); + await pbMock.collection("quotes").update(quote.id, { status: "processed" }); + await pbMock.collection("quote_items").create({ + quote: quote.id, + description: "M16 hex bolt", + quantity: 1, + cost_price: 2.39, + selling_price: 3.11, + markup: 30, + supplier: "sup-acme", + is_processed: true, + moved_to_quote: null, + }); + + const outcome = await generateQuotePdf(quote.id, { letterhead: "CNL" }); + expect(outcome.kind).toBe("ok"); + if (outcome.kind !== "ok") throw new Error("expected ok outcome"); + expect(outcome.status).toBe("approval_pending"); + }); +}); + // ───────────────────────────────────────────────────────────────────────────── // generateQuotePdf — auto-claim (Option 1) // diff --git a/src/lib/server/quote-pdf-service.ts b/src/lib/server/quote-pdf-service.ts index 1c4aaee..bd97626 100644 --- a/src/lib/server/quote-pdf-service.ts +++ b/src/lib/server/quote-pdf-service.ts @@ -46,6 +46,9 @@ import type { * implementation — PDF generation is the moment the quote * becomes "ready for rep review" and replaces the old * "Submit for Approval" button. + * - EXCEPTION (2026-10-16): delivery-note quotes + * (`quote.delivery_note` set) are NOT flipped. They skip the + * approval queue entirely — see the flip block below. * - When the quote is already in `approval_pending` / * `partial_approval_pending`, the route does an idempotent * regen — `pdf_generated_at` gets bumped but status stays @@ -477,9 +480,28 @@ export async function generateQuotePdf( : {}; let newStatus = quote.status; let pdfGeneratedAt: string | null = quote.pdf_generated_at || null; + /** + * Delivery-note quotes skip the approval queue by design (see + * `sendQuoteEmail`'s `isDeliveryNoteQuote` branch — it accepts + * `delivery_note` / `claimed` / `processed` and flips straight to + * `sending`). That design was silently defeated here: rendering the + * PDF is the operator's only pricing step for a DN quote, and this + * function auto-flipped ANY non-terminal quote to + * `approval_pending`, so every DN quote was pushed into the rep's + * queue before it could ever be sent. The skip-approval path was + * unreachable — the status it gates on had already been changed. + * + * Fix 2026-10-16: exempt DN quotes from the flip. They keep their + * own status (`delivery_note` → `claimed` → `processed`) while the + * PDF is stamped, so the operator's send button on the quote page + * hits the DN branch and goes straight out. The approval queue now + * contains only quotes that genuinely need a rep's eyes. + */ + const isDeliveryNoteQuote = !!quote.delivery_note; if ( TERMINAL_STATUSES.has(quote.status) || - APPROVAL_STATES.has(quote.status) + APPROVAL_STATES.has(quote.status) || + isDeliveryNoteQuote ) { // Idempotent / no-flip path: still update the timestamp so // we know the PDF was regenerated, but don't change the