diff --git a/src/components/delivery-notes/delivery-note-editor-dialog.test.tsx b/src/components/delivery-notes/delivery-note-editor-dialog.test.tsx new file mode 100644 index 0000000..1a137dc --- /dev/null +++ b/src/components/delivery-notes/delivery-note-editor-dialog.test.tsx @@ -0,0 +1,214 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { render, screen, waitFor, within } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; + +import { DeliveryNoteEditorDialog } from "./delivery-note-editor-dialog"; +import { formatCurrency } from "@/lib/pb-helpers"; + +// ─── Mocks ────────────────────────────────────────────────────────── +// +// The dialog reads the note through `fetch` (the `/detail` route) +// and writes through `fetch` (PATCH .../pricing). Both are stubbed +// so no request reaches the network: `detailItems` drives what the +// grid renders, and `patchBodies` records what Save would send — +// the assertion that actually matters here is on the payload, not on +// the styling. +// +// PocketBase is stubbed because the dialog pulls the session token +// straight off `authStore` and refuses to load without one. + +vi.mock("@/lib/pocketbase", () => ({ + __esModule: true, + default: () => ({ authStore: { token: "test-token" } }), +})); + +const NOTE_ID = "dn_1_2_3"; +const NOTE_NUMBER = "DN26071501"; + +let detailItems: Array> = []; +let patchBodies: Array> = []; + +function mockFetch() { + return vi.fn(async (url: string, init?: RequestInit) => { + if (String(url).includes("/detail")) { + return { + ok: true, + status: 200, + json: async () => ({ items: detailItems }), + } as unknown as Response; + } + if (init?.method === "PATCH") { + patchBodies.push(JSON.parse(String(init.body))); + return { + ok: true, + status: 200, + json: async () => ({ item: {}, pdfRegenerated: false }), + } as unknown as Response; + } + return { + ok: false, + status: 404, + json: async () => ({ error: "not found" }), + } as unknown as Response; + }); +} + +function renderDialog() { + return render( + {}} + deliveryNoteId={NOTE_ID} + deliveryNoteNumber={NOTE_NUMBER} + />, + ); +} + +const costBox = (i: number) => + screen.getByTestId(`dn-editor-cost-${i}`) as HTMLInputElement; +const markupBox = (i: number) => + screen.getByLabelText(`Markup for line ${i + 1}`) as HTMLInputElement; + +/** + * Assert a row shows `amount` as its selling price. + * + * Two traps in one helper. `formatCurrency` renders en-ZA as + * "R 140,00" — non-breaking space, comma decimal — so the expected + * string must come from the app's own formatter, never a literal. + * And testing-library normalises the NODE's text but compares the + * matcher raw (see `@testing-library/dom/dist/matches.js`: the + * `String(matcher)` branch skips the normaliser), so the nbsp in our + * matcher has to be collapsed here too or the two never compare equal. + */ +function expectSelling(rowIndex: number, amount: number) { + const row = screen.getByTestId(`dn-editor-line-${rowIndex}`); + const expected = formatCurrency(amount).replace(/\s+/g, " ").trim(); + expect(within(row).getByText(expected)).toBeInTheDocument(); +} + +/** Radix only mounts the dialog body once open, so wait for a row. */ +async function waitForGrid() { + await waitFor(() => expect(screen.queryByText("Loading items…")).toBeNull()); +} + +beforeEach(() => { + detailItems = []; + patchBodies = []; + vi.stubGlobal("fetch", mockFetch()); + vi.clearAllMocks(); +}); + +describe("cost price already on file is fixed", () => { + it("renders the cost read-only when the line arrived costed", async () => { + detailItems = [ + { id: "oi1", description: "M16 HEX BOLT", quantity: 4, cost_price: 12.5 }, + ]; + renderDialog(); + await waitForGrid(); + + expect(costBox(0)).toHaveAttribute("readonly"); + expect(costBox(0)).toHaveValue(12.5); + expect(markupBox(0)).not.toHaveAttribute("readonly"); + }); + + it("leaves the cost writable when there is no cost on file", async () => { + detailItems = [{ id: "oi1", description: "M16 HEX BOLT", quantity: 4 }]; + renderDialog(); + await waitForGrid(); + + expect(costBox(0)).not.toHaveAttribute("readonly"); + }); + + it("lets the operator change the markup on a locked line", async () => { + const user = userEvent.setup(); + detailItems = [ + { id: "oi1", description: "M16 HEX BOLT", quantity: 4, cost_price: 100 }, + ]; + renderDialog(); + await waitForGrid(); + + // 25% is seeded because the line is costed but unpriced. + expect(markupBox(0)).toHaveValue(25); + + await user.clear(markupBox(0)); + await user.type(markupBox(0), "40"); + + // Selling re-derives off the FIXED cost: 100 x 1.40. + await waitFor(() => expectSelling(0, 140)); + }); + + it("saves the original cost even after the markup moved", async () => { + const user = userEvent.setup(); + detailItems = [ + { id: "oi1", description: "M16 HEX BOLT", quantity: 4, cost_price: 100 }, + ]; + renderDialog(); + await waitForGrid(); + + await user.clear(markupBox(0)); + await user.type(markupBox(0), "40"); + await user.click(screen.getByTestId("dn-editor-save")); + + await waitFor(() => expect(patchBodies).toHaveLength(1)); + expect(patchBodies[0]).toMatchObject({ + cost_price: 100, + selling_price: 140, + }); + }); + + it("does not let a typed value into a locked cost box", async () => { + const user = userEvent.setup(); + detailItems = [ + { id: "oi1", description: "M16 HEX BOLT", quantity: 4, cost_price: 100 }, + ]; + renderDialog(); + await waitForGrid(); + + await user.click(costBox(0)); + await user.type(costBox(0), "1"); + + expect(costBox(0)).toHaveValue(100); + }); + + it("still accepts a first cost on an uncosted line", async () => { + const user = userEvent.setup(); + detailItems = [{ id: "oi1", description: "M16 HEX BOLT", quantity: 4 }]; + renderDialog(); + await waitForGrid(); + + await user.type(costBox(0), "80"); + // Entering the first cost seeds the default markup (25%), so the + // line is no longer a silent zero-margin line. + await waitFor(() => expect(markupBox(0)).toHaveValue(25)); + expectSelling(0, 100); + + await user.click(screen.getByTestId("dn-editor-save")); + await waitFor(() => expect(patchBodies).toHaveLength(1)); + expect(patchBodies[0]).toMatchObject({ + cost_price: 80, + selling_price: 100, + }); + }); + + it("explains the lock instead of showing a dead input", async () => { + detailItems = [ + { id: "oi1", description: "M16 HEX BOLT", quantity: 4, cost_price: 100 }, + ]; + renderDialog(); + await waitForGrid(); + + expect( + screen.getByText(/cost price is fixed/i), + ).toBeInTheDocument(); + }); + + it("says so when every cost is still open", async () => { + detailItems = [{ id: "oi1", description: "M16 HEX BOLT", quantity: 4 }]; + renderDialog(); + await waitForGrid(); + + expect( + screen.getByText(/no cost is on file yet/i), + ).toBeInTheDocument(); + }); +}); \ No newline at end of file diff --git a/src/components/delivery-notes/delivery-note-editor-dialog.tsx b/src/components/delivery-notes/delivery-note-editor-dialog.tsx index 2ed4c14..30ab798 100644 --- a/src/components/delivery-notes/delivery-note-editor-dialog.tsx +++ b/src/components/delivery-notes/delivery-note-editor-dialog.tsx @@ -25,10 +25,22 @@ * A line that already carries a selling price keeps it — the office may * have priced it deliberately — and shows the markup that price implies * so the two can never quietly disagree. + * + * ## A cost already on file is fixed + * + * The cost cell is read-only on any line that arrived with a cost. + * The operator's job on this screen is to set the MARGIN, and letting + * the cost move while they do that means a stray keystroke silently + * rewrites what the business paid — which changes every downstream + * number on the note without ever looking like a pricing decision. + * A line with no cost on file stays an empty input: that one gets costed + * here. See `costLocked` / `isCostEditable` in `quote-pricing.ts`, and + * the delivery note detail page's edit mode for the deliberate + * "that cost is simply wrong" correction. */ import { useCallback, useEffect, useMemo, useState } from "react"; import { toast } from "sonner"; -import { AlertTriangle, Loader2 } from "lucide-react"; +import { AlertTriangle, Loader2, Lock } from "lucide-react"; import { Button } from "@/components/ui/button"; import { Input } from "@/components/ui/input"; import { @@ -42,6 +54,7 @@ import { import { applyMarkupFloor, hasCostPrice, + isCostEditable, repriceWithCostEntry, totalsForLines, type LinePricing, @@ -127,6 +140,9 @@ export function DeliveryNoteEditorDialog({ ? 25 : 0, sellingPrice: selling, + // Read at load, not derived from `costPrice`: a cost typed + // in during this session must stay fixable until save. + costLocked: hasCostPrice(cost), }; }), ); @@ -150,6 +166,12 @@ export function DeliveryNoteEditorDialog({ const totals = useMemo(() => totalsForLines(lines), [lines]); + /** How many lines arrived with a cost already on file. */ + const lockedCount = useMemo( + () => lines.filter((l) => !isCostEditable(l)).length, + [lines], + ); + const update = (index: number, patch: { costPrice?: number; markupPercent?: number }) => { setLines((prev) => prev.map((line, i) => (i === index ? repriceWithCostEntry(line, patch) : line)), @@ -276,6 +298,7 @@ export function DeliveryNoteEditorDialog({ {lines.map((line, index) => { const costed = hasCostPrice(line.costPrice); + const costLocked = !isCostEditable(line); return ( - - update(index, { costPrice: Number(e.target.value) || 0 }) - } - aria-label={`Cost for line ${index + 1}`} - disabled={saving} - className="h-8 text-right tabular-nums" - /> +
+ {costLocked && ( +
+ {/* One sentence beats a tooltip on every locked cell: + the rule is the whole point of this screen, and a + greyed-out number with no explanation reads as a + broken input. */} +

+

+
Total cost{" "} diff --git a/src/components/delivery-notes/quote-review-dialog.tsx b/src/components/delivery-notes/quote-review-dialog.tsx index bd7c15d..15b0e3b 100644 --- a/src/components/delivery-notes/quote-review-dialog.tsx +++ b/src/components/delivery-notes/quote-review-dialog.tsx @@ -22,11 +22,15 @@ * - It doesn't invent a cost. A line with no cost on file shows a * blank cost and a "no cost on file" marker rather than a * plausible-looking zero that reads as free-of-charge. + * - It doesn't let you edit a cost that's already on file. Same + * rule as the "Edit note" grid one button away: reviewing a + * margin must never silently rewrite what was paid. A blank cost + * stays fillable. See `costLocked` in `quote-pricing.ts`. */ import { useEffect, useMemo, useState } from "react"; import { useRouter } from "next/navigation"; import { toast } from "sonner"; -import { AlertTriangle, Loader2 } from "lucide-react"; +import { AlertTriangle, Loader2, Lock } from "lucide-react"; import { Button } from "@/components/ui/button"; import { Input } from "@/components/ui/input"; import { Label } from "@/components/ui/label"; @@ -42,6 +46,7 @@ import { applyMarkupFloor, buildLinePricing, hasCostPrice, + isCostEditable, repriceLine, repriceWithCostEntry, totalsForLines, @@ -157,6 +162,12 @@ export function QuoteReviewDialog({ [lines], ); + /** How many lines arrived with a cost already on file. */ + const lockedCount = useMemo( + () => lines.filter((l) => !isCostEditable(l)).length, + [lines], + ); + /** Raise any markup left under the floor, on blur and on save. */ const commitFloors = (index: number) => { setLines((prev) => @@ -352,6 +363,7 @@ export function QuoteReviewDialog({ {lines.map((line, index) => { const costed = hasCostPrice(line.costPrice); + const costLocked = !isCostEditable(line); return ( - - updateLine(index, { - costPrice: Number(e.target.value) || 0, - }) - } - className="h-8 text-right tabular-nums" - /> +
+ {costLocked && ( +
+ {/* Same one-line explanation the "Edit note" grid carries. A locked + cost with no explanation reads as a broken input, not + as a rule. */} +

+

+ {/* 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. */} diff --git a/src/lib/quote-pricing.test.ts b/src/lib/quote-pricing.test.ts index 4c15069..634711c 100644 --- a/src/lib/quote-pricing.test.ts +++ b/src/lib/quote-pricing.test.ts @@ -4,6 +4,7 @@ import { buildLinePricing, DEFAULT_MARKUP_PERCENT, hasCostPrice, + isCostEditable, MIN_MARKUP_PERCENT, markupFromCostAndSelling, repriceLine, @@ -219,6 +220,10 @@ describe("selling price cannot collapse to zero on a cheap line", () => { }); describe("repriceLine honours the floor", () => { + // `costLocked: false` = a costed line whose cost is still editable, + // i.e. a cost entered during the current session. These tests are + // about the floor, so the lock is deliberately left off here — the + // locked-line behaviour has its own describe block below. const base = { orderItemId: "i1", description: "M10 FLAT WASHER BLK", @@ -226,6 +231,7 @@ describe("repriceLine honours the floor", () => { costPrice: 0.07, markupPercent: 30, sellingPrice: 0.1, + costLocked: false, }; it("does NOT floor while the operator is typing", () => { @@ -328,6 +334,7 @@ describe("repriceLine", () => { costPrice: 100, markupPercent: 25, sellingPrice: 125, + costLocked: false, }; it("re-derives selling when the markup changes", () => { @@ -415,6 +422,7 @@ describe("repriceWithCostEntry", () => { costPrice: 0, markupPercent: 0, sellingPrice: 0, + costLocked: false, }; it("seeds the default markup when the FIRST cost is typed", () => { @@ -473,3 +481,73 @@ describe("repriceWithCostEntry", () => { }); }); }); + +/** + * A cost price that is already on file is fixed. The grids render it + * read-only, but the real guarantee is in `repriceLine`: a cost + * patch on a locked line is dropped, so no caller — a stray edit + * handler, a future bulk action, a keyboard shortcut — can quietly + * rewrite what was paid while the operator believes they only moved + * the markup. + */ +describe("a cost price already on file is fixed", () => { + const locked = buildLinePricing([ + { id: "i1", description: "Bolt", quantity: 2, cost_price: 100 }, + ])[0]; + + it("marks a costed line locked at load", () => { + expect(locked.costLocked).toBe(true); + expect(isCostEditable(locked)).toBe(false); + }); + + it("leaves an uncosted line open", () => { + const [open] = buildLinePricing([{ id: "i2", cost_price: 0 }]); + expect(open.costLocked).toBe(false); + expect(isCostEditable(open)).toBe(true); + }); + + it("still lets the markup move — that's the point of the screen", () => { + const r = repriceWithCostEntry(locked, { markupPercent: 40 }); + expect(r.costPrice).toBe(100); + expect(r.markupPercent).toBe(40); + expect(r.sellingPrice).toBe(140); + }); + + it("drops a cost patch instead of applying it", () => { + const r = repriceWithCostEntry(locked, { costPrice: 1 }); + expect(r.costPrice).toBe(100); + // And therefore the selling price still reflects the real cost — + // a rejected cost must not leave the margin quietly derived from + // the rejected number. + expect(r.sellingPrice).toBe(125); + }); + + it("drops a cost patch even when it arrives with a markup", () => { + const r = repriceWithCostEntry(locked, { costPrice: 999, markupPercent: 50 }); + expect(r.costPrice).toBe(100); + expect(r.markupPercent).toBe(50); + expect(r.sellingPrice).toBe(150); + }); + + it("cannot be unlocked by re-running the cost-entry path", () => { + const r = repriceWithCostEntry(locked, { costPrice: 0 }); + expect(r.costLocked).toBe(true); + expect(r.costPrice).toBe(100); + }); + + it("leaves the lock standing across a bulk re-markup", () => { + const bulk = locked; + const r = repriceWithCostEntry(bulk, { markupPercent: MIN_MARKUP_PERCENT }, 25); + expect(r.costLocked).toBe(true); + expect(r.sellingPrice).toBe(118); + }); + + it("keeps the lock across the commit-time floor pass", () => { + // `applyMarkupFloor` + a re-price is what save actually runs. If + // that path could drop the flag, the second open of the dialog + // would quietly unlock a cost. + const committed = repriceLine(locked, { markupPercent: 0 }, { floor: true }); + expect(committed.costLocked).toBe(true); + expect(committed.markupPercent).toBe(MIN_MARKUP_PERCENT); + }); +}); diff --git a/src/lib/quote-pricing.ts b/src/lib/quote-pricing.ts index 5902679..2879a71 100644 --- a/src/lib/quote-pricing.ts +++ b/src/lib/quote-pricing.ts @@ -149,6 +149,19 @@ export function hasCostPrice(cost: number | null | undefined): boolean { return Number.isFinite(n) && n > 0; } +/** + * True when the operator may type into this line's cost cell. + * + * The single source of truth for the "cost on file is fixed" rule, so + * the pricing grid and the review grid can't disagree about when a + * cost is locked. Paired with `repriceLine`, which enforces the same + * rule in the state — the UI hiding an input is a convenience, this + * is the guarantee. + */ +export function isCostEditable(line: Pick): boolean { + return line.costLocked !== true; +} + /** * Default markup to seed a line with when the operator hasn't chosen * one. Deliberately a constant rather than a per-customer or @@ -167,6 +180,20 @@ export interface LinePricing { markupPercent: number; /** Derived from cost + markup; never typed independently. */ sellingPrice: number; + /** + * True when the line ARRIVED carrying a cost price, so the cost + * cell is read-only and `repriceLine` refuses cost patches. + * + * Captured at load rather than derived from the live value, and + * that is the whole point: a cost the operator types in during + * this session must stay correctable until they save. Locking on + * the live value would mean one mis-typed digit has to be escaped + * by cancelling the dialog and starting over. After a save the row + * carries the cost, so the next load sets this true and the cell is + * fixed — which is the rule as stated: cost on file is fixed, a + * blank cost is fillable. + */ + costLocked: boolean; } export interface LineTotals { @@ -235,6 +262,7 @@ export function buildLinePricing( costPrice, markupPercent: markupFromCostAndSelling(costPrice, existingSelling), sellingPrice: existingSelling, + costLocked: hasCost, }; } @@ -248,6 +276,7 @@ export function buildLinePricing( sellingPrice: hasCost ? sellingFromCostAndMarkup(costPrice, markupPercent) : 0, + costLocked: hasCost, }; }); } @@ -256,6 +285,14 @@ export function buildLinePricing( * 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. + * + * A `costPrice` patch on a LOCKED line is silently dropped rather + * than applied. The grids disable the cost cell, but a disabled + * input is a UI convention, not an invariant — a stray patch (a + * future bulk action, a keyboard shortcut, a half-merged rebase) + * would otherwise be able to rewrite what the business paid while + * the operator believes they only touched the markup. Dropping it + * here means the cost can only ever change on a line that had none. */ export function repriceLine( line: LinePricing, @@ -263,7 +300,9 @@ export function repriceLine( opts: { floor?: boolean } = {}, ): LinePricing { const costPrice = - patch.costPrice !== undefined ? round2(Number(patch.costPrice) || 0) : line.costPrice; + patch.costPrice !== undefined && isCostEditable(line) + ? round2(Number(patch.costPrice) || 0) + : line.costPrice; const requestedMarkup = patch.markupPercent !== undefined ? round2(Number(patch.markupPercent) || 0)