fix: don't allow BOM's item code at any level of child items (#27157)

* refactor: bom recursion checking

* fix: dont allow bom recursion

if same item_code is added in child items at any level, it shouldn't be allowed.

* test: add test for bom recursion

* test: fix broken prodplan test using recursive bom

* test: fix recursive bom in tests

erpnext/manufacturing/doctype/bom/bom.py

@@ -447,25 +447,29 @@ class BOM(WebsiteGenerator):
 				frappe.throw(_("Quantity required for Item {0} in row {1}").format(m.item_code, m.idx))
 			check_list.append(m)
 
-	def check_recursion(self, bom_list=[]):
+	def check_recursion(self, bom_list=None):
 		""" Check whether recursion occurs in any bom"""
+		def _throw_error(bom_name):
+			frappe.throw(_("BOM recursion: {0} cannot be parent or child of {0}").format(bom_name))
+
 		bom_list = self.traverse_tree()
-		bom_nos = frappe.get_all('BOM Item', fields=["bom_no"],
+		child_items = frappe.get_all('BOM Item', fields=["bom_no", "item_code"],
-			filters={'parent': ('in', bom_list), 'parenttype': 'BOM'})
+			filters={'parent': ('in', bom_list), 'parenttype': 'BOM'}) or []
 
-		raise_exception = False
+		child_bom = {d.bom_no for d in child_items}
-		if bom_nos and self.name in [d.bom_no for d in bom_nos]:
+		child_items_codes = {d.item_code for d in child_items}
-			raise_exception = True
 
-		if not raise_exception:
+		if self.name in child_bom:
-			bom_nos = frappe.get_all('BOM Item', fields=["parent"],
+			_throw_error(self.name)
-				filters={'bom_no': self.name, 'parenttype': 'BOM'})
 
-			if self.name in [d.parent for d in bom_nos]:
+		if self.item in child_items_codes:
-				raise_exception = True
+			_throw_error(self.item)
 
-		if raise_exception:
+		bom_nos = frappe.get_all('BOM Item', fields=["parent"],
-			frappe.throw(_("BOM recursion: {0} cannot be parent or child of {1}").format(self.name, self.name))
+			filters={'bom_no': self.name, 'parenttype': 'BOM'}) or []
+
+		if self.name in {d.parent for d in bom_nos}:
+			_throw_error(self.name)
 
 	def traverse_tree(self, bom_list=None):
 		def _get_children(bom_no):

erpnext/manufacturing/doctype/bom/test_bom.py

@@ -280,6 +280,46 @@ class TestBOM(unittest.TestCase):
 			self.assertEqual(reqd_item.qty, created_item.qty)
 			self.assertEqual(reqd_item.exploded_qty, created_item.exploded_qty)
 
+	def test_bom_recursion_1st_level(self):
+		"""BOM should not allow BOM item again in child"""
+		item_code = "_Test BOM Recursion"
+		make_item(item_code, {'is_stock_item': 1})
+
+		bom = frappe.new_doc("BOM")
+		bom.item = item_code
+		bom.append("items", frappe._dict(item_code=item_code))
+		with self.assertRaises(frappe.ValidationError) as err:
+			bom.save()
+
+		self.assertTrue("recursion" in str(err.exception).lower())
+		frappe.delete_doc("BOM", bom.name, ignore_missing=True)
+
+	def test_bom_recursion_transitive(self):
+		item1 = "_Test BOM Recursion"
+		item2 = "_Test BOM Recursion 2"
+		make_item(item1, {'is_stock_item': 1})
+		make_item(item2, {'is_stock_item': 1})
+
+		bom1 = frappe.new_doc("BOM")
+		bom1.item = item1
+		bom1.append("items", frappe._dict(item_code=item2))
+		bom1.save()
+		bom1.submit()
+
+		bom2 = frappe.new_doc("BOM")
+		bom2.item = item2
+		bom2.append("items", frappe._dict(item_code=item1))
+
+		with self.assertRaises(frappe.ValidationError) as err:
+			bom2.save()
+			bom2.submit()
+
+		self.assertTrue("recursion" in str(err.exception).lower())
+
+		bom1.cancel()
+		frappe.delete_doc("BOM", bom1.name, ignore_missing=True, force=True)
+		frappe.delete_doc("BOM", bom2.name, ignore_missing=True, force=True)
+
 	def test_bom_with_process_loss_item(self):
 		fg_item_non_whole, fg_item_whole, bom_item = create_process_loss_bom_items()
 

erpnext/manufacturing/doctype/production_plan/test_production_plan.py

@@ -288,6 +288,7 @@ class TestProductionPlan(unittest.TestCase):
 		self.assertEqual(warehouses, expected_warehouses)
 
 	def test_get_sales_order_with_variant(self):
+		rm_item = create_item('PIV_RM', valuation_rate = 100)
 		if not frappe.db.exists('Item', {"item_code": 'PIV'}):
 			item = create_item('PIV', valuation_rate = 100)
 			variant_settings = {
@@ -300,20 +301,20 @@ class TestProductionPlan(unittest.TestCase):
 			}
 			item.update(variant_settings)
 			item.save()
-			parent_bom = make_bom(item = 'PIV', raw_materials = ['PIV'])
+			parent_bom = make_bom(item = 'PIV', raw_materials = [rm_item.item_code])
 		if not frappe.db.exists('BOM', {"item": 'PIV'}):
-			parent_bom = make_bom(item = 'PIV', raw_materials = ['PIV'])
+			parent_bom = make_bom(item = 'PIV', raw_materials = [rm_item.item_code])
 		else:
 			parent_bom = frappe.get_doc('BOM', {"item": 'PIV'})
 
 		if not frappe.db.exists('Item', {"item_code": 'PIV-RED'}):
 			variant = create_variant("PIV", {"Colour": "Red"})
 			variant.save()
-			variant_bom = make_bom(item = variant.item_code, raw_materials = [variant.item_code])
+			variant_bom = make_bom(item = variant.item_code, raw_materials = [rm_item.item_code])
 		else:
 			variant = frappe.get_doc('Item', 'PIV-RED')
 		if not frappe.db.exists('BOM', {"item": 'PIV-RED'}):
-			variant_bom = make_bom(item = variant.item_code, raw_materials = [variant.item_code])
+			variant_bom = make_bom(item = variant.item_code, raw_materials = [rm_item.item_code])
 
 		"""Testing when item variant has a BOM"""
 		so = make_sales_order(item_code="PIV-RED", qty=5)

erpnext/manufacturing/doctype/work_order/test_work_order.py

@@ -675,13 +675,18 @@ class TestWorkOrder(unittest.TestCase):
 
 	def test_valuation_rate_missing_on_make_stock_entry(self):
 		item_name = 'Test Valuation Rate Missing'
+		rm_item = '_Test raw material item'
 		make_item(item_name, {
 			"is_stock_item": 1,
 			"include_item_in_manufacturing": 1,
 		})
+		make_item('_Test raw material item', {
+			"is_stock_item": 1,
+			"include_item_in_manufacturing": 1,
+		})
 
 		if not frappe.db.get_value('BOM', {'item': item_name}):
-			make_bom(item=item_name, raw_materials=[item_name], rm_qty=1)
+			make_bom(item=item_name, raw_materials=[rm_item], rm_qty=1)
 
 		company = "_Test Company with perpetual inventory"
 		source_warehouse = create_warehouse("Test Valuation Rate Missing Warehouse", company=company)