fix: no validation on item defaults (#27548)

* fix: no validation on item defaults (#27393) * fix: no validation on item defaults * fix: cache value while validating * test: item default company relation * fix: reorder validations * refactor: add guard conditions on update_defaults * test: add default warehouse for item group * fix: validate item defaults for item groups Co-authored-by: Ankush Menat <ankush@iwebnotes.com> (cherry picked from commit 5eba1cc) # Conflicts: # erpnext/stock/doctype/item/item.py * fix: resolve conflict and remove typehints for py2 Co-authored-by: Saqib <nextchamp.saqib@gmail.com> Co-authored-by: Ankush Menat <ankush@iwebnotes.com>
frappe · Sep 16, 2021 · efddcbe · efddcbe
1 parent 6f6e390
commit efddcbe
Show file tree

Hide file tree

Showing 4 changed files with 121 additions and 72 deletions.
diff --git a/erpnext/setup/doctype/item_group/item_group.py b/erpnext/setup/doctype/item_group/item_group.py
@@ -33,6 +33,7 @@ def validate(self):
 				self.parent_item_group = _('All Item Groups')
 
 		self.make_route()
+		self.validate_item_group_defaults()
 
 	def on_update(self):
 		NestedSet.on_update(self)
@@ -88,6 +89,10 @@ def get_context(self, context):
 	def delete_child_item_groups_key(self):
 		frappe.cache().hdel("child_item_groups", self.name)
 
+	def validate_item_group_defaults(self):
+		from erpnext.stock.doctype.item.item import validate_item_default_company_links
+		validate_item_default_company_links(self.item_group_defaults)
+
 @frappe.whitelist(allow_guest=True)
 def get_product_list_for_group(product_group=None, start=0, limit=10, search=None):
 	if product_group:

diff --git a/erpnext/setup/doctype/item_group/test_records.json b/erpnext/setup/doctype/item_group/test_records.json
@@ -1,73 +1,74 @@
 [
  {
-  "doctype": "Item Group", 
-  "is_group": 0, 
-  "item_group_name": "_Test Item Group", 
+  "doctype": "Item Group",
+  "is_group": 0,
+  "item_group_name": "_Test Item Group",
   "parent_item_group": "All Item Groups",
   "item_group_defaults": [{
 		"company": "_Test Company",
 		"buying_cost_center": "_Test Cost Center 2 - _TC",
-		"selling_cost_center": "_Test Cost Center 2 - _TC"
+		"selling_cost_center": "_Test Cost Center 2 - _TC",
+		"default_warehouse": "_Test Warehouse - _TC"
 	}]
- }, 
+ },
  {
-  "doctype": "Item Group", 
-  "is_group": 0, 
-  "item_group_name": "_Test Item Group Desktops", 
+  "doctype": "Item Group",
+  "is_group": 0,
+  "item_group_name": "_Test Item Group Desktops",
   "parent_item_group": "All Item Groups"
- }, 
+ },
  {
-  "doctype": "Item Group", 
-  "is_group": 1, 
-  "item_group_name": "_Test Item Group A", 
+  "doctype": "Item Group",
+  "is_group": 1,
+  "item_group_name": "_Test Item Group A",
   "parent_item_group": "All Item Groups"
- }, 
+ },
  {
-  "doctype": "Item Group", 
-  "is_group": 1, 
-  "item_group_name": "_Test Item Group B", 
+  "doctype": "Item Group",
+  "is_group": 1,
+  "item_group_name": "_Test Item Group B",
   "parent_item_group": "All Item Groups"
- }, 
+ },
  {
-  "doctype": "Item Group", 
-  "is_group": 1, 
-  "item_group_name": "_Test Item Group B - 1", 
+  "doctype": "Item Group",
+  "is_group": 1,
+  "item_group_name": "_Test Item Group B - 1",
   "parent_item_group": "_Test Item Group B"
- }, 
+ },
  {
-  "doctype": "Item Group", 
-  "is_group": 1, 
-  "item_group_name": "_Test Item Group B - 2", 
+  "doctype": "Item Group",
+  "is_group": 1,
+  "item_group_name": "_Test Item Group B - 2",
   "parent_item_group": "_Test Item Group B"
- }, 
+ },
  {
-  "doctype": "Item Group", 
-  "is_group": 0, 
-  "item_group_name": "_Test Item Group B - 3", 
+  "doctype": "Item Group",
+  "is_group": 0,
+  "item_group_name": "_Test Item Group B - 3",
   "parent_item_group": "_Test Item Group B"
- }, 
+ },
  {
-  "doctype": "Item Group", 
-  "is_group": 1, 
-  "item_group_name": "_Test Item Group C", 
+  "doctype": "Item Group",
+  "is_group": 1,
+  "item_group_name": "_Test Item Group C",
   "parent_item_group": "All Item Groups"
- }, 
+ },
  {
-  "doctype": "Item Group", 
-  "is_group": 1, 
-  "item_group_name": "_Test Item Group C - 1", 
+  "doctype": "Item Group",
+  "is_group": 1,
+  "item_group_name": "_Test Item Group C - 1",
   "parent_item_group": "_Test Item Group C"
- }, 
+ },
  {
-  "doctype": "Item Group", 
-  "is_group": 1, 
-  "item_group_name": "_Test Item Group C - 2", 
+  "doctype": "Item Group",
+  "is_group": 1,
+  "item_group_name": "_Test Item Group C - 2",
   "parent_item_group": "_Test Item Group C"
- }, 
+ },
  {
-  "doctype": "Item Group", 
-  "is_group": 1, 
-  "item_group_name": "_Test Item Group D", 
+  "doctype": "Item Group",
+  "is_group": 1,
+  "item_group_name": "_Test Item Group D",
   "parent_item_group": "All Item Groups"
  },
  {
@@ -104,4 +105,4 @@
    }
   ]
  }
