[fix] item attribute validation and tests

erpnext/controllers/item_variant.py

@@ -24,22 +24,23 @@ def get_variant(template, args, variant=None):
 	if not args:
 		frappe.throw(_("Please specify at least one attribute in the Attributes table"))
 
-	validate_item_variant_attributes(template, args)
-
 	return find_variant(template, args, variant)
 
 def validate_item_variant_attributes(item, args=None):
+	if isinstance(item, basestring):
+		item = frappe.get_doc('Item', item)
+
 	if not args:
 		args = {d.attribute.lower():d.attribute_value for d in item.attributes}
 
-	attribute_values = get_attribute_values()
+	attribute_values, numeric_values = get_attribute_values()
-
-	numeric_attributes = frappe._dict({d.attribute.lower(): d for d
-		in item.attributes if d.numeric_values==1})
 
 	for attribute, value in args.items():
-		if attribute.lower() in numeric_attributes:
+		if not value:
-			numeric_attribute = numeric_attributes[attribute.lower()]
+			continue
+
+		if attribute.lower() in numeric_values:
+			numeric_attribute = numeric_values[attribute.lower()]
 
 			from_range = numeric_attribute.from_range
 			to_range = numeric_attribute.to_range
@@ -57,22 +58,28 @@ def validate_item_variant_attributes(item, args=None):
 			is_incremental = remainder==0 or remainder==increment
 
 			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}")\
+				frappe.throw(_("Value for Attribute {0} must be within the range of {1} to {2} in the increments of {3} for Item {2}")\
-					.format(attribute, from_range, to_range, increment), InvalidItemAttributeValueError)
+					.format(attribute, from_range, to_range, increment, item.name), 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(
+			frappe.throw(_("Value {0} for Attribute {1} does not exist in the list of valid Item Attribute Values for Item {2}").format(
-				value, attribute), InvalidItemAttributeValueError)
+				value, attribute, item.name), InvalidItemAttributeValueError)
 
 def get_attribute_values():
 	if not frappe.flags.attribute_values:
 		attribute_values = {}
+		numeric_values = {}
 		for t in frappe.get_all("Item Attribute Value", fields=["parent", "attribute_value"]):
-			(attribute_values.setdefault(t.parent.lower(), [])).append(t.attribute_value)
+			attribute_values.setdefault(t.parent.lower(), []).append(t.attribute_value)
+
+		for t in frappe.get_all('Item Attribute',
+			fields=["name", "from_range", "to_range", "increment"], filters={'numeric_values': 1}):
+			numeric_values[t.name.lower()] = t
 
 		frappe.flags.attribute_values = attribute_values
+		frappe.flags.numeric_values = numeric_values
 
-	return frappe.flags.attribute_values
+	return frappe.flags.attribute_values, frappe.flags.numeric_values
 
 def find_variant(template, args, variant_item_code=None):
 	conditions = ["""(iv_attribute.attribute="{0}" and iv_attribute.attribute_value="{1}")"""\
@@ -126,7 +133,7 @@ def create_variant(item, args):
 
 	variant.set("attributes", variant_attributes)
 	copy_attributes_to_variant(template, variant)
-	make_variant_item_code(template, variant)
+	make_variant_item_code(template.item_code, variant)
 
 	return variant
 
@@ -144,7 +151,7 @@ def copy_attributes_to_variant(item, variant):
 		for d in variant.attributes:
 			variant.description += "<p>" + d.attribute + ": " + cstr(d.attribute_value) + "</p>"
 
-def make_variant_item_code(template, variant):
+def make_variant_item_code(template_item_code, variant):
 	"""Uses template's item code and abbreviations to make variant's item code"""
 	if variant.item_code:
 		return
@@ -160,8 +167,10 @@ def make_variant_item_code(template, variant):
 			}, as_dict=True)
 
 		if not item_attribute:
-			# somehow an invalid item attribute got used
 			return
