Merge pull request #25805 from ankush/refactor_item

refactor: item doctype

.git-blame-ignore-revs

@@ -0,0 +1,12 @@
+# Since version 2.23 (released in August 2019), git-blame has a feature
+# to ignore or bypass certain commits.
+#
+# This file contains a list of commits that are not likely what you
+# are looking for in a blame, such as mass reformatting or renaming.
+# You can set this file as a default ignore file for blame by running
+# the following command.
+#
+# $ git config blame.ignoreRevsFile .git-blame-ignore-revs
+
+# This commit just changes spaces to tabs for indentation in some files
+5f473611bd6ed57703716244a054d3fb5ba9cd23

erpnext/stock/doctype/item/item.py

@@ -1,8 +1,6 @@
-# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors
+# Copyright (c) 2021, Frappe Technologies Pvt. Ltd. and Contributors
 # License: GNU General Public License v3. See license.txt
 
-from __future__ import unicode_literals
-
 import itertools
 import json
 import erpnext
@@ -12,7 +10,7 @@ from erpnext.controllers.item_variant import (ItemVariantExistsError,
 		copy_attributes_to_variant, get_variant, make_variant_item_code, validate_item_variant_attributes)
 from erpnext.setup.doctype.item_group.item_group import (get_parent_item_groups, invalidate_cache_for)
 from frappe import _, msgprint
