From aed79e993f51a7b2d4379359e516705115a55fd0 Mon Sep 17 00:00:00 2001 From: Rushabh Mehta Date: Thu, 2 Jun 2016 17:49:16 +0530 Subject: [PATCH] [fix] item attribute validation, fixes #5308 --- erpnext/controllers/item_variant.py | 8 ++--- erpnext/stock/dashboard/item_dashboard.js | 2 +- erpnext/stock/doctype/item/test_item.py | 15 ++++++++- .../doctype/item_attribute/item_attribute.py | 32 +++++++++---------- 4 files changed, 34 insertions(+), 23 deletions(-) diff --git a/erpnext/controllers/item_variant.py b/erpnext/controllers/item_variant.py index 0ea0734676c..41b8b880552 100644 --- a/erpnext/controllers/item_variant.py +++ b/erpnext/controllers/item_variant.py @@ -32,13 +32,13 @@ def validate_item_variant_attributes(item, args): attribute_values = {} for t in frappe.get_all("Item Attribute Value", fields=["parent", "attribute_value"], filters={"parent": ["in", args.keys()]}): - + (attribute_values.setdefault(t.parent.lower(), [])).append(t.attribute_value) numeric_attributes = frappe._dict((t.attribute.lower(), t) for t in \ frappe.db.sql("""select attribute, from_range, to_range, increment from `tabItem Variant Attribute` where parent = %s and numeric_values=1""", (item), as_dict=1)) - + for attribute, value in args.items(): if attribute.lower() in numeric_attributes: numeric_attribute = numeric_attributes[attribute.lower()] @@ -61,10 +61,10 @@ def validate_item_variant_attributes(item, args): if not (is_in_range and is_incremental): frappe.throw(_("Value for Attribute {0} must be within the range of {1} to {2} in the increments of {3}")\ .format(attribute, from_range, to_range, increment), InvalidItemAttributeValueError) - + elif value not in attribute_values.get(attribute.lower(), []): frappe.throw(_("Value {0} for Attribute {1} does not exist in the list of valid Item Attribute Values").format( - value, attribute)) + value, attribute), InvalidItemAttributeValueError) def find_variant(template, args, variant_item_code=None): conditions = ["""(iv_attribute.attribute="{0}" and iv_attribute.attribute_value="{1}")"""\ diff --git a/erpnext/stock/dashboard/item_dashboard.js b/erpnext/stock/dashboard/item_dashboard.js index 1c7e50409df..33296ee0c23 100644 --- a/erpnext/stock/dashboard/item_dashboard.js +++ b/erpnext/stock/dashboard/item_dashboard.js @@ -65,7 +65,7 @@ erpnext.stock.ItemDashboard = Class.extend({ this.max_count = this.max_count; // show more button - if(data.length===21) { + if(data && data.length===21) { this.content.find('.more').removeClass('hidden'); // remove the last element diff --git a/erpnext/stock/doctype/item/test_item.py b/erpnext/stock/doctype/item/test_item.py index a66362df4f6..47f5162332d 100644 --- a/erpnext/stock/doctype/item/test_item.py +++ b/erpnext/stock/doctype/item/test_item.py @@ -6,7 +6,8 @@ import unittest import frappe from frappe.test_runner import make_test_records -from erpnext.controllers.item_variant import create_variant, ItemVariantExistsError, InvalidItemAttributeValueError +from erpnext.controllers.item_variant import (create_variant, ItemVariantExistsError, + InvalidItemAttributeValueError) test_ignore = ["BOM"] test_dependencies = ["Warehouse"] @@ -86,6 +87,18 @@ class TestItem(unittest.TestCase): for key, value in to_check.iteritems(): self.assertEquals(value, details.get(key)) + def test_item_attribute_change_after_variant(self): + frappe.delete_doc_if_exists("Item", "_Test Variant Item-L", force=1) + + variant = create_variant("_Test Variant Item", {"Test Size": "Large"}) + variant.save() + + attribute = frappe.get_doc('Item Attribute', 'Test Size') + attribute.item_attribute_values = [] + + self.assertRaises(InvalidItemAttributeValueError, attribute.save) + frappe.db.rollback() + def test_make_item_variant(self): frappe.delete_doc_if_exists("Item", "_Test Variant Item-L", force=1) diff --git a/erpnext/stock/doctype/item_attribute/item_attribute.py b/erpnext/stock/doctype/item_attribute/item_attribute.py index f2d53451d03..7bcb21a32f5 100644 --- a/erpnext/stock/doctype/item_attribute/item_attribute.py +++ b/erpnext/stock/doctype/item_attribute/item_attribute.py @@ -6,13 +6,27 @@ import frappe from frappe.model.document import Document from frappe import _ +from erpnext.controllers.item_variant import InvalidItemAttributeValueError + + class ItemAttributeIncrementError(frappe.ValidationError): pass class ItemAttribute(Document): + def __setup__(self): + self.flags.ignore_these_exceptions_in_test = [InvalidItemAttributeValueError] + def validate(self): self.validate_numeric() self.validate_duplication() - self.validate_attribute_values() + + def on_update(self): + self.validate_exising_items() + + def validate_exising_items(self): + '''Validate that if there are existing items with attributes, they are valid''' + for item in frappe.db.sql('''select i.name from `tabItem Variant Attribute` iva, `tabItem` i + where iva.attribute = %s and iva.parent = i.name and i.has_variants = 0''', self.name): + frappe.get_doc('Item', item[0]).validate_variant_attributes() def validate_numeric(self): if self.numeric_values: @@ -39,19 +53,3 @@ class ItemAttribute(Document): if d.abbr in abbrs: frappe.throw(_("{0} must appear only once").format(d.abbr)) abbrs.append(d.abbr) - - def validate_attribute_values(self): - # don't validate numeric values - if self.numeric_values: - return - - attribute_values = [] - for d in self.item_attribute_values: - attribute_values.append(d.attribute_value) - - variant_attributes = frappe.db.sql("select DISTINCT attribute_value from `tabItem Variant Attribute` where attribute=%s", self.name) - if variant_attributes: - for d in variant_attributes: - if d[0] and d[0] not in attribute_values: - frappe.throw(_("Attribute Value {0} cannot be removed from {1} as Item Variants \ - exist with this Attribute.").format(d[0], self.name))