fix: don't stack a higher-priority discount rule onto a price override - #54
Conversation
calculate_price scans matching_rules (sorted by priority, descending) and breaks out as soon as it finds a rule with base_price_override, with a comment saying the override "takes precedence". It does, for the running price — but a discount_percentage from a rule visited earlier in the same loop (i.e. a higher-priority rule that had no override of its own) stays in the rule_discount accumulator and gets multiplied into the override price after the loop exits. Confirmed against docs/guides/pricing-rules.md, which draws override and discount as mutually exclusive branches off the same decision point and says an override "stops further rule evaluation" — the current code does the opposite whenever a higher-priority discount- only rule happens to be scanned first. Concretely: a seller configures a $50 flat negotiated rate for an agency (priority 50) alongside an unrelated agency-wide 20% discount rule (priority 100, no override). The discount rule is evaluated first because it outranks the override rule, leaving 0.20 sitting in rule_discount. The override rule is then reached, sets price = 50, and breaks — but the stale 0.20 discount still gets applied afterward, landing the buyer at $40 instead of the $50 the seller configured. Fix: track whether an override fired and skip the trailing discount application when it did. The discount-only path (no override in play) is unchanged. base_price_override had zero test coverage before this — added TestPriceOverrideRules covering the override-alone case, both priority orderings of override vs. discount rule (only one of which was broken), and confirming the ordinary highest-discount-wins path for discount-only rules still works. Full suite: 1404 passed. ruff check / format clean. Closes IABTechLab#53
atc964
left a comment
There was a problem hiding this comment.
Reproduced the bug on main exactly as the issue describes — override configured at $50, discount rule leaks in, buyer pays $40. Silent revenue leak, docs unambiguously say override-wins, and the fix's override_applied gate is the right minimal shape. Adjacent cases verified here: highest-priority override wins among two, volume discount still applies post-override per the documented flow, floor still catches zero/negative. Full suite 1407/0 on the merge, and your override test is genuinely load-bearing.
One change needed before merge, and it's a money-path provenance issue: the fix suppresses the discount from applied_rules but not from the rationale. rule_discount still holds the stale value when _build_rationale runs, so the corrected path emits 'Custom rule: -20% ... Final price: $50.00' — asserting a discount that was not applied. That string flows into quotes, negotiated-deal text, and the MCP/API surfaces, where a negotiating counterparty could anchor on it. Exact ask: zero out rule_discount in the override branch (or gate the rationale segment on not override_applied), plus one test asserting 'Custom rule' is absent from the rationale on the override path.
Noted, not blocking: the tier-discount segment lingers in rationale the same way on main (pre-existing), and base_price_override accepts negatives at the model layer with the floor catching it downstream — both worth follow-ups. Happy to re-review same-day.
Review follow-up on IABTechLab#54 (atc964): the earlier fix correctly gated applied_rules and the price multiply on override_applied, but rule_discount itself still held the stale value from the higher- priority discount rule when _build_rationale ran right after. The corrected code path emitted rationale text like Base price: $100.00 CPM | Agency tier: -10% | Custom rule: -20% | Final price: $50.00 CPM | (Total savings: 50.0%) claiming a 20% discount that was never actually applied to the override price. That rationale string flows into quotes, negotiated- deal text, and MCP/API responses -- a counterparty could anchor negotiation on a discount percentage that doesn't correspond to anything the price actually reflects. Reset rule_discount to 0.0 outright when an override fired, instead of just skipping the price-multiply block -- one variable, one place, covers both the applied_rules list and the rationale string with the same guard. Added test_rationale_does_not_claim_an_unapplied_discount per the review's exact ask: asserts "Custom rule" is absent from the rationale on the override path, and that the final-price line is present. Full suite: 1405 passed. ruff check / format clean.
|
Good catch — fixed in 3593652. rule_discount is now reset to 0.0 outright when override_applied is true, instead of just gating the price-multiply block. Same variable feeds both applied_rules and the rationale string, so one reset covers both — the rationale on the override path no longer mentions "Custom rule" at all now. Added test_rationale_does_not_claim_an_unapplied_discount asserting exactly that: "Custom rule" absent from the rationale, "Final price: $50.00" present, on the same higher-priority-discount-vs-lower-priority-override setup from the original bug. Full suite: 1405 passed. |
|
Updated as per the requested changes above. |
Summary
Fixes #53.
PricingRulesEngine.calculate_pricescansmatching_rules(sorted by priority, descending) and breaks as soon as it finds a rule withbase_price_override, with a comment saying the override "takes precedence." It does — for the runningpricevariable. But adiscount_percentagefrom a rule visited earlier in the same loop (a higher-priority rule with no override of its own) stays in the sharedrule_discountaccumulator and gets multiplied into the override price after the loop exits.Verified against
docs/guides/pricing-rules.mdbefore filing #53 — it draws override and discount as mutually exclusive branches off the same decision point, and states an override "stops further rule evaluation." The current code does the opposite whenever a higher-priority discount-only rule happens to be scanned first.Concrete impact
A seller configures a $50 flat negotiated rate for an agency (priority 50), alongside an unrelated agency-wide 20% discount rule (priority 100, no override). The discount rule is evaluated first — it outranks the override rule — leaving
0.20inrule_discount. The override rule is then reached, setsprice = 50, and breaks. The stale0.20still gets applied afterward: the buyer lands at $40 instead of the $50 the seller configured. Revenue leak, silent.Fix
Track whether an override fired (
override_applied) and skip the trailing discount-application block when it did. The discount-only path (no override in play, multiple discount rules competing) is untouched — still takes the highest discount, not stacked, as documented.Test plan
base_price_overridehad zero test coverage before this — that's how the bug survived since the initial commit. AddedTestPriceOverrideRulesintest_pricing_engine.py:Full suite: 1404 passed, no regressions.
ruff check/format --checkclean.Closes #53