Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: no validation on item defaults #27393

Merged
merged 7 commits into from
Sep 16, 2021
Merged

fix: no validation on item defaults #27393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f3f598a
fix: no validation on item defaults
nextchamp-saqib Sep 8, 2021
1667792
fix: cache value while validating
ankush Sep 8, 2021
aad1282
test: item default company relation
ankush Sep 8, 2021
88b2997
fix: reorder validations
ankush Sep 8, 2021
5059d5e
refactor: add guard conditions on update_defaults
ankush Sep 16, 2021
da9f81a
test: add default warehouse for item group
ankush Sep 16, 2021
9c73b92
fix: validate item defaults for item groups
ankush Sep 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions erpnext/setup/doctype/item_group/item_group.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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:
Expand Down
91 changes: 46 additions & 45 deletions erpnext/setup/doctype/item_group/test_records.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"
},
{
Expand Down Expand Up @@ -104,4 +105,4 @@
}
]
}
]
]
82 changes: 55 additions & 27 deletions erpnext/stock/doctype/item/item.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import copy
import itertools
import json
from typing import List

import frappe
from frappe import _
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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 \
Expand Down Expand Up @@ -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"))
17 changes: 17 additions & 0 deletions erpnext/stock/doctype/item/test_item.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down