Conversation
|
Not so fast, as if the problems were in the other PR, there will be here as well. Let's see the CI. |
|
@pedrobaeza the pre-commit issue has been fixed and all checks are now green. Could you try merging again? /ocabot merge nobump |
|
Sorry @jarcosmts you are not allowed to merge. To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons. If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the |
|
Please squash the last 2 commits into one, as it's the same code. |
39efb37 to
61c9e40
Compare
|
@pedrobaeza squashed the fix commits, all checks are green now. Could you try merging again? /ocabot merge nobump |
|
Sorry @jarcosmts you are not allowed to merge. To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons. If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the |
Done :D |
tonygalmiche
left a comment
There was a problem hiding this comment.
Functional review
Tested on a local Odoo 18.0 instance:
- Pricelist rules applied on a product brand work correctly.
- Confirmed the automated test suite (previously blocking PR #298) now passes: test_combine_analytic_with_product in both account_analytic_brand and sale_analytic_brand succeeds (0 failed, 0 errors of 6 tests), validating the _merge_distribution() fix.
Works as expected.
|
This PR has the |
|
@pedrobaeza Hi Pedro, this PR is ready to merge. Could you help me, please? Thanks! |
|
/ocabot migration pricelist_brand It requires the approval of a PSC or maintainer. Check the previous authors. |
|
The migration issue (#210) has not been updated to reference the current pull request because a previous pull request (#298) is not closed. |
@sbejaoui, could you please help approve this PR? |
|
@pedrobaeza @ThomasBinsfeld It is possible to close the PR #298 and approve this one to merge it. Thanks. |
|
As this one is respecting previous attribution, let's move. /ocabot merge nobump |
|
What a great day to merge this nice PR. Let's do it! |
|
@pedrobaeza The merge process could not be finalized, because command |
|
@OCA/pypi-support to reserve |
|
/ocabot merge nobump |
|
On my way to merge this fine PR! |
|
@sbejaoui The merge process could not be finalized, because command |
|
/ocabot merge nobump |
|
This PR looks fantastic, let's merge it! |
|
@sbejaoui your merge command was aborted due to failed check(s), which you can inspect on this commit of 18.0-ocabot-merge-pr-318-by-sbejaoui-bump-nobump. After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red. |
|
/ocabot rebase |
|
Congratulations, PR rebased to 18.0. |
61c9e40 to
4cf97e1
Compare
|
/ocabot rebase |
…ic plans When merging analytic distributions from different analytic plans, Odoo's _merge_distribution combines keys into a single comma-separated key instead of keeping them separate. This fix ensures distributions for different plans are merged as a simple union.
|
Congratulations, PR rebased to 18.0. |
4cf97e1 to
15406fa
Compare
Refresh of #298 - rebased against latest 18.0.
The original PR #298 had 4 approvals and was marked "ready to merge",
but merge attempts failed due to pre-existing test failures in
account_analytic_brand and sale_analytic_brand (test_combine_analytic_with_product).
Root cause: Odoo 18's _merge_distribution() combines analytic distribution
keys from different analytic plans into a single comma-separated key
(e.g. {'25,24': 100.0}) instead of keeping them separate
({'25': 100.0, '24': 100.0}).
Fix: Override _merge_distribution() in analytic_brand to return a simple
dict union when the distributions come from different analytic plans.