[16.0][FIX] mrp_multi_level: Fix MRP wizard logic#1683
[16.0][FIX] mrp_multi_level: Fix MRP wizard logic#1683danielalvaro1999 wants to merge 1 commit intoOCA:16.0from
Conversation
|
Hi @ChrisOForgeFlow, @LoisRForgeFlow, @JordiBForgeFlow, |
AdrianaSaiz
left a comment
AdrianaSaiz
left a comment
There was a problem hiding this comment.
Code Review Changes Summary:
-
Editable mrp_action field: Users can now change the procurement method
(buy, manufacture, pull, pull_push) before executing. The selected method
is respected via ProcurementGroup._get_rule() override in stock_rule.py. -
Context-aware quantity handling: Added 'source_context' and 'original_qty'
fields to prevent quantity reset when opening wizard from planned orders.- From inventory: recalculates qty from mrp_inventory.to_procure
- From planned order: preserves original planned quantity
-
Company and currency support: Added company_id and currency_id to
procurement values to avoid errors during PO/MO creation and to support
multi-currency purchases. -
Vendor pre-fill for purchases: When mrp_action='buy', the wizard now
pre-fills supplier_id and currency_id from product's seller info. -
Tests added (test_mrp_multi_level.py):
- test_26: Override to 'buy' creates PO
- test_31: _get_rule respects mrp_action parameter
- test_32: UoM onchange fallback to original_qty
- test_33: Override to 'manufacture' creates MO
- test_34: Override to 'pull' creates picking
- test_35: Override to 'pull_push' creates picking
Note: 'push' action not tested as it works on existing moves, not procurements.
| domain = [ | ||
| ("action", "=", self.mrp_action), | ||
| ("location_dest_id", "=", location.id), | ||
| ] | ||
|
|
||
| rule = self.env["stock.rule"].search(domain, limit=1) | ||
|
|
||
| if not rule: | ||
| domain2 = [ | ||
| ("action", "=", self.mrp_action), | ||
| ("warehouse_id", "=", warehouse.id), | ||
| ] | ||
| rule = self.env["stock.rule"].search(domain2, limit=1) | ||
| return rule |
There was a problem hiding this comment.
| domain = [ | |
| ("action", "=", self.mrp_action), | |
| ("location_dest_id", "=", location.id), | |
| ] | |
| rule = self.env["stock.rule"].search(domain, limit=1) | |
| if not rule: | |
| domain2 = [ | |
| ("action", "=", self.mrp_action), | |
| ("warehouse_id", "=", warehouse.id), | |
| ] | |
| rule = self.env["stock.rule"].search(domain2, limit=1) | |
| return rule | |
| company = self.warehouse_id.company_id or self.env.company | |
| domain = [ | |
| ("action", "=", self.mrp_action), | |
| ("location_dest_id", "=", location.id), | |
| ("company_id", "in", [company.id, False]), | |
| ] | |
| rule = self.env["stock.rule"].search(domain, order="sequence", limit=1) | |
| if not rule: | |
| domain2 = [ | |
| ("action", "=", self.mrp_action), | |
| ("warehouse_id", "=", self.warehouse_id.id), | |
| ("company_id", "in", [company.id, False]), | |
| ] | |
| rule = self.env["stock.rule"].search(domain2, order="sequence", limit=1) |
Si hay varias reglas con el mismo action (ej: dos reglas "buy"), limit=1 sin order puede devolver cualquiera. Mejor añadir order="sequence" para que nos devuelva la de mayor prioridad.
En entorno multi-company podría devolver una regla de otra empresa. Por eso habría que añadir filtro por company_id.
| company = self.warehouse_id.company_id or self.env.company | ||
| currency = company.currency_id | ||
|
|
||
| res = { | ||
| "date_planned": self.date_planned, | ||
| "warehouse_id": self.warehouse_id, | ||
| "group_id": group, | ||
| "planned_order_id": self.planned_order_id.id, | ||
| "company_id": company, | ||
| "currency_id": currency.id, | ||
| } | ||
| should_add_supplier = (self.mrp_action == "buy") or ( | ||
| not self.mrp_action and self.supply_method == "buy" | ||
| ) | ||
|
|
||
| if should_add_supplier: | ||
| qty = self.qty or 0.0 | ||
| uom = self.uom_id or self.product_id.uom_id | ||
| supplier_info = self.product_id._select_seller( | ||
| quantity=qty, | ||
| date=self.date_planned, | ||
| uom_id=uom, | ||
| partner_id=self.supplier_id, | ||
| ) | ||
| if not supplier_info: | ||
| supplier_info = self.product_id.seller_ids.filtered( | ||
| lambda s: (not s.company_id or s.company_id == company) | ||
| ).sorted(lambda s: (s.sequence, s.min_qty, s.price))[:1] | ||
|
|
||
| if supplier_info: | ||
| res["supplier_id"] = supplier_info.partner_id.id | ||
| res["currency_id"] = ( | ||
| supplier_info.currency_id or company.currency_id | ||
| ).id | ||
| else: | ||
| res["currency_id"] = company.currency_id.id | ||
|
|
||
| if hasattr(self, "mrp_action") and self.mrp_action: | ||
| res["mrp_action"] = self.mrp_action | ||
|
|
||
| rule = self._get_procurement_rule() | ||
| if rule: | ||
| res["rule_id"] = rule.id | ||
| return res |
There was a problem hiding this comment.
| company = self.warehouse_id.company_id or self.env.company | |
| currency = company.currency_id | |
| res = { | |
| "date_planned": self.date_planned, | |
| "warehouse_id": self.warehouse_id, | |
| "group_id": group, | |
| "planned_order_id": self.planned_order_id.id, | |
| "company_id": company, | |
| "currency_id": currency.id, | |
| } | |
| should_add_supplier = (self.mrp_action == "buy") or ( | |
| not self.mrp_action and self.supply_method == "buy" | |
| ) | |
| if should_add_supplier: | |
| qty = self.qty or 0.0 | |
| uom = self.uom_id or self.product_id.uom_id | |
| supplier_info = self.product_id._select_seller( | |
| quantity=qty, | |
| date=self.date_planned, | |
| uom_id=uom, | |
| partner_id=self.supplier_id, | |
| ) | |
| if not supplier_info: | |
| supplier_info = self.product_id.seller_ids.filtered( | |
| lambda s: (not s.company_id or s.company_id == company) | |
| ).sorted(lambda s: (s.sequence, s.min_qty, s.price))[:1] | |
| if supplier_info: | |
| res["supplier_id"] = supplier_info.partner_id.id | |
| res["currency_id"] = ( | |
| supplier_info.currency_id or company.currency_id | |
| ).id | |
| else: | |
| res["currency_id"] = company.currency_id.id | |
| if hasattr(self, "mrp_action") and self.mrp_action: | |
| res["mrp_action"] = self.mrp_action | |
| rule = self._get_procurement_rule() | |
| if rule: | |
| res["rule_id"] = rule.id | |
| return res | |
| company = self.warehouse_id.company_id or self.env.company | |
| currency = company.currency_id | |
| res = { | |
| "date_planned": self.date_planned, | |
| "warehouse_id": self.warehouse_id, | |
| "group_id": group, | |
| "planned_order_id": self.planned_order_id.id, | |
| "company_id": company, | |
| "currency_id": currency.id, | |
| } | |
| # Añadir mrp_action para que _get_rule lo use | |
| if self.mrp_action: | |
| res["mrp_action"] = self.mrp_action | |
| should_add_supplier = (self.mrp_action == "buy") or ( | |
| not self.mrp_action and self.supply_method == "buy" | |
| ) | |
| if should_add_supplier: | |
| qty = self.qty or 0.0 | |
| uom = self.uom_id or self.product_id.uom_id | |
| supplier_info = self.product_id._select_seller( | |
| quantity=qty, | |
| date=self.date_planned, | |
| uom_id=uom, | |
| partner_id=self.supplier_id, | |
| ) | |
| if not supplier_info: | |
| supplier_info = self.product_id.seller_ids.filtered( | |
| lambda s: (not s.company_id or s.company_id == company) | |
| ).sorted(lambda s: (s.sequence, s.min_qty, s.price))[:1] | |
| if supplier_info: | |
| res["supplierinfo_id"] = supplier_info | |
| res["currency_id"] = ( | |
| supplier_info.currency_id or company.currency_id | |
| ).id | |
| else: | |
| res["currency_id"] = company.currency_id.id | |
| return res |
En este punto la llamada a _get_procurement_rule() y la asignación de rule_id en los values no son necesarias. Solo necesitamots añadir mrp_action a los values para que _get_rule lo use.
Ya no necesitamos buscar la regla aquí ni pasarla en los values. Con la herencia de _get_rule, la regla correcta se selecciona automáticamente dentro del flujo de procurement cuando lee el mrp_action. Solo tenemos que pasarle el mrp_action y Odoo hace el resto.
| def _get_procurement_rule(self): | ||
| self.ensure_one() | ||
|
|
||
| if not self.mrp_action or self.mrp_action == "none": | ||
| return False | ||
|
|
||
| location = self.location_id | ||
| warehouse = self.warehouse_id | ||
|
|
||
| domain = [ | ||
| ("action", "=", self.mrp_action), | ||
| ("location_dest_id", "=", location.id), | ||
| ] | ||
|
|
||
| rule = self.env["stock.rule"].search(domain, limit=1) | ||
|
|
||
| if not rule: | ||
| domain2 = [ | ||
| ("action", "=", self.mrp_action), | ||
| ("warehouse_id", "=", warehouse.id), | ||
| ] | ||
| rule = self.env["stock.rule"].search(domain2, limit=1) | ||
| return rule |
There was a problem hiding this comment.
| def _get_procurement_rule(self): | |
| self.ensure_one() | |
| if not self.mrp_action or self.mrp_action == "none": | |
| return False | |
| location = self.location_id | |
| warehouse = self.warehouse_id | |
| domain = [ | |
| ("action", "=", self.mrp_action), | |
| ("location_dest_id", "=", location.id), | |
| ] | |
| rule = self.env["stock.rule"].search(domain, limit=1) | |
| if not rule: | |
| domain2 = [ | |
| ("action", "=", self.mrp_action), | |
| ("warehouse_id", "=", warehouse.id), | |
| ] | |
| rule = self.env["stock.rule"].search(domain2, limit=1) | |
| return rule |
Esta método hay que eliminarlo, ya no es necesario porque la lógica va a estar en _get_rule del procurement group.
| def test_31_procure_wizard_forced_rule_pull_calls_run_pull(self): | ||
| """Covers the branch where a forced rule triggers `_run_pull`.""" | ||
| mrp_inv = self.mrp_inventory_obj.search( | ||
| [("product_mrp_area_id.product_id", "=", self.fp_1.id)], limit=1 | ||
| ) | ||
| self.assertTrue(mrp_inv) | ||
|
|
||
| wiz = self.mrp_inventory_procure_wiz.with_context( | ||
| active_model="mrp.inventory", | ||
| active_ids=mrp_inv.ids, | ||
| active_id=mrp_inv.id, | ||
| ).create({}) | ||
| self.assertTrue(wiz.item_ids) | ||
| item = wiz.item_ids[0] | ||
| item.qty = 1.0 | ||
|
|
||
| route = self.env["stock.route"].create( | ||
| { | ||
| "name": "Test route (mrp_multi_level)", | ||
| "product_selectable": False, | ||
| "warehouse_selectable": False, | ||
| "company_id": self.company.id, | ||
| } | ||
| ) | ||
|
|
||
| picking_type = item.warehouse_id.in_type_id | ||
| self.assertTrue(picking_type) | ||
|
|
||
| rule = self.env["stock.rule"].create( | ||
| { | ||
| "name": "Test pull rule (mrp_multi_level)", | ||
| "route_id": route.id, | ||
| "action": "pull", | ||
| "location_src_id": item.location_id.id, | ||
| "location_dest_id": item.location_id.id, | ||
| "picking_type_id": picking_type.id, | ||
| "company_id": self.company.id, | ||
| } | ||
| ) | ||
|
|
||
| calls = {"count": 0, "args": None} | ||
|
|
||
| ItemModel = type(item) | ||
| RuleModel = type(rule) | ||
| PGModel = type(self.env["procurement.group"]) | ||
|
|
||
| original_prepare = ItemModel._prepare_procurement_values | ||
| original_run_pull = RuleModel._run_pull | ||
| original_pg_run = PGModel.run | ||
|
|
||
| def _fake_prepare_procurement_values(_self): | ||
| vals = original_prepare(_self) | ||
| vals["rule_id"] = rule.id | ||
| return vals | ||
|
|
||
| def _fake_run_pull(_self, procurements): | ||
| calls["count"] += 1 | ||
| calls["args"] = procurements | ||
| return True | ||
|
|
||
| def _fake_pg_run(_self, procurements): | ||
| raise AssertionError("Shouldn't call procurement.group.run()") | ||
|
|
||
| try: | ||
| ItemModel._prepare_procurement_values = _fake_prepare_procurement_values | ||
| RuleModel._run_pull = _fake_run_pull | ||
| PGModel.run = _fake_pg_run | ||
| wiz.make_procurement() | ||
| finally: | ||
| ItemModel._prepare_procurement_values = original_prepare | ||
| RuleModel._run_pull = original_run_pull | ||
| PGModel.run = original_pg_run | ||
|
|
||
| self.assertEqual(calls["count"], 1) | ||
| self.assertTrue(calls["args"]) | ||
| self.assertEqual(calls["args"][0][1].id, rule.id) |
There was a problem hiding this comment.
| def test_31_procure_wizard_forced_rule_pull_calls_run_pull(self): | |
| """Covers the branch where a forced rule triggers `_run_pull`.""" | |
| mrp_inv = self.mrp_inventory_obj.search( | |
| [("product_mrp_area_id.product_id", "=", self.fp_1.id)], limit=1 | |
| ) | |
| self.assertTrue(mrp_inv) | |
| wiz = self.mrp_inventory_procure_wiz.with_context( | |
| active_model="mrp.inventory", | |
| active_ids=mrp_inv.ids, | |
| active_id=mrp_inv.id, | |
| ).create({}) | |
| self.assertTrue(wiz.item_ids) | |
| item = wiz.item_ids[0] | |
| item.qty = 1.0 | |
| route = self.env["stock.route"].create( | |
| { | |
| "name": "Test route (mrp_multi_level)", | |
| "product_selectable": False, | |
| "warehouse_selectable": False, | |
| "company_id": self.company.id, | |
| } | |
| ) | |
| picking_type = item.warehouse_id.in_type_id | |
| self.assertTrue(picking_type) | |
| rule = self.env["stock.rule"].create( | |
| { | |
| "name": "Test pull rule (mrp_multi_level)", | |
| "route_id": route.id, | |
| "action": "pull", | |
| "location_src_id": item.location_id.id, | |
| "location_dest_id": item.location_id.id, | |
| "picking_type_id": picking_type.id, | |
| "company_id": self.company.id, | |
| } | |
| ) | |
| calls = {"count": 0, "args": None} | |
| ItemModel = type(item) | |
| RuleModel = type(rule) | |
| PGModel = type(self.env["procurement.group"]) | |
| original_prepare = ItemModel._prepare_procurement_values | |
| original_run_pull = RuleModel._run_pull | |
| original_pg_run = PGModel.run | |
| def _fake_prepare_procurement_values(_self): | |
| vals = original_prepare(_self) | |
| vals["rule_id"] = rule.id | |
| return vals | |
| def _fake_run_pull(_self, procurements): | |
| calls["count"] += 1 | |
| calls["args"] = procurements | |
| return True | |
| def _fake_pg_run(_self, procurements): | |
| raise AssertionError("Shouldn't call procurement.group.run()") | |
| try: | |
| ItemModel._prepare_procurement_values = _fake_prepare_procurement_values | |
| RuleModel._run_pull = _fake_run_pull | |
| PGModel.run = _fake_pg_run | |
| wiz.make_procurement() | |
| finally: | |
| ItemModel._prepare_procurement_values = original_prepare | |
| RuleModel._run_pull = original_run_pull | |
| PGModel.run = original_pg_run | |
| self.assertEqual(calls["count"], 1) | |
| self.assertTrue(calls["args"]) | |
| self.assertEqual(calls["args"][0][1].id, rule.id) | |
| def test_31_get_rule_respects_mrp_action(self): | |
| """Test that _get_rule returns rule matching mrp_action when provided.""" | |
| mrp_inv = self.mrp_inventory_obj.search( | |
| [("product_mrp_area_id.product_id", "=", self.fp_1.id)], limit=1 | |
| ) | |
| self.assertTrue(mrp_inv) | |
| wiz = self.mrp_inventory_procure_wiz.with_context( | |
| active_model="mrp.inventory", | |
| active_ids=mrp_inv.ids, | |
| active_id=mrp_inv.id, | |
| ).create({}) | |
| self.assertTrue(wiz.item_ids) | |
| item = wiz.item_ids[0] | |
| route = self.env["stock.route"].create( | |
| { | |
| "name": "Test route (mrp_multi_level)", | |
| "product_selectable": False, | |
| "warehouse_selectable": False, | |
| "company_id": self.company.id, | |
| } | |
| ) | |
| picking_type = item.warehouse_id.in_type_id | |
| pull_rule = self.env["stock.rule"].create( | |
| { | |
| "name": "Test pull rule (mrp_multi_level)", | |
| "route_id": route.id, | |
| "action": "pull", | |
| "location_src_id": self.supplier_location.id, | |
| "location_dest_id": item.location_id.id, | |
| "picking_type_id": picking_type.id, | |
| "company_id": self.company.id, | |
| } | |
| ) | |
| # Veriffy _get_rule returns pull rule when mrp_action='pull' | |
| pg = self.env["procurement.group"] | |
| values = { | |
| "mrp_action": "pull", | |
| "company_id": self.company, | |
| "warehouse_id": item.warehouse_id, | |
| } | |
| found_rule = pg._get_rule(item.product_id, item.location_id, values) | |
| self.assertEqual(found_rule.id, pull_rule.id) | |
| self.assertEqual(found_rule.action, "pull") | |
Reescribir test para verificar el nuevo comportamiento: que _get_rule respeta el mrp_action pasado en los values y devuelve la regla correcta. El test anterior verificaba que se llamaba directamente a _run_pull, lo cual ya no aplica.
danielalvaro1999
force-pushed
the
16.0-fix-mrp_multi_level
branch
from
275e80e to
23a38cf
Compare
| def _get_rule(self, product_id, location_id, values): | ||
| """Override to respect mrp_action from MRP Multi Level wizard.""" | ||
| mrp_action = values.get("mrp_action") | ||
| if mrp_action and mrp_action not in ("none", False): | ||
| company = values.get("company_id", self.env.company) | ||
| if hasattr(company, "id"): | ||
| company_id = company.id | ||
| else: | ||
| company_id = company | ||
| rule = self.env["stock.rule"].search( | ||
| [ | ||
| ("action", "=", mrp_action), | ||
| ("location_dest_id", "=", location_id.id), | ||
| ("company_id", "in", [company_id, False]), | ||
| ], | ||
| order="sequence", | ||
| limit=1, | ||
| ) | ||
| if not rule: | ||
| warehouse = values.get("warehouse_id") | ||
| if warehouse: | ||
| warehouse_id = ( | ||
| warehouse.id if hasattr(warehouse, "id") else warehouse | ||
| ) | ||
| rule = self.env["stock.rule"].search( | ||
| [ | ||
| ("action", "=", mrp_action), | ||
| ("warehouse_id", "=", warehouse_id), | ||
| ("company_id", "in", [company_id, False]), | ||
| ], | ||
| order="sequence", | ||
| limit=1, | ||
| ) | ||
| if rule: | ||
| return rule | ||
| return super()._get_rule(product_id, location_id, values) |
There was a problem hiding this comment.
| def _get_rule(self, product_id, location_id, values): | |
| """Override to respect mrp_action from MRP Multi Level wizard.""" | |
| mrp_action = values.get("mrp_action") | |
| if mrp_action and mrp_action not in ("none", False): | |
| company = values.get("company_id", self.env.company) | |
| if hasattr(company, "id"): | |
| company_id = company.id | |
| else: | |
| company_id = company | |
| rule = self.env["stock.rule"].search( | |
| [ | |
| ("action", "=", mrp_action), | |
| ("location_dest_id", "=", location_id.id), | |
| ("company_id", "in", [company_id, False]), | |
| ], | |
| order="sequence", | |
| limit=1, | |
| ) | |
| if not rule: | |
| warehouse = values.get("warehouse_id") | |
| if warehouse: | |
| warehouse_id = ( | |
| warehouse.id if hasattr(warehouse, "id") else warehouse | |
| ) | |
| rule = self.env["stock.rule"].search( | |
| [ | |
| ("action", "=", mrp_action), | |
| ("warehouse_id", "=", warehouse_id), | |
| ("company_id", "in", [company_id, False]), | |
| ], | |
| order="sequence", | |
| limit=1, | |
| ) | |
| if rule: | |
| return rule | |
| return super()._get_rule(product_id, location_id, values) | |
| def _get_rule(self, product_id, location_id, values): | |
| """Override to respect mrp_action from MRP Multi Level wizard.""" | |
| mrp_action = values.get("mrp_action") | |
| # If there is no valid mrp_action (None, False, 'none', ''), delegate to super | |
| if not mrp_action or mrp_action in ("none", False): | |
| return super()._get_rule(product_id, location_id, values) | |
| # Normalize company (accept record or id) | |
| company = values.get("company_id", self.env.company) | |
| company_id = company.id if hasattr(company, "id") else company | |
| domain = [ | |
| ("action", "=", mrp_action), | |
| ("company_id", "in", [company_id, False]), | |
| ] | |
| rule = self.env["stock.rule"].search( | |
| domain + [("location_dest_id", "=", location_id.id)], | |
| order="sequence", | |
| limit=1, | |
| ) | |
| if not rule: | |
| warehouse = values.get("warehouse_id") | |
| if warehouse: | |
| warehouse_id = ( | |
| warehouse.id if hasattr(warehouse, "id") else warehouse | |
| ) | |
| rule = self.env["stock.rule"].search( | |
| domain + [("warehouse_id", "=", warehouse_id)], | |
| order="sequence", | |
| limit=1, | |
| ) | |
| return rule or super()._get_rule(product_id, location_id, values) |
danielalvaro1999
force-pushed
the
16.0-fix-mrp_multi_level
branch
from
23a38cf to
8dc22e2
Compare
danielalvaro1999
force-pushed
the
16.0-fix-mrp_multi_level
branch
8 times, most recently
from
395fc70 to
204d148
Compare
AdrianaSaiz
force-pushed
the
16.0-fix-mrp_multi_level
branch
7 times, most recently
from
8b8a419 to
4ea3cd3
Compare
…antity reset, and missing company/currency data
AdrianaSaiz
force-pushed
the
16.0-fix-mrp_multi_level
branch
7 times, most recently
from
5024b75 to
697f2b8
Compare
AdrianaSaiz
force-pushed
the
16.0-fix-mrp_multi_level
branch
from
697f2b8 to
5024b75
Compare
danielalvaro1999
changed the title
|
LGTM |
There was a problem hiding this comment.
First of all thanks for contributing. This is a big change for an stable version that is going to be hard for me to merge as is. Here there is a mix of technical fixes and functionality changes.
If you could split this in smaller PRs, particularly if you keep the group override and method selection aside for a moment, we could first focus on the "pure fixes" that are going to be easier to discuss.
Thanks!
|
Thanks for the feedback! The main issue here is that the wizard was ignoring the user's action selection entirely. Regardless of which action the user chose (buy, manufacture, etc.), the system would always follow the product's default route instead. All changes in this PR work together to fix this behavior:
These changes are interdependent - applying them separately would leave the wizard partially broken. Happy to improve the commit messages or add documentation if that would help with the review. Thanks! |
|
Hi @AdrianaSaiz In the PR description you enumerate the changes, based on that and without doing any code review yet:
These 2 are behavior changes. I would like a more functional description of what is the goal or need behind those changes.
A PR like this with the title "Fix MRP wizard logic" is a bit hard to digest. I miss a more functional description of the objectives and reasoning for the changes. I hope you can help me a bit here, I'm trying to find a way to be able to review your changes without spending a lot of time which is going to be difficult for me if I don't understand the motivation clearly. Thanks! |
marcos-mendez
left a comment
marcos-mendez
left a comment
There was a problem hiding this comment.
Automated Review -- Tests Failed
1. Root Cause
The test failure is caused by a missing mrp_action field definition in the mrp.inventory.procure.item model, which is referenced in the procurement_group.py override of _get_rule(). This leads to an AttributeError when trying to access values.get("mrp_action") because the field is not defined in the model.
2. Suggested Fix
Add the mrp_action field to the MrpInventoryProcureItem model in mrp_multi_level/models/mrp_inventory_procure_item.py:
# File: mrp_multi_level/models/mrp_inventory_procure_item.py
from odoo import fields, models
class MrpInventoryProcureItem(models.TransientModel):
_name = "mrp.inventory.procure.item"
_description = "MRP Inventory Procure Item"
# Existing fields...
mrp_action = fields.Selection(
selection=[
("none", "None"),
("buy", "Purchase Order"),
("manufacture", "Manufacturing Order"),
("pull", "Pull From"),
("push", "Push To"),
("pull_push", "Pull & Push"),
],
string="Procurement Method",
help="Method to use for procurement. Can be modified before executing.",
)
# ... rest of the model3. Additional Code Issues
- Missing model definition: The
mrp_inventory_procure_item.pymodel file is not present in the diff, but the code assumes its existence. Ensure this file exists with proper field definitions. - Inconsistent use of
hasattr: Inprocurement_group.py,hasattr(company, "id")is used to check foridattribute. This is safe, but the code can be simplified toisinstance(company, models.BaseModel)for better readability and robustness.
4. Test Improvements
Add a test case to verify the field is correctly defined and accessible:
def test_36_procure_item_mrp_action_field_exists(self):
"""Test that mrp_action field exists and is correctly defined."""
item = self.env["mrp.inventory.procure.item"].create({})
self.assertTrue(hasattr(item, 'mrp_action'))
self.assertEqual(
item._fields['mrp_action'].type,
'selection',
"mrp_action should be a selection field"
)Also, consider using SavepointCase for tests that modify data to ensure isolation and rollback:
from odoo.tests import SavepointCase
class TestMrpMultiLevelProcureAction(SavepointCase):
...This ensures tests are more robust and don't interfere with each other.
⏰ This PR has been open for 81 days.
💤 Last activity was 45 days ago.
Every ignored PR is a contributor who might not come back. Review time matters. (OCA Aging Report)
Reciprocal Review Request
Hi everyone! I found some test failures on this PR and left detailed feedback above. I am happy to discuss or help debug. In the meantime, if any of you get a chance, I would appreciate a look at my open PR(s):
My open PRs across OCA:
- server-tools#3554 [MIG] datetime_formatter: Migration to 18.0
- 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. Thank you!
Environment via OCA Neural Reviewer: Minikube + K8s Job + oca-ci/py3.10-odoo16.0 | Odoo 16.0
Automated review by OCA Neural Reviewer + qwen3-coder:30b
7 participants
[16.0][FIX] mrp_multi_level: Fix MRP wizard logic (PR #1683)
Why
Previously, the MRP Multi Level procurement wizard did not reliably allow users to explicitly choose the procurement method (manufacture/buy/pull/push/pull_push): the actual rule/action used at execution time could be driven by routes/rules and end up different from what the user intended.
This change makes the resulting procurement behavior consistent with the user’s explicit selection.
What changed
1) Wizard: propagate the selected method + keep better context
In
mrp_multi_level/wizards/mrp_inventory_procure.py:source_context(planned order vs inventory)original_qty(used to keep quantities stable when changing the UoM)values["mrp_action"]so the downstream rule selection can use it.2) New file: procurement group override (new flow)
Added
mrp_multi_level/models/procurement_group.py:procurement.group._get_rule()to respectmrp_actionwhen it is provided invalues.company_id in [company, False]order="sequence"to pick the highest priority rule3) Multi-currency POs: avoid cross-currency draft reuse
In
mrp_multi_level/models/stock_rule.py:currency_idin the draft PO lookup domain when present invalues.currency_idis provided (supports both a singledictand groupedvalues).4) UI: make the method clearly editable
In
mrp_multi_level/wizards/mrp_inventory_procure_views.xml:editable="top").buy.5) Planned orders: clearer action + direct entrypoint
In
mrp_multi_level/views/mrp_planned_order_views.xml:mrp_actionvisibility using a badge + decorations.6) Change supply_method string:
7) Tests
In
mrp_multi_level/tests/test_mrp_multi_level.py:_get_rule()respectsmrp_action.Modified files
mrp_multi_level/models/__init__.pymrp_multi_level/models/procurement_group.py(new)mrp_multi_level/models/mrp_inventory.pymrp_multi_level/models/stock_rule.pymrp_multi_level/models/product_mrp_area.pymrp_multi_level/wizards/mrp_inventory_procure.pymrp_multi_level/wizards/mrp_inventory_procure_views.xmlmrp_multi_level/views/mrp_planned_order_views.xmlmrp_multi_level/tests/test_mrp_multi_level.py