[16.0][REF] account_invoice_triple_discount: Consolidate discount in std field

[grindtildeath]

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]

ping @chienandalu @pedrobaeza @Nikul-Chaudhary @SirAionTech @ramiadavid

Comments welcome

[grindtildeath]

FTR: This change originates from this discussion https://github.com/odoo/odoo/pull/145513#pullrequestreview-1774865804

[SirAionTech]

ping @chienandalu @pedrobaeza @Nikul-Chaudhary @SirAionTech @ramiadavid

Comments welcome

Thanks for your contribution! This is a cleaner approach indeed and I have talked about it a few times; I never got to an implementation because I was always told that both approaches had been discussed and the current one had been chosen in the end. I think there was even another module with the same name implementing this approach in some fork.

Before looking at the code, I'd like to know from the authors if this is ok

[ramiadavid]

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: UPDATE purchase_order_line upd SET discount1=pol.discount FROM (SELECT * FROM purchase_order_line WHERE discount IS NOT NULL AND discount1 IS NULL AND discount2 IS NULL AND discount3 IS NULL) as pol WHERE upd.id = pol.id

[grindtildeath]

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 discount1 column exists before copying the value, so that it allows "migrating" to this new computation from v16.0, if we manage to complete this PR.

@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?

[legalsylvain]

@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 :

• populate quickly big tables
• avoid warning / constrains / check like @lmignon mentioned. (but maybe other things (from odoo / OCA or custom modules) can generates errors.
• avoid to alter undesired things. (like write_date, or extra fields that depends on changed field)

Could you elaborate, why use ORM instead of SQL requests ?

[grindtildeath]

@legalsylvain I have nothing against SQL, just wanted to avoid reimplementing the calculation logic that is already defined at python level :grimacing:

Anyway, seems I found my way to avoid the rounding issue, so would be nice if anyone could test this on a large DB.

[ramiadavid]

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!

[lmignon]

@ramiadavid In a first time you could try #1664. It should improve performances.

[ramiadavid]

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

[lmignon]

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 https://github.com/OCA/account-invoicing/pull/1638#issuecomment-2106794584, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEE2WQLHRWGQP7IX2HJAQ3ZCBQBTAVCNFSM6AAAAABBOZKHCGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBWG44TINJYGQ . You are receiving this because you were mentioned.Message ID: @.***>

[legalsylvain]

hi @grindtildeath

• I made a simplification + fix here : https://github.com/grindtildeath/account-invoicing/pull/7 could you take a look ?
• A very big thanks for this PR. I've rarely seen such a complex OCA module made so simple (and so easy to maintain).

Kind regards.

[legalsylvain]

Note : I adapted purchase_discount module to this refactoring. see : https://github.com/OCA/purchase-workflow/pull/2296

[legalsylvain]

Just as a suggestion, why change to optional="show" by default when odoo has the discount field hidden, wouldn't it be more consistent to also leave it hidden by default?

I'm not sure. Odoo set optional="hide" because it considers that maybe the feature is not very used, to make the UI cleaner.

In our context, if we install account_invoice_triple_discount, we can guess that users want to access this field, so optional="show" is not inconsistent for me.

Just a point of view. Matter of taste for me, not a blocking point.

[grindtildeath]

@legalsylvain Thank you so much for your help and your PR :star_struck:

@ramiadavid One one hand, you're right (setting it hidden would be closer to std odoo). On the other hand, those having the module installed are more likely to use these fields. I'd be ok to set it as hidden if that's what other want.

[OCA-git-bot]

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

[legalsylvain]

@grindtildeath : could you :

• rebase (there is conflict)
• bump manually the version of account_global_discount. (See comment)

So we can merge it after. Thanks !

[grindtildeath]

@legalsylvain Actually there's an issue when account_invoice_pricelist is computed as the module updates the discount field when computing price_unit: https://github.com/OCA/account-invoicing/blob/14.0/account_invoice_pricelist/models/account_move.py#L191-L223 :weary:

I need to find more time to refactor that module to have this working 100% but I don't know when I'll be able to.

[legalsylvain]

Hi @grindtildeath Thanks for your answer. Could you at least rebase ? I want to use it with gitaggregate, but for the time being, there are conflict.

For the write of discount in account_invoice_pricelist, indeed, there is some problem overthere. i'll try to make a PoC.

[legalsylvain]

@grindtildeath could you take a look here : https://github.com/grindtildeath/account-invoicing/pull/8

I set an inverse function on discount field.

• It works (see tests).
• it doesn't generate recursive computation when we write on discount field.

what do you think ?

thanks !

[legalsylvain]

@sergiocorato is it now OK for you ?

[grindtildeath]

@legalsylvain the issue as I see it is the inverse function is only executed on write, whereas the update of discount field is executed through an onchange by the compute function. Which means on the UI, when the product is selected, discount is updated but discount1 stay 0 if we don't save. :disappointed:

[legalsylvain]

Hi @grindtildeath. I don't get the use case. Do you mean : with or without account_invoice_pricelist ?

[ramiadavid]

I don't know if you have already seen this, but when an invoice is created from a sales order the discount1 field is not filled with the discount field of the order

[ramiadavid]

I think this should be added:

@api.model_create_multi
def create(self, vals_list):
    for vals in vals_list:
        if vals.get("discount") and not any(vals.get(field) for field in self._get_multiple_discount_field_names()):
            vals["discount1"] = vals.pop("discount")
    return super().create(vals_list)

And this way this would no longer be necessary: https://github.com/OCA/purchase-workflow/blob/2104054c4322916a202a47c6caab692ce582f5fc/purchase_discount/models/purchase_order.py#L112-L119

[legalsylvain]

@ramiadavid : Done here https://github.com/grindtildeath/account-invoicing/pull/9

could you review ?

CC : @grindtildeath

[legalsylvain]

Which means on the UI, when the product is selected, discount is updated but discount1 stay 0 if we don't save. 😞

if Odoo/account module is installed (without any OCA module) :

• on purchase invoice, when we select a product, the price is set based on the 'standard_price' of the product. There is no discount set. (even if there is a discount on the "product.supplierinfo" model.
• on sale invoice, when we select a product, the price is set, based on the 'list_price' of the product. There is no discount set.

could you elaborate ? for the time being Odoo doesn't populate 'discount' field. (unlike the sale and purchase module.)

[ramiadavid]

If you create an invoice from a purchase order (with three discounts) the invoice is created with the three discounts added in the discount1 field.

The problem seems to occur in the _inverse_discount function

http://oca-account-invoicing-16-0-pr1638-64605a2af360.runboat.odoo-community.org/web#id=11&cids=1&menu_id=204&action=319&model=purchase.order&view_type=form

[legalsylvain]

Hi @ramiadavid. Sorry I don't get it. the link you provide does'nt seems to be valid. Could you elaborate the step to reproduce ? Which module installed ? The action to reproduce the bug. (text or printscreen...) thanks !

[ramiadavid]

Yes, install purchase_triple_discount, create purchase order with three discounts, then create invoice from purchase order and discount in invoice its not correct