fix: allow non-explosive recrusive BOMs

Recursion should be allowed as long as child item is not "exploded"
further by a BOM.

erpnext/manufacturing/doctype/bom/bom.py

@@ -22,6 +22,10 @@
 form_grid_templates = {"items": "templates/form_grid/item_grid.html"}
 
 
+class BOMRecursionError(frappe.ValidationError):
+	pass
+
+
 class BOMTree:
 	"""Full tree representation of a BOM"""
 
@@ -553,35 +557,27 @@ 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))
+			frappe.throw(
+				_("BOM recursion: {1} cannot be parent or child of {0}").format(self.name, bom_name),
+				exc=BOMRecursionError,
+			)
 
 		bom_list = self.traverse_tree()
-		child_items = (
-			frappe.get_all(
-				"BOM Item",
-				fields=["bom_no", "item_code"],
-				filters={"parent": ("in", bom_list), "parenttype": "BOM"},
-			)
-			or []
+		child_items = frappe.get_all(
+			"BOM Item",
+			fields=["bom_no", "item_code"],
+			filters={"parent": ("in", bom_list), "parenttype": "BOM"},
 		)
 
-		child_bom = {d.bom_no for d in child_items}
-		child_items_codes = {d.item_code for d in child_items}
-
-		if self.name in child_bom:
-			_throw_error(self.name)
-
-		if self.item in child_items_codes:
-			_throw_error(self.item)
-
-		bom_nos = (
-			frappe.get_all(
-				"BOM Item", fields=["parent"], filters={"bom_no": self.name, "parenttype": "BOM"}
-			)
-			or []
-		)
+		for item in child_items:
+			if self.name == item.bom_no:
+				_throw_error(self.name)
+			if self.item == item.item_code and item.bom_no:
+				# Same item but with different BOM should not be allowed.
+				# Same item can appear recursively once as long as it doesn't have BOM.
+				_throw_error(item.bom_no)
 
-		if self.name in {d.parent for d in bom_nos}:
+		if self.name in {d.bom_no for d in self.items}:
 			_throw_error(self.name)
 
 	def traverse_tree(self, bom_list=None):

erpnext/manufacturing/doctype/bom/test_bom.py

@@ -10,7 +10,7 @@
 from frappe.utils import cstr, flt
 
 from erpnext.buying.doctype.purchase_order.test_purchase_order import create_purchase_order
-from erpnext.manufacturing.doctype.bom.bom import item_query
+from erpnext.manufacturing.doctype.bom.bom import BOMRecursionError, item_query
 from erpnext.manufacturing.doctype.bom_update_tool.bom_update_tool import update_cost
 from erpnext.stock.doctype.item.test_item import make_item
 from erpnext.stock.doctype.stock_reconciliation.test_stock_reconciliation import (
@@ -259,43 +259,36 @@ def test_subcontractor_sourced_item(self):
 
 	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})
+		item_code = make_item(properties={"is_stock_item": 1}).name
 
 		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()
+		with self.assertRaises(BOMRecursionError):
+			bom.items[0].bom_no = bom.name
 			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})
+		item1 = make_item(properties={"is_stock_item": 1}).name
+		item2 = make_item(properties={"is_stock_item": 1}).name
 
 		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))
+		bom2.save()
 
-		with self.assertRaises(frappe.ValidationError) as err:
-			bom2.save()
-			bom2.submit()
+		bom2.items[0].bom_no = bom1.name
+		bom1.items[0].bom_no = bom2.name
 
-		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)
+		with self.assertRaises(BOMRecursionError):
+			bom1.save()
+			bom2.save()
 
 	def test_bom_with_process_loss_item(self):
 		fg_item_non_whole, fg_item_whole, bom_item = create_process_loss_bom_items()