[16.0][MIG] pricelist_brand

[david-s73]

Migration to 16, reference: odoo/odoo#79605

[github-actions]

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

[bosd]

@david-s73 Can you please fix pre-commit?

[sbejaoui]

Code review

[bosd]

The commit history is missing. Can you please follow the migration guide?

[david-s73]

@bosd I can see that all previous commits have been carried over in the migration. Could you clarify what you mean?

[Reyes4711-S73]

LGTM

[Reyes4711-S73]

@sbejaoui Please, can you merge this PR?

[bosd]

@bosd I can see that all previous commits have been carried over in the migration. Could you clarify what you mean?

i was expecting more of a commit history. But it seems that there is none for this module. so it is ok now.

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

[Reyes4711-S73]

@OCA/brand-maintainers Please, can you review/merge this PR?

[andreampiovesana]

merge?

[andreampiovesana]

?

[bosd]

merge?

@max3903 Can you 🙏

[sbejaoui]

/ocabot merge nobump

[OCA-git-bot]

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-187-by-sbejaoui-bump-nobump, awaiting test results.

[OCA-git-bot]

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

[marcos-mendez]

Automated Review -- Tests Failed

1. Root cause of the test failure

The test failure is caused by a missing brand_id field in the pricelist item when the applied_on field is set to '0_product_brand'. The code expects this field to be present and populated, but it is not being set during the test creation, leading to an unexpected behavior or error during pricelist application.

2. Suggested fix

In pricelist_brand/models/product_pricelist.py, ensure that when applied_on is set to '0_product_brand', the brand_id field is properly required and validated. Specifically, add a domain or constraint to enforce that brand_id is set when applied_on is '0_product_brand'.

Additionally, in the test file pricelist_brand/tests/test_product_pricelist.py, ensure that when creating a pricelist item with applied_on='0_product_brand', the brand_id is explicitly set.

3. Additional code issues

• Missing validation: There is no validation to ensure that brand_id is required when applied_on is '0_product_brand'. This could lead to inconsistent data or runtime errors.
• Test coverage gap: The test does not validate that pricelist items with applied_on='0_product_brand' are correctly applied only to products of the specified brand.

4. Test improvements

Add the following test cases to test_product_pricelist.py using SavepointCase for better isolation:

def test_pricelist_item_brand_applied(self):
    # Create a brand
    brand = self.env['product.brand'].create({'name': 'Test Brand'})
    
    # Create a product with the brand
    product = self.env['product.product'].create({
        'name': 'Test Product',
        'brand_id': brand.id,
    })
    
    # Create a pricelist item with brand applied
    pricelist_item = self.env['product.pricelist.item'].create({
        'pricelist_id': self.pricelist.id,
        'applied_on': '0_product_brand',
        'brand_id': brand.id,
        'fixed_price': 100.0,
    })
    
    # Check that the price is applied correctly
    price = self.pricelist.get_product_price(product, 1.0)
    self.assertEqual(price, 100.0)

def test_pricelist_item_brand_not_applied(self):
    # Create two brands
    brand1 = self.env['product.brand'].create({'name': 'Brand 1'})
    brand2 = self.env['product.brand'].create({'name': 'Brand 2'})
    
    # Create a product with brand1
    product = self.env['product.product'].create({
        'name': 'Test Product',
        'brand_id': brand1.id,
    })
    
    # Create a pricelist item with brand2 applied
    pricelist_item = self.env['product.pricelist.item'].create({
        'pricelist_id': self.pricelist.id,
        'applied_on': '0_product_brand',
        'brand_id': brand2.id,
        'fixed_price': 100.0,
    })
    
    # Check that the price is NOT applied (should be default price)
    price = self.pricelist.get_product_price(product, 1.0)
    self.assertNotEqual(price, 100.0)

Use SavepointCase for these tests to ensure that each test runs in isolation and does not affect other tests. This follows OCA testing patterns for better reliability and maintainability.

---

Environment: Minikube + K8s Job + oca-ci/py3.10-odoo16.0 | Odoo 16.0
Automated review by OCA PR Reviewer + qwen3-coder:30b

[marcos-mendez]

Automated Review -- Tests Passed

All tests for pricelist_brand passed successfully on Odoo 16.0.
Environment: Minikube + K8s Job + oca-ci/py3.10-odoo16.0

---

Test Coverage Suggestions

Coverage Gaps

The existing tests cover basic functionality, but there are some gaps:

