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

[16.0][FIX] account_invoice_refund_link - correct link creation #1399

Merged
merged 1 commit into from
Mar 2, 2023

Conversation

pedrocs-exo
Copy link
Contributor

The current implementation creates incorrect links between refund lines and their origin invoice lines. This patch fixes it. It's a very small and obvious change so it should be easy to review and merge.

@pedrobaeza
Copy link
Member

Please squash in one commit and change the commit message to:

[FIX] account_invoice_refund_link: Exclude section/notes origin lines

On the side by side comparison, exclude the lines that are not product, the 
same we do for the refund lines.

@rafaelbn
Copy link
Member

Hello @pedrocasi , thank you very much.

@rafaelbn
Copy link
Member

@ljsalvatierra-factorlibre could you please review? As you migrate it this could be interenting for you 😄 thank you!

@ljsalvatierra-factorlibre

@ljsalvatierra-factorlibre could you please review? As you migrate it this could be interenting for you smile thank you!

Yes, of course.

Thank you @pedrocasi, could you please add unittests? Sorry for the trouble.

On the side by side comparison, exclude the lines that are not products, as that's what we do on the refund lines.
@pedrocs-exo
Copy link
Contributor Author

Added a line section on the unittests reference invoice and adapted the assertions to match.

@ljsalvatierra-factorlibre

Added a line section on the unittests reference invoice and adapted the assertions to match.

Thank you, i'll take a look tomorrow morning

Copy link

@ljsalvatierra-factorlibre ljsalvatierra-factorlibre left a comment

Choose a reason for hiding this comment

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

Thank you for the fix!

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

/ocabot merge patch

I suppose this problem is the same in lower versions. Can you please backport it?

@OCA-git-bot
Copy link
Contributor

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

@OCA-git-bot OCA-git-bot merged commit f5b1731 into OCA:16.0 Mar 2, 2023
@OCA-git-bot
Copy link
Contributor

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

@pedrocs-exo
Copy link
Contributor Author

@pedrobaeza I could backport it but it's not that simple. Before 16.0, there are 3 types of invoices lines: sections, notes or regular lines. All of them link with refund lines and it's working fine. On 16.0 you have other line options, like payment terms, taxes, rounding, etc. The person that migrated the module (@ljsalvatierra-factorlibre ) decided to establish the link only on regular lines (display_type == 'product') and I tend to agree (to be honest I'm not familiar with the implications of the other line types so I prefer to stick with the safe behaviours).

With that being said, in my opinion we shouldn't backport. What do you think?

@pedrobaeza
Copy link
Member

OK then. Let's keep it for now.

@pedrobaeza
Copy link
Member

On #1526, I'm refactoring the way of matching the refund lines against the origin ones, and I see no reason for not linking section and the rest of the lines as well. Please check.

odooNextev pushed a commit to odooNextev/account-invoicing that referenced this pull request Sep 29, 2023
Signed-off-by moylop260
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.

5 participants