From b001f0db9ebf1e0610d046ef194a876ac2f91ca3 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Thu, 26 Aug 2021 18:33:42 +0530 Subject: [PATCH] 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 (cherry picked from commit c07dce940e31f84fb38ea6fd8d2d41bef619f904) # Conflicts: # erpnext/manufacturing/doctype/bom/bom.py # erpnext/manufacturing/doctype/production_plan/test_production_plan.py # erpnext/manufacturing/doctype/work_order/test_work_order.py --- erpnext/manufacturing/doctype/bom/bom.py | 24 +++++++++++ erpnext/manufacturing/doctype/bom/test_bom.py | 40 +++++++++++++++++++ .../production_plan/test_production_plan.py | 20 ++++++++++ .../doctype/work_order/test_work_order.py | 15 +++++++ 4 files changed, 99 insertions(+) diff --git a/erpnext/manufacturing/doctype/bom/bom.py b/erpnext/manufacturing/doctype/bom/bom.py index 1a56116718d..26eb0dd9508 100644 --- a/erpnext/manufacturing/doctype/bom/bom.py +++ b/erpnext/manufacturing/doctype/bom/bom.py @@ -536,6 +536,7 @@ class BOM(WebsiteGenerator): check_list.append(m) def check_recursion(self, bom_list=None): +<<<<<<< HEAD """Check whether recursion occurs in any bom""" def _throw_error(bom_name): @@ -560,6 +561,29 @@ class BOM(WebsiteGenerator): _throw_error(item.bom_no) if self.name in {d.bom_no for d in self.items}: +======= + """ 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() + child_items = frappe.get_all('BOM Item', fields=["bom_no", "item_code"], + filters={'parent': ('in', bom_list), 'parenttype': 'BOM'}) or [] + + 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 [] + + if self.name in {d.parent for d in bom_nos}: +>>>>>>> c07dce940e (fix: don't allow BOM's item code at any level of child items (#27157)) _throw_error(self.name) def traverse_tree(self, bom_list=None): diff --git a/erpnext/manufacturing/doctype/bom/test_bom.py b/erpnext/manufacturing/doctype/bom/test_bom.py index 6425769cf38..03fb61520ba 100644 --- a/erpnext/manufacturing/doctype/bom/test_bom.py +++ b/erpnext/manufacturing/doctype/bom/test_bom.py @@ -297,6 +297,46 @@ class TestBOM(FrappeTestCase): bom1.save() bom2.save() + 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() diff --git a/erpnext/manufacturing/doctype/production_plan/test_production_plan.py b/erpnext/manufacturing/doctype/production_plan/test_production_plan.py index 8cd79202dd8..cb72f0ac55b 100644 --- a/erpnext/manufacturing/doctype/production_plan/test_production_plan.py +++ b/erpnext/manufacturing/doctype/production_plan/test_production_plan.py @@ -376,10 +376,16 @@ class TestProductionPlan(FrappeTestCase): self.assertEqual(warehouses, expected_warehouses) def test_get_sales_order_with_variant(self): +<<<<<<< HEAD "Check if Template BOM is fetched in absence of Variant BOM." 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) +======= + 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) +>>>>>>> c07dce940e (fix: don't allow BOM's item code at any level of child items (#27157)) variant_settings = { "attributes": [ {"attribute": "Colour"}, @@ -388,20 +394,34 @@ class TestProductionPlan(FrappeTestCase): } item.update(variant_settings) item.save() +<<<<<<< HEAD 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=[rm_item.item_code]) +======= + 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 = [rm_item.item_code]) +>>>>>>> c07dce940e (fix: don't allow BOM's item code at any level of child items (#27157)) 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() +<<<<<<< HEAD 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=[rm_item.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 = [rm_item.item_code]) +>>>>>>> c07dce940e (fix: don't allow BOM's item code at any level of child items (#27157)) """Testing when item variant has a BOM""" so = make_sales_order(item_code="PIV-RED", qty=5) diff --git a/erpnext/manufacturing/doctype/work_order/test_work_order.py b/erpnext/manufacturing/doctype/work_order/test_work_order.py index c5c9b5f0d31..80dbed4b273 100644 --- a/erpnext/manufacturing/doctype/work_order/test_work_order.py +++ b/erpnext/manufacturing/doctype/work_order/test_work_order.py @@ -801,6 +801,7 @@ class TestWorkOrder(FrappeTestCase): self.assertEqual(item.valuation_rate, 0) def test_valuation_rate_missing_on_make_stock_entry(self): +<<<<<<< HEAD item_name = "Test Valuation Rate Missing" rm_item = "_Test raw material item" make_item( @@ -819,6 +820,20 @@ class TestWorkOrder(FrappeTestCase): ) if not frappe.db.get_value("BOM", {"item": item_name}): +======= + 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}): +>>>>>>> c07dce940e (fix: don't allow BOM's item code at any level of child items (#27157)) make_bom(item=item_name, raw_materials=[rm_item], rm_qty=1) company = "_Test Company with perpetual inventory"