fix(quotes): stop quote_items markup going negative; round prices up
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.
This commit is contained in:
parent
3bc4633587
commit
2387a71cfe
5 changed files with 386 additions and 46 deletions
|
|
@ -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({
|
|||
<div className="flex items-center justify-between">
|
||||
<span className="text-muted-foreground">
|
||||
{lastEdited === "selling" && parsedSelling != null
|
||||
? `Markup ${formatMarkupPercent(((parsedSelling - (cost || 0)) / (cost || 1)) * 100)}`
|
||||
? `Markup ${formatMarkupPercent(markupFromCostAndSelling(cost || 0, parsedSelling))}`
|
||||
: `Markup ${formatMarkupPercent(parsedMarkup)}`}
|
||||
</span>
|
||||
<span className={`font-semibold tabular-nums ${markupBadgeClasses(livePct)}`}>
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
>
|
||||
<Minus className="h-4 w-4" />
|
||||
|
|
@ -1474,9 +1490,9 @@ export default function ApprovalReviewPage() {
|
|||
</div>
|
||||
<div className="text-xl font-bold tabular-nums">
|
||||
{formatCurrency(
|
||||
Math.round(
|
||||
getEffectiveCost(markupEditItem) *
|
||||
(1 + markupDraft / 100),
|
||||
sellingFromCostAndMarkup(
|
||||
getEffectiveCost(markupEditItem),
|
||||
markupDraft,
|
||||
),
|
||||
)}
|
||||
</div>
|
||||
|
|
@ -1484,10 +1500,10 @@ export default function ApprovalReviewPage() {
|
|||
<div className="text-[10px] text-muted-foreground mt-1">
|
||||
Line total:{" "}
|
||||
{formatCurrency(
|
||||
Math.round(
|
||||
getEffectiveCost(markupEditItem) *
|
||||
(1 + markupDraft / 100),
|
||||
) * (Number(markupEditItem.quantity) || 1)
|
||||
sellingFromCostAndMarkup(
|
||||
getEffectiveCost(markupEditItem),
|
||||
markupDraft,
|
||||
) * (Number(markupEditItem.quantity) || 1),
|
||||
)}{" "}
|
||||
× qty
|
||||
</div>
|
||||
|
|
|
|||
|
|
@ -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, {
|
||||
|
|
|
|||
|
|
@ -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", () => {
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
Loading…
Reference in a new issue