[16.0] [MIG] sale_order_rename: Migration to 16.0

[patrickt-oforce]

No description provided.

[alexey-pelykh]

Code LGTM

[BhaveshHeliconia]

LGTM!

[rousseldenis]

/ocabot migration sale_order_rename

[rousseldenis]

@patrickt-oforce Have you tried something like this ?

result = self._read_group(
            domain=[('name', 'in', self.mapped('name')],
            fields=['company_id', 'ids:array_agg(id)'],
            groupby=['company_id'],
        )
for res in result:
    if res.get('company_id_count', 0) >= 2:
        duplicate_orders = self.browse(res['ids'])
        raise (...)

In order to not do as much queries as recordset amount.

[patrickt-oforce]

Hi @rousseldenis, sorry i've seen the comment only now; i try and write here if ok

[alexey-pelykh]

Review: sale_order_rename migration to 16.0

Thank you for the migration work. The module structure, tests, manifest, and i18n are all in good shape. A few issues need to be addressed before merge.

Issues requiring changes

1. Use ValidationError instead of UserError in @api.constrains (correctness)

Odoo convention is to raise odoo.exceptions.ValidationError from @api.constrains methods, not UserError. UserError is intended for business logic checks in button actions / write methods, while ValidationError is the standard for constraint violations. See Odoo ORM docs on @api.constrains.

2. N+1 query in _check_unique_name_in_company (performance)

As already noted by @rousseldenis, the current implementation executes one search_count per record in the recordset. For batch writes this becomes O(n) queries. A single read_group or a raw SQL query checking for duplicates across all records in self would be more efficient. Please address the suggestion from the earlier review comment.

3. _logger.error() before raising exception (best practice)

Logging at ERROR level right before raising a UserError/ValidationError is redundant -- Odoo already logs unhandled exceptions. If the intent is audit/debug tracing, _logger.debug() or _logger.warning() would be more appropriate. Logging at ERROR level for a normal validation failure creates noise in production logs.

Suggestions (non-blocking)

• Copyright year in __manifest__.py: The header says Copyright 2020-2022 CorporateHub but the migration was done in 2025. Consider updating to 2020-2025.
• Missing readme/USAGE.rst: The description mentions renaming in Draft state, but there is no usage documentation. A brief explanation of how to use the feature would help users.

Overall the module is well-structured with good test coverage. Once the ValidationError and N+1 issues are resolved, this should be ready.

Review posted via CorporateHub OCA review campaign

[alexey-pelykh]

For @api.constrains methods, the Odoo convention is to raise ValidationError instead of UserError.

from odoo.exceptions import ValidationError

And correspondingly change the raise UserError(...) to raise ValidationError(...) on line 32.

[alexey-pelykh]

This loop executes one search_count per record, which is an N+1 query pattern. For batch operations (e.g., importing multiple sale orders), this becomes costly.

Consider using read_group or a single SQL query to detect duplicates across all records in self at once, as suggested by @rousseldenis in an earlier comment.

[alexey-pelykh]

_logger.error() is too noisy here -- this is a normal validation failure, not an unexpected system error. Odoo will log the exception anyway. Consider _logger.debug() or remove the logging entirely.