[18.0] [MIG] pricelist_brand

[ThomasBinsfeld]

Backport of 19.0 migration : #297

[ThomasBinsfeld]

@Alexgars73

[BhaveshHeliconia]

Code and functional review LGTM!

[jarcosmts]

Code review LGTM. The migration looks clean:

• Manifest: version 18.0.1.0.0, correct depends on \product_brand\
• Build system: \pyproject.toml\ with \whool\ ✅
• Models: proper \selection_add\ for \�pplied_on/\display_applied_on\ with Odoo 18 \ondelete\ support, correct \create/\write\ consistency logic
• Tests: create own data (no dependency on demo data), use _get_contextual_price()\ ✅
• Views: no \�ttrs, uses modern \invisible/
  equired\ ✅
• Readme: .md\ fragments ✅

Ready for production.

[juanparmer]

Code review LGTM. The migration from 19.0 to 18.0 is clean and follows OCA conventions and the Migration-to-version-18.0 guide:

✅ Correct adaptations for Odoo 18:

• Manifest version \18.0.1.0.0\
• \expression.AND\ instead of \Domain.AND\ (correct for Odoo 18)
• README/Weblate badges pointing to 18.0
• \pyproject.toml\ with \whool\ build system
• Proper \selection_add\ + \ondelete\ on \�pplied_on\ / \display_applied_on\
• \create/\write\ consistency logic clearing conflicting fields
• Modern \invisible/
  equired\ in views (no deprecated \�ttrs)
• Tests create own data, no dependency on demo data
• Uses \self.env._\ for translations

Minor suggestion:
The test file (193 lines) is slightly behind the 19.0 version (#297, 248 lines). It's missing a few tests present in 19.0 (e.g., _is_applicable_for\ with template/\min_quantity, _onchange_display_applied_on, _onchange_rule_content, \product.template\ pricing). Nothing blocking, but worth adding for consistency.

[ThomasBinsfeld]

Code review LGTM. The migration from 19.0 to 18.0 is clean and follows OCA conventions and the Migration-to-version-18.0 guide:

✅ Correct adaptations for Odoo 18:

• Manifest version \18.0.1.0.0\
• \expression.AND\ instead of \Domain.AND\ (correct for Odoo 18)
• README/Weblate badges pointing to 18.0
• \pyproject.toml\ with \whool\ build system
• Proper \selection_add\ + \ondelete\ on \�pplied_on\ / \display_applied_on\
• \create/\write\ consistency logic clearing conflicting fields
• Modern \invisible/
  equired\ in views (no deprecated \�ttrs)
• Tests create own data, no dependency on demo data
• Uses \self.env._\ for translations

Minor suggestion: The test file (193 lines) is slightly behind the 19.0 version (#297, 248 lines). It's missing a few tests present in 19.0 (e.g., _is_applicable_for\ with template/\min_quantity, _onchange_display_applied_on, _onchange_rule_content, \product.template\ pricing). Nothing blocking, but worth adding for consistency.

Thanks for your review. I updated this branch with the last changes of the 19.0 migration PR (#297).

[jarcosmts]

@pedrobaeza Could you help us with this PR? It's already been approved.

[OCA-git-bot]

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]

/ocabot migration pricelist_brand
/ocabot merge nobump

[OCA-git-bot]

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

[OCA-git-bot]

@pedrobaeza your merge command was aborted due to failed check(s), which you can inspect on this commit of 18.0-ocabot-merge-pr-298-by-pedrobaeza-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.

[pedrobaeza]

This requires to be rebased and checked the problem.

[jarcosmts]

Hi @ThomasBinsfeld, the merge failed but the issue is not with pricelist_brand — all 4 of its tests pass correctly.

The failure is in two pre-existing modules on the 18.0 branch:

• �ccount_analytic_brand → est_combine_analytic_with_product
• sale_analytic_brand → est_combine_analytic_with_product

The error: When two �ccount.analytic.distribution.model records match (one for brand, one for product) on different analytic plans, Odoo's _merge_distribution() produces a combined key like {'25,24': 100.0} instead of separate keys {'25': 100.0, '24': 100.0}.

How to fix it: The tests expect the distributions to be merged as separate keys, but Odoo core concatenates them. You can either:

1. Adjust the test assertions to match Odoo's actual behavior (change the expected dict)
2. Override _merge_distribution() in �nalytic_brand to handle different-plan models with a simple dict union

Once those two tests pass, your PR should merge without issues.

[jarcosmts]

@pedrobaeza Just to clarify, the merge failure is not related to pricelist_brand. All its tests pass (4/4).

The failing tests are in �ccount_analytic_brand and sale_analytic_brand ( est_combine_analytic_with_product), which are pre-existing modules on 18.0. It seems like a bug in how Odoo's _merge_distribution() handles analytic distributions across different plans.

Would it be possible to merge this PR despite the unrelated failures, or should the failing modules be fixed first?

[pedrobaeza]

Sometimes, PRs affect other modules although it seems unrelated (but I'm not saying this is the case). Anyway, branch must be green for being able to merge.

[andreampiovesana]

LGTM

[andreampiovesana]

@pedrobaeza all green + 4 approvals
is time to merge?

[pedrobaeza]

/ocabot merge nobump

[OCA-git-bot]

On my way to merge this fine PR!
Prepared branch 18.0-ocabot-merge-pr-298-by-pedrobaeza-bump-nobump, awaiting test results.

[OCA-git-bot]

@pedrobaeza your merge command was aborted due to failed check(s), which you can inspect on this commit of 18.0-ocabot-merge-pr-298-by-pedrobaeza-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.

[pedrobaeza]

Please rebase and check the CI problem.