From 22e292ce3605ade026ead9785a4d5535366778fe Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Sat, 11 Apr 2026 07:09:12 +0000 Subject: [PATCH] chore: update PR knowledge base --- kb/.manifest.json | 5 +- kb/INDEX.md | 9 ++-- ...te-capped-discount-once-and-delegate-to.md | 45 ++++++++++++++++++ ...lation-rules-pre-post-tax-and-scope-are.md | 46 +++++++++++++++++++ ...t-caps-and-rates-above-1-0-can-silently.md | 43 +++++++++++++++++ 5 files changed, 144 insertions(+), 4 deletions(-) create mode 100644 kb/architecture_decision/refactor-to-calculate-capped-discount-once-and-delegate-to.md create mode 100644 kb/domain_knowledge/discount-calculation-rules-pre-post-tax-and-scope-are.md create mode 100644 kb/gotcha/negative-discount-caps-and-rates-above-1-0-can-silently.md diff --git a/kb/.manifest.json b/kb/.manifest.json index 0740517..29e9757 100644 --- a/kb/.manifest.json +++ b/kb/.manifest.json @@ -14,5 +14,8 @@ "3066907115": "architecture_decision/consolidating-percentage-and-absolute-discount-calculations.md", "3066907152": "domain_knowledge/clarify-discount-rate-input-format-and-tax-application.md", "4226957527": "code_pattern/the-discount-calculation-uses-a-multiplier-approach-1.md", - "4226957619": "gotcha/the-new-percentage-discount-function-lacks-input-validation.md" + "4226957619": "gotcha/the-new-percentage-discount-function-lacks-input-validation.md", + "3067724989": "gotcha/negative-discount-caps-and-rates-above-1-0-can-silently.md", + "3067725001": "architecture_decision/refactor-to-calculate-capped-discount-once-and-delegate-to.md", + "3067725011": "domain_knowledge/discount-calculation-rules-pre-post-tax-and-scope-are.md" } \ No newline at end of file diff --git a/kb/INDEX.md b/kb/INDEX.md index 0f86c6f..cc1a100 100644 --- a/kb/INDEX.md +++ b/kb/INDEX.md @@ -1,26 +1,29 @@ # Knowledge Base Index -## Architecture Decision (3) +## Architecture Decision (4) - [Chose to maintain backward compatibility through an alias while explicitly rejecting dual parameter usage to enforce a clear migration path.](architecture_decision/chose-to-maintain-backward-compatibility-through-an-alias.md) - [Consolidating percentage and absolute discount calculations through a single normalization path reduces maintenance burden and prevents pricing logic divergence.](architecture_decision/consolidating-percentage-and-absolute-discount-calculations.md) - [Question about whether the discount API should support both absolute amounts and percentages, and the naming implications of choosing one representation.](architecture_decision/question-about-whether-the-discount-api-should-support-both.md) +- [Refactor to calculate capped discount once and delegate to shared helper to avoid repeating pricing math and consolidate tax flow responsibility.](architecture_decision/refactor-to-calculate-capped-discount-once-and-delegate-to.md) ## Code Pattern (2) - [Adding a helper function for discount pricing calculations that applies discounts before tax computation.](code_pattern/adding-a-helper-function-for-discount-pricing-calculations.md) - [The discount calculation uses a multiplier approach (1 - discount_rate) rather than direct subtraction, which is a common pattern for composable percentage calculations.](code_pattern/the-discount-calculation-uses-a-multiplier-approach-1.md) -## Domain Knowledge (4) +## Domain Knowledge (5) - [Clarify discount_rate input format and tax application semantics in documentation since they directly affect invoice calculations.](domain_knowledge/clarify-discount-rate-input-format-and-tax-application.md) - [discount_amount represents an absolute value rather than a percentage, with tax calculated post-discount; percentage discounts would be handled separately.](domain_knowledge/discount-amount-represents-an-absolute-value-rather-than-a.md) +- [Discount calculation rules (pre/post-tax and scope) are business logic that must be explicitly documented to prevent invoice behavior misunderstandings.](domain_knowledge/discount-calculation-rules-pre-post-tax-and-scope-are.md) - [Tax calculation order relative to discounts is a business rule that needs explicit documentation.](domain_knowledge/tax-calculation-order-relative-to-discounts-is-a-business.md) - [The pricing rule applies absolute discounts before tax calculation, with the business logic now documented in code and demonstrated in test cases.](domain_knowledge/the-pricing-rule-applies-absolute-discounts-before-tax.md) -## Gotcha (7) +## Gotcha (8) - [Discount amounts are not validated, allowing negative totals and tax calculations to propagate invalid charges downstream.](gotcha/discount-amounts-are-not-validated-allowing-negative-totals.md) +- [Negative discount caps and rates above 1.0 can silently invert discounts into surcharges or make subtotals negative while appearing mathematically correct.](gotcha/negative-discount-caps-and-rates-above-1-0-can-silently.md) - [Renaming a function parameter breaks backward compatibility for callers using keyword arguments, with no migration path provided.](gotcha/renaming-a-function-parameter-breaks-backward-compatibility.md) - [Renaming a function parameter breaks backward compatibility for callers using keyword arguments without providing a deprecation path.](gotcha/renaming-a-function-parameter-breaks-backward-compatibility-2.md) - [The function now prevents negative charges and taxes by rejecting negative discounts and clamping subtotals to zero, addressing unexpected edge case behavior.](gotcha/the-function-now-prevents-negative-charges-and-taxes-by.md) diff --git a/kb/architecture_decision/refactor-to-calculate-capped-discount-once-and-delegate-to.md b/kb/architecture_decision/refactor-to-calculate-capped-discount-once-and-delegate-to.md new file mode 100644 index 0000000..6b3180e --- /dev/null +++ b/kb/architecture_decision/refactor-to-calculate-capped-discount-once-and-delegate-to.md @@ -0,0 +1,45 @@ +--- +pr_url: https://github.com/Galzi1/github-pr-kb-demo/pull/7 +pr_title: "feat: add capped percentage discount helper for demo review" +comment_url: https://github.com/Galzi1/github-pr-kb-demo/pull/7#discussion_r3067725001 +author: "Galzi1" +date: 2026-04-11T07:08:18+00:00 +category: architecture_decision +confidence: 0.92 +needs_review: false +comment_id: 3067725001 +--- + +# Refactor to calculate capped discount once and delegate to shared helper to avoid repeating pricing math and consolidate tax flow responsibility. + +## Context + +A new capped percentage discount helper is being added to support demo review functionality. The implementation requires handling both percentage-based discounts and ensuring they are properly constrained within acceptable bounds. + +## Decision + +The capped percentage discount calculation is delegated to the existing absolute-discount helper rather than implementing independent pricing mathematics. The percentage discount amount is calculated first, then passed to the calculate_total_with_discount function, allowing the absolute-discount path to maintain responsibility for clamping discount values and managing tax calculation flows. + +## Consequences + +This architectural approach consolidates pricing mathematics and discount clamping logic into a single code path, reducing duplication of discount calculation and tax handling logic across multiple helpers. The absolute-discount helper becomes the authoritative implementation for these concerns rather than having the logic replicated in percentage-based discount variants. + +``` +@@ -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.""" ++ ++ discount_amount = min(subtotal * discount_rate, max_discount_amount) ++ discounted_subtotal = subtotal - discount_amount ++ return calculate_total(discounted_subtotal, sales_tax_rate=sales_tax_rate) +``` diff --git a/kb/domain_knowledge/discount-calculation-rules-pre-post-tax-and-scope-are.md b/kb/domain_knowledge/discount-calculation-rules-pre-post-tax-and-scope-are.md new file mode 100644 index 0000000..1a67733 --- /dev/null +++ b/kb/domain_knowledge/discount-calculation-rules-pre-post-tax-and-scope-are.md @@ -0,0 +1,46 @@ +--- +pr_url: https://github.com/Galzi1/github-pr-kb-demo/pull/7 +pr_title: "feat: add capped percentage discount helper for demo review" +comment_url: https://github.com/Galzi1/github-pr-kb-demo/pull/7#discussion_r3067725011 +author: "Galzi1" +date: 2026-04-11T07:08:19+00:00 +category: domain_knowledge +confidence: 0.92 +needs_review: false +comment_id: 3067725011 +--- + +# Discount calculation rules (pre/post-tax and scope) are business logic that must be explicitly documented to prevent invoice behavior misunderstandings. + +## Context + +A capped percentage discount helper feature has been added for demo review purposes. This helper function applies a percentage-based discount with a maximum limit. + +## Key Insight + +The behavior of the maximum discount amount parameter requires explicit documentation to prevent misunderstandings. Specifically, two aspects need clarification: + +1. Whether the maximum discount amount represents a currency value calculated before or after tax application +2. Whether the cap operates at the order level (applying once across the entire order) or at the line item level (applying individually to each line item) + +Currently, this information may only be inferrable from the implementation details rather than being clearly stated in the documentation. + +## Implications + +Not stated in the source comment. + +``` +@@ -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.""" +``` diff --git a/kb/gotcha/negative-discount-caps-and-rates-above-1-0-can-silently.md b/kb/gotcha/negative-discount-caps-and-rates-above-1-0-can-silently.md new file mode 100644 index 0000000..e7978ad --- /dev/null +++ b/kb/gotcha/negative-discount-caps-and-rates-above-1-0-can-silently.md @@ -0,0 +1,43 @@ +--- +pr_url: https://github.com/Galzi1/github-pr-kb-demo/pull/7 +pr_title: "feat: add capped percentage discount helper for demo review" +comment_url: https://github.com/Galzi1/github-pr-kb-demo/pull/7#discussion_r3067724989 +author: "Galzi1" +date: 2026-04-11T07:08:17+00:00 +category: gotcha +confidence: 0.92 +needs_review: false +comment_id: 3067724989 +--- + +# Negative discount caps and rates above 1.0 can silently invert discounts into surcharges or make subtotals negative while appearing mathematically correct. + +## Symptom + +Discount calculations may produce unexpected results when invalid parameters are used. A negative cap value or a discount rate exceeding 1.0 can cause the discount to function as a surcharge or reduce the subtotal to negative values, while the calculation still appears mathematically sound. + +## Root Cause + +The capped percentage discount helper lacks validation for its input parameters. Specifically, the `discount_rate` and `max_discount_amount` values are not checked for logical constraints before being applied to the calculation. + +## Fix or Workaround + +Validate both the `discount_rate` and `max_discount_amount` parameters before performing discount calculations. Ensure the discount rate does not exceed 1.0 and that the maximum discount amount is not negative, preventing silent conversion of the discount into a surcharge or subtotal inversion. + +``` +@@ -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.""" ++ ++ discount_amount = min(subtotal * discount_rate, max_discount_amount) +```