[16.0][FIX] account_invoice_fixed_discount,account_invoice_triple_discount: Remove excludes

[hildickethan]

Follow-up to the changes merged in #1840
Since having these modules in excludes ends up being a bit problematic and messy in github and existing installations that might have both installed, I think the better approach is nullifying the functionality of one of them when both are installed.
IMO if both are set the fixed discount should take priority, since the general sentiment when adding a fixed discount you're thinking "I just want to discount 20€ from this line".

Optimally, you would have a way of both co-existing and computing taking each other into account (ideally also the general and global discount modules would be included in this), but that would require a glue module. Since these modules have co-existed until now without that feature taken care of, I don't think it's a practical use case and not worth the time to implement it until someone needs it.

With these changes the modules can be co-installed and you can use either, just not simultaneously on the same line as the fixed will take priority.

If this seems OK, I will supersede OCA/sale-workflow#2885 and add them so we can get that merged also and finish this refactor

Screenshots

[legalsylvain]

Hi @hildickethan Thanks a lot to try to fix this problem.

Unfortunately, the current design you propose generate trouble in many places I think. For exemple, the data in the reports will be wrong, when discount_fixed is set, also the PDF will display bad things to customers.

Since these modules have co-existed until now without that feature taken care of,

In fact no. AFAIK, both modules never worked together. But the problem was hidden. If you don't think so, please mention the version where both module installed was working correctly. Thanks !

[hildickethan]

Since these modules have co-existed until now without that feature taken care of,

In fact no. AFAIK, both modules never worked together. But the problem was hidden. If you don't think so, please mention the version where both module installed was working correctly. Thanks !

What I meant is that they could be co-installed even if they didn't work together, sorry I could've worded it better

Unfortunately, the current design you propose generate trouble in many places I think. For exemple, the data in the reports will be wrong, when discount_fixed is set, also the PDF will display bad things to customers.

While this is true, it's easily fixable by the user setting the triple fields to 0 after deciding they want a fixed discount instead. Realistically since they never worked together, I don't think there is anyone that is going to be upset about that.

But this comment did make me check the report and I saw that the modules messed with each other there, never showing the fixed discount. I just pushed some changes to fix that

If you have a fixed discount

With both, yes it looks bad but like I said IMO if someone ever got themselves into this position they can also fix it

[legalsylvain]

What I meant is that they could be co-installed even if they didn't work together, sorry I could've worded it better

Sorry, but I'm not conviced by the objective of this PR. Could we talk about it in the general issue, to avoid you loose time implementing something ? OCA/sale-workflow#3638

[legalsylvain]

if I summary, do you think it's possible to add a little glue module that

• adds a warning for end users, as you proposed here OCA/sale-workflow#3638 (comment) (or better, to hide triple discount fields), if fixed discount is != 0.

Thanks !

[hildickethan]

@legalsylvain With the new changes and the glue module this is what I have ended up with, what do you think? Screenshots inside the details

Example invoice PDF with a line with fixed discount and triple (wrong), normal triple discount and a normal fixed discount

I chose to hide the fixed discount cell when it has to show triple discount, else it would do something like `0,00€ (or 77.50%)`, but it does look a bit ugly so I'm not sure about it. Not keen on calculating back the € value of the total discount to show in the field. Maybe a value like N/A would be better UX. But it is the most extreme case of using both discounts on the same invoice albeit different lines (in this case the user would correct the first one after seeing the warning and avoid the 99% on the pdf)

Invoice with only fixed discounts

Invoice with only % discounts

[hildickethan]

Using the copier template it deleted the readme addons content, last time I just copied it back but since now there's a new module being added I'm not sure. Does the bot generate the readme when merging and I can just leave it as is, or should I run the script myself?
Generated it myself

[legalsylvain]

Thanks a lot !

Code review. No test. Lgtm !

Thanks you a lot to take time for this compatibility

[hildickethan]

One last push for i18n files so the warning can be translated

[legalsylvain]

@OCA/accounting-maintainers : could you take a look over there ?

thanks !

[lmignon]

LGTM (Functional tests) + Code review

[legalsylvain]

/ocabot merge nobump

[OCA-git-bot]

Sorry @legalsylvain you are not allowed to merge.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

[legalsylvain]

hi @OCA/accounting-maintainers Could you merge this one ?
thanks !

[sergiocorato]

Only a little remark, only code check, thanks!

[sergiocorato]

depends on #2268

[sergiocorato]

@hildickethan a rebase now should be very useful!

[hildickethan]

@sergiocorato tests added + rebase

[sergiocorato]

Good work!
/ocabot merge major

[OCA-git-bot]

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-2109-by-sergiocorato-bump-major, awaiting test results.

[OCA-git-bot]

Congratulations, your PR was merged at 2dccb17. Thanks a lot for contributing to OCA. ❤️