From 97c3cd4d7e8f7afb1d556d242bd53a43f8d92b8c Mon Sep 17 00:00:00 2001 From: Dave Date: Sun, 4 Oct 2026 21:17:31 +0200 Subject: [PATCH] feat(delivery-notes): review pricing before a quote is created MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Create quote button fired blind: the quote appeared with whatever pricing happened to be on the order items, and a wrong price was only discoverable by finding the quote afterwards. A new review modal puts the check in front of the create. Modal (components/delivery-notes/quote-review-dialog.tsx) - Letterhead picker. DN-backed quotes never set company_letterhead, so the renderer defaulted every one of them to CNL. - Cost and markup per line; selling price is DERIVED, never typed, so the two cannot disagree. See src/lib/quote-pricing.ts. - A no-cost line shows "No cost on file" and stays at R0.00 rather than deriving a fake price off a zero cost, which reads as a deliberate free-of-charge item. - Bulk "set all markups" field — typing 25% into eight lines one at a time is the tedium the modal exists to remove. - Totals, with margin as a share of REVENUE not cost: 25% markup is a 20% margin, and conflating them quietly under-prices a job. - Nothing persists until Generate. Cancel discards the whole grid. Backend - create-quote now accepts { letterhead, items: [{ orderItemId, costPrice, markupPercent }] } and derives selling server-side. - Overrides are keyed by order_item id and validated against THIS note's lines. An unknown id aborts the create rather than moving another order's goods into this customer's quote. - Quote items now go out as one PocketBase batch instead of a loop of individual creates. The batch is transactional: the old loop could fail on line 5 of 8 and leave a real quote header attached to half its lines. The header still has to be created first (the batch API can't reference a record created in the same batch), so a failed batch deletes the orphaned header rather than leaving it visible. - Malformed bodies are 400s the operator can read, not 500s. "A quote already exists" and "not in draft" are now 409s. Tests - 33 new: pricing maths, and the route's validation guards. Verified the guards bite by removing them (2 fail) and restored. - The existing service test's PB mock gained createBatch, modelled as genuinely all-or-nothing — the compensation path depends on that guarantee being real. Full suite: 3615 passing. tsc clean. --- src/app/(app)/delivery-notes/page.tsx | 118 ++--- .../[id]/create-quote/route.test.ts | 135 ++++++ .../delivery-notes/[id]/create-quote/route.ts | 113 ++++- .../delivery-notes/quote-review-dialog.tsx | 447 ++++++++++++++++++ src/lib/quote-pricing.test.ts | 211 +++++++++ src/lib/quote-pricing.ts | 189 ++++++++ src/lib/server/delivery-notes-service.test.ts | 57 +++ src/lib/server/delivery-notes-service.ts | 128 ++++- 8 files changed, 1317 insertions(+), 81 deletions(-) create mode 100644 src/app/api/v1/delivery-notes/[id]/create-quote/route.test.ts create mode 100644 src/components/delivery-notes/quote-review-dialog.tsx create mode 100644 src/lib/quote-pricing.test.ts create mode 100644 src/lib/quote-pricing.ts diff --git a/src/app/(app)/delivery-notes/page.tsx b/src/app/(app)/delivery-notes/page.tsx index 2d69546..94ffd20 100644 --- a/src/app/(app)/delivery-notes/page.tsx +++ b/src/app/(app)/delivery-notes/page.tsx @@ -19,6 +19,7 @@ import { } from "@/components/ui/dialog"; import { CreateAdHocDeliveryNoteDialog } from "@/components/delivery-notes/create-adhoc-dialog"; import { DeliveryNoteItemsExpanded } from "@/components/delivery-notes/items-expanded"; +import { QuoteReviewDialog } from "@/components/delivery-notes/quote-review-dialog"; import { PdfPreviewDialog, QuotePdfPreviewDialog, @@ -215,6 +216,16 @@ export default function DeliveryNotesPage() { // navigates to the new delivery note's detail view. const [createOpen, setCreateOpen] = useState(false); + // "Create quote" review modal. Holds enough of the note to label + // the modal and to refresh the right row afterwards; the pricing + // grid itself is owned by the dialog. + const [quoteReview, setQuoteReview] = useState<{ + open: boolean; + noteId: string; + number: string; + customer: string; + }>({ open: false, noteId: "", number: "", customer: "" }); + // PDF preview dialog state. The D icon on every row opens // this instead of opening the PDF in a new tab — operator // stays in the list, can compare two DNs side by side, doesn't @@ -663,74 +674,42 @@ export default function DeliveryNotesPage() { }, []); // ── Action: Create Quote from Delivery Note ───────────────────── - // Calls the API route, which creates a Quote with status - // "delivery_note" + a quote_item per order_item, then flips the - // delivery note's status to "sent". On success we navigate the - // operator to the new quote so they can fill in prices. The - // server returns the new quote's id + quote_number. - const handleCreateQuote = useCallback( - async (note: DeliveryNote) => { + // The Q icon no longer fires the create blind. It opens the review + // modal, where the operator confirms letterhead, cost and markup + // per line, and the quote is created from what they see. The modal + // POSTs itself; this handler only owns the state around it. + const openQuoteReview = useCallback((note: DeliveryNote) => { + setQuoteReview({ + open: true, + noteId: note.id, + number: note.delivery_note_number, + customer: note.expand?.customer?.customer ?? "", + }); + }, []); + + // Fired after the modal has created the quote. Refreshes the list + // so the row's Q icon flips to "open" mode and the status badge + // changes, then auto-opens the new quote's PDF so the operator can + // print or download it straight away. + const handleQuoteCreated = useCallback( + async (note: DeliveryNote, result: { id: string; quote_number: string }) => { markBusy(note.id); try { - const res = await fetch(`/api/v1/delivery-notes/${note.id}/create-quote`, { - method: "POST", - }); - if (!res.ok) { - let message = `Failed to create quote (${res.status})`; - try { - const data: unknown = await res.json(); - if ( - data && - typeof data === "object" && - "error" in data && - typeof (data as { error?: unknown }).error === "string" - ) { - message = (data as { error: string }).error; - } - } catch { - // ignore non-json - } - toast.error(message); - return; - } - const body = (await res.json()) as { - id: string; - quote_number: string; - }; - // Stay on the list page — the user explicitly asked - // for "create the quote here or there, without - // opening the quotes card". We refresh so the row's - // Q icon switches from "create" to "open" mode and - // the status badge flips to "Sent", then auto-open - // the new quote's PDF in the preview dialog so the - // operator can immediately print/download the - // freshly-generated PDF (or move on — the dialog - // has a Close button). - const quoteId = body.id; - const quoteNumber = body.quote_number; await fetchNotes(); - // Read the new quote back from the refreshed - // `quoteByDnId` map so the preview dialog has the - // full record (letterhead, customer, etc.) and - // doesn't need a second round-trip. The map is - // updated by `fetchNotes` before this line runs - // because `fetchNotes` awaits the refresh. const newQuote = quoteByDnId.get(note.id) ?? { - id: quoteId, - quote_number: quoteNumber, + id: result.id, + quote_number: result.quote_number, company_letterhead: "", } as Quote; setPreviewQuote(newQuote); toast.success( - `Created quote ${quoteNumber} for ${note.delivery_note_number}`, + `Created quote ${result.quote_number} for ${note.delivery_note_number}`, ); - } catch (err: unknown) { - toast.error(err instanceof Error ? err.message : "Failed to create quote"); } finally { markIdle(note.id); } }, - [fetchNotes, markBusy, markIdle, quoteByDnId, router], + [fetchNotes, markBusy, markIdle, quoteByDnId], ); // ── Action: Capture Customer PO ───────────────────────────────── @@ -935,10 +914,10 @@ export default function DeliveryNotesPage() { + ))} + + + + {/* Bulk markup. Typing a percentage on eight lines one at + a time is the tedium this whole modal exists to remove. */} +
+
+ + { + if (e.key !== "Enter") return; + e.preventDefault(); + const v = Number((e.target as HTMLInputElement).value); + if (Number.isFinite(v)) applyMarkupToAll(v); + }} + onBlur={(e) => { + const v = Number(e.target.value); + if (Number.isFinite(v) && e.target.value !== "") { + applyMarkupToAll(v); + } + }} + /> +
+

+ Press Enter or click away to apply to every costed line. +

+
+ + {/* Price grid. Cost and markup are the inputs; selling is + derived and shown, not typed. */} +
+ + + + + + + + + + + + + {lines.map((line, index) => { + const costed = hasCostPrice(line.costPrice); + return ( + + + + + + + + + ); + })} + +
ItemQtyCostMarkup %SellingLine total
+
+ {line.description || "(no description)"} +
+ {!costed && ( +
+ + No cost on file +
+ )} + 		+ {line.quantity} + + + updateLine(index, { + costPrice: Number(e.target.value) || 0, + }) + } + className="h-8 text-right tabular-nums" + /> + + + updateLine(index, { + markupPercent: Number(e.target.value) || 0, + }) + } + className="h-8 text-right tabular-nums" + /> + + {costed ? formatCurrency(line.sellingPrice) : "—"} + + {costed + ? formatCurrency(line.sellingPrice * line.quantity) + : "—"} +
+
+ + {/* Totals. Margin is shown as a share of REVENUE, not of + cost — 25% markup is a 20% margin, and conflating them + is the classic way to quietly under-price a job. */} +
+ + Total cost{" "} + + {formatCurrency(totals.costTotal)} + + + + Total selling{" "} + + {formatCurrency(totals.sellingTotal)} + + + + Margin{" "} + 0 + ? "font-semibold tabular-nums text-success" + : "font-semibold tabular-nums text-muted-foreground" + } + > + {formatCurrency(totals.marginValue)} ( + {totals.marginPercent.toFixed(1)}%) + + +
+ + {unpriced.length > 0 && ( +

+ + {unpriced.length} line{unpriced.length === 1 ? " has" : "s have"} no + cost on file. Enter a cost price before sending, or the quote will + go out with {unpriced.length === 1 ? "that line" : "those lines"} at + R0.00. +

+ )} + + )} + + + + + + + + ); +} diff --git a/src/lib/quote-pricing.test.ts b/src/lib/quote-pricing.test.ts new file mode 100644 index 0000000..e33d4ca --- /dev/null +++ b/src/lib/quote-pricing.test.ts @@ -0,0 +1,211 @@ +import { describe, it, expect } from "vitest"; +import { + buildLinePricing, + DEFAULT_MARKUP_PERCENT, + hasCostPrice, + markupFromCostAndSelling, + repriceLine, + round2, + sellingFromCostAndMarkup, + totalsForLines, +} from "./quote-pricing"; + +/** + * A wrong selling price here is invisible: it renders as a plausible + * number, the total still sums, and no screen says "error". These + * tests pin the arithmetic that the review modal's totals depend on. + */ + +describe("round2", () => { + it("kills float drift rather than surfacing it", () => { + // 100 * 1.15 is 114.99999999999999 in IEEE-754. That has to reach + // the quote as 115, not as a 14-decimal number. + expect(sellingFromCostAndMarkup(100, 15)).toBe(115); + expect(round2(0.1 + 0.2)).toBe(0.3); + }); + + it("rounds half away from zero, not to even", () => { + expect(round2(1.005)).toBe(1.01); + expect(round2(2.675)).toBe(2.68); + }); + + it("coerces non-finite input to 0 rather than NaN", () => { + expect(round2(Number.NaN)).toBe(0); + expect(round2(Number.POSITIVE_INFINITY)).toBe(0); + }); +}); + +describe("markup convention", () => { + it("treats markup as a PERCENTAGE, not a multiplier", () => { + // 25 means 25%, so cost 100 -> 125. Getting this backwards turns + // a 25% margin into a 25x price and nobody would notice until + // someone tried to pay it. + expect(sellingFromCostAndMarkup(100, 25)).toBe(125); + expect(sellingFromCostAndMarkup(100, 0)).toBe(100); + expect(sellingFromCostAndMarkup(100, 100)).toBe(200); + }); + + it("round-trips cost -> selling -> markup", () => { + for (const [cost, markup] of [ + [100, 25], + [57.5, 33.3], + [1234.56, 18.75], + [1, 99], + ] as const) { + const selling = sellingFromCostAndMarkup(cost, markup); + const back = markupFromCostAndSelling(cost, selling); + expect(Math.abs(back - markup)).toBeLessThan(0.02); + } + }); + + it("reports zero markup for a zero cost instead of dividing by zero", () => { + expect(markupFromCostAndSelling(0, 500)).toBe(0); + }); +}); + +describe("hasCostPrice", () => { + it("requires a positive finite number", () => { + expect(hasCostPrice(0)).toBe(false); + expect(hasCostPrice(null)).toBe(false); + expect(hasCostPrice(undefined)).toBe(false); + expect(hasCostPrice(Number.NaN)).toBe(false); + expect(hasCostPrice(0.01)).toBe(true); + }); +}); + +describe("buildLinePricing", () => { + it("applies the default markup to a cost-only line", () => { + const [line] = buildLinePricing([ + { id: "i1", description: "Bolt", quantity: 2, cost_price: 100 }, + ]); + expect(line).toMatchObject({ + orderItemId: "i1", + quantity: 2, + costPrice: 100, + markupPercent: DEFAULT_MARKUP_PERCENT, + sellingPrice: 125, + }); + }); + + it("respects an EXISTING selling price instead of re-marking it", () => { + // A human priced this before the delivery note existed. Silently + // replacing it with the default markup would overwrite a decision. + const [line] = buildLinePricing([ + { id: "i1", cost_price: 100, selling_price: 160 }, + ]); + expect(line.sellingPrice).toBe(160); + expect(line.markupPercent).toBe(60); + }); + + it("leaves a no-cost line at zero rather than quoting a fake R0.00", () => { + // cost 0 x (1 + 25%) is still 0 — and a selling price of 0 looks + // like a deliberate free-of-charge item. It has to read as + // "no cost on file" in the UI, which means a real 0 here. + const [line] = buildLinePricing([{ id: "i1", cost_price: 0 }], 25); + expect(line.costPrice).toBe(0); + expect(line.markupPercent).toBe(0); + expect(line.sellingPrice).toBe(0); + expect(hasCostPrice(line.costPrice)).toBe(false); + }); + + it("defaults a missing quantity to 1", () => { + const [line] = buildLinePricing([{ id: "i1", cost_price: 50 }]); + expect(line.quantity).toBe(1); + }); + + it("maps 1:1 in the order it was given", () => { + const lines = buildLinePricing([ + { id: "a", cost_price: 10 }, + { id: "b", cost_price: 20 }, + { id: "c", cost_price: 0 }, + ]); + expect(lines.map((l) => l.orderItemId)).toEqual(["a", "b", "c"]); + }); +}); + +describe("repriceLine", () => { + const base = { + orderItemId: "i1", + description: "Bolt", + quantity: 1, + costPrice: 100, + markupPercent: 25, + sellingPrice: 125, + }; + + it("re-derives selling when the markup changes", () => { + expect(repriceLine(base, { markupPercent: 40 }).sellingPrice).toBe(140); + }); + + it("re-derives selling when the cost changes", () => { + const r = repriceLine(base, { costPrice: 200 }); + expect(r.sellingPrice).toBe(250); + expect(r.markupPercent).toBe(25); + }); + + it("collapses to zero when the cost is zeroed out", () => { + // Editing cost to 0 must NOT leave the old selling price hanging + // on the line — that is a R125 charge on a free item. + const r = repriceLine(base, { costPrice: 0 }); + expect(r.sellingPrice).toBe(0); + }); + + it("does not mutate the input line", () => { + repriceLine(base, { markupPercent: 99 }); + expect(base.sellingPrice).toBe(125); + expect(base.markupPercent).toBe(25); + }); + + it("leaves untouched fields alone", () => { + const r = repriceLine(base, { markupPercent: 50 }); + expect(r.orderItemId).toBe("i1"); + expect(r.description).toBe("Bolt"); + expect(r.quantity).toBe(1); + }); +}); + +describe("totalsForLines", () => { + it("multiplies by quantity and sums both sides", () => { + // 100 @ 25% x3 = 375, and 50 @ 25% x2 = 125. + const totals = totalsForLines([ + { ...buildLinePricing([{ id: "a", cost_price: 100, quantity: 3 }])[0] }, + { ...buildLinePricing([{ id: "b", cost_price: 50, quantity: 2 }])[0] }, + ]); + expect(totals.costTotal).toBe(400); + expect(totals.sellingTotal).toBe(500); + expect(totals.marginValue).toBe(100); + }); + + it("reports margin as a share of REVENUE, not of cost", () => { + // cost 100 -> selling 125 is a 25% markup but only a 20% margin. + // Conflating the two is the classic pricing mistake. + const totals = totalsForLines([ + buildLinePricing([{ id: "a", cost_price: 100, selling_price: 125 }])[0], + ]); + expect(totals.marginPercent).toBe(20); + }); + + it("is zero-safe on an empty set and on no-revenue lines", () => { + expect(totalsForLines([])).toEqual({ + costTotal: 0, + sellingTotal: 0, + marginValue: 0, + marginPercent: 0, + }); + const noCost = totalsForLines([ + buildLinePricing([{ id: "a", cost_price: 0 }])[0], + ]); + expect(noCost.marginPercent).toBe(0); + }); + + it("keeps cost and selling totals in step after an edit", () => { + // The modal recomputes these live on every keystroke, so an + // inconsistency here shows up as footer numbers that don't match + // the visible rows. + let lines = buildLinePricing([{ id: "a", cost_price: 100, quantity: 4 }]); + lines = [repriceLine(lines[0], { markupPercent: 30 })]; + const totals = totalsForLines(lines); + expect(totals.costTotal).toBe(400); + expect(totals.sellingTotal).toBe(520); + }); +}); diff --git a/src/lib/quote-pricing.ts b/src/lib/quote-pricing.ts new file mode 100644 index 0000000..21b6f8f --- /dev/null +++ b/src/lib/quote-pricing.ts @@ -0,0 +1,189 @@ +/** + * Quote pricing maths for the "create quote from a delivery note" + * review modal. + * + * Pulled into its own module because this is the one place in the + * create-quote flow where a wrong answer is invisible: a selling + * price that's out by a few cents still renders as a plausible + * number, the total still sums, and nothing anywhere says "error". + * The operator only finds out when the customer is invoiced. + * + * The house rule, used consistently across `quotes.create`, + * `bulk_update_quote_items` and `quote_items.create`: + * + * selling = cost x (1 + markup / 100) + * + * Markup is the INPUT; selling price is DERIVED. Letting both be + * typed independently is how a 25% markup and a 30% selling price + * end up disagreeing on the same line with nothing to flag it. + */ + +/** Round to 2dp, avoiding the float drift that makes 100*1.15 come + * out as 114.99999999999999. */ +export function round2(value: number): number { + if (!Number.isFinite(value)) return 0; + return Math.round((value + Number.EPSILON) * 100) / 100; +} + +/** + * Selling price for a cost + markup pair. Markup is a percentage, so + * 25 means 25% (i.e. cost * 1.25), NOT 25x — the same convention the + * rest of the codebase uses. + */ +export function sellingFromCostAndMarkup( + cost: number, + markupPercent: number, +): number { + const costNum = Number(cost) || 0; + const markupNum = Number(markupPercent) || 0; + return round2(costNum * (1 + markupNum / 100)); +} + +/** + * The markup percentage implied by a cost + selling pair. The inverse + * of the above. Returns 0 for a zero cost rather than dividing by + * zero — a line with no cost price genuinely has no margin to quote. + */ +export function markupFromCostAndSelling( + cost: number, + selling: number, +): number { + const costNum = Number(cost) || 0; + if (costNum <= 0) return 0; + const sellingNum = Number(selling) || 0; + return round2(((sellingNum - costNum) / costNum) * 100); +} + +/** True when a line has a usable, fixed cost price to work from. */ +export function hasCostPrice(cost: number | null | undefined): boolean { + const n = Number(cost); + return Number.isFinite(n) && n > 0; +} + +/** + * Default markup to seed a line with when the operator hasn't chosen + * one. Deliberately a constant rather than a per-customer or + * per-category lookup: there is no markup table in the schema today, + * and inventing a "smart" default would be guessing at money. + */ +export const DEFAULT_MARKUP_PERCENT = 25; + +export interface LinePricing { + orderItemId: string; + description: string; + quantity: number; + /** Cost carried from the order item. 0 means "nothing on file". */ + costPrice: number; + /** Operator-editable. */ + markupPercent: number; + /** Derived from cost + markup; never typed independently. */ + sellingPrice: number; +} + +export interface LineTotals { + costTotal: number; + sellingTotal: number; + /** sellingTotal - costTotal */ + marginValue: number; + /** margin as a percentage of selling total. 0 when there's no revenue. */ + marginPercent: number; +} + +/** Roll a set of lines up into the footer figures. */ +export function totalsForLines(lines: LinePricing[]): LineTotals { + let costTotal = 0; + let sellingTotal = 0; + for (const line of lines) { + const qty = Number(line.quantity) || 0; + costTotal += (Number(line.costPrice) || 0) * qty; + sellingTotal += (Number(line.sellingPrice) || 0) * qty; + } + const marginValue = round2(sellingTotal - costTotal); + return { + costTotal: round2(costTotal), + sellingTotal: round2(sellingTotal), + marginValue, + marginPercent: + sellingTotal > 0 ? round2((marginValue / sellingTotal) * 100) : 0, + }; +} + +/** + * Build the opening price grid for a delivery note's lines. + * + * `fallbackMarkup` is applied only where a real cost exists — a line + * with no cost gets markup 0 and selling 0, because deriving a selling + * price from a zero cost produces a confidently wrong R0.00 that looks + * like a deliberate free-of-charge item. Those lines are surfaced as + * "no cost on file" in the UI instead. + */ +export function buildLinePricing( + items: Array<{ + id: string; + description?: string | null; + quantity?: number | null; + cost_price?: number | null; + selling_price?: number | null; + }>, + fallbackMarkup: number = DEFAULT_MARKUP_PERCENT, +): LinePricing[] { + return items.map((it) => { + const costPrice = round2(Number(it.cost_price) || 0); + const quantity = Number(it.quantity) || 1; + + // A line that already carries a selling price keeps it — the + // office may have priced it deliberately before the DN existed, + // and silently re-marking it to a default would override a human + // decision. Only cost-only lines get the default markup. + const existingSelling = round2(Number(it.selling_price) || 0); + const hasCost = hasCostPrice(costPrice); + + if (existingSelling > 0) { + return { + orderItemId: it.id, + description: it.description ?? "", + quantity, + costPrice, + markupPercent: markupFromCostAndSelling(costPrice, existingSelling), + sellingPrice: existingSelling, + }; + } + + const markupPercent = hasCost ? fallbackMarkup : 0; + return { + orderItemId: it.id, + description: it.description ?? "", + quantity, + costPrice, + markupPercent, + sellingPrice: hasCost + ? sellingFromCostAndMarkup(costPrice, markupPercent) + : 0, + }; + }); +} + +/** + * Re-derive one line's selling price after its cost or markup changed. + * Kept as a pure function so the modal's edit handler is a single + * call and the derivation rule lives in exactly one place. + */ +export function repriceLine( + line: LinePricing, + patch: { costPrice?: number; markupPercent?: number }, +): LinePricing { + const costPrice = + patch.costPrice !== undefined ? round2(Number(patch.costPrice) || 0) : line.costPrice; + const markupPercent = + patch.markupPercent !== undefined + ? Number(patch.markupPercent) || 0 + : line.markupPercent; + return { + ...line, + costPrice, + markupPercent, + sellingPrice: hasCostPrice(costPrice) + ? sellingFromCostAndMarkup(costPrice, markupPercent) + : 0, + }; +} diff --git a/src/lib/server/delivery-notes-service.test.ts b/src/lib/server/delivery-notes-service.test.ts index 956dd89..1d1b5e2 100644 --- a/src/lib/server/delivery-notes-service.test.ts +++ b/src/lib/server/delivery-notes-service.test.ts @@ -100,7 +100,64 @@ const { store, getOrCreate, where, pbMock, clearStore } = vi.hoisted(() => { Object.assign(item, data); return item; }, + async delete(id: string) { + getOrCreate(name).delete(id); + return true; + }, }), + /** + * Transactional batch, matching PocketBase's real API. The + * create-quote path builds every quote_item in one batch rather + * than a loop of individual creates, so the mock has to honour + * it — without this the queued creates go nowhere and the tests + * that assert on quote_items fail with an empty result. + * + * All-or-nothing is modelled too: a create that throws poisons + * the batch so `send()` applies nothing. That is the real + * guarantee, and the compensation path in the service depends on + * it being real rather than best-effort. + */ + createBatch() { + const queued: Array<{ + name: string; + run: () => Promise>; + }> = []; + return { + collection(name: string) { + return { + create(data: Record) { + queued.push({ + name, + run: async () => { + const id = `mock-${Math.random().toString(36).slice(2, 10)}`; + const item = { id, ...data }; + getOrCreate(name).set(id, item); + return item; + }, + }); + return this; + }, + }; + }, + async send() { + // Two phases: run everything against a scratch copy first + // so a mid-batch failure can't leave half the rows behind. + const snapshot = new Map( + Object.entries(store).map(([k, v]) => [k, new Map(v)]), + ); + const results: Array> = []; + try { + for (const op of queued) { + results.push(await op.run()); + } + } catch (err) { + for (const [k, v] of snapshot) store[k] = v; + throw err; + } + return results; + }, + }; + }, }; return { store, getOrCreate, where, pbMock, clearStore }; }); diff --git a/src/lib/server/delivery-notes-service.ts b/src/lib/server/delivery-notes-service.ts index 48962b7..ec8c85d 100644 --- a/src/lib/server/delivery-notes-service.ts +++ b/src/lib/server/delivery-notes-service.ts @@ -38,6 +38,7 @@ import { renderDeliveryNotePdf } from "@/lib/pdf/render"; import { compressBuffer } from "@/lib/server/attachment-compressor"; import { buildDeliveryNoteLine } from "@/lib/pdf/delivery-note-document"; import { firstFileName } from "@/lib/file-fields"; +import { sellingFromCostAndMarkup } from "@/lib/quote-pricing"; import { DELIVERY_NOTE_LETTERHEAD_CODE, deliveryNoteLetterheadCode, @@ -54,7 +55,6 @@ import type { DeliveryNoteStatus, OrderItem, Quote, - QuoteItem, } from "@/lib/types"; const DELIVERY_NOTES = "delivery_notes"; @@ -161,8 +161,66 @@ export interface CreateQuoteFromDeliveryNoteResult { * exists, the route surfaces a 409 — the operator can re-open the * existing quote instead of creating a duplicate. */ +/** + * Best-effort removal of a half-built quote. + * + * The item batch is transactional but the HEADER is created before it + * (the lines need its id, and the PocketBase batch API can't reference + * a record created in the same batch). So the one failure mode we + * have to close by hand is "header written, lines not". + * + * A failure here is logged loudly rather than swallowed: it means a + * stray empty quote is now sitting in the operator's list and someone + * needs to know to remove it. + */ +async function deleteOrphanQuote( + pb: Awaited>, + quoteId: string, +): Promise { + try { + await pb.collection(QUOTES).delete(quoteId); + } catch (err) { + console.error( + `[delivery-notes-service] FAILED to remove half-built quote ${quoteId}. An empty quote may now be visible in the list and should be deleted manually.`, + err, + ); + } +} + +/** + * Per-line pricing the operator reviewed and confirmed in the + * "create quote" modal. + * + * Keyed by `order_item` id, never by array position — the modal + * renders whatever lines the detail endpoint returned, and a client + * that reordered them (or sent three overrides for a four-line note) + * must not silently re-price the wrong goods. `orderItemId` is + * validated against this delivery note's own lines server-side. + */ +export interface QuotePriceOverride { + orderItemId: string; + costPrice: number; + /** Percentage margin. Selling price is derived from it. */ + markupPercent: number; +} + +export interface CreateQuoteFromDeliveryNoteOptions { + /** + * Which letterhead the quote is rendered on. `CNL` matches the + * historical default; DN-backed quotes never used to set this, so + * the operator picks it in the modal instead. + */ + letterhead?: "CNL" | "ISHERWOOD"; + /** + * Operator-confirmed pricing. Omit entirely (the old behaviour) to + * fall back to whatever the order_items already carry. + */ + items?: QuotePriceOverride[]; +} + export async function createQuoteFromDeliveryNote( deliveryNoteId: string, + options: CreateQuoteFromDeliveryNoteOptions = {}, ): Promise { const pb = await getAdminPocketBase(); @@ -226,6 +284,9 @@ export async function createQuoteFromDeliveryNote( // and know instantly "this came from a delivery note, not // from a customer email". rfq_number: dn.delivery_note_number, + // The operator picks the letterhead in the review modal. Left + // unset by older callers, the renderer falls back to CNL. + company_letterhead: options.letterhead ?? "CNL", description: // `??` only falls back on null/undefined, so an empty // `dn.notes` would pass through as an empty description. @@ -235,14 +296,48 @@ export async function createQuoteFromDeliveryNote( }; const quote = await pb.collection(QUOTES).create(quoteCreate); + // ── Pricing overrides ──────────────────────────────────────── + // Validate BEFORE writing anything, so a bad id fails while the + // note is still untouched rather than after a quote exists. + const overrideById = new Map(); + for (const o of options.items ?? []) { + const target = items.find((it) => it.id === o.orderItemId); + if (!target) { + // The client sent a line that isn't on THIS delivery note. + // Pricing it here would quietly move another order's goods + // into this customer's quote. + await deleteOrphanQuote(pb, quote.id); + throw new Error( + `Pricing override references order item ${o.orderItemId}, which is not on delivery note ${dn.delivery_note_number}.`, + ); + } + overrideById.set(o.orderItemId, o); + } + // Create one quote_item per order_item. We deliberately DON'T // touch order_item status here — the items stay at // item_status="delivery_note" until the operator clicks // "Mark Invoiced". The quote and the order_items are two views // of the same physical goods; flipping order_items here would // break the Invoiced-tab count before any invoice exists. + // + // These go out as ONE PocketBase batch rather than a loop of + // individual creates. The batch is transactional: a failure on + // line 5 of 8 aborts the whole thing instead of leaving a real + // quote header attached to 4 of its 8 lines — which is exactly + // the state that made the old loop unrecoverable without manual + // cleanup in PB. + const batch = pb.createBatch(); for (const it of items) { - const itemCreate: Record = { + const override = overrideById.get(it.id); + const costPrice = override + ? Number(override.costPrice) || 0 + : Number(it.cost_price ?? 0); + const sellingPrice = override + ? sellingFromCostAndMarkup(costPrice, override.markupPercent) + : Number(it.selling_price ?? 0); + + batch.collection(QUOTE_ITEMS).create({ quote: quote.id, quote_number: quoteNumber, description: it.description ?? "", @@ -253,15 +348,30 @@ export async function createQuoteFromDeliveryNote( supplier: it.supplier ?? null, // Costs always carry over — the operator needs to see what // the office paid to set a margin. - cost_price: Number(it.cost_price ?? 0), - // Selling price is best-effort: copy if set, otherwise 0 - // and pricing_source="manual" so the UI flags it. - selling_price: Number(it.selling_price ?? 0), + cost_price: costPrice, + // Selling price is best-effort when unoverridden: copy if set, + // otherwise 0 and pricing_source="manual" so the UI flags it. + selling_price: sellingPrice, notes: it.notes ?? "", sku: it.sku ?? "", - pricing_source: it.selling_price ? it.pricing_source ?? "" : "manual", - }; - await pb.collection(QUOTE_ITEMS).create(itemCreate); + // An override is a deliberate human decision, so it's recorded + // as one — same marker the operator's own quote-page edits use. + pricing_source: override + ? "manual" + : it.selling_price + ? it.pricing_source ?? "" + : "manual", + }); + } + + try { + await batch.send(); + } catch (batchErr) { + // Compensate: the header exists but its lines don't. Remove it so + // the operator isn't left with a quote that looks real and has + // some of its lines missing. + await deleteOrphanQuote(pb, quote.id); + throw batchErr; } // v3-C: also attach the delivery note to the new quote so