+			# frappe.throw(_('Invalid attribute {0} {1}').format(frappe.bold(attr.attribute),
+			# 	frappe.bold(attr.attribute_value)), title=_('Invalid Attribute'),
+			# 	exc=InvalidItemAttributeValueError)
 
 		if item_attribute[0].numeric_values:
 			# don't generate item code if one of the attributes is numeric
@@ -170,7 +179,7 @@ def make_variant_item_code(template, variant):
 		abbreviations.append(item_attribute[0].abbr)
 
 	if abbreviations:
-		variant.item_code = "{0}-{1}".format(template.item_code, "-".join(abbreviations))
+		variant.item_code = "{0}-{1}".format(template_item_code, "-".join(abbreviations))
 
 	if variant.item_code:
 		variant.item_name = variant.item_code

erpnext/stock/doctype/item/item.py

@@ -13,7 +13,8 @@ from frappe.website.website_generator import WebsiteGenerator
 from erpnext.setup.doctype.item_group.item_group import invalidate_cache_for, get_parent_item_groups
 from frappe.website.render import clear_cache
 from frappe.website.doctype.website_slideshow.website_slideshow import get_slideshow
-from erpnext.controllers.item_variant import get_variant, copy_attributes_to_variant, ItemVariantExistsError
+from erpnext.controllers.item_variant import (get_variant, copy_attributes_to_variant,
+	make_variant_item_code, validate_item_variant_attributes, ItemVariantExistsError)
 
 class DuplicateReorderRows(frappe.ValidationError): pass
 
@@ -36,12 +37,7 @@ class Item(WebsiteGenerator):
 		if frappe.db.get_default("item_naming_by")=="Naming Series":
 			if self.variant_of:
 				if not self.item_code:
-					item_code_suffix = ""
+					self.item_code = make_variant_item_code(self.variant_of, self)
-					for attribute in self.attributes:
-						attribute_abbr = frappe.db.get_value("Item Attribute Value",
-							{"parent": attribute.attribute, "attribute_value": attribute.attribute_value}, "abbr")
-						item_code_suffix += "-" + str(attribute_abbr or attribute.attribute_value)
-					self.item_code = str(self.variant_of) + item_code_suffix
 			else:
 				from frappe.model.naming import make_autoname
 				self.item_code = make_autoname(self.naming_series+'.#####')
@@ -123,8 +119,8 @@ class Item(WebsiteGenerator):
 	def set_opening_stock(self):
 		'''set opening stock'''
 		if not self.valuation_rate:
-			frappe.throw(_("Valuation Rate is mandatory if Opening Stock entered"))		
+			frappe.throw(_("Valuation Rate is mandatory if Opening Stock entered"))
-		
+
 		from erpnext.stock.doctype.stock_entry.stock_entry_utils import make_stock_entry
 
 		# default warehouse, or Stores
@@ -224,12 +220,12 @@ class Item(WebsiteGenerator):
 					file_doc.make_thumbnail()
 
 				self.thumbnail = file_doc.thumbnail_url
-				
+
 	def validate_fixed_asset(self):
 		if self.is_fixed_asset:
 			if self.is_stock_item:
 				frappe.throw(_("Fixed Asset Item must be a non-stock item."))
-				
+
 			if not self.asset_category:
 				frappe.throw(_("Asset Category is mandatory for Fixed Asset item"))
 
@@ -453,7 +449,7 @@ class Item(WebsiteGenerator):
 
 	def cant_change(self):
 		if not self.get("__islocal"):
