Skip to content

feat: add capped percentage discount helper for demo review #7

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Galzi1 merged 1 commit into from
Apr 11, 2026
Merged

feat: add capped percentage discount helper for demo review #7

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions demo_app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,16 @@ def calculate_total_with_percentage_discount(

discounted_subtotal = subtotal * (1 - discount_rate)
return calculate_total(discounted_subtotal, sales_tax_rate=sales_tax_rate)


def calculate_total_with_capped_percentage_discount(
subtotal: float,
sales_tax_rate: float,
discount_rate: float,
max_discount_amount: float,
) -> float:
"""Apply a percentage discount before tax, capped by an absolute amount."""

@Galzi1 Galzi1 Apr 11, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document whether max_discount_amount is a currency amount before tax and whether the cap applies per order or per line item. That rule changes invoice behavior, so it should be explicit in the docstring rather than inferred from the implementation.


discount_amount = min(subtotal * discount_rate, max_discount_amount)

@Galzi1 Galzi1 Apr 11, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should validate both discount_rate and max_discount_amount here. A negative cap or a rate above 1.0 can silently turn the cap into a surcharge or drive the subtotal negative while still looking mathematically valid.

discounted_subtotal = subtotal - discount_amount
Comment on lines +52 to +53

@qodo-code-review qodo-code-review Bot Apr 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Negative cap increases price 🐞 Bug ≡ Correctness

calculate_total_with_capped_percentage_discount() allows max_discount_amount < 0, which makes
discount_amount negative and increases the subtotal instead of discounting it. This can silently
overcharge by turning a discount into a surcharge.
Agent Prompt 
### Issue description
`calculate_total_with_capped_percentage_discount()` accepts `max_discount_amount` without validation. If `max_discount_amount` is negative, `min(subtotal * discount_rate, max_discount_amount)` selects the negative cap, producing a negative `discount_amount` that *increases* the subtotal.

### Issue Context
The codebase already treats absolute discount amounts as non-negative (see `calculate_total_with_discount()` raising on negative `discount_amount`). `max_discount_amount` is also an absolute amount and should follow the same rule.

### Fix Focus Areas
- demo_app.py[44-54]

### Suggested fix
- Add an input check:
  - `if max_discount_amount < 0: raise ValueError("max_discount_amount must be non-negative")`
- (Optionally) also ensure `discount_amount` is never negative by clamping: `discount_amount = max(discount_amount, 0.0)`

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

return calculate_total(discounted_subtotal, sales_tax_rate=sales_tax_rate)

@Galzi1 Galzi1 Apr 11, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would calculate the capped discount amount here and then delegate to calculate_total_with_discount so the absolute-discount path stays responsible for clamping and tax flow instead of repeating pricing math in multiple helpers.

Comment on lines +52 to +54

@qodo-code-review qodo-code-review Bot Apr 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

2. Discount can exceed subtotal 🐞 Bug ≡ Correctness

calculate_total_with_capped_percentage_discount() does not bound discount_amount to the order
subtotal, so it can produce a negative discounted_subtotal and negative taxed totals. This is
inconsistent with calculate_total_with_discount(), which explicitly clamps the discounted subtotal
to >= 0.
Agent Prompt 
### Issue description
`calculate_total_with_capped_percentage_discount()` can compute a `discount_amount` greater than `subtotal` and return a negative total (because `discounted_subtotal` can go below 0 and `calculate_total()` will happily compute a negative taxed amount).

### Issue Context
`calculate_total_with_discount()` already enforces the invariant that totals should not go negative by clamping the discounted subtotal to `>= 0.0`.

### Fix Focus Areas
- demo_app.py[52-54]

### Suggested fix
- Ensure the discount can’t exceed the subtotal and clamp the resulting subtotal:
  - `discount_amount = min(subtotal * discount_rate, max_discount_amount, subtotal)`
  - `discounted_subtotal = max(subtotal - discount_amount, 0.0)`
- Keep the final call to `calculate_total(discounted_subtotal, sales_tax_rate=...)` unchanged.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Loading