1. Domain filtering logic in _get_applicable_rules_domain is not fully tested for edge cases (e.g., empty brands, multiple brands)
2. Constraint validation for product_brand_id is tested, but not the case where applied_on is changed from "Brand" to another option
3. Name and price computation logic is tested, but not thoroughly for all combinations of brand + other applied_on values
4. Create/write consistency logic in create and write methods lacks tests for:
   • Changing applied_on from "Brand" to other values
   • Ensuring that conflicting fields are cleared correctly

Suggested Test Cases

def test_pricelist_item_brand_domain_filtering(self):
    """Test that pricelist rules are correctly filtered by brand in domain."""
    # Create products with different brands
    brand1 = self.env['product.brand'].create({'name': 'Brand1'})
    brand2 = self.env['product.brand'].create({'name': 'Brand2'})
    product1 = self.env['product.template'].create({
        'name': 'Product1', 'product_brand_id': brand1.id
    })
    product2 = self.env['product.template'].create({
        'name': 'Product2', 'product_brand_id': brand2.id
    })
    pricelist = self.env['product.pricelist'].create({'name': 'Test Pricelist'})
    item = self.env['product.pricelist.item'].create({
        'pricelist_id': pricelist.id,
        'applied_on': '25_brand',
        'product_brand_id': brand1.id,
        'fixed_price': 100.0,
    })
    # Ensure rule applies only to brand1 products
    domain = pricelist._get_applicable_rules_domain(product1, False)
    self.assertIn(item.id, self.env['product.pricelist.item'].search(domain).ids)
    domain = pricelist._get_applicable_rules_domain(product2, False)
    self.assertNotIn(item.id, self.env['product.pricelist.item'].search(domain).ids)

def test_pricelist_item_brand_constraint_on_write(self):
    """Test that constraint is enforced when changing applied_on to 'Brand'."""
    pricelist = self.env['product.pricelist'].create({'name': 'Test Pricelist'})
    item = self.env['product.pricelist.item'].create({
        'pricelist_id': pricelist.id,
        'applied_on': '3_global',
        'fixed_price': 100.0,
    })
    # Change to brand rule without specifying brand - should raise ValidationError
    with self.assertRaises(ValidationError):
        item.write({'applied_on': '25_brand'})

def test_pricelist_item_brand_create_consistency(self):
    """Test that creating a brand rule clears conflicting fields."""
    pricelist = self.env['product.pricelist'].create({'name': 'Test Pricelist'})
    item = self.env['product.pricelist.item'].create({
        'pricelist_id': pricelist.id,
        'applied_on': '25_brand',
        'product_brand_id': self.env['product.brand'].create({'name': 'Test'}).id,
        'product_id': self.env['product.product'].create({'name': 'Test'}).id,  # Should be cleared
        'fixed_price': 100.0,
    })
    self.assertFalse(item.product_id)
    self.assertFalse(item.product_tmpl_id)
    self.assertFalse(item.categ_id)

Codecov Risk

The following methods have high risk of reduced coverage if not tested:

• _get_applicable_rules_domain - domain construction logic
• create and write methods in ProductPricelistItem - field clearing logic
• _compute_name_and_price - name formatting logic

These methods involve complex conditional logic and field interactions that are not fully covered by current tests.

---

⚠️ PR Aging Alert: CRITICAL

This PR by @david-s73 has been waiting for 769 days — that is over 25 months without being merged or closed.

Every ignored PR is a contributor who might not come back. Review time matters. (OCA Aging Report)

---

Reciprocal Review Request

Hi everyone! Thanks for your contribution to OCA. I reviewed and approved this PR. If any of you have a moment, I would really appreciate a review on my open PR(s):

My open PRs across OCA:

• server-tools#3548 [18.0][MIG] base_kanban_stage: Migration to 18.0
• hr-attendance#262 [16.0][ADD] Hr_attendance_idsecure: iDSecure (ControliD) attendance integration
• stock-logistics-workflow#2276 [16.0][ADD] stock_move_line_devaluation
• stock-logistics-workflow#2275 [16.0][ADD] Stock move line analytic account
• stock-logistics-workflow#2268 [16.0][ADD] stock_move_line_picking_partner
• purchase-workflow#2694 [16.0][IMP]Purchase workflow added to review state & exception fix

Reviewing each other's work helps the whole community move forward faster. Thank you!

---

Automated review by OCA Neural Reviewer + qwen3-coder:30b

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

[Reyes4711-S73]

@OCA/brand-maintainers Please, can you review/merge this PR?