-from frappe.utils import (cint, cstr, flt, formatdate, get_timestamp, getdate,
+from frappe.utils import (cint, cstr, flt, formatdate, getdate,
 		now_datetime, random_string, strip, get_link_to_form, nowtime)
 from frappe.utils.html_utils import clean_html
 from frappe.website.doctype.website_slideshow.website_slideshow import \
@@ -21,8 +19,6 @@ from frappe.website.doctype.website_slideshow.website_slideshow import \
 from frappe.website.render import clear_cache
 from frappe.website.website_generator import WebsiteGenerator
 
-from six import iteritems
-
 
 class DuplicateReorderRows(frappe.ValidationError):
 	pass
@@ -76,8 +72,6 @@ class Item(WebsiteGenerator):
 		if not self.description:
 			self.description = self.item_name
 
-		# if self.is_sales_item and not self.get('is_item_from_hub'):
-		# 	self.publish_in_hub = 1
 
 	def after_insert(self):
 		'''set opening stock and item price'''
@@ -129,7 +123,7 @@ class Item(WebsiteGenerator):
 		self.cant_change()
 		self.update_show_in_website()
 
-		if not self.get("__islocal"):
+		if not self.is_new():
 			self.old_item_group = frappe.db.get_value(self.doctype, self.name, "item_group")
 			self.old_website_item_groups = frappe.db.sql_list("""select item_group
 					from `tabWebsite Item Group`
@@ -203,7 +197,7 @@ class Item(WebsiteGenerator):
 	def make_route(self):
 		if not self.route:
 			return cstr(frappe.db.get_value('Item Group', self.item_group,
-					'route')) + '/' + self.scrub((self.item_name if self.item_name else self.item_code) + '-' + random_string(5))
+					'route')) + '/' + self.scrub((self.item_name or self.item_code) + '-' + random_string(5))
 
 	def validate_website_image(self):
 		if frappe.flags.in_import:
@@ -258,7 +252,6 @@ class Item(WebsiteGenerator):
 					"attached_to_name": self.name
 				})
 			except frappe.DoesNotExistError:
-				pass
 				# cleanup
 				frappe.local.message_log.pop()
 
@@ -362,47 +355,49 @@ class Item(WebsiteGenerator):
 				context.update(get_slideshow(self))
 
 	def set_attribute_context(self, context):
-		if self.has_variants:
+		if not self.has_variants:
-			attribute_values_available = {}
+			return
-			context.attribute_values = {}
-			context.selected_attributes = {}
 
-			# load attributes
+		attribute_values_available = {}
-			for v in context.variants:
+		context.attribute_values = {}
-				v.attributes = frappe.get_all("Item Variant Attribute",
+		context.selected_attributes = {}
-					  fields=["attribute", "attribute_value"],
-					  filters={"parent": v.name})
-				# make a map for easier access in templates
-				v.attribute_map = frappe._dict({})
-				for attr in v.attributes:
-					v.attribute_map[attr.attribute] = attr.attribute_value
 
-				for attr in v.attributes:
+		# load attributes
-					values = attribute_values_available.setdefault(attr.attribute, [])
+		for v in context.variants:
-					if attr.attribute_value not in values:
+			v.attributes = frappe.get_all("Item Variant Attribute",
-						values.append(attr.attribute_value)
+				fields=["attribute", "attribute_value"],
+				filters={"parent": v.name})
+			# make a map for easier access in templates
+			v.attribute_map = frappe._dict({})
+			for attr in v.attributes:
+				v.attribute_map[attr.attribute] = attr.attribute_value
 
-					if v.name == context.variant.name:
+			for attr in v.attributes:
-						context.selected_attributes[attr.attribute] = attr.attribute_value
+				values = attribute_values_available.setdefault(attr.attribute, [])
+				if attr.attribute_value not in values:
+					values.append(attr.attribute_value)
 
-			# filter attributes, order based on attribute table
+				if v.name == context.variant.name:
-			for attr in self.attributes:
+					context.selected_attributes[attr.attribute] = attr.attribute_value
-				values = context.attribute_values.setdefault(attr.attribute, [])
 
-				if cint(frappe.db.get_value("Item Attribute", attr.attribute, "numeric_values")):
+		# filter attributes, order based on attribute table
-					for val in sorted(attribute_values_available.get(attr.attribute, []), key=flt):
+		for attr in self.attributes:
-						values.append(val)
+			values = context.attribute_values.setdefault(attr.attribute, [])
 
-				else:
+			if cint(frappe.db.get_value("Item Attribute", attr.attribute, "numeric_values")):
-					# get list of values defined (for sequence)
+				for val in sorted(attribute_values_available.get(attr.attribute, []), key=flt):
-					for attr_value in frappe.db.get_all("Item Attribute Value",
+					values.append(val)
-						fields=["attribute_value"],
-						filters={"parent": attr.attribute}, order_by="idx asc"):
 
-						if attr_value.attribute_value in attribute_values_available.get(attr.attribute, []):
+			else:
-							values.append(attr_value.attribute_value)
+				# get list of values defined (for sequence)
+				for attr_value in frappe.db.get_all("Item Attribute Value",
+					fields=["attribute_value"],
+					filters={"parent": attr.attribute}, order_by="idx asc"):
 
-			context.variant_info = json.dumps(context.variants)
+					if attr_value.attribute_value in attribute_values_available.get(attr.attribute, []):
+						values.append(attr_value.attribute_value)
+
+		context.variant_info = json.dumps(context.variants)
 
 	def set_disabled_attributes(self, context):
 		"""Disable selection options of attribute combinations that do not result in a variant"""
@@ -521,7 +516,7 @@ class Item(WebsiteGenerator):
 
 	def validate_item_type(self):
 		if self.has_serial_no == 1 and self.is_stock_item == 0 and not self.is_fixed_asset:
-			msgprint(_("'Has Serial No' can not be 'Yes' for non-stock item"), raise_exception=1)
+			frappe.throw(_("'Has Serial No' can not be 'Yes' for non-stock item"))
 
 		if self.has_serial_no == 0 and self.serial_no_series:
 			self.serial_no_series = None
@@ -542,10 +537,7 @@ class Item(WebsiteGenerator):
 
 	def fill_customer_code(self):
 		""" Append all the customer codes and insert into "customer_code" field of item table """
-		cust_code = []
+		self.customer_code = ','.join(d.ref_code for d in self.get("customer_items", []))
-		for d in self.get('customer_items'):
-			cust_code.append(d.ref_code)
-		self.customer_code = ','.join(cust_code)
 
 	def check_item_tax(self):
 		"""Check whether Tax Rate is not entered twice for same Tax Type"""
@@ -742,23 +734,25 @@ class Item(WebsiteGenerator):
 
 	def update_template_item(self):
 		"""Set Show in Website for Template Item if True for its Variant"""
-		if self.variant_of:
+		if not self.variant_of:
-			if self.show_in_website:
+			return
-				self.show_variant_in_website = 1
-				self.show_in_website = 0
 
-			if self.show_variant_in_website:
+		if self.show_in_website:
-				# show template
+			self.show_variant_in_website = 1
-				template_item = frappe.get_doc("Item", self.variant_of)
+			self.show_in_website = 0
 
-				if not template_item.show_in_website:
+		if self.show_variant_in_website:
-					template_item.show_in_website = 1
+			# show template
-					template_item.flags.dont_update_variants = True
+			template_item = frappe.get_doc("Item", self.variant_of)
-					template_item.flags.ignore_permissions = True
+
-					template_item.save()
+			if not template_item.show_in_website:
+				template_item.show_in_website = 1
+				template_item.flags.dont_update_variants = True
+				template_item.flags.ignore_permissions = True
+				template_item.save()
 
 	def validate_item_defaults(self):
-		companies = list(set([row.company for row in self.item_defaults]))
+		companies = {row.company for row in self.item_defaults}
 
 		if len(companies) != len(self.item_defaults):
 			frappe.throw(_("Cannot set multiple Item Defaults for a company."))
@@ -813,7 +807,7 @@ class Item(WebsiteGenerator):
 				frappe.throw(_("Item has variants."))
 
 	def validate_attributes_in_variants(self):
-		if not self.has_variants or self.get("__islocal"):
+		if not self.has_variants or self.is_new():
 			return
 
 		old_doc = self.get_doc_before_save()
@@ -901,7 +895,7 @@ class Item(WebsiteGenerator):
 				frappe.throw(_("Variant Based On cannot be changed"))
 
 	def validate_uom(self):
-		if not self.get("__islocal"):
+		if not self.is_new():
 			check_stock_uom_with_bin(self.name, self.stock_uom)
 		if self.has_variants:
 			for d in frappe.db.get_all("Item", filters={"variant_of": self.name}):
@@ -959,20 +953,20 @@ class Item(WebsiteGenerator):
 				d.variant_of = self.variant_of
 
 	def cant_change(self):
-		if not self.get("__islocal"):
+		if self.is_new():
-			fields = ("has_serial_no", "is_stock_item", "valuation_method", "has_batch_no")
+			return
 
-			values = frappe.db.get_value("Item", self.name, fields, as_dict=True)
+		fields = ("has_serial_no", "is_stock_item", "valuation_method", "has_batch_no")
-			if not values.get('valuation_method') and self.get('valuation_method'):
-				values['valuation_method'] = frappe.db.get_single_value("Stock Settings", "valuation_method") or "FIFO"
 
-			if values:
+		values = frappe.db.get_value("Item", self.name, fields, as_dict=True)
-				for field in fields:
+		if not values.get('valuation_method') and self.get('valuation_method'):
-					if cstr(self.get(field)) != cstr(values.get(field)):
+			values['valuation_method'] = frappe.db.get_single_value("Stock Settings", "valuation_method") or "FIFO"
-						if not self.check_if_linked_document_exists(field):
+
-							break # no linked document, allowed
+		if values:
-						else:
+			for field in fields:
-							frappe.throw(_("As there are existing transactions against item {0}, you can not change the value of {1}").format(self.name, frappe.bold(self.meta.get_label(field))))
+				if cstr(self.get(field)) != cstr(values.get(field)):
+					if self.check_if_linked_document_exists(field):
+						frappe.throw(_("As there are existing transactions against item {0}, you can not change the value of {1}").format(self.name, frappe.bold(self.meta.get_label(field))))
 
 	def check_if_linked_document_exists(self, field):
 		linked_doctypes = ["Delivery Note Item", "Sales Invoice Item", "POS Invoice Item", "Purchase Receipt Item",
@@ -1054,56 +1048,42 @@ def make_item_price(item, price_list_name, item_price):
 	}).insert()
 
 def get_timeline_data(doctype, name):
-	'''returns timeline data based on stock ledger entry'''
+	"""get timeline data based on Stock Ledger Entry. This is displayed as heatmap on the item page."""
-	out = {}
-	items = dict(frappe.db.sql('''select posting_date, count(*)
-		from `tabStock Ledger Entry` where item_code=%s
-			and posting_date > date_sub(curdate(), interval 1 year)
-			group by posting_date''', name))
 
-	for date, count in iteritems(items):
+	items = frappe.db.sql("""select unix_timestamp(posting_date), count(*)
-		timestamp = get_timestamp(date)
+							from `tabStock Ledger Entry`
-		out.update({timestamp: count})
+							where item_code=%s and posting_date > date_sub(curdate(), interval 1 year)
+							group by posting_date""", name)
 
-	return out
+	return dict(items)
 
 
-def validate_end_of_life(item_code, end_of_life=None, disabled=None, verbose=1):
+
+def validate_end_of_life(item_code, end_of_life=None, disabled=None):
 	if (not end_of_life) or (disabled is None):
 		end_of_life, disabled = frappe.db.get_value("Item", item_code, ["end_of_life", "disabled"])
 
 	if end_of_life and end_of_life != "0000-00-00" and getdate(end_of_life) <= now_datetime().date():
-		msg = _("Item {0} has reached its end of life on {1}").format(item_code, formatdate(end_of_life))
+		frappe.throw(_("Item {0} has reached its end of life on {1}").format(item_code, formatdate(end_of_life)))
-		_msgprint(msg, verbose)
 
 	if disabled:
-		_msgprint(_("Item {0} is disabled").format(item_code), verbose)
+		frappe.throw(_("Item {0} is disabled").format(item_code))
 
 
-def validate_is_stock_item(item_code, is_stock_item=None, verbose=1):
+def validate_is_stock_item(item_code, is_stock_item=None):
 	if not is_stock_item:
 		is_stock_item = frappe.db.get_value("Item", item_code, "is_stock_item")
 
 	if is_stock_item != 1:
-		msg = _("Item {0} is not a stock Item").format(item_code)
+		frappe.throw(_("Item {0} is not a stock Item").format(item_code))
-
-		_msgprint(msg, verbose)
 
 
-def validate_cancelled_item(item_code, docstatus=None, verbose=1):
+def validate_cancelled_item(item_code, docstatus=None):
 	if docstatus is None:
 		docstatus = frappe.db.get_value("Item", item_code, "docstatus")
 
 	if docstatus == 2:
-		msg = _("Item {0} is cancelled").format(item_code)
+		frappe.throw(_("Item {0} is cancelled").format(item_code))
-		_msgprint(msg, verbose)
-
-def _msgprint(msg, verbose):
-	if verbose:
-		msgprint(msg, raise_exception=True)
-	else:
-		raise frappe.ValidationError(msg)
-
 
 def get_last_purchase_details(item_code, doc_name=None, conversion_rate=1.0):
 	"""returns last purchase details in stock uom"""
@@ -1203,27 +1183,25 @@ def check_stock_uom_with_bin(item, stock_uom):
 	if stock_uom == frappe.db.get_value("Item", item, "stock_uom"):
 		return
 
-	matched = True
 	ref_uom = frappe.db.get_value("Stock Ledger Entry",
 							   {"item_code": item}, "stock_uom")
 
 	if ref_uom:
 		if cstr(ref_uom) != cstr(stock_uom):
-			matched = False
+			frappe.throw(_("Default Unit of Measure for Item {0} cannot be changed directly because you have already made some transaction(s) with another UOM. You will need to create a new Item to use a different Default UOM.").format(item))
-	else:
-		bin_list = frappe.db.sql("select * from tabBin where item_code=%s", item, as_dict=1)
-		for bin in bin_list:
-			if (bin.reserved_qty > 0 or bin.ordered_qty > 0 or bin.indented_qty > 0
-								or bin.planned_qty > 0) and cstr(bin.stock_uom) != cstr(stock_uom):
-				matched = False
-				break
 
-		if matched and bin_list:
+	bin_list = frappe.db.sql("""
-			frappe.db.sql("""update tabBin set stock_uom=%s where item_code=%s""", (stock_uom, item))
+			select * from tabBin where item_code = %s
+				and (reserved_qty > 0 or ordered_qty > 0 or indented_qty > 0 or planned_qty > 0)
+				and stock_uom != %s
+			""", (item, stock_uom), as_dict=1)
+
+	if bin_list:
+		frappe.throw(_("Default Unit of Measure for Item {0} cannot be changed directly because you have already made some transaction(s) with another UOM. You need to either cancel the linked documents or create a new Item.").format(item))
+
+	# No SLE or documents against item. Bin UOM can be changed safely.
+	frappe.db.sql("""update tabBin set stock_uom=%s where item_code=%s""", (stock_uom, item))
 
-	if not matched:
-		frappe.throw(
-			_("Default Unit of Measure for Item {0} cannot be changed directly because you have already made some transaction(s) with another UOM. You will need to create a new Item to use a different Default UOM.").format(item))
 
 def get_item_defaults(item_code, company):
 	item = frappe.get_cached_doc('Item', item_code)
@@ -1264,45 +1242,59 @@ def get_item_details(item_code, company=None):
 
 @frappe.whitelist()
 def get_uom_conv_factor(uom, stock_uom):
-	uoms = [uom, stock_uom]
+	""" Get UOM conversion factor from uom to stock_uom
-	value = ""
+		e.g. uom = "Kg", stock_uom = "Gram" then returns 1000.0
-	uom_details = frappe.db.sql("""select to_uom, from_uom, value from `tabUOM Conversion Factor`\
+	"""
-		where to_uom in ({0})
+	if uom == stock_uom:
-		""".format(', '.join([frappe.db.escape(i, percent=False) for i in uoms])), as_dict=True)
+		return 1.0
 
-	for d in uom_details:
+	from_uom, to_uom = uom, stock_uom   # renaming for readability
-		if d.from_uom == stock_uom and d.to_uom == uom:
-			value = 1/flt(d.value)
-		elif d.from_uom == uom and d.to_uom == stock_uom:
-			value = d.value
 
-	if not value:
+	exact_match = frappe.db.get_value("UOM Conversion Factor", {"to_uom": to_uom, "from_uom": from_uom}, ["value"], as_dict=1)
-		uom_stock = frappe.db.get_value("UOM Conversion Factor", {"to_uom": stock_uom}, ["from_uom", "value"], as_dict=1)
+	if exact_match:
-		uom_row = frappe.db.get_value("UOM Conversion Factor", {"to_uom": uom}, ["from_uom", "value"], as_dict=1)
+		return exact_match.value
 
-		if uom_stock and uom_row:
+	inverse_match = frappe.db.get_value("UOM Conversion Factor", {"to_uom": from_uom, "from_uom": to_uom}, ["value"], as_dict=1)
-			if uom_stock.from_uom == uom_row.from_uom:
+	if inverse_match:
-				value = flt(uom_stock.value) * 1/flt(uom_row.value)
+		return 1 / inverse_match.value
+
+	# This attempts to try and get conversion from intermediate UOM.
+	# case:
+	#            g -> mg = 1000
+	#            g -> kg = 0.001
+	# therefore  kg -> mg = 1000  / 0.001 = 1,000,000
+	intermediate_match = frappe.db.sql("""
+			select (first.value / second.value) as value
+			from `tabUOM Conversion Factor` first
+			join `tabUOM Conversion Factor` second
+				on first.from_uom = second.from_uom
+			where
+				first.to_uom = %(to_uom)s
+				and second.to_uom = %(from_uom)s
+			limit 1
+			""", {"to_uom": to_uom, "from_uom": from_uom}, as_dict=1)
+
+	if intermediate_match:
+		return intermediate_match[0].value
 
-	return value
 
 @frappe.whitelist()
-def get_item_attribute(parent, attribute_value=''):
+def get_item_attribute(parent, attribute_value=""):
+	"""Used for providing auto-completions in child table."""
 	if not frappe.has_permission("Item"):
-		frappe.msgprint(_("No Permission"), raise_exception=1)
+		frappe.throw(_("No Permission"))
 
 	return frappe.get_all("Item Attribute Value", fields = ["attribute_value"],
-		filters = {'parent': parent, 'attribute_value': ("like", "%%%s%%" % attribute_value)})
+		filters = {'parent': parent, 'attribute_value': ("like", f"%{attribute_value}%")})
 
 def update_variants(variants, template, publish_progress=True):
-	count=0
+	total = len(variants)
-	for d in variants:
+	for count, d in enumerate(variants, start=1):
 		variant = frappe.get_doc("Item", d)
 		copy_attributes_to_variant(template, variant)
 		variant.save()
-		count+=1
 		if publish_progress:
-				frappe.publish_progress(count*100/len(variants), title = _("Updating Variants..."))
+			frappe.publish_progress(count / total * 100, title=_("Updating Variants..."))
 
 def on_doctype_update():
 	# since route is a Text column, it needs a length for indexing

erpnext/stock/doctype/item/test_item.py

@@ -10,14 +10,15 @@ from frappe.test_runner import make_test_objects
 from erpnext.controllers.item_variant import (create_variant, ItemVariantExistsError,
 	InvalidItemAttributeValueError, get_variant)
 from erpnext.stock.doctype.item.item import StockExistsForTemplate, InvalidBarcode
-from erpnext.stock.doctype.item.item import get_uom_conv_factor
+from erpnext.stock.doctype.item.item import (get_uom_conv_factor, get_item_attribute,
+	validate_is_stock_item, get_timeline_data)
 from erpnext.stock.doctype.stock_entry.stock_entry_utils import make_stock_entry
 from erpnext.stock.get_item_details import get_item_details
+from erpnext.tests.utils import change_settings
 
-from six import iteritems
 
 test_ignore = ["BOM"]
-test_dependencies = ["Warehouse", "Item Group", "Item Tax Template", "Brand"]
+test_dependencies = ["Warehouse", "Item Group", "Item Tax Template", "Brand", "Item Attribute"]
 
 def make_item(item_code, properties=None):
 	if frappe.db.exists("Item", item_code):
@@ -98,7 +99,7 @@ class TestItem(unittest.TestCase):
 			"ignore_pricing_rule": 1
 		})
 
-		for key, value in iteritems(to_check):
+		for key, value in to_check.items():
 			self.assertEqual(value, details.get(key))
 
 	def test_item_tax_template(self):
@@ -194,7 +195,7 @@ class TestItem(unittest.TestCase):
 			"plc_conversion_rate": 1,
 			"customer": "_Test Customer",
 		})
-		for key, value in iteritems(sales_item_check):
+		for key, value in sales_item_check.items():
 			self.assertEqual(value, sales_item_details.get(key))
 
 		purchase_item_check = {
@@ -215,7 +216,7 @@ class TestItem(unittest.TestCase):
 			"plc_conversion_rate": 1,
 			"supplier": "_Test Supplier",
 		})
-		for key, value in iteritems(purchase_item_check):
+		for key, value in purchase_item_check.items():
 			self.assertEqual(value, purchase_item_details.get(key))
 
 	def test_item_attribute_change_after_variant(self):
@@ -375,6 +376,14 @@ class TestItem(unittest.TestCase):
 		self.assertEqual(item_doc.uoms[1].uom, "Kg")
 		self.assertEqual(item_doc.uoms[1].conversion_factor, 1000)
 
+	def test_uom_conv_intermediate(self):
+		factor = get_uom_conv_factor("Pound", "Gram")
+		self.assertAlmostEqual(factor, 453.592, 3)
+
+	def test_uom_conv_base_case(self):
+		factor = get_uom_conv_factor("m", "m")
+		self.assertEqual(factor, 1.0)
+
 	def test_item_variant_by_manufacturer(self):
 		fields = [{'field_name': 'description'}, {'field_name': 'variant_based_on'}]
 		set_item_variant_settings(fields)
@@ -464,7 +473,7 @@ class TestItem(unittest.TestCase):
 		self.assertEqual(len(matching_barcodes), 1)
 		details = matching_barcodes[0]
 
-		for key, value in iteritems(barcode_properties):
+		for key, value in barcode_properties.items():
 			self.assertEqual(value, details.get(key))
 
 		# Add barcode again - should cause DuplicateEntryError
@@ -480,6 +489,89 @@ class TestItem(unittest.TestCase):
 		new_barcode.barcode_type = 'EAN'
 		self.assertRaises(InvalidBarcode, item_doc.save)
 
+	def test_heatmap_data(self):
+		import time
+		data = get_timeline_data("Item", "_Test Item")
+		self.assertTrue(isinstance(data, dict))
+
+		now = time.time()
+		one_year_ago = now - 366 * 24 * 60 * 60
+
+		for timestamp, count in data.items():
+			self.assertIsInstance(timestamp, int)
+			self.assertTrue(one_year_ago <= timestamp <= now)
+			self.assertIsInstance(count, int)
+			self.assertTrue(count >= 0)
+
+	def test_index_creation(self):
+		"check if index is getting created in db"
+		from erpnext.stock.doctype.item.item import on_doctype_update
+		on_doctype_update()
+
+		indices = frappe.db.sql("show index from tabItem", as_dict=1)
+		expected_columns = {"item_code", "item_name", "item_group", "route"}
+		for index in indices:
+			expected_columns.discard(index.get("Column_name"))
+
+		if expected_columns:
+			self.fail(f"Expected db index on these columns: {', '.join(expected_columns)}")
+
+	def test_attribute_completions(self):
+		expected_attrs = {"Small", "Extra Small", "Extra Large", "Large", "2XL", "Medium"}
+
+		attrs = get_item_attribute("Test Size")
+		received_attrs = {attr.attribute_value for attr in attrs}
+		self.assertEqual(received_attrs, expected_attrs)
+
+		attrs = get_item_attribute("Test Size", attribute_value="extra")
+		received_attrs = {attr.attribute_value for attr in attrs}
+		self.assertEqual(received_attrs, {"Extra Small", "Extra Large"})
+
+	def test_check_stock_uom_with_bin(self):
+		# this item has opening stock and stock_uom set in test_records.
+		item = frappe.get_doc("Item", "_Test Item")
+		item.stock_uom = "Gram"
+		self.assertRaises(frappe.ValidationError, item.save)
+
+	def test_check_stock_uom_with_bin_no_sle(self):
+		from erpnext.stock.stock_balance import update_bin_qty
+		item = create_item("_Item with bin qty")
+		item.stock_uom = "Gram"
+		item.save()
+
+		update_bin_qty(item.item_code, "_Test Warehouse - _TC", {
+			"reserved_qty": 10
+		})
+
+		item.stock_uom = "Kilometer"
+		self.assertRaises(frappe.ValidationError, item.save)
+
+		update_bin_qty(item.item_code, "_Test Warehouse - _TC", {
+			"reserved_qty": 0
+		})
+
+		item.load_from_db()
+		item.stock_uom = "Kilometer"
+		try:
+			item.save()
+		except frappe.ValidationError as e:
+			self.fail(f"UoM change not allowed even though no SLE / BIN with positive qty exists: {e}")
+
+	def test_validate_stock_item(self):
+		self.assertRaises(frappe.ValidationError, validate_is_stock_item, "_Test Non Stock Item")
+
+		try:
+			validate_is_stock_item("_Test Item")
+		except frappe.ValidationError as e:
+			self.fail(f"stock item considered non-stock item: {e}")
+
+	@change_settings("Stock Settings", {"item_naming_by": "Naming Series"})
+	def test_autoname_series(self):
+		item = frappe.new_doc("Item")
+		item.item_group = "All Item Groups"
+		item.save()  # if item code saved without item_code then series worked
+
+
 def set_item_variant_settings(fields):
 	doc = frappe.get_doc('Item Variant Settings')
 	doc.set('fields', fields)
@@ -494,23 +586,24 @@ def make_item_variant():
 
 test_records = frappe.get_test_records('Item')
 
-def create_item(item_code, is_stock_item=None, valuation_rate=0, warehouse=None, is_customer_provided_item=None,
+def create_item(item_code, is_stock_item=1, valuation_rate=0, warehouse="_Test Warehouse - _TC",
-	customer=None, is_purchase_item=None, opening_stock=None, company=None):
+		is_customer_provided_item=None, customer=None, is_purchase_item=None, opening_stock=0,
+		company="_Test Company"):
 	if not frappe.db.exists("Item", item_code):
 		item = frappe.new_doc("Item")
 		item.item_code = item_code
 		item.item_name = item_code
 		item.description = item_code
 		item.item_group = "All Item Groups"
-		item.is_stock_item = is_stock_item or 1
+		item.is_stock_item = is_stock_item
-		item.opening_stock = opening_stock or 0
+		item.opening_stock = opening_stock
-		item.valuation_rate = valuation_rate or 0.0
+		item.valuation_rate = valuation_rate
 		item.is_purchase_item = is_purchase_item
 		item.is_customer_provided_item = is_customer_provided_item
 		item.customer = customer or ''
 		item.append("item_defaults", {
-			"default_warehouse": warehouse or '_Test Warehouse - _TC',
+			"default_warehouse": warehouse,
-			"company": company or "_Test Company"
+			"company": company
 		})
 		item.save()
 	else:

erpnext/stock/doctype/stock_reconciliation/stock_reconciliation.py

@@ -96,7 +96,7 @@ class StockReconciliation(StockController):
 
 	def validate_data(self):
 		def _get_msg(row_num, msg):
-			return _("Row # {0}: ").format(row_num+1) + msg
+			return _("Row # {0}:").format(row_num+1) + " " + msg
 
 		self.validation_messages = []
 		item_warehouse_combinations = []
@@ -167,8 +167,8 @@ class StockReconciliation(StockController):
 			item = frappe.get_doc("Item", item_code)
 
 			# end of life and stock item
-			validate_end_of_life(item_code, item.end_of_life, item.disabled, verbose=0)
+			validate_end_of_life(item_code, item.end_of_life, item.disabled)
-			validate_is_stock_item(item_code, item.is_stock_item, verbose=0)
+			validate_is_stock_item(item_code, item.is_stock_item)
 
 			# item should not be serialized
 			if item.has_serial_no and not row.serial_no and not item.serial_no_series:
@@ -179,10 +179,10 @@ class StockReconciliation(StockController):
 				raise frappe.ValidationError(_("Batch no is required for batched item {0}").format(item_code))
 
 			# docstatus should be < 2
-			validate_cancelled_item(item_code, item.docstatus, verbose=0)
+			validate_cancelled_item(item_code, item.docstatus)
 
 		except Exception as e:
-			self.validation_messages.append(_("Row # ") + ("%d: " % (row.idx)) + cstr(e))
+			self.validation_messages.append(_("Row #") + " " + ("%d: " % (row.idx)) + cstr(e))
 
 	def update_stock_ledger(self):
 		"""	find difference between current and expected entries

erpnext/tests/utils.py

@@ -1,7 +1,8 @@
-# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors
+# Copyright (c) 2021, Frappe Technologies Pvt. Ltd. and Contributors
 # License: GNU General Public License v3. See license.txt
 
-from __future__ import unicode_literals
+import copy
+from contextlib import contextmanager
 
 import frappe
 
@@ -41,3 +42,38 @@ def create_test_contact_and_address():
 	contact.add_email("test_contact_customer@example.com", is_primary=True)
 	contact.add_phone("+91 0000000000", is_primary_phone=True)
 	contact.insert()
+
+
+@contextmanager
+def change_settings(doctype, settings_dict):
+	""" A context manager to ensure that settings are changed before running
+	function and restored after running it regardless of exceptions occured.
+	This is useful in tests where you want to make changes in a function but
+	don't retain those changes.
+	import and use as decorator to cover full function or using `with` statement.
+
+	example:
+	@change_settings("Stock Settings", {"item_naming_by": "Naming Series"})
+	def test_case(self):
+		...
+	"""
+
+	try:
+		settings = frappe.get_doc(doctype)
+		# remember setting
+		previous_settings = copy.deepcopy(settings_dict)
+		for key in previous_settings:
+			previous_settings[key] = getattr(settings, key)
+
+		# change setting
+		for key, value in settings_dict.items():
+			setattr(settings, key, value)
+		settings.save()
+		yield # yield control to calling function
+
+	finally:
+		# restore settings
+		settings = frappe.get_doc(doctype)
+		for key, value in previous_settings.items():
+			setattr(settings, key, value)
+		settings.save()