From 5eba1ccd51488a4b5d02382b94d659c71e73e36d Mon Sep 17 00:00:00 2001 From: Saqib Date: Thu, 16 Sep 2021 17:31:37 +0530 Subject: [PATCH] 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 --- .../setup/doctype/item_group/item_group.py | 5 + .../doctype/item_group/test_records.json | 91 ++++++++++--------- erpnext/stock/doctype/item/item.py | 82 +++++++++++------ erpnext/stock/doctype/item/test_item.py | 17 ++++ 4 files changed, 123 insertions(+), 72 deletions(-) diff --git a/erpnext/setup/doctype/item_group/item_group.py b/erpnext/setup/doctype/item_group/item_group.py index b26c6a49b4cf..ab50a58c4fb8 100644 --- a/erpnext/setup/doctype/item_group/item_group.py +++ b/erpnext/setup/doctype/item_group/item_group.py @@ -39,6 +39,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) @@ -134,6 +135,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 index 146da87bddc4..ce1d718375ac 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 50fdd3845d0a..86737f29b0ca 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 @@ 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() @@ -782,35 +784,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 \ @@ -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")) diff --git a/erpnext/stock/doctype/item/test_item.py b/erpnext/stock/doctype/item/test_item.py index 0ed27610200e..e911d35db38d 100644 --- a/erpnext/stock/doctype/item/test_item.py +++ b/erpnext/stock/doctype/item/test_item.py @@ -232,6 +232,23 @@ def test_item_defaults(self): 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)