[emiliesoutiras]

With the get_view() function in the res.brand.mixin template, all view overrides for the brand_id field are systematically overwritten by the Python definition of the field.

As a result, the sale_brand.view_order_form_brand view is useless because the definition of the SaleOrder.brand_id field already sets it to read-only in all states other than ‘draft’.

This proposal intends to make the "readonly" attribute of the brand_id field in views overridable via a Python function.

[OCA-git-bot]

Hi @sbejaoui,
some modules you are maintaining are being modified, check this out!

[emiliesoutiras]

cc @Kev-Roche @metaminux

[Kev-Roche]

Functional review LGTM

[marcos-mendez]

Automated Review -- Tests Passed

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

---

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

[marcos-mendez]

Automated Review -- Tests Passed

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

---

Test Coverage Suggestions

Coverage gaps

The new methods _get_brand_id_readonly_domain() and is_brand_id_field() are introduced but not tested. The logic in setup_modifiers() that handles the readonly domain extension is also not directly tested — especially the case where modifiers["readonly"] is a list vs a single domain.

Suggested test cases

def test_setup_modifiers_readonly_domain_extension(self):
    """Test that readonly domain is correctly extended when brand_id field is present."""
    # Create a record and simulate setup_modifiers call
    # Assert that readonly domain is extended properly when _get_brand_id_readonly_domain returns a domain

def test_is_brand_id_field_returns_correctly(self):
    """Test that is_brand_id_field correctly identifies brand_id field."""
    # Call is_brand_id_field with brand_id and other fields
    # Assert correct boolean return values

def test_get_brand_id_readonly_domain_returns_domain(self):
    """Test that _get_brand_id_readonly_domain returns a proper domain or False."""
    # Override _get_brand_id_readonly_domain to return a domain
    # Assert that it's correctly handled in setup_modifiers

Codecov risk

• _get_brand_id_readonly_domain() returns False by default, so it may not be tested if no subclass overrides it.
• is_brand_id_field() is a helper that could be tested for correctness, especially if used in multiple places.
• setup_modifiers() logic involving expression.OR is not covered, especially in list vs non-list readonly cases.

Recommendation: Add tests for these specific logic branches to ensure full coverage.

---

⏰ This PR has been open for 51 days.

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:

• 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

[BhaveshHeliconia]

Why can't we simply remove "states" from 'brand_id' field in python field definition in 'sale_brand' module and let view side condition handle this behaviour? It also serves the same purpose and by this way, 'brand' module's 'get_view' remains untouched.

[metaminux]

Why can't we simply remove "states" from 'brand_id' field in python field definition in 'sale_brand' module and let view side condition handle this behaviour? It also serves the same purpose and by this way, 'brand' module's 'get_view' remains untouched.

Besides, the states attribute has been defined on sale.order in 12.0 by #23 which also modified account.move in account_brand module in the same manner...

[sbejaoui]

agree with @BhaveshHeliconia

[metaminux]

Hello @emiliesoutiras

Can you modify this PR to only remove states attribute from the brand_id field in sale_order.py ( sale_brand module) and in account_move.py (account_brand module) ?
This has already been done in 17.0 and newer versions...

[emiliesoutiras]

Hi @metaminux

States removed from brand_id in sale_brand and account_brand, as in 17.0. However, in 16.0 this alone is not enough: res.brand.mixin.get_view() rebuilds the modifiers of the brand_id node from scratch (field definition + injected attrs), which drops the readonly coming from the view. Without states, brand_id would become editable on confirmed orders / posted invoices. I replaced my previous change with a one-line fix in setup_modifiers() that keeps the modifiers already computed by the view, and added tests checking the readonly modifier in both modules.

[metaminux]

Hello @emiliesoutiras

You're totally right, we need to reinject "modifiers" computed in get_view() earlier with all aggregated views because brand_use_level should only influence "invisible" and/or "required".
In which state we want to set brand_id shouldn't be fixed here.

However, It seems to me the tests added here only show that "readonly" is set to True if state is not "draft", but this is what is intended by the views in account_brand and sale_brand. This is not showing that a view that inherits from one of these and modifying the "readonly" attribute is not taken into account. Am I wrong ?

@BhaveshHeliconia can you review again now that states are removed from the brand_id fields in the python field definition ?

[BhaveshHeliconia]

LGTM!