[16.0] [REF] account_invoice_triple_discount Consolidate discount in std field (Follow-up of #1638) #1840
Conversation
legalsylvain
changed the title
|
CI is green ! |
account_invoice_triple_discount/migrations/16.0.2.0.0/post-migrate.py
Outdated
Show resolved
Hide resolved
|
Hi.
Well, that PR doesn't change that point. (the incompatibility exists for the time being). It is just mandatory to explicit the incompatibility in the CI, because tests are now failing. @grindtildeath : There was a problem in the previous implementation, regarding the digits precision. I fixed it here 62d5d3f, could you take a look ? @lmignon: I replaced the migration script by a unique SQL request. Data before the migration (16.0.1.0.3)
id | quantity | price_unit | discount | discount2 | discount3 | price_subtotal
----+----------+------------+----------+-----------+-----------+----------------
40 | 1.00 | 1000.00 | 10.00 | 20.00 | 30.00 | 504.00
43 | 1.00 | 1000.00 | 5.00 | 9.00 | 13.00 | 752.12
44 | 1.00 | 1000.00 | 1.11 | 2.22 | 3.33 | 934.75
Data after the migration (16.0.2.0.0)
id | quantity | price_unit | discount | discount1 | discount2 | discount3 | price_subtotal
----+----------+------------+--------------------+-----------+-----------+-----------+----------------
40 | 1.00 | 1000.00 | 49.6 | 10.00 | 20.00 | 30.00 | 504.00
43 | 1.00 | 1000.00 | 24.7885 | 5.00 | 9.00 | 13.00 | 752.12
44 | 1.00 | 1000.00 | 6.5252895786 | 1.11 | 2.22 | 3.33 | 934.75
|
| openupgrade.logged_query( | ||
| env.cr, | ||
| """ | ||
| UPDATE account_move_line |
There was a problem hiding this comment.
@legalsylvain Your script doesn't take into account the discount_type to compute the discount value....
There was a problem hiding this comment.
Hi @lmignon. could you elaborate ? I don't understand what is discount_type.
In V16 :
$ rgrep "discount_type" ./ --include=*.py
./addons/l10n_it_edi/models/account_edi_format.py: discount_type = discount_element.xpath('.//Tipo')
./addons/l10n_it_edi/models/account_edi_format.py: if discount_type and discount_type[0].text == 'MG':
./addons/l10n_it_edi/models/account_edi_format.py: discount_type = discount_element.xpath('.//Tipo')
./addons/l10n_it_edi/models/account_edi_format.py: if discount_type and discount_type[0].text == 'MG':
./addons/l10n_it_edi/models/account_invoice.py: def discount_type(discount):
./addons/l10n_it_edi/models/account_invoice.py: 'discount_type': discount_type,
There was a problem hiding this comment.
Sorry... it's not in this addon... it's into sale_triple_discount https://github.com/OCA/sale-workflow/blob/16.0/sale_triple_discount/models/sale_order_line.py#L26
|
@grindtildeath : could you take a look here, as it is your original work ? |
There was a problem hiding this comment.
Thanks for taking this over @legalsylvain 🤗
I'll be on holidays for the next 10 days so feel free to merge together with sale_triple_discount refactor.
|
I have no right to merge this one. @OCA/accounting-maintainers : could you merge this one ? Thanks ! |
|
/ocabot merge major |
|
What a great day to merge this nice PR. Let's do it! |
|
@pedrobaeza your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-1840-by-pedrobaeza-bump-major. After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red. |
Until now, the triple discount feature did use the standard Odoo field as the first discount field, adding only extra fields for the second and the third discount. This implied we had to override any function using discount to consider the other discounts properly. By adding an extra field discount1 to store the first discount, it allows to redefine the standard discount field to a computed field that will consolidate the triple discount from the other fields, and avoid the need to redefine anything relying on the discount field as it will already consider the triple discounts.
…orithm [FIX] account_invoice_triple_discount: populate discount1 field to avoid inconsistency if database contains account.move.line with not null discount before the installation of the module [REF] account_invoice_triple_discount: Add GRAP as coauthor of the module [FIX] account_invoice_triple_discount: Manage Correct creation of account move lines, where 'discount' field is set
In case this module is installed and sale module is also installed but not sale_triple_discount, or in case sale_triple_discount is not updated to its latest version, make sure that setting discount, without having discount1 defined, will not mess with the triple discounts.
…nt_invoice_triple_discount is installed
- set modules as incompatible in the manifest. (For the time being, there is only a informative message in the README of the module account_invoice_fixed_discount) - Run separated tests for both modules.
…eld 'discount' as the field is now a technical field that should not have limitation. For exemple in the default case where digits precision of Discount is 2, if discounts are 05%, 09% and 13% the main discount is 24.7885 % (and not 24.79)
…he field 'discount' and reduce complexity
legalsylvain
force-pushed
the
16.0-ref-account_invoice_triple_discount
branch
from
dfa322e to
f3e9a4a
Compare
|
@pedrobaeza. I just rebased this PR and the test are all green. could you try to merge again ? sorry for disturbing you, I have no right on accounting repo for the time being. |
|
/ocabot merge major |
|
Hey, thanks for contributing! Proceeding to merge this for you. |
|
Hi. i don't understand the CI for the time being. Step to reproduce :
(82 bot cmmits, 3 pages) Result:
It is always the same diff.
CC : @OCA/core-maintainers |
|
That's because of the rebel modules, that export the POT separately. Thus, one CI adds some texts, and the other removes them, so this can't be merged unless you compatibilize them and put them all together. |
Finish #1638
Basically
Get all the commits from the previous @grindtildeath PR.
remove this commit 01df667 that was introducing incompatibility between
account_invoice_triple_discountandaccount_invoice_global_discountthat can works together in fact.Add
account_global_discountworking ifaccount_invoice_triple_discountis installed. With that commit, the remark of @pedrobaeza should be answered.account_invoice_triple_discountandaccount_invoice_fixed_discount. This commit is a fix in fact, as the exclusion is written in the documentation, but the CI and the copier answers don't mention that exclusion.Let's see if all CI is green.