Conversation
|
@Kev-Roche Maybe adding the dependency in |
8b53ede to
ad88aee
Compare
alexey-pelykh
left a comment
There was a problem hiding this comment.
Thank you for this module -- the concept of per-product warehouse routing rules is useful. I found several issues that should be addressed before merging.
Summary of findings
| # | File | Severity | Finding |
|---|---|---|---|
| 1 | models/product.py |
⛔ blocker | Hardcoded French string "Entrepôt par défaut" as field label; OCA requires English |
| 2 | models/product.py:25 |
⛔ blocker | Deprecated force_company_id context key (already flagged by @rousseldenis) |
| 3 | models/sale_order.py:77 |
⛔ blocker | _() wraps a dynamically-built string (+ ", ".join(...)) -- the full string is never in .po, so translation is impossible |
| 4 | views/sale_order.xml:35 |
⛔ blocker | column_invisible compares Boolean to string 'False' instead of False -- column visibility will not work correctly |
| 5 | models/sale_order.py:83-84 |
⛔ blocker | action_confirm assigns a multi-record recordset to rec.warehouse_id (Many2one) when warehouse_rule_need_change is truthy but lines have mixed warehouse rules -- will crash |
| 6 | models/sale_warehouse_rule.py:57-82 |
Uniqueness constraint mutates domain by index and does not exclude self (("id", "!=", rule.id) is missing), risking false positives |
|
| 7 | tests/test_sale_warehouse_rule.py:6 |
SavepointCase is deprecated since Odoo 15; use TransactionCase for 16.0 |
|
| 8 | views/res_config_settings.xml:14 |
Settings label is in French; should be English with translation in .po |
|
| 9 | i18n/ |
Missing .pot template file (i18n/sale_warehouse_rule.pot) |
|
| 10 | models/sale_order.py:72-74 |
💡 nit | variant_warehouse_id is a Many2one (always 0 or 1 record) -- .filtered() on it is technically correct but misleading |
See inline comments for details.
|
|
||
| variant_warehouse_id = fields.Many2one( | ||
| comodel_name="stock.warehouse", | ||
| string="Entrepôt par défaut", |
There was a problem hiding this comment.
Field string is in French ("Entrepôt par défaut"). OCA convention requires English for all user-facing strings; translations belong in .po files.
Suggestion:
string="Default Warehouse",| @api.depends_context("allowed_company_ids", "force_company_id") | ||
| def _compute_variant_warehouse_id(self): | ||
| for product in self: | ||
| company_id = self.env.context.get("force_company_id") or self.env.company.id |
There was a problem hiding this comment.
+1 to @rousseldenis's earlier comment. force_company_id is deprecated in Odoo 16. Use self.env.company (which respects with_company()).
The @api.depends_context should then only list "allowed_company_ids" (drop "force_company_id").
| warehouses = warehouse_ids + rec.warehouse_id | ||
| rec.warehouse_rule_info = _( | ||
| "The delivery will be sent from multiple warehouses: " | ||
| + ", ".join(warehouses.mapped("name")) |
There was a problem hiding this comment.
The translatable string is built by concatenating _() with the warehouse names:
_("The delivery will be sent from multiple warehouses: " + ", ".join(...))This means the full string (including warehouse names) is passed to _(), so it will never match a .po entry.
Use a format placeholder instead:
_("The delivery will be sent from multiple warehouses: %s") % ", ".join(warehouses.mapped("name"))There was a problem hiding this comment.
You should use translation function arguments to pass parameters
| for rec in self: | ||
| if rec.warehouse_rule_need_change: | ||
| warehouse_id = rec.order_line.mapped("product_id.variant_warehouse_id") | ||
| rec.warehouse_id = warehouse_id |
There was a problem hiding this comment.
When warehouse_rule_need_change is truthy, mapped("product_id.variant_warehouse_id") can return multiple warehouse records (e.g. if only some products have the same warehouse rule). Assigning a multi-record recordset to rec.warehouse_id (a Many2one) will raise a ValueError.
This code path is only correct when all lines share a single warehouse, but the earlier condition in _compute_warehouse_rule_message that sets warehouse_rule_need_change already verifies len(warehouse_ids) == 1. However, action_confirm re-fetches the warehouses without the same guard. If the order lines changed between compute and confirm, this could crash.
Suggestion: add a safety check, e.g.:
warehouse_ids = rec.order_line.mapped("product_id.variant_warehouse_id")
if len(warehouse_ids) == 1:
rec.warehouse_id = warehouse_ids| rule.applied_on = "2_template" | ||
|
|
||
| @api.constrains("product_id", "attribute_value_ids", "warehouse_id") | ||
| def _check_warehouse_rule_uniqueness(self): |
There was a problem hiding this comment.
The uniqueness constraint logic mutates the domain list by index (domain[1] = ..., domain[2] = ...), which is fragile and hard to follow.
More importantly, the domain does not exclude the current record (("id", "!=", rule.id)) -- when updating an existing rule, search_count will find the rule itself and raise a false ValidationError.
Consider:
- Adding
("id", "!=", rule.id)to the base domain. - Building separate, explicit domains for each case instead of mutating one shared list.
| <attribute name="invisible">0</attribute> | ||
| <attribute | ||
| name="attrs" | ||
| >{'readonly': [('parent.state', '!=', 'draft')], 'column_invisible': [('parent.show_sale_line_warehouse_column', '=', 'False')]}</attribute> |
There was a problem hiding this comment.
'column_invisible': [('parent.show_sale_line_warehouse_column', '=', 'False')]show_sale_line_warehouse_column is a Boolean field, but it is compared to the string 'False'. In Odoo 16 this will not evaluate correctly.
Should be:
'column_invisible': [('parent.show_sale_line_warehouse_column', '=', False)]| from odoo.tests import Form, SavepointCase | ||
|
|
||
|
|
||
| class TestSaleWarehouseRule(SavepointCase): |
There was a problem hiding this comment.
SavepointCase has been deprecated since Odoo 15. For 16.0, use TransactionCase:
from odoo.tests import Form, TransactionCase
class TestSaleWarehouseRule(TransactionCase):| <label | ||
| for="show_sale_line_warehouse_column" | ||
| string="Afficher la colonne entrepôt sur les lignes de commande" | ||
| /> |
There was a problem hiding this comment.
The label text is in French:
string="Afficher la colonne entrepôt sur les lignes de commande"OCA modules must use English for all view strings. Use:
string="Show Warehouse Column on Sale Order Lines"and add the French translation to fr.po.
This module allows to force the source warehouse on sale order line from the product, depending of some warehouse rules.
cc @chafique-delli , @sebastienbeau
depends on : OCA/product-attribute#2022