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.

Sign up for GitHub

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

Jump to bottom

[16.0][MIG] sale_report_margin: Migration to 16.0 #175

Merged
merged 10 commits into from
Jun 20, 2023
Merged

[16.0][MIG] sale_report_margin: Migration to 16.0 #175

merged 10 commits into from
Jun 20, 2023

Conversation

flalexg
Copy link

@flalexg flalexg commented Jun 5, 2023

No description provided.

@flalexg flalexg mentioned this pull request Jun 5, 2023
Open
10 tasks
@polqubiq
Copy link

polqubiq commented Jun 6, 2023

LGTM

@JanQubiQ
Copy link

JanQubiQ commented Jun 7, 2023

LGTM @flalexg . It seems the README is missing to be migrated. Could you check it, please?

@amarcosg
Copy link

amarcosg commented Jun 7, 2023

I made a branch from yours to increment coverage. if you find it right we can merge/cherry-pick the commit so it pases tests. factorlibre#4

@flalexg
Copy link
Author

flalexg commented Jun 7, 2023

LGTM @flalexg . It seems the README is missing to be migrated. Could you check it, please?

Hi! I checked the README and I think its okey. When its merged the new README will be generated, but it has de the fragments.

amarcosg
amarcosg approved these changes Jun 7, 2023
View reviewed changes
Copy link

@amarcosg amarcosg left a comment

Choose a reason for hiding this comment

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

LGTM

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

@pedrobaeza pedrobaeza changed the title [MIG] sale_report_margin: Migration to 16.0 [16.0][MIG] sale_report_margin: Migration to 16.0 Jun 11, 2023
@pedrobaeza
Copy link
Member

/ocabot migration sale_report_margin

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Jun 11, 2023
pedrobaeza
pedrobaeza reviewed Jun 11, 2023
View reviewed changes
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.

Please name the commit adding the tests also with the module name according guidelines:

https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#71commit-message

sale_report_margin/reports/sale_report.py Outdated
def _group_by_sale(self):
res = super()._group_by_sale()
res += """,
l.purchase_price"""
Copy link
Member

Choose a reason for hiding this comment

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

Being this one a measure, is it needed to be grouped by it?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @pedrobaeza! Sorry for the late response. You are right, its unnecessary. Deleted the function. Thank you!

Copy link
Member

Choose a reason for hiding this comment

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

I still see it in the PR.

@flalexg flalexg force-pushed the 16.0-mig-sale-report-margin branch from 7c7a9f1 to a52077c Compare June 16, 2023 07:30
@hugo-cordoba hugo-cordoba force-pushed the 16.0-mig-sale-report-margin branch 3 times, most recently from c183e77 to 4e17aa8 Compare June 16, 2023 08:43
@flalexg
Copy link
Author

flalexg commented Jun 16, 2023

@pedrobaeza now I remember why I used de group by.

psycopg2.errors.GroupingError: column "l.purchase_price" must appear in the GROUP BY clause or be used in an aggregate function
LINE 64: l.purchase_price AS purchase_price

Shall we keep the first commit?

pedrobaeza
pedrobaeza reviewed Jun 16, 2023
View reviewed changes
sale_report_margin/reports/sale_report.py Outdated

def _select_additional_fields(self):
res = super()._select_additional_fields()
res["purchase_price"] = "l.purchase_price"
Copy link
Member

Choose a reason for hiding this comment

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

You have to overwrite _select_sale instead and do SUM(l.purchase_price) for having this correctly and not grouping by.

Copy link
Member

Choose a reason for hiding this comment

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

Well, maybe this can serve:

Suggested change
res["purchase_price"] = "l.purchase_price"
res["purchase_price"] = "SUM(l.purchase_price)"

@hugo-cordoba
Copy link

Hi @pedrobaeza!

@flalexg and I have been reviewing it, and, with the change you propose it works as expected.

Thank you!

@pedrobaeza
Copy link
Member

Great. Note that it's not the same that the module installs without error than it works as expected, and with the group by, you would find ugly totals or unfolded results when using the module.

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

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

@OCA-git-bot OCA-git-bot merged commit 2948af5 into OCA:16.0 Jun 20, 2023
@OCA-git-bot
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pedrobaeza pedrobaeza pedrobaeza left review comments

@amarcosg amarcosg amarcosg approved these changes

@polqubiq polqubiq polqubiq approved these changes

Assignees
No one assigned
Labels
approved merged 🎉 ready to merge
Projects
None yet
Milestone
16.0
Development

Successfully merging this pull request may close these issues.