-]
+]
diff --git a/erpnext/stock/doctype/item/item.py b/erpnext/stock/doctype/item/item.py
@@ -6,6 +6,7 @@
 import itertools
 import json
 import erpnext
+
 import frappe
 import copy
 from erpnext.controllers.item_variant import (ItemVariantExistsError,
@@ -121,9 +122,9 @@ def validate(self):
 		self.validate_fixed_asset()
 		self.validate_retain_sample()
 		self.validate_uom_conversion_factor()
-		self.validate_item_defaults()
 		self.validate_customer_provided_part()
 		self.update_defaults_from_item_group()
+		self.validate_item_defaults()
 		self.validate_auto_reorder_enabled_in_stock_settings()
 		self.cant_change()
 		self.update_show_in_website()
@@ -758,35 +759,39 @@ def validate_item_defaults(self):
 		if len(companies) != len(self.item_defaults):
 			frappe.throw(_("Cannot set multiple Item Defaults for a company."))
 
+		validate_item_default_company_links(self.item_defaults)
+
+
 	def update_defaults_from_item_group(self):
 		"""Get defaults from Item Group"""
-		if self.item_group and not self.item_defaults:
-			item_defaults = frappe.db.get_values("Item Default", {"parent": self.item_group},
-				['company', 'default_warehouse','default_price_list','buying_cost_center','default_supplier',
-				'expense_account','selling_cost_center','income_account'], as_dict = 1)
-			if item_defaults:
-				for item in item_defaults:
-					self.append('item_defaults', {
-						'company': item.company,
-						'default_warehouse': item.default_warehouse,
-						'default_price_list': item.default_price_list,
-						'buying_cost_center': item.buying_cost_center,
-						'default_supplier': item.default_supplier,
-						'expense_account': item.expense_account,
-						'selling_cost_center': item.selling_cost_center,
-						'income_account': item.income_account
-					})
-			else:
-				warehouse = ''
-				defaults = frappe.defaults.get_defaults() or {}
+		if self.item_defaults or not self.item_group:
+			return
 
-				# To check default warehouse is belong to the default company
-				if defaults.get("default_warehouse") and defaults.company and frappe.db.exists("Warehouse",
-					{'name': defaults.default_warehouse, 'company': defaults.company}):
-						self.append("item_defaults", {
-							"company": defaults.get("company"),
-							"default_warehouse": defaults.default_warehouse
-						})
+		item_defaults = frappe.db.get_values("Item Default", {"parent": self.item_group},
+			['company', 'default_warehouse','default_price_list','buying_cost_center','default_supplier',
+			'expense_account','selling_cost_center','income_account'], as_dict = 1)
+		if item_defaults:
+			for item in item_defaults:
+				self.append('item_defaults', {
+					'company': item.company,
+					'default_warehouse': item.default_warehouse,
+					'default_price_list': item.default_price_list,
+					'buying_cost_center': item.buying_cost_center,
+					'default_supplier': item.default_supplier,
+					'expense_account': item.expense_account,
+					'selling_cost_center': item.selling_cost_center,
+					'income_account': item.income_account
+				})
+		else:
+			defaults = frappe.defaults.get_defaults() or {}
+
+			# To check default warehouse is belong to the default company
+			if defaults.get("default_warehouse") and defaults.company and frappe.db.exists("Warehouse",
+				{'name': defaults.default_warehouse, 'company': defaults.company}):
+					self.append("item_defaults", {
+						"company": defaults.get("company"),
+						"default_warehouse": defaults.default_warehouse
+					})
 
 	def update_variants(self):
 		if self.flags.dont_update_variants or \
@@ -1237,3 +1242,24 @@ def update_variants(variants, template, publish_progress=True):
 def on_doctype_update():
 	# since route is a Text column, it needs a length for indexing
 	frappe.db.add_index("Item", ["route(500)"])
+
+def validate_item_default_company_links(item_defaults):
+	for item_default in item_defaults:
+		for doctype, field in [
+			['Warehouse', 'default_warehouse'],
+			['Cost Center', 'buying_cost_center'],
+			['Cost Center', 'selling_cost_center'],
+			['Account', 'expense_account'],
+			['Account', 'income_account']
+		]:
+			if item_default.get(field):
+				company = frappe.db.get_value(doctype, item_default.get(field), 'company', cache=True)
+				if company and company != item_default.company:
+					frappe.throw(_("Row #{}: {} {} doesn't belong to Company {}. Please select valid {}.")
+						.format(
+							item_default.idx,
+							doctype,
+							frappe.bold(item_default.get(field)),
+							frappe.bold(item_default.company),
+							frappe.bold(frappe.unscrub(field))
+						), title=_("Invalid Item Defaults"))
diff --git a/erpnext/stock/doctype/item/test_item.py b/erpnext/stock/doctype/item/test_item.py
@@ -219,6 +219,23 @@ def test_item_defaults(self):
 		for key, value in iteritems(purchase_item_check):
 			self.assertEqual(value, purchase_item_details.get(key))
 
+	def test_item_default_validations(self):
+
+		with self.assertRaises(frappe.ValidationError) as ve:
+			make_item("Bad Item defaults", {
+				"item_group": "_Test Item Group",
+				"item_defaults": [{
+					"company": "_Test Company 1",
+					"default_warehouse": "_Test Warehouse - _TC",
+					"expense_account": "Stock In Hand - _TC",
+					"buying_cost_center": "_Test Cost Center - _TC",
+					"selling_cost_center": "_Test Cost Center - _TC",
+				}]
+			})
+
+		self.assertTrue("belong to company" in str(ve.exception).lower(),
+				msg="Mismatching company entities in item defaults should not be allowed.")
+
 	def test_item_attribute_change_after_variant(self):
 		frappe.delete_doc_if_exists("Item", "_Test Variant Item-L", force=1)