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/services/sub_assembly_queries.py b/erpnext/manufacturing/doctype/production_plan/services/sub_assembly_queries.py index a86c24521ab..134dee34a2e 100644 --- a/erpnext/manufacturing/doctype/production_plan/services/sub_assembly_queries.py +++ b/erpnext/manufacturing/doctype/production_plan/services/sub_assembly_queries.py @@ -179,22 +179,25 @@ def _sub_assembly_rm_query(company, bom_no, include_non_stock_items, planned_qty .on((item.name == item_uom.parent) & (item_uom.uom == item.purchase_uom)) .select(*_sub_assembly_rm_columns(bei, bom, item, item_default, item_uom, planned_qty)) .where(_sub_assembly_rm_filter(bei, bom, item, bom_no, include_non_stock_items)) - .groupby(bei.item_code, bei.stock_uom) + .groupby(bei.item_code, bei.stock_uom, bei.bom_no, bei.is_phantom_item) ).run(as_dict=True) def _sub_assembly_rm_columns(bei, bom, item, item_default, item_uom, planned_qty): - # only item_code/stock_uom are grouped; every other column is 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. + # Grouped by item_code/stock_uom plus bom_no/is_phantom_item: those two MUST come from the same + # BOM Item row -- the consumer keys on (item_code, bom_no) and recurses on is_phantom_item, so an + # independent Max() per column could pair a bom_no from one line with is_phantom_item from another + # and recurse into the wrong sub-BOM. Grouping them keeps the pair coherent and the GROUP BY valid + # on postgres. The remaining columns are functionally dependent on the grouped item; Max() returns + # their single value on both engines. return [ (IfNull(Sum(bei.stock_qty / IfNull(bom.quantity, 1)), 0) * planned_qty).as_("qty"), Max(item.item_name).as_("item_name"), Max(item.name).as_("item_code"), Max(bei.description).as_("description"), bei.stock_uom, - Max(bei.is_phantom_item).as_("is_phantom_item"), - Max(bei.bom_no).as_("bom_no"), + bei.is_phantom_item, + bei.bom_no, Max(item.min_order_qty).as_("min_order_qty"), Max(bei.source_warehouse).as_("source_warehouse"), Max(item.default_material_request_type).as_("default_material_request_type"), diff --git a/erpnext/manufacturing/doctype/production_plan/test_production_plan.py b/erpnext/manufacturing/doctype/production_plan/test_production_plan.py index 8253e420f1a..e720bd96319 100644 --- a/erpnext/manufacturing/doctype/production_plan/test_production_plan.py +++ b/erpnext/manufacturing/doctype/production_plan/test_production_plan.py @@ -2862,6 +2862,104 @@ class TestProductionPlan(ERPNextTestSuite): "The phantom BOM was not re-exploded for the second po_item.", ) + def test_sub_assembly_rm_query_keeps_bom_no_phantom_pair_coherent(self): + """bom_no and is_phantom_item must stay paired to the same BOM Item line. + + When a component is listed more than once in a sub-assembly BOM pointing at different + sub-BOMs (one phantom, one not), grouping only by (item_code, stock_uom) collapsed both + lines into one row, and the independent Max() per column could pair the phantom flag of + one line with the bom_no of the other. The consumer keys on (item_code, bom_no) and + recurses on is_phantom_item, so an incoherent pair recurses into the wrong sub-BOM. + Grouping also by (bom_no, is_phantom_item) yields one coherent row per distinct sub-BOM. + """ + from erpnext.manufacturing.doctype.production_plan.services.sub_assembly_queries import ( + _sub_assembly_rm_query, + ) + + rm_phantom = make_item(properties={"is_stock_item": 1, "valuation_rate": 10}).name + rm_normal = make_item(properties={"is_stock_item": 1, "valuation_rate": 10}).name + component = make_item(properties={"is_stock_item": 1, "valuation_rate": 10}).name + + # Phantom sub-BOM first (smaller auto-name); non-phantom second (larger name) -> the name + # the old Max(bom_no) would pick, while Max(is_phantom_item)=1 came from the phantom line. + 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() + normal_bom = make_bom(item=component, raw_materials=[rm_normal]) + + # Sub-assembly BOM lists `component` twice, once via each sub-BOM. + sa_item = make_item(properties={"is_stock_item": 1, "valuation_rate": 10}).name + sa_bom = make_bom(item=sa_item, raw_materials=[component], do_not_save=True) + sa_bom.items[0].bom_no = phantom_bom.name + component_doc = frappe.get_doc("Item", component) + sa_bom.append( + "items", + { + "item_code": component, + "qty": 1, + "uom": component_doc.stock_uom, + "stock_uom": component_doc.stock_uom, + "bom_no": normal_bom.name, + }, + ) + sa_bom.save() + sa_bom.submit() + + rows = _sub_assembly_rm_query( + company="_Test Company", bom_no=sa_bom.name, include_non_stock_items=1, planned_qty=1 + ) + by_bom_no = {row.bom_no: row for row in rows if row.item_code == component} + + # One coherent row per distinct sub-BOM, each carrying its own phantom flag. + self.assertIn(phantom_bom.name, by_bom_no) + self.assertIn(normal_bom.name, by_bom_no) + 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") diff --git a/erpnext/manufacturing/report/bom_stock_analysis/bom_stock_analysis.py b/erpnext/manufacturing/report/bom_stock_analysis/bom_stock_analysis.py index 95522adfdb6..e787451e57b 100644 --- a/erpnext/manufacturing/report/bom_stock_analysis/bom_stock_analysis.py +++ b/erpnext/manufacturing/report/bom_stock_analysis/bom_stock_analysis.py @@ -233,13 +233,33 @@ def get_bom_data(filters): else: query = query.where(bin.warehouse == filters.get("warehouse")) - if bom_item_table == "BOM Item": - query = query.select( - Max(bom_item.bom_no).as_("bom_no"), Max(bom_item.is_phantom_item).as_("is_phantom_item") - ) - data = query.run(as_dict=True) - return explode_phantom_boms(data, filters) if bom_item_table == "BOM Item" else data + + if bom_item_table == "BOM Item": + # bom_no + is_phantom_item drive whether/which sub-BOM explode_phantom_boms recurses into, so + # they must come from the SAME BOM Item line. Aggregating each independently (Max) could pair a + # bom_no from one line with is_phantom_item from another when an item_code repeats in the BOM. + # Rows are grouped by item_code (one qty_per_unit total per component), so pick one coherent + # representative line: the first line, but upgrade to the first phantom line if any exists, so a + # phantom sub-BOM is never dropped just because a non-phantom line happens to be listed first. + representative = {} + for line in frappe.get_all( + "BOM Item", + filters={"parent": filters.get("bom"), "parenttype": "BOM"}, + fields=["item_code", "bom_no", "is_phantom_item"], + order_by="idx", + ): + existing = representative.get(line.item_code) + if existing is None or (line.is_phantom_item and not existing.is_phantom_item): + representative[line.item_code] = line + for row in data: + line = representative.get(row.item_code) + if line: + row.bom_no = line.bom_no + row.is_phantom_item = line.is_phantom_item + return explode_phantom_boms(data, filters) + + return data def explode_phantom_boms(data, filters): diff --git a/erpnext/manufacturing/report/bom_stock_analysis/test_bom_stock_analysis.py b/erpnext/manufacturing/report/bom_stock_analysis/test_bom_stock_analysis.py index 70717377ff7..592f577b936 100644 --- a/erpnext/manufacturing/report/bom_stock_analysis/test_bom_stock_analysis.py +++ b/erpnext/manufacturing/report/bom_stock_analysis/test_bom_stock_analysis.py @@ -79,6 +79,73 @@ class TestBOMStockAnalysis(ERPNextTestSuite): ) self.assertEqual(footer.get("description"), expected_min) + def _build_duplicate_component_bom(self, phantom_first): + """Parent BOM that lists one `component` twice, once via a phantom sub-BOM and once via a + non-phantom sub-BOM. `phantom_first` controls which line is at idx 1. Returns the names of + (parent_bom, rm_phantom, rm_normal, component).""" + rm_phantom = make_item(properties={"is_stock_item": 1, "valuation_rate": 10}).name + rm_normal = make_item(properties={"is_stock_item": 1, "valuation_rate": 10}).name + component = make_item(properties={"is_stock_item": 1, "valuation_rate": 10}).name + + # Phantom sub-BOM created first -> smaller auto-name; non-phantom second -> larger name, + # which is exactly what the old Max(bom_no) would (incorrectly) pick. + 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() + normal_bom = make_bom(item=component, raw_materials=[rm_normal]) + + fg_item = make_item(properties={"is_stock_item": 1, "valuation_rate": 10}).name + first_bom, second_bom = ( + (phantom_bom.name, normal_bom.name) if phantom_first else (normal_bom.name, phantom_bom.name) + ) + parent = make_bom(item=fg_item, raw_materials=[component], do_not_save=True) + parent.items[0].bom_no = first_bom + 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, + "bom_no": second_bom, + }, + ) + parent.save() + parent.submit() + return parent.name, rm_phantom, rm_normal, component + + def _assert_phantom_exploded(self, parent_bom, rm_phantom, rm_normal, component): + raw_data = bom_stock_analysis_report(filters={"qty_to_make": 1, "bom": parent_bom})[1] + items = {row.get("item") for row in raw_data if row} + # Phantom sub-BOM exploded -> its raw material appears; the component row is replaced. + self.assertIn(rm_phantom, items) + self.assertNotIn(component, items) + # The non-phantom line's sub-BOM must NOT be mis-exploded. + self.assertNotIn(rm_normal, items) + + def test_phantom_explosion_picks_coherent_sub_bom(self): + """bom_no and is_phantom_item must come from the SAME BOM Item line. + + When a component is listed more than once in a BOM pointing at different sub-BOMs + (one phantom, one not), the report groups both lines into a single row by item_code. + Aggregating bom_no and is_phantom_item with independent Max() could pair the phantom + flag of one line with the bom_no of the other, so explode_phantom_boms recurses into + the wrong sub-BOM. We now take one coherent representative line, so the phantom sub-BOM + is the one exploded. + """ + self._assert_phantom_exploded(*self._build_duplicate_component_bom(phantom_first=True)) + + def test_phantom_explosion_when_phantom_line_is_not_first(self): + """The phantom flag must win regardless of line order. + + If the non-phantom line is listed first (idx 1) and the phantom line second, a naive + first-line representative would drop the phantom flag and skip the sub-BOM explosion. + The representative is phantom-preferring, so the phantom sub-BOM is still exploded. + """ + self._assert_phantom_exploded(*self._build_duplicate_component_bom(phantom_first=False)) + def split_data_and_footer(raw_data): """Separate component rows from the footer row. Skips blank spacer rows."""