[17.0][MIG] sale_order_merge

[edescalona]

@BinhexTeam

This PR was based on [8.0] sale_order_merge, so several changes were made.

The merge criteria are defined as follows:

• Same customer, shipping address, warehouse, and company.
• Orders must be in "Draft" status.
• You can also merge confirmed orders if you enable the Merge Order Confirmation setting.

ping @StefanRijnhart @gurneyalex

[rousseldenis]

/ocabot migration sale_order_merge

[rousseldenis]

LGTM even if I'm wondering if compute method for 'merge_with' could be improved

[edescalona]

Hi @rousseldenis , maybe it can be improved, but well, try to remove only what doesn't work in the target version.

[edescalona]

Hi @rousseldenis @hildickethan , I'm going to make the changes you mentioned to me to adapt them to the guidelines. Thank you for your feedback.

[edescalona]

Hi @rousseldenis @hildickethan , I'm covering the tests, I'll do squash at the end.

[edescalona]

ping @rousseldenis @hildickethan

[hildickethan]

Hi @rousseldenis @hildickethan , I'm going to make the changes you mentioned to me to adapt them to the guidelines. Thank you for your feedback.

@edescalona what happened to this? Was there some issue with migrating the fields?

[edescalona]

ping @hildickethan @rousseldenis

[alexey-pelykh]

Review: [17.0][MIG] sale_order_merge

Thanks for the migration effort -- this is a useful module. I reviewed the code for Odoo 17 API compatibility, OCA conventions, and correctness. CI is green on tests (OCB and Odoo) and pre-commit, which is good. A few items below.

---

Bug: wrong order name in merge chatter message

wizard_sale_order_merge.py, merge() method, line 162

The cancelled orders post a message referencing themselves rather than the target order:

order.message_post(body=_("Merged into %(order)s") % {"order": order.name})

order.name here is the name of the order being cancelled. Should be self.order_id.name (the merge target).

Additionally at line 165, self.to_merge.mapped("name") includes the target itself. Consider using orders.mapped("name") (where orders = self.to_merge - self.order_id, already computed above).

---

Deprecated key: view_type in action dicts

view_type was removed from ir.actions.act_window in Odoo 13. Two occurrences:

1. sale_order.py:192 -- action_button_merge() return dict
2. wizard_sale_order_merge.py:130 -- open_sale() return dict

Please remove the "view_type": "form" lines from both.

---

Unresolved review feedback: field naming conventions

@hildickethan raised this earlier and it has not been addressed:

• merge_with (Many2many on sale.order) -- OCA convention is to suffix relational fields: consider merge_with_ids or mergeable_order_ids
• to_merge (Many2many on wizard) -- consider to_merge_ids
• mergeable (Many2many on wizard) -- consider mergeable_ids

The order_id M2O already follows the convention correctly.

---

_compute_merge_with performs a search per record

The compute method runs self.search([("merge_with", "=", sale.id)]) inside a for sale in self loop. For list views with many orders this could be expensive. Since the field is non-stored, consider whether merge_ok alone is sufficient to gate the UI button and the full merge_with set can be resolved lazily only when the merge button is clicked.

---

_get_orders_selected uses _read_group as a per-record iterator

_read_group with groupby=["id"] produces one group per record, which is equivalent to iterating self. A simpler direct iteration (for order in self: ...) would improve readability and avoid relying on the id pseudo-field's return type from _read_group.

---

merge_moves -- invoice line reassignment bypasses recomputation

Line 59 writes move_id directly on invoice_line_ids to move them between account.move records. This bypasses the accounting engine's recomputation of tax lines, sequences, and balanced-entry checks. It only applies to draft invoices which limits risk, but verifying the merged invoice renders correctly after this operation (and potentially calling _onchange_invoice_line_ids() or _compute_amount() on the full move) would be safer.

---

Minor / style

• License header year: The copyright headers carry only 2016 (from the 8.0 original). Consider adding the current year and the migrating author per OCA conventions.
• _merge_order_by_states reads ir.config_parameter via sudo() on every call, and it is called multiple times per flow. Consider caching per-request.
• Missing @api.depends on _compute_merge_with: non-stored computed fields without @api.depends are recomputed on every access. Adding an explicit empty depends or a docstring comment would help future maintainers understand the intent.

---

Overall the module is functional and well-tested. The merge message bug and the deprecated view_type keys are the most important items to fix before merge. The naming convention feedback from the earlier review also remains open.

[alexey-pelykh]

Bug: order.name is the name of the order being cancelled, not the merge target. This should be self.order_id.name:

Suggested change

	order.message_post(body=_("Merged into %(order)s") % {"order": order.name})
	order.message_post(body=_("Merged into %(order)s") % {"order": self.order_id.name})

[alexey-pelykh]

self.to_merge includes the target order (self.order_id), so the message lists the target among "merged into this one". Consider using orders (already defined above as self.to_merge - self.order_id):

Suggested change

	% {"orders": ",".join(self.to_merge.mapped("name"))}
	% {"orders": ",".join(orders.mapped("name"))}

[alexey-pelykh]

view_type was removed from ir.actions.act_window in Odoo 13. Please remove this line.

[alexey-pelykh]

view_type was removed from ir.actions.act_window in Odoo 13. Please remove this line.

[alexey-pelykh]

Thank you for migrating this module to 17.0. I have reviewed the code and found several issues that should be addressed.

Summary:

• One bug in the merge message (wrong order name referenced)
• Stale view_type keys from old Odoo versions that should be removed for a clean 17.0 migration
• Translation files reference old 8.0 model paths that no longer exist
• Potential data integrity concern in picking merge logic
• Performance concern in _compute_merge_with

See inline comments for details.

[alexey-pelykh]

Bug: order.name here refers to the source order being iterated, not the target order. This message is posted on the source order and should say it was merged into the target, so this should be self.order_id.name:

order.message_post(body=_("Merged into %(order)s") % {"order": self.order_id.name})

[alexey-pelykh]

view_type was removed in Odoo 17.0 (it was deprecated since ~14.0). This key is silently ignored but should be removed for a clean migration. Same applies to the open_sale method in the wizard.

[alexey-pelykh]

The op and arg parameters are ignored. This means the search method only works correctly for the specific ("merge_with", "=", sale.id) usage, but any other operator (e.g. !=, in) would return wrong results silently. Consider at minimum validating that op == "=" and raising NotImplementedError for unsupported operators, or better yet, handling common operators.

[alexey-pelykh]

This compute method performs a self.search(...) call for each record in the recordset, which is an N+1 pattern. For a list view with many orders this could be slow. Consider whether this can be optimized, e.g. by batching the search or restructuring the computation. At minimum, adding a note about the performance implications would be helpful.

[alexey-pelykh]

When merging pickings, move_line_ids (stock.move.line) are reassigned to the target picking, but the corresponding move_ids (stock.move) are not moved. This can leave stock.move records pointing to the source picking, which will then be missing its move lines. The stock.move records should also be reassigned to the target picking before unlinking the source pickings (which doesn't seem to happen either -- source pickings with no moves/move_lines are left dangling).

[alexey-pelykh]

This inherits base.res_config_settings_view_form which places the setting in the General Settings page. Since this is a sale-related setting, it should inherit sale.res_config_settings_view_form and use an appropriate xpath to place it in the Sales settings section. The xpath //setting[@id='no_edit_order'] would need to be adjusted accordingly.

[alexey-pelykh]

Same as the other action dict: remove the view_type key, which was removed in Odoo 17.0.