Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[14.0] FIX account_invoice_triple_discount - _recompute_tax_lines involving only relevant lines: invoice_line_ids #1592

Merged
merged 1 commit into from
Nov 8, 2023

Conversation

eLBati
Copy link
Member

@eLBati eLBati commented Oct 31, 2023

Copy link

@odooNextev odooNextev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@francesco-ooops
Copy link
Contributor

@eLBati can you add module name in PR title and commit name?

@eLBati eLBati force-pushed the 14.0-account_invoice_triple_discount branch from cccae5d to 747083a Compare October 31, 2023 09:37
@eLBati eLBati changed the title [14.0] FIX _recompute_tax_lines involving only relevant lines: invoice_line_ids [14.0] FIX account_invoice_triple_discount - _recompute_tax_lines involving only relevant lines: invoice_line_ids Oct 31, 2023
@eLBati
Copy link
Member Author

eLBati commented Oct 31, 2023

@eLBati can you add module name in PR title and commit name?

Done

@francesco-ooops
Copy link
Contributor

cc @SirAionTech since you previously reviewed #1442

@OCA-git-bot
Copy link
Contributor

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). 🤖

1 similar comment
@OCA-git-bot
Copy link
Contributor

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). 🤖

@francesco-ooops
Copy link
Contributor

@OCA/accounting-maintainers can this be merged please? thank you!

@francesco-ooops
Copy link
Contributor

@tafaRU related to OCA/l10n-italy#3681

Copy link
Contributor

@sergiocorato sergiocorato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sergiocorato
Copy link
Contributor

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

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

@OCA-git-bot OCA-git-bot merged commit 07b26bc into OCA:14.0 Nov 8, 2023
9 checks passed
@OCA-git-bot
Copy link
Contributor

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

@francesco-ooops
Copy link
Contributor

@SirAionTech in your opinion, does this need to be fw-ported to v16?

@SirAionTech
Copy link

@SirAionTech in your opinion, does this need to be fw-ported to v16?

I haven't had time recently to look into this.
If I remember correctly from the long thread in #1442, I was not very convinced of this change in the first place because I don't see how this is fixing this module.
So I have some concerns about back/forward-porting this.

Maybe a little recap of the history of this change might help understanding if it is needed in the future (16.0):
this change was originally needed for fixing the migration of l10n_it_fatturapa_out_triple_discount (OCA/l10n-italy#3269).

It just happens that I have done the migration of the same module to 16.0(OCA/l10n-italy#3720); since this last PR is ok without this, I'd assume that this is not needed in 16.0.

TL;DR: no

@francesco-ooops
Copy link
Contributor

@sergiocorato @OCA/accounting-maintainers all PRs in sale-workflow v14 are failing due to this PR.

Option 1) merge fix #1605

Option 2) revert this change due to doubts expressed regarding to PR and fix in #1592 (comment), #1605 (comment), #1605 (comment)

Personally I find the second more favourable. This problem is not present in v16.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants