Merge pull request #56090 from mihir-kandoi/pg-bom-rowshape

fix(manufacturing): make arbitrary GROUP-BY picks deterministic in BOM & transfer queries

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"),
 	]
 
 

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"),

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):
 	"""

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 (

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")

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):

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."""