-			vals = frappe.db.get_value("Item", self.name, ["has_serial_no", "is_stock_item", 
+			vals = frappe.db.get_value("Item", self.name, ["has_serial_no", "is_stock_item",
 				"valuation_method", "has_batch_no", "is_fixed_asset"], as_dict=True)
 
 			if vals and ((self.is_stock_item != vals.is_stock_item) or
@@ -463,7 +459,7 @@ class Item(WebsiteGenerator):
 					if self.check_if_linked_document_exists():
 						frappe.throw(_("As there are existing transactions for this item, \
 							you can not change the values of 'Has Serial No', 'Has Batch No', 'Is Stock Item' and 'Valuation Method'"))
-							
+
 			if vals and not self.is_fixed_asset and self.is_fixed_asset != vals.is_fixed_asset:
 				asset = frappe.db.get_all("Asset", filters={"item_code": self.name, "docstatus": 1}, limit=1)
 				if asset:
@@ -650,6 +646,8 @@ class Item(WebsiteGenerator):
 				frappe.throw(_("Item variant {0} exists with same attributes")
 					.format(variant), ItemVariantExistsError)
 
+			validate_item_variant_attributes(self, args)
+
 def get_timeline_data(doctype, name):
 	'''returns timeline data based on stock ledger entry'''
 	return dict(frappe.db.sql('''select unix_timestamp(posting_date), count(*)

erpnext/stock/doctype/item/test_item.py

@@ -36,6 +36,9 @@ def make_item(item_code, properties=None):
 	return item
 
 class TestItem(unittest.TestCase):
+	def setUp(self):
+		frappe.flags.attribute_values = None
+
 	def get_item(self, idx):
 		item_code = test_records[idx].get("item_code")
 		if not frappe.db.exists("Item", item_code):
@@ -96,6 +99,9 @@ class TestItem(unittest.TestCase):
 		attribute = frappe.get_doc('Item Attribute', 'Test Size')
 		attribute.item_attribute_values = []
 
+		# reset flags
+		frappe.flags.attribute_values = None
+
 		self.assertRaises(InvalidItemAttributeValueError, attribute.save)
 		frappe.db.rollback()
 
@@ -112,10 +118,18 @@ class TestItem(unittest.TestCase):
 
 	def test_make_item_variant_with_numeric_values(self):
 		# cleanup
+		for d in frappe.db.get_all('Item', filters={'variant_of':
+				'_Test Numeric Template Item'}):
+			frappe.delete_doc_if_exists("Item", d.name)
+
 		frappe.delete_doc_if_exists("Item", "_Test Numeric Template Item")
-		frappe.delete_doc_if_exists("Item", "_Test Numeric Variant-L-1.5")
 		frappe.delete_doc_if_exists("Item Attribute", "Test Item Length")
 
+		frappe.db.sql('''delete from `tabItem Variant Attribute`
+			where attribute="Test Item Length"''')
+
+		frappe.flags.attribute_values = None
+
 		# make item attribute
 		frappe.get_doc({
 			"doctype": "Item Attribute",
@@ -143,7 +157,8 @@ class TestItem(unittest.TestCase):
 			"default_warehouse": "_Test Warehouse - _TC"
 		})
 
-		variant = create_variant("_Test Numeric Template Item", {"Test Size": "Large", "Test Item Length": 1.1})
+		variant = create_variant("_Test Numeric Template Item",
+			{"Test Size": "Large", "Test Item Length": 1.1})
 		self.assertEquals(variant.item_code, None)
 		variant.item_code = "_Test Numeric Variant-L-1.1"
 		variant.item_name = "_Test Numeric Variant Large 1.1m"

erpnext/stock/doctype/item_attribute/item_attribute.py

@@ -16,6 +16,7 @@ class ItemAttribute(Document):
 		self.flags.ignore_these_exceptions_in_test = [InvalidItemAttributeValueError]
 
 	def validate(self):
+		frappe.flags.attribute_values = None
 		self.validate_numeric()
 		self.validate_duplication()
 
@@ -26,7 +27,7 @@ class ItemAttribute(Document):
 		'''Validate that if there are existing items with attributes, they are valid'''
 		for item in frappe.db.sql('''select distinct 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):
-			validate_item_variant_attributes(frappe.get_doc('Item', item[0]))
+			validate_item_variant_attributes(item[0])
 
 	def validate_numeric(self):
 		if self.numeric_values: