[14.0] [FIX] account_invoice_triple_discount: for in invoice_line_ids if exists, else on line_ids

[odooNextev]

…ts, else on line_ids
fix #1592 (comment)

[francesco-ooops]

cc @eLBati @sergiocorato

[francesco-ooops]

@OCA/accounting-maintainers this looks urgent to me

[francesco-ooops]

@SirAionTech what do you think of this one?

[aleuffre]

Code review LGTM

chore: please adjust the commit message following the guidelines: https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#commit-message

suggestion (non-blocking): since this is a rather intricate and delicate matter (had to read multiple issues in multiple languages to understand the context), I think adding a test would be helpful in avoiding regressions and it would implicitly also explain the reason behind the change. If not a test, at least a comment.

chore: once approved, please FW to v15 and 16!

[francesco-ooops]

@ferran-S73 fyi

[sergiocorato]

I think it would be better to check if it is_invoice instead.

[odooNextev]

Working on the broken test, is_invoice returns True but without invoice_line_ids. So in this case it will not fix the test

[SirAionTech]

@SirAionTech what do you think of this one?

I think there should be a test in this module that explains what this change is fixing and ensures that there are no regressions in the future.
Basically I agree with #1605 (review), but I'm more strict 👼.
Changing this module to fix tests of another module, in another repo too, makes little sense in my opinion.

If this module ain't broken, don't fix it.

[anmarmo1]

Functional review is ok.

[eLBati]

Thanks
(rif OCA/l10n-italy#3681 (comment))

[eLBati]

Changing this module to fix tests of another module, in another repo too, makes little sense in my opinion

Why?

[SirAionTech]

Changing this module to fix tests of another module, in another repo too, makes little sense in my opinion

Why?

Because in my opinion, a module should test its own behavior.
This avoids regressions: otherwise nothing would stop someone else from changing back this code: CI stays 🟢, so why not?

If triple discount works good for invoices, why is it breaking sales? I see a few options:

• Is account_invoice_triple_discount not working good? It should be reproducible with this module alone, so there should be a test in here.
• Is sale_triple_discount expecting something different from this module? Why? Maybe because it was relying on some wrong behavior of account_invoice_triple_discount? There might be something to fix in there.

[francesco-ooops]

@pedrobaeza can I ask your input here?

the urgency is due to sale-workflow 14 tests currently failing

[eLBati]

This avoids regressions: otherwise nothing would stop someone else from changing back this code: CI stays 🟢, so why not?

This makes sense.
@odooNextev did you already try to add a test that makes account_invoice_triple_discount fail without the changes of this PR?
Otherwise I can work on this next week.

[odooNextev]

This makes sense. @odooNextev did you already try to add a test that makes account_invoice_triple_discount fail without the changes of this PR? Otherwise I can work on this next week.

We tried adding a test from l10n_it_fatturapa_in like this: https://github.com/OCA/l10n-italy/blob/14.0/l10n_it_fatturapa_in/tests/test_import_fatturapa_xml.py#L658

You can found it in the old PR: https://github.com/OCA/account-invoicing/pull/1442/files#diff-3c2a0c335b3658f70e6cc646d2f82097e866dbcd7267a0b3497df142d9cd2392R184

[pedrobaeza]

This patch sounds weird, but let's continue for unblocking the CI:

/ocabot merge patch

[OCA-git-bot]

What a great day to merge this nice PR. Let's do it!
Prepared branch 14.0-ocabot-merge-pr-1605-by-pedrobaeza-bump-patch, awaiting test results.

[OCA-git-bot]

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