From 41da9eb7fc355ee2eb4f0df6b200355652939f0a Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Thu, 18 Jun 2026 16:14:30 +0530 Subject: [PATCH 1/3] fix(manufacturing): keep bom_no/is_phantom_item pair coherent in sub-BOM resolution When a component is listed more than once in a BOM pointing at different sub-BOMs (e.g. one phantom, one not), two queries grouped the duplicate lines into a single row and aggregated bom_no and is_phantom_item with *independent* Max(). That could pair the phantom flag of one line with the bom_no of another, so the consumer recursed into the wrong sub-BOM: - sub_assembly_queries._sub_assembly_rm_query keys on (item_code, bom_no) and recurses on is_phantom_item. An incoherent pair sent raw-material resolution down the wrong sub-assembly BOM. - bom_stock_analysis.get_bom_data: explode_phantom_boms recurses into bom_no only when is_phantom_item is set; an incoherent pair exploded a non-phantom sub-BOM as if it were phantom (or vice-versa). Fix: - sub_assembly_queries: group also by (bom_no, is_phantom_item) so each distinct sub-BOM is its own coherent row. - bom_stock_analysis: drop the two independent Max()es and attach a single representative line (lowest idx) per item_code before exploding. This was previously undefined SQL (loose GROUP BY); the fix makes MariaDB and Postgres agree on a deterministic, coherent pairing. Other Max()-wrapped columns are functionally dependent on the grouped item and keep their value on both engines. Tests (fail on the old code, pass on both engines): - test_phantom_explosion_picks_coherent_sub_bom: duplicate-component BOM explodes the phantom sub-BOM, not the lexically-larger non-phantom one. - test_sub_assembly_rm_query_keeps_bom_no_phantom_pair_coherent: the query returns one coherent row per distinct sub-BOM with the right phantom flag. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../services/sub_assembly_queries.py | 15 +++-- .../production_plan/test_production_plan.py | 55 +++++++++++++++++++ .../bom_stock_analysis/bom_stock_analysis.py | 28 ++++++++-- .../test_bom_stock_analysis.py | 49 +++++++++++++++++ 4 files changed, 135 insertions(+), 12 deletions(-) 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..c8073ba5892 100644 --- a/erpnext/manufacturing/doctype/production_plan/test_production_plan.py +++ b/erpnext/manufacturing/doctype/production_plan/test_production_plan.py @@ -2862,6 +2862,61 @@ 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 create_production_plan(**args): """ 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..51450599084 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,29 @@ 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. + # Take the first (lowest idx) line per item_code as the coherent representative. + first_line = {} + 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", + ): + first_line.setdefault(line.item_code, line) + for row in data: + line = first_line.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..e6dc760ff6b 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,55 @@ class TestBOMStockAnalysis(ERPNextTestSuite): ) self.assertEqual(footer.get("description"), expected_min) + 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 (lowest idx), so the + phantom sub-BOM is the one exploded. + """ + 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]) + + # Parent lists `component` twice: phantom line first (idx 1), non-phantom second (idx 2). + 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 + 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": normal_bom.name, + }, + ) + parent.save() + parent.submit() + + raw_data = bom_stock_analysis_report(filters={"qty_to_make": 1, "bom": parent.name})[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 split_data_and_footer(raw_data): """Separate component rows from the footer row. Skips blank spacer rows.""" From b579dbc1e60c7c56dfcb6d9ee2f196cee03c10f4 Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Thu, 18 Jun 2026 16:27:23 +0530 Subject: [PATCH 2/3] fix(manufacturing): prefer the phantom line as bom_stock_analysis representative Address review on #56090: get_bom_data groups components by item_code, so it picks one representative BOM Item line for the (bom_no, is_phantom_item) pair. Taking the first line by idx dropped the phantom flag when a non-phantom line was listed before the phantom one, so explode_phantom_boms skipped the sub-BOM. Keep one row per item_code (preserving the qty_per_unit total per component rather than widening the GROUP BY), but make the representative phantom- preferring: the first line, upgraded to the first phantom line if any exists. A phantom sub-BOM is therefore never dropped due to line order, on either engine. Adds test_phantom_explosion_when_phantom_line_is_not_first (phantom line at idx 2) alongside the existing idx-1 case; both pass on MariaDB and Postgres and the new one fails on the naive first-line representative. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../bom_stock_analysis/bom_stock_analysis.py | 12 +++-- .../test_bom_stock_analysis.py | 48 +++++++++++++------ 2 files changed, 41 insertions(+), 19 deletions(-) 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 51450599084..e787451e57b 100644 --- a/erpnext/manufacturing/report/bom_stock_analysis/bom_stock_analysis.py +++ b/erpnext/manufacturing/report/bom_stock_analysis/bom_stock_analysis.py @@ -239,17 +239,21 @@ def get_bom_data(filters): # 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. - # Take the first (lowest idx) line per item_code as the coherent representative. - first_line = {} + # 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", ): - first_line.setdefault(line.item_code, line) + 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 = first_line.get(row.item_code) + line = representative.get(row.item_code) if line: row.bom_no = line.bom_no row.is_phantom_item = line.is_phantom_item 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 e6dc760ff6b..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,16 +79,10 @@ class TestBOMStockAnalysis(ERPNextTestSuite): ) self.assertEqual(footer.get("description"), expected_min) - 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 (lowest idx), so the - phantom sub-BOM is the one exploded. - """ + 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 @@ -101,10 +95,12 @@ class TestBOMStockAnalysis(ERPNextTestSuite): phantom_bom.submit() normal_bom = make_bom(item=component, raw_materials=[rm_normal]) - # Parent lists `component` twice: phantom line first (idx 1), non-phantom second (idx 2). 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 = phantom_bom.name + parent.items[0].bom_no = first_bom component_doc = frappe.get_doc("Item", component) parent.append( "items", @@ -113,21 +109,43 @@ class TestBOMStockAnalysis(ERPNextTestSuite): "qty": 1, "uom": component_doc.stock_uom, "stock_uom": component_doc.stock_uom, - "bom_no": normal_bom.name, + "bom_no": second_bom, }, ) parent.save() parent.submit() + return parent.name, rm_phantom, rm_normal, component - raw_data = bom_stock_analysis_report(filters={"qty_to_make": 1, "bom": parent.name})[1] + 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.""" From fe0465f16ef1e6415c217bcb1809ddd9c94d6fd9 Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Thu, 18 Jun 2026 19:03:26 +0530 Subject: [PATCH 3/3] fix(manufacturing): deterministic arbitrary-pick in BOM explosion & WO transfer tracking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same class as the sub_assembly_queries / bom_stock_analysis fixes in this PR: two more merged queries wrapped a non-functionally-dependent column in Max(), so the engines can disagree and the value is wrong for the case the column drives. bom_explosion._subitems_query — Max(is_phantom_item): Rows are grouped 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() returns 1 and the real raw material is silently dropped from the plan. Use Min(): an item is phantom only when EVERY line for it is phantom, so a real material is never lost. required_items._material_transfer_qty_by_item — Max(original_item): original_item is the output dict key. The same item B can be transferred both for itself (original_item NULL) and as a substitute for required item A (original_item=A). Grouping by item_code alone with Max() merged the two and credited B's whole transfer to A, leaving B at 0. Group by (item_code, original_item) and accumulate into the keyed dict so each transfer is credited to the right required item (two rows can resolve to one key, e.g. A's own transfer and B-for-A, hence += not plain assignment). Both were previously undefined SQL (loose GROUP BY); the fix makes MariaDB and Postgres agree on the correct, deterministic value. Other Max()-wrapped columns in these queries are functionally dependent on the grouped item and unchanged. Tests (fail on the old code, pass on both engines): - test_subitems_query_keeps_real_rm_listed_alongside_phantom - test_transferred_qty_not_misattributed_between_item_and_its_substitute Co-Authored-By: Claude Opus 4.8 (1M context) --- .../production_plan/services/bom_explosion.py | 11 ++-- .../production_plan/test_production_plan.py | 43 ++++++++++++++++ .../work_order/services/required_items.py | 18 +++++-- .../doctype/work_order/test_work_order.py | 51 +++++++++++++++++++ 4 files changed, 114 insertions(+), 9 deletions(-) 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")