Closed flalexg closed 1 year ago
LGTM @flalexg . It seems the README is missing to be migrated. Could you check it, please?
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. https://github.com/factorlibre/margin-analysis/pull/4
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.
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). 🤖
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). 🤖
/ocabot migration sale_report_margin
@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?
Hi @pedrobaeza!
@flalexg and I have been reviewing it, and, with the change you propose it works as expected.
Thank you!
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
This PR looks fantastic, let's merge it! Prepared branch 16.0-ocabot-merge-pr-175-by-pedrobaeza-bump-nobump, awaiting test results.
Congratulations, your PR was merged at f84626ece5c0f649c27b6537463735380c3845c1. Thanks a lot for contributing to OCA. ❤️
LGTM