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_report_grouped_by_picking: do not show negative values with partially invoiced services #303

Conversation

manuelregidor
Copy link
Contributor

@manuelregidor manuelregidor commented Mar 6, 2024

With this fix, partially invoiced services that come from sale order lines do not show negative values in invoices PDFs grouped by pickings.

Consider the following sale order:
grouped_1

and the following invoice, in which the amount of the invoiced service quantity is less than the quantity ordered:
grouped_2

Before this fix, there were two lines for the service product: in one of them, quantity is 5, which is the amount in the sale order; in the other one, quantity is -3, which is the amount that needs to be substracted from the other line in order to obtain the amount present in the invoice:
grouped_3

With the fix, there is only one line in the PDF that contains the amount that is actually invoiced.
grouped_4

@pedrobaeza pedrobaeza requested a review from CarlosRoca13 March 6, 2024 15:39
@manuelregidor
Copy link
Contributor Author

@HaraldPanten

@manuelregidor manuelregidor changed the title [FIX]account_invoice_report_grouped_by_picking: do not show negative values with partially delivered services [FIX]account_invoice_report_grouped_by_picking: do not show negative values with partially invoiced services Mar 6, 2024
@manuelregidor manuelregidor force-pushed the 16.0-fix-account_invoice_report_grouped_by_picking branch from 0559ba4 to ab05195 Compare March 6, 2024 15:53
Copy link

@HaraldPanten HaraldPanten left a comment

Choose a reason for hiding this comment

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

Functional review. LGTM

Copy link
Member

@etobella etobella left a comment

Choose a reason for hiding this comment

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

Seems logic, can you add a test?

@pedrobaeza pedrobaeza changed the title [FIX]account_invoice_report_grouped_by_picking: do not show negative values with partially invoiced services [16.0][FIX]account_invoice_report_grouped_by_picking: do not show negative values with partially invoiced services Mar 7, 2024
@pedrobaeza pedrobaeza added this to the 16.0 milestone Mar 7, 2024
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.

Sorry for stopping this, but I see that your change impacts both lines with product, consumable or service, so this may alter some of the previous behaviors. Should you limit it to services?

Also I would like to have a test exercising the specific problem for avoiding future regressions.

@manuelregidor manuelregidor force-pushed the 16.0-fix-account_invoice_report_grouped_by_picking branch from ab05195 to b7382e6 Compare March 7, 2024 07:29
@HaraldPanten
Copy link

Hi @pedrobaeza Any news about this? The error is in several versions, and it should be fixed.

@pedrobaeza
Copy link
Member

@carlosdauden can you confirm that you see the change correct?

@manuelregidor manuelregidor force-pushed the 16.0-fix-account_invoice_report_grouped_by_picking branch from b7382e6 to 23d8a9b Compare March 12, 2024 09:23
@manuelregidor manuelregidor force-pushed the 16.0-fix-account_invoice_report_grouped_by_picking branch from 23d8a9b to 326607d Compare March 12, 2024 14:46
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.

OK, let's go with this with the current consensus, and let's see if any problem is detected:

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-303-by-pedrobaeza-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit b970a31 into OCA:16.0 Mar 12, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

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

@HaraldPanten HaraldPanten deleted the 16.0-fix-account_invoice_report_grouped_by_picking branch March 12, 2024 17:49
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.

7 participants