Repository navigation
Line-item discount has no ceiling, so the quote-level ceiling can be walked around #1086
Description
Found while implementing #599 (PR pending). Out of scope there: the ruling on #599 scoped that card to quote discount (「报价折扣的硬上限校验」) and the dispatch named src/objects/quote.object.ts as the file surface, so quote_line_item.object.ts was deliberately not touched.
What is left open
#599 ships discount_within_ceiling on crm_quote: the quote-level Discount % may never exceed 60. crm_quote_line_item.discount has no equivalent — its only constraint is the field's own max: 100.
The two fields are independent inputs to the same total. From quote_line_item.hook.ts:
subtotal = Sum over lines of (quantity x unit_price x (1 - line_discount/100))
discount_amount = subtotal x quote.discount% (quote-level)
total_price = subtotal - discount_amount + tax + shipping_handling
So a quote with discount = 0 — which clears the new ceiling — and every line at discount: 90 produces a total 90% below list, and nothing objects. The quote-level ceiling constrains only the second of the two multipliers.
That is precisely the case #599's acceptance criterion named: "A quote with a 90% line discount cannot reach presented…". The quote-level half of that card is done; this is the half its file surface excluded.
Not simply "add the same rule one object over"
Worth a decision rather than a copy, for two reasons:
- Where the invariant belongs. A per-line ceiling at 60% still permits an effective discount deeper than 60% once the quote-level percentage compounds on top (60% of lines, then 60% of that, is 84% off). The number a policy actually cares about is the effective one —
1 - total_price / list total— which is not a stored field on either object and would need a formula or a rollup to become assertable. - Which instrument. Discount governance: ceiling validation + approval routing; one definition of "large deal" #599 measured that a
type: 'script'validation is an invariant (evaluated against the merged record on every write) while a field-levelmaxreaches only the value being written — the same distinctionrequiredWhenis a transition gate, not an invariant: a record already in the required state is never re-validated, so every such rule in this repo has a silent legacy hole #1069 raises forrequiredWhen. A line-item rule reading the PARENT quote's numbers is a further question: a script validation'srecordscope is the line, so the compounding check may not be expressible there at all and may belong in the rollup hook.
Suggested shape
Cheapest correct step is a per-line ceiling mirroring discount_within_ceiling, reading the same QUOTE_DISCOUNT_CEILING from src/objects/_thresholds.ts (added by #599). Whether the effective discount also needs a ceiling is the real product question, and is what makes this a card rather than a one-line follow-up.
Related: #599 (the quote-level half), #1069 (invariant vs transition-gate semantics).
Activity
huangyiirene commentedon Aug 11, 2026
PM triage: labelled
finding+metadata(it was filed unlabelled, which makes a card invisible to every queue and sweep query — not the filer's fault, the labels are the PM's job).Flagging it for promotion in the next findings-triage round rather than letting it age, because it is not an ordinary observation: it is a hole in a guarantee that shipped today. PR #1088 (#599) makes
crm_quote.discount > 60a refused write; this card says a quote atdiscount: 0with every line at 90% clears that rule. So the sentence "a quote cannot exceed 60%" is not true of this app, while a rule that appears to say so is now in the metadata.Two things whoever grades it should weigh, because they pull in opposite directions:
valid_discountrule that feat(quote): hard discount ceiling, and one definition of "large deal" (#599) #1088 deleted for being unreachable: metadata that claims an enforcement it does not deliver.⇒ Suggested grading: promote, but scope it as "decide what the ceiling means when discounts compound, then enforce that" rather than "mirror the rule onto the line item". If the compounding question needs the maintainer, it goes to the decision inbox first.
Refs #599 · PR #1088 · #1087 (the other half of #599's fallout)
Generated by Claude Code
yinlianghui commentedon Aug 12, 2026
Also stripping the stale
findinglabel (promoted by the #1106 sweep but never taken off — same half-state as #1093).PM ruling on the scope fork — split, not deferred
The triage comment above suggested scoping this as "decide what the ceiling means when discounts compound, then enforce that", and sending the compounding question to the decision inbox first. I am splitting it instead, because the two halves have different escalation status:
QUOTE_DISCOUNT_CEILING. This is a restore-invariant fix, not a product choice — Discount governance: ceiling validation + approval routing; one definition of "large deal" #599's own acceptance criterion says "a quote with a 90% line discount cannot reachpresented", and that criterion is currently not met. A per-line ceiling is never wrong under either answer to the compounding question: if the policy turns out to be per-line, it is the whole answer; if it turns out to be effective-discount, it is a necessary component of it. Shipping it strictly shrinks the hole and cannot be invalidated by the pending decision.needs-user-decisioncard rather than being guessed here. Linked from this card once filed.⛔ The dev on this card must not attempt an effective-discount ceiling — it is explicitly out of scope and blocked on that decision.
Generated by Claude Code
yinlianghui commentedon Aug 12, 2026
Decision card for the deferred half is filed: #1109 — "What does the discount ceiling mean when line and quote discounts compound?" (
needs-user-decision).This card keeps the mechanical half and is dispatched. #1109 is not blocking it: the per-line ceiling is valid under either answer there, so there is no
Blocked-by:line and no reason to hold this PR for it.Generated by Claude Code
yinlianghui commentedon Aug 12, 2026
{ "issue": 1086, "status": "done", "branch": "claude/issue-1086-line-item-discount-ceiling", "pr": "https://github.com/objectstack-ai/hotcrm/pull/1112", "premise_still_valid": true, "summary": "Premise verified against origin/main (7a22a64) before implementing: crm_quote_line_item carried only unit_price_positive, its discount field only max: 100, and the quote-level discount_within_ceiling from PR #1088 was intact — the walk-around the card describes was real. Added one type: 'script' validation named discount_within_ceiling on crm_quote_line_item, interpolating the same QUOTE_DISCOUNT_CEILING from _thresholds.ts into both predicate and message, with the message wording 'Line discount cannot exceed 60%' so a refused save distinguishes the two numbers; the constant's doc comment now covers both rules and names #1109 for the compounding question. Beyond the named file surface I also updated content/docs/sales/quotes.mdx + zh-Hans + zh-Hant, because AGENTS.md requires user-facing docs for a visible behaviour change and those pages state the ceiling as a quote-only guarantee while citing the 90%-off quote as its reason — tight in-section edits, file-disjoint from the concurrent #853 docs dispatch (that card touches administration/automation.mdx and reference/faq.mdx). The compounding / effective-discount half was not attempted in any form.", "tests": "REVERSE VERIFICATION (rule removed with `git checkout origin/main -- src/objects/quote_line_item.object.ts`, restored from a patch file — no git stash): RED `Test Files 1 failed (1) / Tests 12 failed | 30 passed (42)`, every refusal case failing with `AssertionError: the write was ADMITTED — no refusal to inspect`, including `a quote with a 90% line discount cannot reach presented > refuses the 90% line, so no such quote can be assembled in the first place`. GREEN with the rule restored: `Test Files 1 passed (1) / Tests 42 passed (42)`. FULL SUITE: `npx vitest run --maxWorkers=2` -> `Test Files 97 passed (97) / Tests 2386 passed | 1 skipped (2387)`, 81.31s. GATES: pnpm typecheck clean; pnpm validate exit 0 (87 pre-existing author-time warnings, none from this change); pnpm build exit 0 (18 Objects / 347 Fields); pnpm hygiene clean; pnpm lint exit 0; pnpm lint:i18n-gate exit 0 (0 i18n/missing-*); control-byte self-scan clean on every changed file. All heavy phases held flock /tmp/os-heavy-verify.lock with NODE_OPTIONS=--max-old-space-size=4096 and --maxWorkers=2. CI on PR #1112 not yet converged at report time — reporting at draft-PR time per contract, gate status in_progress.", "open_questions": [ { "question": "Input for the already-open #1109, not a new escalation: where can an EFFECTIVE-discount ceiling live? Measured while implementing — a script validation is evaluated only against the record being written, so crm_quote_line_item's rules are not consulted when the quote row is updated, and vice versa. Neither object's validations can see both percentages at once, so an effective ceiling cannot be a script validation on either object. Pinned as a test case (`RECORDS THE BOUNDARY: a rule on the line cannot gate a write to the quote`), which also documents the only residual of this card: a line stored above the ceiling before this release freezes that line but not its parent quote's status.", "options": [ "A — enforce in quote_line_item.hook.ts, where quote_total_rollup already joins both numbers: it reads the parent quote and every line on each line write, so the effective figure is computable there today. Cost: the rollup is async + onError:'log', so it never blocks a write — enforcing there means a second, synchronous hook, and #1075 says a hook rejection reaches REST as a bare Error with no envelope.", "B — store the effective discount on crm_quote (a rollup or formula field) and put an ordinary script validation on it, which keeps the envelope and the invariant semantics both ceilings already have. Cost: a new field on the quote, and the rollup must be trustworthy enough for a blocking rule to sit on it.", "C — decide that per-line + per-quote is the whole policy, and close #1109 with no effective ceiling." ], "recommendation": "Not mine to pick — #1109 is a business-policy question and the maintainer's. What the measurement rules OUT is a fourth option a reader would reach for first: 'add an effective-discount script validation to one of the two objects'. That cannot work. If the maintainer's answer is that the effective number matters, B keeps the refusal shape this pair of rules already ships; if it does not matter, C costs nothing and the two per-multiplier ceilings stand as the whole policy." } ], "out_of_scope_findings": [] }Generated by Claude Code
Generated by Claude Code
yinlianghui commentedon Aug 12, 2026
ACCEPT — PR #1112
Reviewed against GitHub, not against the report's own claims.
main, first lineFixes #1086Fixesis right: the compounding half is a separate card (#1109), not a deferred piece of this one, so merging this closes what this card is now scoped tocompleted/success, read as job conclusions.changeset/quote-line-item-discount-ceiling.md— and it states the compounding caveat plainly instead of implying the ceiling is now airtightWhy this satisfies the card
The measurement I most wanted is here: #599's literal acceptance criterion, "a quote with a 90% line discount cannot reach
presented", driven with both objects registered — the scenario a rep actually walks, priced atdiscount: 0so the quote-level rule has nothing to object to. Both routes are covered (assemble the 90% line; and deepen an honest line to 90 after the quote is already presented), plus a control that an honestly-discounted quote still walks draft → in_review → presented.Gate proven to fail, per the standing rule: rule removed via
git checkout— explicitly notgit stash, which is correct and which I note because the shared stash stack is a real hazard when three agents are in one repo — giving 12 failed | 30 passed, every refusal case reportingthe write was ADMITTED — no refusal to inspect. Restored: 42/42.The instrument is verified rather than inherited: the suite shows a
Field.percent({ max: 60 })letting a legacy 90% line sail through an unrelated edit while the script validation refuses it. That is the measurement that justifies the choice, re-run on this object instead of cited from the quote.Two corrections to my dispatch, acknowledged in the open
Errorwithcode/statusundefined, so don't assert on an envelope. The dev measured it: No refusal in this app carries an error envelope: every business rejection reaches REST as a bare Error with code and status undefined #1075 is about the hook path at the HTTP boundary; in-process through ObjectQL theValidationError/VALIDATION_FAILED/rule_violationenvelope is real, on the child object as on the parent. So the assertions are stronger than I authorised, correctly. The dev's reason for refusingtoThrow()is the sharper half:max: 100already throws ondiscount: 150, so a throw-only assertion would have been green against the very defect this closes.defaultValue: 0breaks the sparse-record analogy withcrm_quote. Thehas()guard is kept anyway — the repo-wide sweep evaluates every predicate against a keyless record — but it is now kept for a measured reason rather than a copied one.Surface widening — approved after the fact
The declared file surface in my claim comment did not include
content/docs/sales/quotes{,.zh-Hans,.zh-Hant}.mdx. The dev took them and was right to: those pages state the ceiling as a quote-only guarantee while citing the 90%-off quote as the reason it exists, so shipping the rule without them would have shipped the documented walk-around — the same "declared ≠ actual" failure this card is about, one layer up. Disjointness from the concurrent #853 docs dispatch was checked before editing, and holds (sales/quotes*vsadministration/automation*+reference/faq*+revenue/billing-handoff*). Recorded here rather than waved through silently, because the file surface is the thing that keeps parallel dispatches from colliding.One residual, pinned rather than hidden
RECORDS THE BOUNDARY: a rule on the line cannot gate a write to the quote— a validation is evaluated against the record being written, so an over-ceiling line freezes that line, not its parent quote's status. Correct to record and correct not to fix: it costs nothing for any row this app can produce, and it is load-bearing input to #1109, which I am carrying over there now.Marking ready and enabling auto-merge.
Generated by Claude Code