[16.0][REF] sale_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.

Relates to:

• [16.0][REF] account_invoice_triple_discount: Consolidate discount in std field account-invoicing#1638

[grindtildeath]

ping @nicomacr @jjscarafia @tafaRU @chienandalu @SirAionTech @jbaudoux @leemannd @lethuthao2403 @nguyenminhchien

[chienandalu]

I like the approach 👍 (we don't maintain this module currently though)

[francesco-ooops]

@grindtildeath any plan to bring this to completion?

[grindtildeath]

@francesco-ooops I didn't have any time during the last few weeks and I'm going on holidays the next 2 ones, so I won't be able to work on it before some time.

IMO the issue to get this merged is recomputation of old moves in periods closed by fiscal year lock date as discussed in OCA/account-invoicing#1638

[jjscarafia]

Hi @grindtildeath
We like the approach. Actually we've a similar legacy approach that we're using here

We've created this module before this OCA one and never migrate to use the OCA one due to lack of time but also because somewhow we like more our approach (same as you share).

Last thing, I believe this kind of changes are not good to be done on an already working version since long time. Maybe better for 17 or 18?

[grindtildeath]

@jjscarafia Thanks for sharing your work.

It's not easy finding a good solution while the discount field is already computed by Odoo to store a discount coming from the pricelist.
I found a compromise here in the last version, would love to have feedback from those who are used to this module.

[grindtildeath]

@legalsylvain rebased

[legalsylvain]

• Could you take a look on red CI ?
• you have to remove changes in test-requirement
• I think cherry pick [FIX] sale_fixed_discount, sale_triple_discount: modules are incompatibles and can not work together by design. #3598 could help.

[grindtildeath]

@legalsylvain I'm sorry but I don't have any time left to spend on that 😞

I still did rebase this PR on 16.0 and dropped changes in test requirements/oca dependencies. However I tried cherry-picking your commits and there are conflicts on .github/workflows/test.yml

Maybe you can fix the conflict and open a PR targeting this branch? I'll merge it as soon as possible.

Otherwise feel free to open a new PR and take over this work if it's easier for you.

[legalsylvain]

Hi @grindtildeath. Thanks for your time.

I still did rebase this PR on 16.0 and dropped changes in test requirements/oca dependencies. However I tried cherry-picking your commits and there are conflicts on .github/workflows/test.yml

No worry with test.yml, it is an autogenerated file. you can recreate it with copier copy gh:oca/oca-addons-repo-template . --trust when there is conflict.

Maybe you can fix the conflict and open a PR targeting this branch? I'll merge it as soon as possible.

Done here.

Otherwise feel free to open a new PR and take over this work if it's easier for you.

That's not possible. I don't use sale_triple_discount. i just want to unblock many PRs that are stucked, since account_triple_discount refactor has been merged and not this one.
In my PR, I fixed an error sale_order_general_discount. But I faced other errors with sale_pricelist_triple_discount for exemple. I think this current PR has to be finished.
Could you take a look ?
Thanks !

[victoralmau]

Can we unlock this? Another possibility will be to exclude these modules from the CI because they are locked to different PRs.

[rousseldenis]

@grindtildeath @legalsylvain Due to : OCA/account-invoicing#1840

All repos depending on them are not running tests. We should land on this.

[rousseldenis]

@grindtildeath My question: shouldn't it be possible to make both modules compatible after refactor?

[victoralmau]

This is still pending, is there any proposal to correct tests?

[rousseldenis]

@legalsylvain In fact, this is quite bad that OCA/account-invoicing#1840 has been merged and not this one...

I think I will try to make current sale_triple_discount to work with a freezed version of account_triple_discount (before refactor).

[legalsylvain]

@legalsylvain In fact, this is quite bad that OCA/account-invoicing#1840 has been merged and not this one...

Indeed ! It's a pity. Thanks for your quick fix.

[github-actions]

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.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

[hildickethan]

@grindtildeath does this not break every _compute_discount that inherits the base Odoo one? they either won't run, will error or won't write onto discount since it isn't its compute anymore
any reason for this approach here and not on the invoice version?
regardless I will continue looking into bringing this up to date as I know you don't have time

[hildickethan]

I made a new PR #3907 based on this one to take another look at this refactoring, there are some different changes compared to this one but the general idea is the same
The use cases and module combinations I checked all seemed to work as expected, but I would appreciate it if some people involved with this PR could check it out and see if we can close this issue and be able to properly migrate higher versions

[rousseldenis]

@grindtildeath

[github-actions]

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.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

[grindtildeath]

Closed in favour of #3907