From 2387a71cfe482c95b0a7540e3c88bffeba96ecd2 Mon Sep 17 00:00:00 2001 From: Dave Date: Thu, 8 Oct 2026 07:13:07 +0200 Subject: [PATCH] fix(quotes): stop quote_items markup going negative; round prices up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Quoteers reported markup changing to a random number after saving or clicking away, and lines carrying markup = -100 that nobody had edited. 51 quote_items rows were affected (40 of them exactly -100), all with a real cost and selling_price = 0. All 51 reproduced exactly from ((selling - cost) / cost * 100) — none were typed by a human. Two independent defects, both price-rounding: 1. The approvals screen priced with Math.round(cost * (1 + m/100)) and no precision argument — round to WHOLE RAND. A 30% markup on a R0.07 washer became R0.00, and a selling price of 0 re-derives as exactly -100. Rounding is now a 2dp ceiling: R0.07 @ 30% -> R0.10. A ceiling can never collapse a positive price to zero, so that failure mode is structurally unreachable rather than merely patched. 2. Round-to-nearest also undercut cheap lines generally: 30% on R0.07 rounded to R0.09, which re-derives as 28.6% — the "random number". Rounding up means the price can only drift in the customer's favour. Markup now floors at MIN_MARKUP_PERCENT (18) once a line has a cost, applied in one place so every screen inherits it. Scoped to priced lines: an unpriced draft (no cost) is undecided, not 0% margin, and stays writable — 1,181 live rows depend on that. Applied at derivation (not just on save) so -100 can no longer be computed, plus server-side in review-approve as the authoritative gate. order_items already carried markup min=0 plus a Math.max(0, …) clamp, which is why no order row was ever affected. quote_items had neither. Verified: 3,817 tests pass, typecheck clean, lint delta zero, and all 16 of the real broken rows re-priced through the new logic resolve to a valid markup and a non-zero selling price (was 16/16 broken). Existing negative rows are left untouched — re-pricing live quotes is a separate, deliberate decision. --- src/app/(app)/quotes/[id]/page.tsx | 73 +++++-- src/app/(app)/quotes/approvals/[id]/page.tsx | 50 +++-- .../v1/quotes/[id]/review-approve/route.ts | 21 +- src/lib/quote-pricing.test.ts | 188 +++++++++++++++++- src/lib/quote-pricing.ts | 100 +++++++++- 5 files changed, 386 insertions(+), 46 deletions(-) diff --git a/src/app/(app)/quotes/[id]/page.tsx b/src/app/(app)/quotes/[id]/page.tsx index 827490e..8e7bfb9 100644 --- a/src/app/(app)/quotes/[id]/page.tsx +++ b/src/app/(app)/quotes/[id]/page.tsx @@ -92,6 +92,12 @@ import type { OrderInvoiceSummary } from "@/lib/order-invoices"; import { InvoiceBadges } from "@/components/orders/invoice-badges"; import { AuditTimeline } from "@/components/audit-timeline"; import { quoteStatusClassName, isResubmittableStatus, resubmitStatusMessage } from "@/lib/quote-status"; +import { + MIN_MARKUP_PERCENT, + applyMarkupFloor, + markupFromCostAndSelling, + sellingFromCostAndMarkup, +} from "@/lib/quote-pricing"; import QuoteEmailCard from "@/components/quote-email-card"; import RewriteQuoteModal from "@/components/rewrite-quote-modal"; @@ -3358,11 +3364,23 @@ function PriceUpdateDialog({ setLastEdited("markup"); return; } - const initialMarkup = item.markup != null - ? item.markup - : (item.cost_price != null && item.selling_price != null && item.cost_price > 0) + // Derive the opening markup from (selling − cost) / cost, but NEVER + // below MIN_MARKUP_PERCENT. The old unguarded version returned a + // literal -100 for a line that had a cost but no selling price + // yet — `(0 - 0.07) / 0.07 * 100` — so the dialog opened with + // "-100" sitting in the markup box, waiting for the operator to + // hit Save. That is the reported "I never typed minus a hundred" + // case: they didn't, the screen did, and Save persisted it. + const derived = + item.cost_price != null && item.selling_price != null && item.cost_price > 0 ? ((item.selling_price - item.cost_price) / item.cost_price) * 100 : null; + const initialMarkup = + item.markup != null + ? item.markup + : derived == null + ? null + : Math.max(MIN_MARKUP_PERCENT, derived); setMarkupStr(initialMarkup == null ? "" : String(roundTo1(initialMarkup))); setSellingStr(item.selling_price == null ? "" : String(item.selling_price)); setLastEdited("markup"); @@ -3377,7 +3395,7 @@ function PriceUpdateDialog({ // The displayed "live" markup % — derived from whichever side the // user isn't actively editing so the colour update stays in sync. const livePct = cost != null && cost > 0 && parsedSelling != null - ? ((parsedSelling - cost) / cost) * 100 + ? markupFromCostAndSelling(cost, parsedSelling) : parsedMarkup; const handleSubmit = async (e: React.FormEvent) => { @@ -3389,7 +3407,14 @@ function PriceUpdateDialog({ // safety measure — the server keeps whatever it has if we send the // same value, and we never want to risk overwriting a real cost // price with a transient undefined. - const finalMarkup = parsedSelling == null ? null : parsedMarkup; + // + // The floor is applied HERE as the final client-side gate before + // the write. Everything upstream (seeding effect, both onChange + // handlers) already floors, so this is belt-and-braces — but this + // is the line that actually persists the value, so it is the one + // that must never be allowed to write a negative. + const finalMarkup = + parsedSelling == null ? null : applyMarkupFloor(parsedMarkup ?? 0, cost ?? 0); await onSave(item.id, cost, finalMarkup, parsedSelling); }; @@ -3435,15 +3460,25 @@ function PriceUpdateDialog({ type="number" inputMode="decimal" step="0.1" + min={MIN_MARKUP_PERCENT} value={markupStr} onChange={(e) => { - setMarkupStr(e.target.value); + const raw = e.target.value; + setMarkupStr(raw); setLastEdited("markup"); - const m = Number(e.target.value); - if (cost != null && cost > 0 && Number.isFinite(m)) { - // Recompute selling = cost × (1 + m/100), rounded - // to 2dp for currency niceness. - setSellingStr((cost * (1 + m / 100)).toFixed(2)); + const m = Number(raw); + if (cost != null && cost > 0 && Number.isFinite(m) && raw.trim() !== "") { + // Floor the markup as it's typed, so the selling + // price can never be derived from a sub-floor + // (or negative) margin. + const safe = Math.max(MIN_MARKUP_PERCENT, m); + // Round UP, not to nearest. Round-to-nearest on a + // cheap line lands on a selling price that no + // longer matches the markup the operator chose — + // 30% on a R0.07 washer came back as 28.6%. A + // ceiling can only ever drift upward, and can + // never collapse to R0.00 the way it did before. + setSellingStr(sellingFromCostAndMarkup(cost, safe).toFixed(2)); } }} placeholder="20" @@ -3465,13 +3500,19 @@ function PriceUpdateDialog({ step="0.01" value={sellingStr} onChange={(e) => { - setSellingStr(e.target.value); + const raw = e.target.value; + setSellingStr(raw); setLastEdited("selling"); - const s = Number(e.target.value); + const s = Number(raw); if (cost != null && cost > 0 && Number.isFinite(s) && s >= 0) { - const m = ((s - cost) / cost) * 100; + // THIS is where -100 came from. Typing "0" (or + // pausing mid-type on "0.10") evaluated + // ((0 - cost) / cost) * 100 = -100 and wrote it + // straight into the markup box, which Save then + // persisted. The floor is applied at derivation. + const m = markupFromCostAndSelling(cost, s); setMarkupStr(String(roundTo1(m))); - } else if (cost != null && cost > 0 && e.target.value.trim() === "") { + } else if (cost != null && cost > 0 && raw.trim() === "") { // Operator cleared selling — clear markup too so // the persisted state stays consistent. setMarkupStr(""); @@ -3490,7 +3531,7 @@ function PriceUpdateDialog({
{lastEdited === "selling" && parsedSelling != null - ? `Markup ${formatMarkupPercent(((parsedSelling - (cost || 0)) / (cost || 1)) * 100)}` + ? `Markup ${formatMarkupPercent(markupFromCostAndSelling(cost || 0, parsedSelling))}` : `Markup ${formatMarkupPercent(parsedMarkup)}`} diff --git a/src/app/(app)/quotes/approvals/[id]/page.tsx b/src/app/(app)/quotes/approvals/[id]/page.tsx index 7840282..6ca2941 100644 --- a/src/app/(app)/quotes/approvals/[id]/page.tsx +++ b/src/app/(app)/quotes/approvals/[id]/page.tsx @@ -56,6 +56,11 @@ import { cn } from "@/lib/utils"; import getPocketBase from "@/lib/pocketbase"; import { formatDate, formatCurrency } from "@/lib/pb-helpers"; import { quoteStatusClassName, formatQuoteStatus } from "@/lib/quote-status"; +import { + MIN_MARKUP_PERCENT, + applyMarkupFloor, + sellingFromCostAndMarkup, +} from "@/lib/quote-pricing"; import { AiItemHistoryTrigger } from "@/components/ai-item-history-trigger"; import { isPlaceholderCustomer } from "@/lib/placeholder-customers"; import { SendQuoteModal } from "@/components/quotes/send-quote-modal"; @@ -382,7 +387,11 @@ export default function ApprovalReviewPage() { }; const applyMarkupDraft = () => { if (!markupEditItemId) return; - const value = Math.max(0, Math.round(markupDraft)); + // Whole percentage, and never below the floor. The floor was + // already `Math.max(0, …)`; 0 was never the intent — the intent is + // "don't let a rep dial a line down to nothing", and the house + // minimum for a priced line is MIN_MARKUP_PERCENT. + const value = applyMarkupFloor(Math.round(markupDraft), markupEditCost); setPendingChanges((prev) => { const next = { ...prev }; const current = next[markupEditItemId] ?? {}; @@ -393,13 +402,14 @@ export default function ApprovalReviewPage() { // The selling is what the PDF will show, so it MUST stay // consistent with the markup on commit. // - // Round to the nearest whole currency unit (the rep asked for - // "31 or 32, not 31.23" — we want clean numbers on the PDF). - // `Math.round` rounds half-up which matches the intuition of - // "round up or down to nearest". Using Math.round (not - // toFixed) avoids floats like 31.000000000000004 leaking - // through and saves us a parseFloat. - const newSelling = Math.round(cost * (1 + value / 100)); + // Round UP to 2dp, never to the nearest whole rand. The old + // `Math.round(...)` with no precision turned a 30% markup on a + // R0.07 washer into a selling price of **R0.00** — the row then + // re-derived as (0 − 0.07)/0.07 = **−100%**, which is how 40 + // rows ended up with a minus-one-hundred markup that nobody typed. + // A ceiling can never collapse a positive price to zero, so that + // failure mode is now structurally unreachable from this screen. + const newSelling = sellingFromCostAndMarkup(cost, value); next[markupEditItemId] = { ...current, markup: value, @@ -417,6 +427,12 @@ export default function ApprovalReviewPage() { [markupEditItemId, items] ); + // Cost of the line currently open in the markup editor. Resolved + // here (rather than inside the setPendingChanges updater) so the + // floor can be applied to the draft value BEFORE it goes into + // state — the updater only runs the recompute. + const markupEditCost = markupEditItem ? getEffectiveCost(markupEditItem) : 0; + const hasPendingChanges = Object.keys(pendingChanges).length > 0; const pendingChangesList = useMemo( () => @@ -1417,9 +1433,9 @@ export default function ApprovalReviewPage() { type="button" variant="outline" size="icon" - onClick={() => setMarkupDraft((m) => Math.max(0, m - 1))} + onClick={() => setMarkupDraft((m) => Math.max(MIN_MARKUP_PERCENT, m - 1))} aria-label="Decrease markup by 1%" - disabled={markupDraft <= 0} + disabled={markupDraft <= MIN_MARKUP_PERCENT} className="h-10 w-10" > @@ -1474,9 +1490,9 @@ export default function ApprovalReviewPage() {
{formatCurrency( - Math.round( - getEffectiveCost(markupEditItem) * - (1 + markupDraft / 100), + sellingFromCostAndMarkup( + getEffectiveCost(markupEditItem), + markupDraft, ), )}
@@ -1484,10 +1500,10 @@ export default function ApprovalReviewPage() {
Line total:{" "} {formatCurrency( - Math.round( - getEffectiveCost(markupEditItem) * - (1 + markupDraft / 100), - ) * (Number(markupEditItem.quantity) || 1) + sellingFromCostAndMarkup( + getEffectiveCost(markupEditItem), + markupDraft, + ) * (Number(markupEditItem.quantity) || 1), )}{" "} × qty
diff --git a/src/app/api/v1/quotes/[id]/review-approve/route.ts b/src/app/api/v1/quotes/[id]/review-approve/route.ts index b6f8732..8a64a7b 100644 --- a/src/app/api/v1/quotes/[id]/review-approve/route.ts +++ b/src/app/api/v1/quotes/[id]/review-approve/route.ts @@ -6,6 +6,11 @@ import { type SendQuoteEmailPayload, } from "@/lib/server/quotes-service"; import { getServerIdentity } from "@/lib/dev-identity"; +import { + applyMarkupFloor, + markupFromCostAndSelling, + sellingFromCostAndMarkup, +} from "@/lib/quote-pricing"; import type { Quote, QuoteItem } from "@/lib/types"; /** @@ -266,13 +271,17 @@ export async function POST( if (change.selling_price !== undefined) { selling = change.selling_price; - markup = - cost > 0 - ? Number((((selling - cost) / cost) * 100).toFixed(2)) - : 0; + markup = cost > 0 ? markupFromCostAndSelling(cost, selling) : 0; } else if (change.markup !== undefined) { - markup = change.markup; - selling = Number((cost * (1 + markup / 100)).toFixed(2)); + // Floor applied server-side, on the value that is about to + // be written. This is the authoritative gate: the client + // floors too, but a client is not a guarantee, and this + // route is reachable from the rep UI, the detail page and + // any future caller. `sellingFromCostAndMarkup` rounds UP, + // so a cheap line can no longer collapse to a selling + // price of 0 — the state that re-derived as -100. + markup = applyMarkupFloor(change.markup, cost); + selling = cost > 0 ? sellingFromCostAndMarkup(cost, markup) : 0; } await pb.collection("quote_items").update(change.itemId, { diff --git a/src/lib/quote-pricing.test.ts b/src/lib/quote-pricing.test.ts index 4f356e4..a53f3bf 100644 --- a/src/lib/quote-pricing.test.ts +++ b/src/lib/quote-pricing.test.ts @@ -1,12 +1,15 @@ import { describe, it, expect } from "vitest"; import { + applyMarkupFloor, buildLinePricing, DEFAULT_MARKUP_PERCENT, hasCostPrice, + MIN_MARKUP_PERCENT, markupFromCostAndSelling, repriceLine, repriceWithCostEntry, round2, + round2Up, sellingFromCostAndMarkup, totalsForLines, } from "./quote-pricing"; @@ -74,6 +77,177 @@ describe("hasCostPrice", () => { }); }); +/** + * ── The minus-one-hundred regression ────────────────────────────── + * + * 40 live quote_items rows were found holding `markup = -100`, every + * one of them with a real cost (R0.04 … R0.38) and `selling_price = 0`. + * Nobody typed -100. The derivation `(selling - cost) / cost * 100` + * returns exactly -100 whenever selling is 0 and cost is positive, so + * any screen that derived markup from a line that had a cost but no + * selling price yet produced -100 and persisted it. + * + * These tests pin the fix at each of the three layers: derivation, + * the floor helper, and the round-up that stops selling collapsing + * to zero in the first place. + */ +describe("round2Up", () => { + it("never rounds DOWN", () => { + expect(round2Up(1.201)).toBe(1.21); + expect(round2Up(1.209)).toBe(1.21); + expect(round2Up(1.2)).toBe(1.2); + }); + + it("can never turn a positive number into zero — the structural guarantee", () => { + // This is the whole point. `round2` (to nearest) sends 0.07 × 1.30 + // = 0.091 to 0.09, but on a 4-cent washer the same operation + // collapses the SELLING price to R0.00, and a selling price of 0 + // is precisely what re-derives as -100%. + for (const cost of [0.04, 0.06, 0.07, 0.16, 0.38, 1.19]) { + for (const markup of [15, 20, 25, 30, 45]) { + const selling = round2Up(cost * (1 + markup / 100)); + expect(selling).toBeGreaterThan(0); + } + } + }); + + it("keeps an exact 2dp boundary from being bumped a whole cent by float drift", () => { + // 0.056 × 1.25 computes as 0.07000000000000001, which a naive + // ceil takes to R0.08 — a whole cent overcharge on every such line. + // The EPSILON guard pulls it back to the R0.07 it actually is. + expect((0.056 * 1.25) * 100).toBeGreaterThan(Math.round(0.056 * 1.25 * 100)); // drift + expect(round2Up(0.056 * 1.25)).toBe(0.07); + expect(round2Up(0.112 * 1.25)).toBe(0.14); + expect(round2Up(0.168 * 1.25)).toBe(0.21); + }); + + it("coerces non-finite input to 0 rather than NaN", () => { + expect(round2Up(Number.NaN)).toBe(0); + expect(round2Up(Number.POSITIVE_INFINITY)).toBe(0); + }); +}); + +describe("the -100 bug: markup derived from a zero selling price", () => { + it("used to return exactly -100, and must never do so again", () => { + // The raw formula, unguarded, for the real costs found in the DB. + for (const cost of [0.04, 0.06, 0.07, 0.16, 0.25, 0.38, 1.19]) { + expect(((0 - cost) / cost) * 100).toBe(-100); + // And the fix: + expect(markupFromCostAndSelling(cost, 0)).toBe(MIN_MARKUP_PERCENT); + } + }); + + it("floors a selling price set BELOW cost rather than reporting a loss", () => { + // "KEY STEEL, 18 X 11mm" sat at cost 321 / selling 182.95 = -43.01%. + expect(markupFromCostAndSelling(321, 182.95)).toBe(MIN_MARKUP_PERCENT); + // The 1.19 / 1.00 family that produced -15.97, -6.54, -4.76, -7.41. + expect(markupFromCostAndSelling(1.19, 1)).toBe(MIN_MARKUP_PERCENT); + expect(markupFromCostAndSelling(1.07, 1)).toBe(MIN_MARKUP_PERCENT); + }); + + it("still reports the truth above the floor", () => { + expect(markupFromCostAndSelling(100, 125)).toBe(25); + expect(markupFromCostAndSelling(100, 120)).toBe(20); + // Exactly on the floor passes through untouched. + expect(markupFromCostAndSelling(100, 118)).toBe(MIN_MARKUP_PERCENT); + }); + + it("does NOT round DOWN into the floor", () => { + // 17.4% is under the floor and must read as the floor, not 17. + expect(markupFromCostAndSelling(100, 117.4)).toBe(MIN_MARKUP_PERCENT); + }); +}); + +describe("applyMarkupFloor", () => { + it("raises a sub-floor markup on a priced line", () => { + expect(applyMarkupFloor(0, 100)).toBe(MIN_MARKUP_PERCENT); + expect(applyMarkupFloor(8, 100)).toBe(MIN_MARKUP_PERCENT); + expect(applyMarkupFloor(-100, 0.07)).toBe(MIN_MARKUP_PERCENT); + }); + + it("leaves an above-floor markup exactly as chosen", () => { + expect(applyMarkupFloor(18, 100)).toBe(18); + expect(applyMarkupFloor(22, 100)).toBe(22); + expect(applyMarkupFloor(50, 100)).toBe(50); + }); + + it("does NOT floor an unpriced draft — 1,181 live rows depend on this", () => { + // cost 0 + markup 0 means "nothing decided yet", not "0% margin". + // Flooring these would block creating a quote line at all. + expect(applyMarkupFloor(0, 0)).toBe(0); + expect(applyMarkupFloor(0, null)).toBe(0); + expect(applyMarkupFloor(0, undefined)).toBe(0); + }); + + it("floors a NEGATIVE markup even on a cheap line", () => { + expect(applyMarkupFloor(-100, 0.07)).toBe(MIN_MARKUP_PERCENT); + expect(applyMarkupFloor(-43.01, 321)).toBe(MIN_MARKUP_PERCENT); + }); + + it("coerces garbage to the floor rather than propagating NaN", () => { + expect(applyMarkupFloor(Number.NaN, 100)).toBe(MIN_MARKUP_PERCENT); + }); +}); + +describe("selling price cannot collapse to zero on a cheap line", () => { + it("keeps a 30% markup on the real washer/nut costs as a real price", () => { + // The approvals screen used `Math.round(cost * (1 + m/100))` with + // no precision argument, which is round-to-WHOLE-RAND: every one of + // these became R0.00 and then re-derived as -100%. + for (const cost of [0.04, 0.06, 0.07, 0.16, 0.38]) { + const selling = sellingFromCostAndMarkup(cost, 30); + expect(selling).toBeGreaterThan(0); + expect(selling).toBeGreaterThan(cost); + } + }); + + it("rounds UP so the quoted markup is never undercut", () => { + // 30% on R0.07 is R0.091. Ceiling → R0.10 (a real 42.9% margin, so + // the customer is never under-quoted). Rounding to nearest would + // give R0.09, which re-derives as 28.6% — the "my number changed to + // something random" complaint, and an UNDERQUOTE besides. + expect(sellingFromCostAndMarkup(0.07, 30)).toBe(0.1); + expect(sellingFromCostAndMarkup(0.06, 25)).toBe(0.08); + // Proof the old value was an underquote: + expect(Math.round(0.07 * 1.3 * 100) / 100).toBeLessThan(0.07 * 1.3); + }); + + it("leaves a price that is already clean alone", () => { + expect(sellingFromCostAndMarkup(100, 25)).toBe(125); + expect(sellingFromCostAndMarkup(12.5, 25)).toBe(15.63); + }); +}); + +describe("repriceLine honours the floor", () => { + const base = { + orderItemId: "i1", + description: "M10 FLAT WASHER BLK", + quantity: 1, + costPrice: 0.07, + markupPercent: 30, + sellingPrice: 0.1, + }; + + it("floors a sub-floor markup instead of writing it through", () => { + const r = repriceLine(base, { markupPercent: 0 }); + expect(r.markupPercent).toBe(MIN_MARKUP_PERCENT); + expect(r.sellingPrice).toBeGreaterThan(0); + }); + + it("floors the bulk 'apply markup to all lines' action", () => { + // This is the path the delivery-note review modal's blur handler + // takes. It used to write whatever it was handed. + const r = repriceLine(base, { markupPercent: -100 }); + expect(r.markupPercent).toBe(MIN_MARKUP_PERCENT); + }); + + it("still allows the deliberate thin margins reps actually use", () => { + expect(repriceLine(base, { markupPercent: 18 }).markupPercent).toBe(18); + expect(repriceLine(base, { markupPercent: 20 }).markupPercent).toBe(20); + expect(repriceLine(base, { markupPercent: 22 }).markupPercent).toBe(22); + }); +}); + describe("buildLinePricing", () => { it("applies the default markup to a cost-only line", () => { const [line] = buildLinePricing([ @@ -245,11 +419,17 @@ describe("repriceWithCostEntry", () => { expect(r.sellingPrice).toBe(280); }); - it("does not override a margin the operator set to zero on purpose", () => { - // Typing markup 0 into the same keystroke is explicit intent. + it("BEHAVIOUR CHANGE: an explicit 0 markup on a COSTED line is now floored", () => { + // This test used to assert markupPercent === 0 ("explicit intent"). + // That is exactly the hole the -100 rows came through: a zero-margin + // line is a line that can be rounded, re-derived or nudged into a + // negative one. The house rule is now "never below MIN_MARKUP_PERCENT + // once a line has a cost", so an explicit 0 is raised like any other + // sub-floor value. Uncosted drafts are untouched — see + // applyMarkupFloor. const r = repriceWithCostEntry(noCost, { costPrice: 100, markupPercent: 0 }); - expect(r.markupPercent).toBe(0); - expect(r.sellingPrice).toBe(100); + expect(r.markupPercent).toBe(MIN_MARKUP_PERCENT); + expect(r.sellingPrice).toBe(118); }); it("seeds nothing when the cost is cleared back to zero", () => { diff --git a/src/lib/quote-pricing.ts b/src/lib/quote-pricing.ts index 6fbb494..ca89fa1 100644 --- a/src/lib/quote-pricing.ts +++ b/src/lib/quote-pricing.ts @@ -25,10 +25,83 @@ export function round2(value: number): number { return Math.round((value + Number.EPSILON) * 100) / 100; } +/** + * Round UP to 2dp — never down. + * + * This is the fix for the "R0.07 became R0.00" class of bug, and the + * reason is structural rather than clever: a ceiling can never turn a + * positive number into zero, so anything derived through this can never + * land on a selling price of 0. Round-to-nearest can: 0.07 * 1.30 is + * 0.091, and `Math.round` sends that to a clean R0.09 — but a 30% + * markup on a 4-cent washer rounds the *selling* price to zero, and + * every later re-read of that row (`(0 - cost) / cost * 100`) reports + * -100% margin on a line nobody ever priced below cost. + * + * The EPSILON subtraction keeps a value that is already exactly on a + * 2dp boundary from being pushed up a whole cent by float drift + * (0.07 * 1.30 can compute as 0.09100000000000001). + */ +export function round2Up(value: number): number { + if (!Number.isFinite(value)) return 0; + // `|| 0` normalises the -0 that ceil produces for tiny negatives + // (-0 is falsy). It serialises to "0" but fails `Object.is(-0, 0)`, + // which would make a future test mysteriously fail. + return Math.ceil((value - Number.EPSILON) * 100) / 100 || 0; +} + +/** + * The floor for a markup an operator has actually chosen. + * + * Deliberately NOT zero. A quote that goes out at a loss — or at zero — + * is the failure mode this whole module exists to prevent, and the + * order-item side already carries `min=0` plus a `Math.max(0, …)` + * clamp for exactly this reason. Quotes had neither, which is why 51 + * quote_items rows were sitting on a negative markup while not one + * order_items row ever was. + * + * Scoped to lines that HAVE a cost. An unpriced draft (cost 0, markup + * 0) is not a 0% margin — it is an undecided line — and must stay + * writable, or creating a quote line would require inventing a margin + * on a line with no cost on file. See `applyMarkupFloor`. + */ +export const MIN_MARKUP_PERCENT = 18; + +/** + * Apply the markup floor to a value the operator chose. + * + * Only ever RAISES a sub-floor markup, and only for a line with a real + * cost — the two conditions that keep this from breaking the ~1,181 + * unpriced draft rows and the ~503 deliberate zero-margin rows already + * in the database. A zero-cost line passes through untouched so a + * blank line can still be saved as a draft. + * + * A negative markup (the -100 rows) is always raised. There is no + * legitimate below-cost quote in this workflow, and the user's rule is + * explicit: never below MIN_MARKUP_PERCENT. + */ +export function applyMarkupFloor( + markupPercent: number, + cost: number | null | undefined, +): number { + const markup = Number(markupPercent); + if (!Number.isFinite(markup)) return MIN_MARKUP_PERCENT; + // No cost on file = nothing has been priced yet. Leave it alone. + if (!(Number(cost) > 0)) return 0; + return Math.max(MIN_MARKUP_PERCENT, markup); +} + /** * 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. + * + * Rounds UP (never to nearest, never down). Rounding to nearest is + * what let a 30% markup on a R0.07 washer produce a selling price of + * R0.09 and then a *re-derived* markup of 28.6% — the "it changed my + * number to something random" complaint. Rounding up guarantees the + * selling price is always at least the markup implies, so the price + * can only ever drift in the customer's favour, and never lands on + * the zero that produced the -100 rows. */ export function sellingFromCostAndMarkup( cost: number, @@ -36,22 +109,38 @@ export function sellingFromCostAndMarkup( ): number { const costNum = Number(cost) || 0; const markupNum = Number(markupPercent) || 0; - return round2(costNum * (1 + markupNum / 100)); + return round2Up(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. + * + * The floor is applied HERE, at derivation, not only at save. This is + * the function that produced the -100s: a line with a cost of R0.07 + * and a selling price of R0 (nothing typed yet) yields + * `(0 - 0.07) / 0.07 * 100` = exactly -100, and that number was + * written to the database on save. Flooring at the point of + * derivation means the -100 can no longer be *computed*, let alone + * stored — the screens that showed it and the screen that persisted it + * are fixed by the same edit. */ export function markupFromCostAndSelling( cost: number, selling: number, + floorPercent: number = MIN_MARKUP_PERCENT, ): number { const costNum = Number(cost) || 0; if (costNum <= 0) return 0; const sellingNum = Number(selling) || 0; - return round2(((sellingNum - costNum) / costNum) * 100); + const raw = ((sellingNum - costNum) / costNum) * 100; + // Never round DOWN to the floor — a genuinely-negative derivation + // means "selling is below cost", which is exactly the state the + // floor exists to prevent. Round UP, so a 17.4% derivation reads as + // the 18% floor rather than being trimmed further toward the margin. + if (raw < floorPercent) return floorPercent; + return round2(raw); } /** True when a line has a usable, fixed cost price to work from. */ @@ -174,10 +263,15 @@ export function repriceLine( ): LinePricing { const costPrice = patch.costPrice !== undefined ? round2(Number(patch.costPrice) || 0) : line.costPrice; - const markupPercent = + const requestedMarkup = patch.markupPercent !== undefined ? Number(patch.markupPercent) || 0 : line.markupPercent; + // Floor applies HERE, at reprice, so every caller inherits it. The + // "apply markup to all lines" bulk action and the delivery-note + // review modal both land in this function, and both previously wrote + // whatever markup they were handed straight through. + const markupPercent = applyMarkupFloor(requestedMarkup, costPrice); return { ...line, costPrice,