diff --git a/erpnext/manufacturing/doctype/production_plan/services/bom_explosion.py b/erpnext/manufacturing/doctype/production_plan/services/bom_explosion.py index d0993980342..07503465451 100644 --- a/erpnext/manufacturing/doctype/production_plan/services/bom_explosion.py +++ b/erpnext/manufacturing/doctype/production_plan/services/bom_explosion.py @@ -116,9 +116,12 @@ def _subitems_query(company, bom_no, include_non_stock_items, parent_qty, planne def _subitem_columns(bom_item, bom, item, item_default, item_uom, parent_qty, planned_qty): qty = IfNull(parent_qty * Sum(bom_item.stock_qty / IfNull(bom.quantity, 1)) * planned_qty, 0).as_("qty") - # only item_code is grouped; the rest are functionally dependent on the grouped item (item - # attributes) or arbitrary per BOM Item on MySQL -> Max() keeps the GROUP BY valid on postgres - # while returning the same value MySQL picked. + # only item_code is grouped; the remaining item-attribute columns are functionally dependent on it, + # so Max() returns their single value on both engines. is_phantom_item is the exception: the same + # item_code can sit on a phantom line and a real-RM line in one BOM, and get_subitems() drops any + # row whose is_phantom_item is truthy. Max() would let a single phantom line mask the real material + # and silently drop it; Min() instead treats the item as phantom only when EVERY line is phantom, so + # a real raw material is never lost. Deterministic and identical on MariaDB and Postgres. return [ bom_item.item_code, Max(item.default_material_request_type).as_("default_material_request_type"), @@ -136,7 +139,7 @@ def _subitem_columns(bom_item, bom, item, item_default, item_uom, parent_qty, pl Max(item_uom.conversion_factor).as_("conversion_factor"), Max(bom.item).as_("main_bom_item"), Max(bom.name).as_("main_bom"), - Max(bom_item.is_phantom_item).as_("is_phantom_item"), + Min(bom_item.is_phantom_item).as_("is_phantom_item"), ] diff --git a/erpnext/manufacturing/doctype/production_plan/test_production_plan.py b/erpnext/manufacturing/doctype/production_plan/test_production_plan.py index c8073ba5892..e720bd96319 100644 --- a/erpnext/manufacturing/doctype/production_plan/test_production_plan.py +++ b/erpnext/manufacturing/doctype/production_plan/test_production_plan.py @@ -2917,6 +2917,49 @@ class TestProductionPlan(ERPNextTestSuite): self.assertEqual(by_bom_no[phantom_bom.name].is_phantom_item, 1) self.assertEqual(by_bom_no[normal_bom.name].is_phantom_item, 0) + def test_subitems_query_keeps_real_rm_listed_alongside_phantom(self): + """bom_explosion._subitems_query groups BOM lines by item_code, and get_subitems() drops any + grouped row whose is_phantom_item is truthy. When one item_code is listed in a BOM both as a + phantom sub-assembly and as a plain raw material, Max(is_phantom_item)=1 made get_subitems + silently drop the real material. Min(is_phantom_item) keeps it (phantom only when every line + is phantom) and is deterministic on MariaDB and Postgres. + """ + from erpnext.manufacturing.doctype.production_plan.services.bom_explosion import _subitems_query + + component = make_item(properties={"is_stock_item": 1, "valuation_rate": 10}).name + rm_phantom = make_item(properties={"is_stock_item": 1, "valuation_rate": 10}).name + + phantom_bom = make_bom(item=component, raw_materials=[rm_phantom], do_not_save=True) + phantom_bom.is_phantom_bom = 1 + phantom_bom.save() + phantom_bom.submit() + # the phantom BOM is auto-set as the component's default; clear it so the second component line + # stays a plain (non-phantom) raw material instead of inheriting the phantom BOM as its bom_no. + frappe.db.set_value("Item", component, "default_bom", "") + frappe.clear_document_cache("Item", component) + + fg_item = make_item(properties={"is_stock_item": 1, "valuation_rate": 10}).name + parent = make_bom(item=fg_item, raw_materials=[component], do_not_save=True) + parent.items[0].bom_no = phantom_bom.name # phantom line -> is_phantom_item = 1 + component_doc = frappe.get_doc("Item", component) + parent.append( + "items", + { + "item_code": component, + "qty": 1, + "uom": component_doc.stock_uom, + "stock_uom": component_doc.stock_uom, + }, + ) # plain raw-material line (no bom_no) -> is_phantom_item = 0 + parent.save() + parent.submit() + + rows = _subitems_query("_Test Company", parent.name, 1, 1, 1) + component_rows = [r for r in rows if r.item_code == component] + self.assertEqual(len(component_rows), 1) + # Min() keeps the real material; the old Max() returned 1 and get_subitems dropped it. + self.assertEqual(component_rows[0].is_phantom_item, 0) + def create_production_plan(**args): """ diff --git a/erpnext/manufacturing/doctype/work_order/services/required_items.py b/erpnext/manufacturing/doctype/work_order/services/required_items.py index b94130163b9..a8a415ca4fc 100644 --- a/erpnext/manufacturing/doctype/work_order/services/required_items.py +++ b/erpnext/manufacturing/doctype/work_order/services/required_items.py @@ -191,17 +191,25 @@ class RequiredItemsService: frappe.qb.from_(ste) .inner_join(ste_child) .on(ste_child.parent == ste.name) - # original_item is arbitrary per grouped item_code on MySQL -> Max() keeps the GROUP BY valid - # on postgres while returning the same value (it is only used as a dict key fallback below) + # original_item becomes the output dict key below, so it must stay coherent per row: the + # same item_code can be transferred both for itself (original_item NULL) and as a substitute + # for another required item (original_item set). Max() over a single item_code group could + # pick the substitute's original_item and misattribute the item's own transfer to it. Group + # by (item_code, original_item) so each pair sums separately, then accumulate into the keyed + # dict (two distinct rows can resolve to the same key, e.g. A's own transfer and B-for-A). .select( ste_child.item_code, - fn.Max(ste_child.original_item).as_("original_item"), + ste_child.original_item, fn.Sum(ste_child.transfer_qty).as_("qty"), ) .where(self._material_transfer_filter(ste, is_return)) - .groupby(ste_child.item_code) + .groupby(ste_child.item_code, ste_child.original_item) ) - return frappe._dict({d.original_item or d.item_code: d.qty for d in (query.run(as_dict=1) or [])}) + qty_by_item = frappe._dict() + for d in query.run(as_dict=1) or []: + key = d.original_item or d.item_code + qty_by_item[key] = (qty_by_item.get(key) or 0.0) + flt(d.qty) + return qty_by_item def _material_transfer_filter(self, ste, is_return): return ( diff --git a/erpnext/manufacturing/doctype/work_order/test_work_order.py b/erpnext/manufacturing/doctype/work_order/test_work_order.py index 650f64342ea..f65cd78f178 100644 --- a/erpnext/manufacturing/doctype/work_order/test_work_order.py +++ b/erpnext/manufacturing/doctype/work_order/test_work_order.py @@ -4815,6 +4815,57 @@ class TestWorkOrder(ERPNextTestSuite): # generated qty (3.0 for 8 units) differs from the BOM-scaled qty (7.5 for 20 units) self.assertEqual(flt(row.qty, 6), 3.0) + def test_transferred_qty_not_misattributed_between_item_and_its_substitute(self): + """When one item is transferred both for itself and as a substitute for another required item, + each transfer must be credited to the right required item. + + _material_transfer_qty_by_item grouped Stock Entry Detail by item_code only and picked + Max(original_item); for item B transferred once for itself (original_item NULL) and once as a + substitute for A (original_item=A), Max picked A and credited B's whole transfer to A, leaving + B at 0. Grouping by (item_code, original_item) and accumulating into the keyed dict attributes + each transfer correctly, deterministically on MariaDB and Postgres. + """ + from erpnext.manufacturing.doctype.work_order.services.required_items import RequiredItemsService + + source_warehouse = "Stores - _TC" + fg_item = make_item("Test WO SelfSub FG", {"is_stock_item": 1}).name + item_a = make_item("Test WO SelfSub RM A", {"is_stock_item": 1, "allow_alternative_item": 1}).name + item_b = make_item("Test WO SelfSub RM B", {"is_stock_item": 1, "allow_alternative_item": 1}).name + + # B is a registered alternative for A + if not frappe.db.exists("Item Alternative", {"item_code": item_a, "alternative_item_code": item_b}): + frappe.get_doc( + { + "doctype": "Item Alternative", + "item_code": item_a, + "alternative_item_code": item_b, + "two_way": 1, + } + ).insert() + + # stock B generously (covers B-for-A plus B-for-itself) + for item, qty in ((item_a, 50), (item_b, 100)): + test_stock_entry.make_stock_entry( + item_code=item, target=source_warehouse, qty=qty, basic_rate=100 + ) + + make_bom(item=fg_item, source_warehouse=source_warehouse, raw_materials=[item_a, item_b]) + wo = make_wo_order_test_record(item=fg_item, qty=10, source_warehouse=source_warehouse) + + transfer = frappe.get_doc(make_stock_entry(wo.name, "Material Transfer for Manufacture", 10)) + transfer.save() + # substitute B for the A line; the existing B line stays as B's own transfer + for d in transfer.items: + if d.item_code == item_a: + d.item_code = item_b + d.original_item = item_a + transfer.submit() + + qty_by_item = RequiredItemsService(wo)._material_transfer_qty_by_item(is_return=0) + # B transferred as a substitute for A -> credited to A; B transferred for itself -> credited to B. + self.assertEqual(flt(qty_by_item.get(item_a)), 10.0) + self.assertEqual(flt(qty_by_item.get(item_b)), 10.0) + def get_reserved_entries(voucher_no, warehouse=None): doctype = frappe.qb.DocType("Stock Reservation Entry")