From 2538e5188714f23cd8136bf6f598812b81c06d5b Mon Sep 17 00:00:00 2001 From: Anand Doshi Date: Tue, 18 Aug 2015 23:30:56 +0530 Subject: [PATCH 1/2] Fixes to Item Variant Attribute. Fixes #3905 --- erpnext/stock/doctype/item/item.py | 117 ++++++++++++------ erpnext/stock/doctype/item/test_item.py | 23 ++-- .../doctype/item_attribute/item_attribute.py | 6 +- 3 files changed, 101 insertions(+), 45 deletions(-) diff --git a/erpnext/stock/doctype/item/item.py b/erpnext/stock/doctype/item/item.py index 1ad52e098b..f465cc663f 100644 --- a/erpnext/stock/doctype/item/item.py +++ b/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[0][0] != self.name: + frappe.throw(_("Item variant {0} exists with same attributes").format(variant[0][0]), ItemVariantExistsError) def validate_end_of_life(item_code, end_of_life=None, verbose=1): if not end_of_life: @@ -488,45 +489,87 @@ 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): diff --git a/erpnext/stock/doctype/item/test_item.py b/erpnext/stock/doctype/item/test_item.py index 488bd05716..428bd37d00 100644 --- a/erpnext/stock/doctype/item/test_item.py +++ b/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"): diff --git a/erpnext/stock/doctype/item_attribute/item_attribute.py b/erpnext/stock/doctype/item_attribute/item_attribute.py index 7383de30ee..a82a3a8c22 100644 --- a/erpnext/stock/doctype/item_attribute/item_attribute.py +++ b/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 From 90fd6fee234eefd4aab4ba50898e285a81e5c983 Mon Sep 17 00:00:00 2001 From: Anand Doshi Date: Wed, 19 Aug 2015 10:21:24 +0530 Subject: [PATCH 2/2] [fix] generate item code for variant --- erpnext/stock/doctype/item/item.js | 28 +++++++++---------- erpnext/stock/doctype/item/item.py | 44 +++++++++++++++++++++++++++--- 2 files changed, 54 insertions(+), 18 deletions(-) diff --git a/erpnext/stock/doctype/item/item.js b/erpnext/stock/doctype/item/item.js index 477d3063ec..e95babe228 100644 --- a/erpnext/stock/doctype/item/item.js +++ b/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('%(item)s', { 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, diff --git a/erpnext/stock/doctype/item/item.py b/erpnext/stock/doctype/item/item.py index f465cc663f..20a064bc75 100644 --- a/erpnext/stock/doctype/item/item.py +++ b/erpnext/stock/doctype/item/item.py @@ -352,8 +352,8 @@ class Item(WebsiteGenerator): args[d.attribute] = d.attribute_value variant = get_variant(self.variant_of, args) - if variant and variant[0][0] != self.name: - frappe.throw(_("Item variant {0} exists with same attributes").format(variant[0][0]), ItemVariantExistsError) + 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: @@ -572,8 +572,10 @@ def find_variant(item, args): 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: @@ -581,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): @@ -600,3 +605,34 @@ def copy_attributes_to_variant(item, variant): variant.description += "\n" for d in variant.attributes: variant.description += "

" + d.attribute + ": " + cstr(d.attribute_value) + "

" + +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