[16.0][REF] account_invoice_triple_discount: Consolidate discount in std field #1638
Conversation
|
ping @chienandalu @pedrobaeza @Nikul-Chaudhary @SirAionTech @ramiadavid Comments welcome |
grindtildeath
force-pushed
the
16.0-ref-account_invoice_triple_discount
branch
2 times, most recently
from
05a1c30
to
42d31cb
Compare
|
FTR: This change originates from this discussion odoo/odoo#145513 (review) |
Thanks for your contribution! Before looking at the code, I'd like to know from the authors if this is ok |
| ("discount3", "!=", 0), | ||
| ] | ||
| ) | ||
| env["account.move.line"]._compute_discount() |
There was a problem hiding this comment.
IMO this will raise an exception for all the lines with a date prior to the fiscal year lock date...
There was a problem hiding this comment.
I guess it should be done by sql. Otherwise, it will raise recomputation for total od account move line. This is not desired (and forbidden)
…scal year When computing the taxes and the totals on account.move.line it's required to prevent calls to the write method on account.move or triggering recompute of unexpected fields while we assign a temporary value to the discount field to use the aggregated one bedore calling the base implementation. Otherwise the update of the discount field will invalidate totals and ... on related accoun.move and trigger calls to the write method of account.move. Any call to the write method will trigger a set of check to see if the balance is still right or check if you try to write on a record prior to the fiscal year close date or .... At the end, every time you try to access to the taxes on the records, some errors could be unexpeced raised due to the trick a temporarily changing the discount value of the field while computing others fields. In the future we should uses dedicated fields for 3 discount fields and make the native discount field a computed one with readonly=False. This approach would simplify a lot the code. See OCA#1638
There was a problem hiding this comment.
Nice refactoring and simplification !
Some remarks inline.
I guess purchase module should be changed in the same time, don't you think ?
https://github.com/OCA/purchase-workflow/blob/16.0/purchase_triple_discount/__manifest__.py#L13
| @@ -0,0 +1,29 @@ | |||
| # Copyright 2024 Camptocamp SA | |||
| # License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl) | |||
| import openupgrade | |||
There was a problem hiding this comment.
Hi. It should be openupgradelib, don't you think ?
| ("discount3", "!=", 0), | ||
| ] | ||
| ) | ||
| env["account.move.line"]._compute_discount() |
There was a problem hiding this comment.
I guess it should be done by sql. Otherwise, it will raise recomputation for total od account move line. This is not desired (and forbidden)
|
I am migrating purchase_triple_discount to v17 and I am using this method, I have added a post_ini_hook to move all the values from "discount" to discount1 when the module is installed in a database that already has discount lines, I should not add it here too ? In my case I have used: |
|
Sorry for the delay in answering here, I had some holidays and then lot of catch up to do. @ramiadavid Thanks for working on the migration. IMO the migration script from 16.0 to 17.0 should check if a @lmignon @legalsylvain Thanks for the warning on the fiscal year lock date. Indeed the compute will not work with the field set. Do you think it's an acceptable solution to remove the fiscal year lock date before calling the compute and resetting it afterwards? |
Hi. SQL is is totally adequate to :
Could you elaborate, why use ORM instead of SQL requests ? |
grindtildeath
force-pushed
the
16.0-ref-account_invoice_triple_discount
branch
from
19e60f3
to
eefdd9d
Compare
|
@legalsylvain I have nothing against SQL, just wanted to avoid reimplementing the calculation logic that is already defined at python level 😬 Anyway, seems I found my way to avoid the rounding issue, so would be nice if anyone could test this on a large DB. |
|
I was just seeing some performance problems when confirming or creating invoices with many lines (500). I know it is not very common, but in this case we need to create them and it takes up to 10 minutes to process it. And the problem has brought me exactly here, the method that exists until now makes many updates on all the lines when the taxes are recalculated (even if they do not have discount2 or discount3) because it does not check it. I was thinking about uploading a patch, but with this change you are making here it would be solved since we stopped writing the discount field twice for each line. If I can collaborate on something, you just have to tell me. To finish as soon as possible. Thank you! |
|
@ramiadavid In a first time you could try #1664. It should improve performances. |
|
I'll try it, I hadn't seen it since the title is not well written... |
|
The title is right; it fixes a bug when taxes are recomputed on a closed
fiscal year. As a side effect, it improves the compute method and avoid
useless recompute...
…On Mon, May 13, 2024 at 8:58 AM David Ramia ***@***.***> wrote:
I'll try it, I hadn't seen it since the title is not well written...
[16.0][FIX] *account_invoice _riple_discount*: Fix tax recompute on
closed fiscal year
—
Reply to this email directly, view it on GitHub
<#1638 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEE2WQLHRWGQP7IX2HJAQ3ZCBQBTAVCNFSM6AAAAABBOZKHCGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBWG44TINJYGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
grindtildeath
force-pushed
the
16.0-ref-account_invoice_triple_discount
branch
from
eefdd9d
to
9a08ba8
Compare
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.
grindtildeath
force-pushed
the
16.0-ref-account_invoice_triple_discount
branch
2 times, most recently
from
eee2af2
to
9b57b34
Compare
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.