[IMP] purchase_deposit: make the deposit over total check optional - #3172
OCA-git-bot merged 1 commit into
Conversation
|
Hi @HviorForgeFlow! Can you review the fix please? Thanks |
4429596 to
135b966
Compare
|
Dropped the manifest version bump here too, per @HviorForgeFlow's review on #3142 — ocabot does it on merge, so no |
|
For whoever merges: the red |
|
@Saran440 @TheerayutEncoder could you take a look as you worked on the feature to be removed here in #2788 |
Saran440
left a comment
Saran440
left a comment
There was a problem hiding this comment.
Code reviewed 👍
Sorry, I missed this use case before. I think this check should be configurable rather than hard-coded.
246c15a to
b630ab1
Compare
mav-adhoc
changed the title
|
@Saran440 good point — done: the check is back, behind a company setting ( |
b630ab1 to
20bde28
Compare
HviorForgeFlow
left a comment
HviorForgeFlow
left a comment
There was a problem hiding this comment.
That looks much better, I was affraid of removing the feature without asking their contributors.
Just a small nitpick...
| domain=[("type", "=", "service")], | ||
| help="Default product used for payment advances.", | ||
| ) | ||
| purchase_deposit_limit_to_total = fields.Boolean( |
There was a problem hiding this comment.
What about renaming to purchase_deposit_limit_order_total?
|
This PR has the |
The check added in OCA#2788 refuses legitimate deposits and offers no way out from the UI, so it now depends on a company setting, "Limit Deposits to Order Total", disabled by default: - it compares the deposit lines' price_unit (tax excluded) against order.amount_total (tax included), so the effective ceiling depends on whether the order has taxes. Deposit lines also carry product_qty = 0 and never contribute to amount_total. - when the order is in a foreign currency and the vendor invoices the deposits in the company currency, each deposit line holds an amount converted at the rate of its own bill date, so the sum it adds up has no single right value to compare against. - 16.0, 17.0 and 19.0 do not have the check, so a disabled default keeps 18.0 in line with the other branches. The error message now states the amounts and the currency, and its msgid is static again: it was built with an f-string and never translated. The configuration fragment is renamed to CONFIGURE.md, the name the readme generator looks for, so that it reaches README.rst at last.
20bde28 to
d133011
Compare
|
/ocabot merge minor |
|
What a great day to merge this nice PR. Let's do it! |
|
@mav-adhoc mind you to forward the change to 19.0, thanks |
|
Congratulations, your PR was merged at 85dfc30. Thanks a lot for contributing to OCA. ❤️ |
|
@HviorForgeFlow Done! #3182 |
Forward port of OCA#3172. The check added in OCA#2788 never reached this branch, so it arrives here already behind the company setting "Limit Deposits to Order Total", disabled by default: the module keeps the behaviour it has today, and a deployment that wants the ceiling enables it in Purchase > Configuration > Settings. It is not enforced by default because: - it compares the deposit lines' price_unit (tax excluded) against order.amount_total (tax included), so the effective ceiling depends on whether the order has taxes. Deposit lines also carry product_qty = 0 and never contribute to amount_total. - when the order is in a foreign currency and the vendor invoices the deposits in the company currency, each deposit line holds an amount converted at the rate of its own bill date, so the sum it adds up has no single right value to compare against. The error message states the amounts and the currency, so that a refusal can be read without opening the order. The configuration fragment is renamed to CONFIGURE.md, the name the readme generator looks for, so that it reaches README.rst at last.
8 participants
What
Puts the "deposit can't be greater than the PO total" check added in #2788 (18.0
only) behind a new company setting, Limit Deposits to Order Total, disabled by
default. Previous version of this PR removed the check; made it optional instead,
as @Saran440 suggested in the review.
Why
The check refuses legitimate deposits, and there is no way around it from the UI.
1. The two sides of the comparison are not the same base. It sums the deposit
lines'
price_unit, which is tax excluded, and compares it againstorder.amount_total, which is tax included. The effective ceiling therefore depends onwhether the order carries taxes: the untaxed total on a tax exempt order, the tax
included total on a taxed one. Deposit lines also carry
product_qty = 0, so they nevercontribute to
amount_totalthemselves.2. On a multi-currency order the sum it compares has no single right value. When the
order is in a foreign currency and the vendor bills the deposits in the company
currency, each deposit line holds an amount converted from the bill. Whichever rate that
conversion uses, the sum cannot serve as a hard limit:
the order floats with the exchange rate. An order advanced in fixed instalments of the
company currency stops adding up, and the last instalment is refused even though every
single amount is right.
purchase.order.currency_rate, the basis ofamount_total_cc) a fully advanced order does add up — but only while that rate isstill meaningful. On an order paid over a year or more in a high-inflation company
currency, the order rate is stale and it inflates every later deposit instead.
I measured both bases against a set of real orders: each one unblocks some and blocks
others, several by a wide margin. That is not a bug in either conversion — an exchange
difference on a prepayment is real and has to be recognised somewhere. But it has to be
recognised in the accounting of the bills, not charged against a commercial control that
then refuses the operation.
Why the setting is disabled by default. 16.0, 17.0 and 19.0 do not have this check,
so a disabled default keeps 18.0 in line with the other branches, and the two points
above mean an enabled default keeps refusing operations that are correct. There is no
migration script: the setting starts disabled everywhere, and a deployment that wants
the ceiling enables it in Purchase > Configuration > Settings. For reference, core
salehandles the same situation with a non-blocking warning in the wizard(
display_invoice_amount_warning) rather than an error.The error message is now diagnosable. It states the deposit total, the order total
and the currency, and its msgid is static again: it was built with an f-string inside
self.env._(), which made the msgid dynamic and the string untranslatable.The readme fragment is renamed
CONFIGURATION.rst→CONFIGURE.md, the name the readmegenerator looks for; the old one never reached
README.rst.README.rstitself is leftto the bot, as the CI expects for PRs that touch readme fragments.
Test plan
test_not_allow_deposit_greater_than_po_totalis restored, with the setting enabled.test_allow_deposit_greater_than_po_total_by_defaultcovers the default: the samedeposit that the check refuses is created.
0 failed, 0 error(s) of 8 tests.The deposits of P00045 would add up to $ 5,000.00, more than its total amount of $ 4,830.00.