[16.0][IMP] product_margin_classification: improve taxes computation#245
Conversation
huguesdk
force-pushed
the
16.0-imp-product_margin_classification-handle_fixed_taxes
branch
from
3e68581 to
536d5ab
Compare
| # this code assumes that taxes having price_include set to | ||
| # True and of amount_type "percent" have also | ||
| # include_base_amount set to True, while those of amount_type | ||
| # "fixed" have include_base_amount set to False. |
There was a problem hiding this comment.
Better to write a test and raise a not implemented error, if conditions are not checked.
Also, i'm not sur it will work if there are fixed and percent taxes, in various order.
Dont you think ?
There was a problem hiding this comment.
yes, i agree with you. i was hesitating to add a check and raise an error, but since the current code doesn't do it (while already assuming the condition on "percent" taxes), i didn’t do it. i will add it. note that it might break current configurations.
about multiple tax combinations, i added some tests to test the most common cases.
There was a problem hiding this comment.
instead of adding exceptions, i improved the code to handle cases with include_base_amount True or False. what do you think?
* add support for taxes of amount_type "fixed". * take the include_base_amount field into account.
huguesdk
force-pushed
the
16.0-imp-product_margin_classification-handle_fixed_taxes
branch
from
536d5ab to
a2b6bc8
Compare
huguesdk
changed the title
|
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
github-actions
Bot
added
the
stale
legalsylvain
left a comment
legalsylvain
left a comment
There was a problem hiding this comment.
/ocabot merge minor
|
On my way to merge this fine PR! |
|
Congratulations, your PR was merged at f5639d1. Thanks a lot for contributing to OCA. ❤️ |
3 participants
include_base_amountfield into account.