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>

erpnext/setup/doctype/item_group/item_group.py

@@ -39,6 +39,7 @@ class ItemGroup(NestedSet, WebsiteGenerator):
 				self.parent_item_group = _('All Item Groups')
 
 		self.make_route()
+		self.validate_item_group_defaults()
 
 	def on_update(self):
 		NestedSet.on_update(self)
@@ -134,6 +135,10 @@ class ItemGroup(NestedSet, WebsiteGenerator):
 	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:

erpnext/setup/doctype/item_group/test_records.json

@@ -7,7 +7,8 @@
   "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"
 	}]
  },
  {

erpnext/stock/doctype/item/item.py

@@ -4,6 +4,7 @@
 import copy
 import itertools
 import json
+from typing import List
 
 import frappe
 from frappe import _
@@ -36,6 +37,7 @@ from erpnext.setup.doctype.item_group.item_group import (
 	get_parent_item_groups,
 	invalidate_cache_for,
 )
+from erpnext.stock.doctype.item_default.item_default import ItemDefault
 
 
 class DuplicateReorderRows(frappe.ValidationError):
@@ -134,9 +136,9 @@ class Item(WebsiteGenerator):
 		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()
@@ -782,35 +784,39 @@ class Item(WebsiteGenerator):
 		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:
+		if self.item_defaults or not self.item_group:
-			item_defaults = frappe.db.get_values("Item Default", {"parent": self.item_group},
+			return
-				['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 {}
 
-				# To check default warehouse is belong to the default company
+		item_defaults = frappe.db.get_values("Item Default", {"parent": self.item_group},
-				if defaults.get("default_warehouse") and defaults.company and frappe.db.exists("Warehouse",
+			['company', 'default_warehouse','default_price_list','buying_cost_center','default_supplier',
-					{'name': defaults.default_warehouse, 'company': defaults.company}):
+			'expense_account','selling_cost_center','income_account'], as_dict = 1)
-						self.append("item_defaults", {
+		if item_defaults:
-							"company": defaults.get("company"),
+			for item in item_defaults:
-							"default_warehouse": defaults.default_warehouse
+				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 \
@@ -1328,3 +1334,25 @@ def on_doctype_update():
 @erpnext.allow_regional
 def set_item_tax_from_hsn_code(item):
 	pass
+
+
+def validate_item_default_company_links(item_defaults: List[ItemDefault]) -> None:
+	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"))

erpnext/stock/doctype/item/test_item.py

@@ -232,6 +232,23 @@ class TestItem(unittest.TestCase):
 		for key, value in purchase_item_check.items():
 			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)