[18.0][FIX] account_payment_term_extension: fix percentage calculation with fixed amount on first line - #932
Conversation
… fixed amount on first line When a payment term has a fixed amount on the first line and percentage lines after it, the percentage lines were calculated on the full invoice total instead of the remaining amount after deducting the fixed amount. This caused incorrect amounts and in some cases negative amounts on the last payment term line. Fix: use remaining_amount instead of total_amount in compute_line_amount for percentage calculations. Closes: OCA#924
OCA-git-bot
added
series:18.0
mod:account_payment_term_extension
There was a problem hiding this comment.
Pull request overview
Fixes incorrect installment amounts in account_payment_term_extension when payment terms start with a fixed amount followed by percentage lines (issue #924), aiming to compute percentages on the residual base after fixed amounts.
Changes:
- Update
compute_line_amount()to useremaining_amountas the base for percent-based lines. - Update
_compute_terms()to passremaining_amountinto percent-based computations (instead oftotal_amount).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| account_payment_term_extension/models/account_payment_term_line.py | Changes percent computation base from total_amount to remaining_amount. |
| account_payment_term_extension/models/account_payment_term.py | Adjusts percent branch to call compute_line_amount() using remaining_amount values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Compute the amount for a payment term line. | ||
| In case of procent computation, use the payment | ||
| term line rounding if defined | ||
|
|
||
| :param total_amount: total balance to pay | ||
| :param remaining_amount: total amount minus sum of previous lines | ||
| computed amount | ||
| :returns: computed amount for this line | ||
| """ | ||
| self.ensure_one() | ||
| if self.value == "fixed": | ||
| return float_round(self.value_amount, precision_digits=precision_digits) | ||
| elif self.value in ("percent", "percent_amount_untaxed"): | ||
| amt = total_amount * self.value_amount / 100.0 | ||
| # Use remaining_amount to correctly handle fixed amounts on previous lines | ||
| amt = remaining_amount * self.value_amount / 100.0 | ||
| if self.amount_round: |
There was a problem hiding this comment.
compute_line_amount() now ignores total_amount for percent-based lines and uses remaining_amount instead. The docstring still describes remaining_amount as "total minus sum of previous lines", which would imply compounding percentages if callers actually pass a running residual. If the intended behavior is "total minus fixed amounts" (so multiple percent lines share the same base), please update the docstring/comment to reflect that contract so future call sites don’t accidentally change semantics.
| # Percentage amounts - use remaining_amount for correct calculation | ||
| # when there are fixed amounts on previous lines | ||
| line_amount = line.compute_line_amount( | ||
| total_amount, remaining_amount, precision_digits | ||
| remaining_amount, remaining_amount, precision_digits | ||
| ) | ||
| company_line_amount = line.compute_line_amount( | ||
| total_amount_currency, | ||
| remaining_amount_currency, | ||
| remaining_amount_currency, | ||
| company_precision_digits, | ||
| ) |
There was a problem hiding this comment.
This change modifies the core payment-term amount computation logic but there’s no test covering the fixed-first-line + subsequent percent-lines case described in issue #924. Adding a regression test for the 400 + 33% + 33% + 34% example (asserting the resulting line_ids amounts and that the last line is not negative) would prevent future reintroductions.
| # Percentage amounts - use remaining_amount for correct calculation | ||
| # when there are fixed amounts on previous lines | ||
| line_amount = line.compute_line_amount( | ||
| total_amount, remaining_amount, precision_digits | ||
| remaining_amount, remaining_amount, precision_digits | ||
| ) | ||
| company_line_amount = line.compute_line_amount( | ||
| total_amount_currency, | ||
| remaining_amount_currency, | ||
| remaining_amount_currency, | ||
| company_precision_digits, | ||
| ) |
There was a problem hiding this comment.
The percentage-amount branch now passes remaining_amount into compute_line_amount, but remaining_amount is initialized once to the invoice total and is never reduced when fixed lines are processed. As a result, percentage lines will still be computed on the full total and the reported negative-last-line scenario will persist. Consider updating remaining_amount/remaining_amount_currency inside the loop when a fixed line is applied (deduct the fixed line amount from the base used for subsequent percent lines), and keep passing total_amount/total_amount_currency as the first argument for clarity/future compatibility.
|
Hi @pedrobaeza — would you have a moment to review this fix? When a payment term has a fixed amount on the first line and percentage lines after, the percentages were being calculated on the full total instead of the remaining amount after deducting the fixed amount. This caused incorrect and sometimes negative amounts. The fix changes This has been open since April — any feedback would be appreciated. Thanks! |
pedrobaeza
changed the title
pedrobaeza
left a comment
pedrobaeza
left a comment
There was a problem hiding this comment.
Please add a test exercising the problem
Description
When a payment term has a fixed amount on the first line and percentage lines after it, the percentage lines are calculated on the full invoice total instead of the remaining amount after deducting the fixed amount.
This causes incorrect amounts and in some cases negative amounts on the last payment term line.
Steps to reproduce
Create a payment term with:
Create an invoice for 1000€ with this payment term
Expected:
Actual (bug):
Fix
Changed in both and to use instead of for percentage calculations. This ensures that percentage lines are calculated on the residual amount after fixed amounts have been deducted.
Related Issue
Closes #924
Contributor from SMWLAB community. First OCA PR!