Merge pull request #3906 from anandpdoshi/item-variant-fixes

Fixes to Item Variant Attribute

erpnext/stock/doctype/item/item.js

@@ -9,7 +9,7 @@ frappe.ui.form.on("Item", {
 		if (frm.doc.variant_of){
 			frm.fields_dict["attributes"].grid.set_column_disp("attribute_value", true);
 		}
-		
+
 	},
 
 	refresh: function(frm) {
@@ -34,7 +34,7 @@ frappe.ui.form.on("Item", {
 			frm.add_custom_button(__("Show Variants"), function() {
 				frappe.set_route("List", "Item", {"variant_of": frm.doc.name});
 			}, "icon-list", "btn-default");
-			
+
 			frm.add_custom_button(__("Make Variant"), function() {
 				erpnext.item.make_variant()
 			}, "icon-list", "btn-default");
@@ -57,7 +57,7 @@ frappe.ui.form.on("Item", {
 		}
 
 		erpnext.item.toggle_reqd(frm);
-		
+
 		erpnext.item.toggle_attributes(frm);
 	},
 
@@ -93,7 +93,7 @@ frappe.ui.form.on("Item", {
 	is_stock_item: function(frm) {
 		erpnext.item.toggle_reqd(frm);
 	},
-	
+
 	has_variants: function(frm) {
 		erpnext.item.toggle_attributes(frm);
 	}
@@ -193,10 +193,10 @@ $.extend(erpnext.item, {
 			validated = 0;
 		}
 	},
-	
+
 	make_variant: function(doc) {
 		var fields = []
-		
+
 		for(var i=0;i< cur_frm.doc.attributes.length;i++){
 			var fieldtype, desc;
 			var row = cur_frm.doc.attributes[i];
@@ -221,8 +221,8 @@ $.extend(erpnext.item, {
 			title: __("Make Variant"),
 			fields: fields
 		});
-		
-		d.set_primary_action(__("Make"), function() {	
+
+		d.set_primary_action(__("Make"), function() {
 			args = d.get_values();
 			if(!args) return;
 			frappe.call({
@@ -234,8 +234,8 @@ $.extend(erpnext.item, {
 				callback: function(r) {
 					// returns variant item
 					if (r.message) {
-						var variant = r.message[0];
-						var msgprint_dialog = frappe.msgprint(__("Item Variant {0} already exists with same attributes", 
+						var variant = r.message;
+						var msgprint_dialog = frappe.msgprint(__("Item Variant {0} already exists with same attributes",
 							[repl('<a href="#Form/Item/%(item_encoded)s" class="strong variant-click">%(item)s</a>', {
 								item_encoded: encodeURIComponent(variant),
 								item: variant
@@ -251,7 +251,7 @@ $.extend(erpnext.item, {
 							method:"erpnext.stock.doctype.item.item.create_variant",
 							args: {
 								"item": cur_frm.doc.name,
-								"param": d.get_values()
+								"args": d.get_values()
 							},
 							callback: function(r) {
 								var doclist = frappe.model.sync(r.message);
@@ -262,17 +262,17 @@ $.extend(erpnext.item, {
 				}
 			});
 		});
-				
+
 		d.show();
 
 		$.each(d.fields_dict, function(i, field) {
-			
+
 			if(field.df.fieldtype !== "Data") {
 				return;
 			}
 
 			$(field.input_area).addClass("ui-front");
-			
+
 			field.$input.autocomplete({
 				minLength: 0,
 				minChars: 0,

erpnext/stock/doctype/item/item.py

@@ -13,6 +13,7 @@ from frappe.website.doctype.website_slideshow.website_slideshow import get_slide
 
 class WarehouseNotSet(frappe.ValidationError): pass
 class ItemTemplateCannotHaveStock(frappe.ValidationError): pass
+class ItemVariantExistsError(frappe.ValidationError): pass
 
 class Item(WebsiteGenerator):
 	website = frappe._dict(
@@ -63,7 +64,7 @@ class Item(WebsiteGenerator):
 		self.synced_with_hub = 0
 		self.validate_has_variants()
 		self.validate_stock_for_template_must_be_zero()
-		self.validate_template_attributes()
+		self.validate_attributes()
 		self.validate_variant_attributes()
 
 		if not self.get("__islocal"):
@@ -331,11 +332,11 @@ class Item(WebsiteGenerator):
 			if template_uom != self.stock_uom:
 				frappe.throw(_("Default Unit of Measure for Variant must be same as Template"))
 
-	def validate_template_attributes(self):
-		if self.has_variants:
+	def validate_attributes(self):
+		if self.has_variants or self.variant_of:
 			attributes = []
 			if not self.attributes:
-				frappe.throw(_("Attribute is mandatory for Item Template"))
+				frappe.throw(_("Attribute table is mandatory"))
 			for d in self.attributes:
 				if d.attribute in attributes:
 					frappe.throw(_("Attribute {0} selected multiple times in Attributes Table".format(d.attribute)))
@@ -351,8 +352,8 @@ class Item(WebsiteGenerator):
 				args[d.attribute] = d.attribute_value
 
 			variant = get_variant(self.variant_of, args)
-			if variant and not variant[0][0] == self.name:
-				frappe.throw(_("Item variant {0} exists with same attributes".format(variant[0][0])	))
+			if variant and variant != self.name:
+				frappe.throw(_("Item variant {0} exists with same attributes").format(variant), ItemVariantExistsError)
 
 def validate_end_of_life(item_code, end_of_life=None, verbose=1):
 	if not end_of_life:
@@ -488,49 +489,93 @@ def check_stock_uom_with_bin(item, stock_uom):
 
 @frappe.whitelist()
 def get_variant(item, args):
-	if not type(args) == dict:
+	"""Validates Attributes and their Values, then looks for an exactly matching Item Variant
+
+		:param item: Template Item
+		:param args: A dictionary with "Attribute" as key and "Attribute Value" as value
+	"""
+	if isinstance(args, basestring):
 		args = json.loads(args)
-	attributes = {}
-	numeric_attributes = []
-	for t in frappe.db.get_all("Item Attribute Value", fields=["parent", "attribute_value"]):
-		attributes.setdefault(t.parent, []).append(t.attribute_value)
 
-	for t in frappe.get_list("Item Attribute", filters={"numeric_values":1}):
-		numeric_attributes.append(t.name)
+	if not args:
+		frappe.throw(_("Please specify at least one attribute in the Attributes table"))
 
-	for d in args:
-		if d in numeric_attributes:
-			values = frappe.db.sql("""select from_range, to_range, increment from `tabItem Variant Attribute` \
-				where parent = %s and attribute = %s""", (item, d), as_dict=1)[0]
+	validate_item_variant_attributes(item, args)
 
-			if (not values.from_range < cint(args[d]) < values.to_range) or ((cint(args[d]) - values.from_range) % values.increment != 0):
-				frappe.throw(_("Attribute value {0} for attribute {1} must be within range of {2} to {3} and in increments of {4}")
-					.format(args[d], d, values.from_range, values.to_range, values.increment))
-		else:
-			if args[d] not in attributes.get(d):
-				frappe.throw(_("Attribute value {0} for attribute {1} does not exist \
-					in Item Attribute Master.").format(args[d], d))
+	return find_variant(item, args)
 
-	conds=""
-	attributes = ""
-	for d in args:
-		if conds:
-			conds+= " and "
-			attributes+= ", "
+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, [])).append(t.attribute_value)
 
-		conds += """ exists(select iv.name from `tabItem Variant Attribute` iv where iv.parent = i.name and
-			 iv.attribute= "{0}" and iv.attribute_value= "{1}")""".format(d, args[d])
-		attributes += "'{0}'".format(d)
+	numeric_attributes = [t.name for t in frappe.get_list("Item Attribute", filters={"numeric_values":1,
+		"parent": ["in", args.keys()]})]
 
-	conds += """and not exists (select iv.name from `tabItem Variant Attribute` iv where iv.parent = i.name and
-		iv.attribute not in ({0}))""".format(attributes)
+	template_item = frappe.get_doc("Item", item)
+	template_item_attributes = frappe._dict((d.attribute, d) for d in template_item.attributes)
 
-	variant=  frappe.db.sql("""select i.name from tabItem i where {0}""".format(conds))
-	return variant
+	for attribute, value in args.items():
+
+		if attribute in numeric_attributes:
+			template_attribute = template_item_attributes[attribute]
+
+			if template_attribute.increment == 0:
+				# defensive validation to prevent ZeroDivisionError
+				frappe.throw(_("Increment for Attribute {0} cannot be 0").format(attribute))
+
+			from_range = template_attribute.from_range
+			to_range = template_attribute.to_range
+			increment = template_attribute.increment
+
+			if not ( (from_range <= flt(value) <= to_range) and (flt(value) - from_range) % increment == 0 ):
+				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))
+
+		elif value not in attribute_values[attribute]:
+			frappe.throw(_("Value {0} for Attribute {1} does not exist in the list of valid Item Attribute Values").format(
+				value, attribute))
+
+def find_variant(item, args):
+	conditions = ["""(iv_attribute.attribute="{0}" and iv_attribute.attribute_value="{1}")"""\
+		.format(frappe.db.escape(key), frappe.db.escape(value)) for key, value in args.items()]
+
+	conditions = " or ".join(conditions)
+
+	# use approximate match and shortlist possible variant matches
+	# it is approximate because we are matching using OR condition
+	# and it need not be exact match at this stage
+	# this uses a simpler query instead of using multiple exists conditions
+	possible_variants = frappe.db.sql_list("""select name from `tabItem` item
+		where variant_of=%s and exists (
+			select name from `tabItem Variant Attribute` iv_attribute
+				where iv_attribute.parent=item.name
+				and ({conditions})
+		)""".format(conditions=conditions), item)
+
+	for variant in possible_variants:
+		variant = frappe.get_doc("Item", variant)
+
+		if len(args.keys()) == len(variant.get("attributes")):
+			# has the same number of attributes and values
+			# assuming no duplication as per the validation in Item
+			match_count = 0
+
+			for attribute, value in args.items():
+				for row in variant.attributes:
+					if row.attribute==attribute and row.attribute_value==value:
+						# this row matches
+						match_count += 1
+						break
+
+			if match_count == len(args.keys()):
+				return variant.name
 
 @frappe.whitelist()
-def create_variant(item, param):
-	args = json.loads(param)
+def create_variant(item, args):
+	if isinstance(args, basestring):
+		args = json.loads(args)
+
 	variant = frappe.new_doc("Item")
 	variant_attributes = []
 	for d in args:
@@ -538,9 +583,12 @@ def create_variant(item, param):
 			"attribute": d,
 			"attribute_value": args[d]
 		})
+
 	variant.set("attributes", variant_attributes)
 	template = frappe.get_doc("Item", item)
 	copy_attributes_to_variant(template, variant)
+	make_variant_item_code(template, variant)
+
 	return variant
 
 def copy_attributes_to_variant(item, variant):
@@ -557,3 +605,34 @@ def copy_attributes_to_variant(item, variant):
 		variant.description += "\n"
 		for d in variant.attributes:
 			variant.description += "<p>" + d.attribute + ": " + cstr(d.attribute_value) + "</p>"
+
+def make_variant_item_code(template, variant):
+	"""Uses template's item code and abbreviations to make variant's item code"""
+	if variant.item_code:
+		return
+
+	abbreviations = []
+	for attr in variant.attributes:
+		item_attribute = frappe.db.sql("""select i.numeric_values, v.abbr
+			from `tabItem Attribute` i left join `tabItem Attribute Value` v
+				on (i.name=v.parent)
+			where i.name=%(attribute)s and v.attribute_value=%(attribute_value)s""", {
+				"attribute": attr.attribute,
+				"attribute_value": attr.attribute_value
+			}, as_dict=True)
+
+		if not item_attribute:
+			# somehow an invalid item attribute got used
+			return
+
+		if item_attribute[0].numeric_values:
+			# don't generate item code if one of the attributes is numeric
+			return
+
+		abbreviations.append(item_attribute[0].abbr)
+
+	if 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/test_item.py

@@ -6,7 +6,7 @@ import unittest
 import frappe
 
 from frappe.test_runner import make_test_records
-from erpnext.stock.doctype.item.item import WarehouseNotSet, ItemTemplateCannotHaveStock, create_variant
+from erpnext.stock.doctype.item.item import WarehouseNotSet, ItemTemplateCannotHaveStock, create_variant, ItemVariantExistsError
 from erpnext.stock.doctype.stock_entry.test_stock_entry import make_stock_entry
 
 test_ignore = ["BOM"]
@@ -98,13 +98,22 @@ class TestItem(unittest.TestCase):
 
 		for key, value in to_check.iteritems():
 			self.assertEquals(value, details.get(key))
-		
+
 	def test_make_item_variant(self):
-		if not frappe.db.exists("Item", "_Test Variant Item-S"):
-			variant = create_variant("_Test Variant Item", """{"Test Size": "Small"}""")
-			variant.item_code = "_Test Variant Item-S"
-			variant.item_name = "_Test Variant Item-S"
-			variant.save()
+		if frappe.db.exists("Item", "_Test Variant Item-L"):
+			frappe.delete_doc("Item", "_Test Variant Item-L")
+			frappe.delete_doc("Item", "_Test Variant Item-L2")
+
+		variant = create_variant("_Test Variant Item", """{"Test Size": "Large"}""")
+		variant.item_code = "_Test Variant Item-L"
+		variant.item_name = "_Test Variant Item-L"
+		variant.save()
+
+		# doing it again should raise error
+		variant = create_variant("_Test Variant Item", """{"Test Size": "Large"}""")
+		variant.item_code = "_Test Variant Item-L2"
+		variant.item_name = "_Test Variant Item-L2"
+		self.assertRaises(ItemVariantExistsError, variant.save)
 
 def make_item_variant():
 	if not frappe.db.exists("Item", "_Test Variant Item-S"):

erpnext/stock/doctype/item_attribute/item_attribute.py

@@ -16,9 +16,13 @@ class ItemAttribute(Document):
 		if self.numeric_values:
 			self.set("item_attribute_values", [])
 			if not self.from_range or not self.to_range:
-				frappe.throw(_("Please specify from/to Range"))
+				frappe.throw(_("Please specify from/to range"))
+
 			elif self.from_range > self.to_range:
 				frappe.throw(_("From Range cannot be greater than to Range"))
+
+			if not self.increment:
+				frappe.throw(_("Increment cannot be 0"))
 		else:
 			self.from_range = self.to_range = self.increment = 0