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>
diff --git a/erpnext/setup/doctype/item_group/item_group.py b/erpnext/setup/doctype/item_group/item_group.py
index b26c6a4..ab50a58 100644
--- a/erpnext/setup/doctype/item_group/item_group.py
+++ b/erpnext/setup/doctype/item_group/item_group.py
@@ -39,6 +39,7 @@
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 @@
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
index 146da87..ce1d718 100644
--- 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 @@
}
]
}
-]
\ No newline at end of file
+]
diff --git a/erpnext/stock/doctype/item/item.py b/erpnext/stock/doctype/item/item.py
index 50fdd38..86737f2 100644
--- a/erpnext/stock/doctype/item/item.py
+++ b/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 @@
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 @@
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 @@
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 \
@@ -1328,3 +1334,25 @@
@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"))
diff --git a/erpnext/stock/doctype/item/test_item.py b/erpnext/stock/doctype/item/test_item.py
index 0ed2761..e911d35 100644
--- a/erpnext/stock/doctype/item/test_item.py
+++ b/erpnext/stock/doctype/item/test_item.py
@@ -232,6 +232,23 @@
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)