From 330353a5ced3d40dc2e699a4481234de205b1fd7 Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@iwebnotes.com>
Date: Sat, 22 May 2021 22:07:45 +0530
Subject: [PATCH 01/27] refactor: use frappe.throw instread of recreating

_msgprint was basically duplicating behvaiour of frappe.throw
---
 erpnext/stock/doctype/item/item.py            | 25 ++++++-------------
 .../stock_reconciliation.py                   |  6 ++---
 2 files changed, 10 insertions(+), 21 deletions(-)

diff --git a/erpnext/stock/doctype/item/item.py b/erpnext/stock/doctype/item/item.py
index dbac79465ee..174c87b48de 100644
--- a/erpnext/stock/doctype/item/item.py
+++ b/erpnext/stock/doctype/item/item.py
@@ -1068,42 +1068,31 @@ def get_timeline_data(doctype, name):
 	return out
 
 
-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))
-		_msgprint(msg, verbose)
+		frappe.throw(_("Item {0} has reached its end of life on {1}").format(item_code, formatdate(end_of_life)))
 
 	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)
-
-		_msgprint(msg, verbose)
+		frappe.throw(_("Item {0} is not a stock Item").format(item_code))
 
 
-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)
-		_msgprint(msg, verbose)
-
-def _msgprint(msg, verbose):
-	if verbose:
-		msgprint(msg, raise_exception=True)
-	else:
-		raise frappe.ValidationError(msg)
-
+		frappe.throw(_("Item {0} is cancelled").format(item_code))
 
 def get_last_purchase_details(item_code, doc_name=None, conversion_rate=1.0):
 	"""returns last purchase details in stock uom"""
diff --git a/erpnext/stock/doctype/stock_reconciliation/stock_reconciliation.py b/erpnext/stock/doctype/stock_reconciliation/stock_reconciliation.py
index 7e216d61818..96b1cadaaf2 100644
--- a/erpnext/stock/doctype/stock_reconciliation/stock_reconciliation.py
+++ b/erpnext/stock/doctype/stock_reconciliation/stock_reconciliation.py
@@ -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_is_stock_item(item_code, item.is_stock_item, verbose=0)
+			validate_end_of_life(item_code, item.end_of_life, item.disabled)
+			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,7 +179,7 @@ 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))

From 4a2dbd4885777e435282df7afe8631f157f7a0a8 Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@iwebnotes.com>
Date: Sat, 22 May 2021 22:11:30 +0530
Subject: [PATCH 02/27] refactor: cleanup get_timeline_data, remove py2

---
 erpnext/stock/doctype/item/item.py | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/erpnext/stock/doctype/item/item.py b/erpnext/stock/doctype/item/item.py
index 174c87b48de..61d7e56d138 100644
--- a/erpnext/stock/doctype/item/item.py
+++ b/erpnext/stock/doctype/item/item.py
@@ -12,7 +12,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 +21,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
@@ -1054,18 +1052,15 @@ 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'''
-	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))
+	"""get timeline data based on Stock Ledger Entry. This is displayed as heatmap on the item page."""
 
-	for date, count in iteritems(items):
-		timestamp = get_timestamp(date)
-		out.update({timestamp: count})
+	items = frappe.db.sql("""select unix_timestamp(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)
+
+	return dict(items)
 
-	return out
 
 
 def validate_end_of_life(item_code, end_of_life=None, disabled=None):

From ad58a8164aeabfe0c87c54052ec5ba3db4c1ca56 Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@iwebnotes.com>
Date: Sat, 22 May 2021 22:58:22 +0530
Subject: [PATCH 03/27] refactor: code cleanup

minor fixes for improving code quality
---
 erpnext/stock/doctype/item/item.py | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/erpnext/stock/doctype/item/item.py b/erpnext/stock/doctype/item/item.py
index 61d7e56d138..0b92e27152d 100644
--- a/erpnext/stock/doctype/item/item.py
+++ b/erpnext/stock/doctype/item/item.py
@@ -201,7 +201,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:
@@ -256,7 +256,6 @@ class Item(WebsiteGenerator):
 					"attached_to_name": self.name
 				})
 			except frappe.DoesNotExistError:
-				pass
 				# cleanup
 				frappe.local.message_log.pop()
 

From 0b4858d8e5d84723d82544721784d60e8541e3c2 Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@iwebnotes.com>
Date: Sat, 22 May 2021 22:59:52 +0530
Subject: [PATCH 04/27] refactor: eliminate unnecessary loop, container casts

---
 erpnext/stock/doctype/item/item.py | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/erpnext/stock/doctype/item/item.py b/erpnext/stock/doctype/item/item.py
index 0b92e27152d..c41dd67727a 100644
--- a/erpnext/stock/doctype/item/item.py
+++ b/erpnext/stock/doctype/item/item.py
@@ -539,10 +539,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 = []
-		for d in self.get('customer_items'):
-			cust_code.append(d.ref_code)
-		self.customer_code = ','.join(cust_code)
+		self.customer_code = ','.join(d.ref_code for d in self.get("customer_items", []))
 
 	def check_item_tax(self):
 		"""Check whether Tax Rate is not entered twice for same Tax Type"""
@@ -755,7 +752,7 @@ class Item(WebsiteGenerator):
 					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."))

From 83e6e2e68aec4ef9b6095652a83d19e44bf90e31 Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@iwebnotes.com>
Date: Sat, 22 May 2021 23:01:50 +0530
Subject: [PATCH 05/27] refactor: add guard clause for readability

---
 erpnext/stock/doctype/item/item.py | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/erpnext/stock/doctype/item/item.py b/erpnext/stock/doctype/item/item.py
index c41dd67727a..b665eb8e466 100644
--- a/erpnext/stock/doctype/item/item.py
+++ b/erpnext/stock/doctype/item/item.py
@@ -953,20 +953,22 @@ class Item(WebsiteGenerator):
 				d.variant_of = self.variant_of
 
 	def cant_change(self):
-		if not self.get("__islocal"):
-			fields = ("has_serial_no", "is_stock_item", "valuation_method", "has_batch_no")
+		if self.get("__islocal"):
+			return
 
-			values = frappe.db.get_value("Item", self.name, fields, as_dict=True)
-			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"
+		fields = ("has_serial_no", "is_stock_item", "valuation_method", "has_batch_no")
 
-			if values:
-				for field in fields:
-					if cstr(self.get(field)) != cstr(values.get(field)):
-						if not self.check_if_linked_document_exists(field):
-							break # no linked document, allowed
-						else:
-							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))))
+		values = frappe.db.get_value("Item", self.name, fields, as_dict=True)
+		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:
+			for field in fields:
+				if cstr(self.get(field)) != cstr(values.get(field)):
+					if not self.check_if_linked_document_exists(field):
+						break # no linked document, allowed
+					else:
+						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",

From 931c886f92c34453f87b54e315b8f3614a10df48 Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@iwebnotes.com>
Date: Sat, 22 May 2021 23:06:16 +0530
Subject: [PATCH 06/27] fix: not checking all fields

`break` will break out of the loop without checking remaining fields.
---
 erpnext/stock/doctype/item/item.py | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/erpnext/stock/doctype/item/item.py b/erpnext/stock/doctype/item/item.py
index b665eb8e466..d09a4aa0dc2 100644
--- a/erpnext/stock/doctype/item/item.py
+++ b/erpnext/stock/doctype/item/item.py
@@ -965,9 +965,7 @@ class Item(WebsiteGenerator):
 		if values:
 			for field in fields:
 				if cstr(self.get(field)) != cstr(values.get(field)):
-					if not self.check_if_linked_document_exists(field):
-						break # no linked document, allowed
-					else:
+					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):

From 4b484d741d81834ad9749e9395b2510397b7ae09 Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@iwebnotes.com>
Date: Sat, 22 May 2021 23:10:54 +0530
Subject: [PATCH 07/27] refactor: use is_new() instead of __islocal

Interface over implementation.
---
 erpnext/stock/doctype/item/item.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/erpnext/stock/doctype/item/item.py b/erpnext/stock/doctype/item/item.py
index d09a4aa0dc2..7906923e6f7 100644
--- a/erpnext/stock/doctype/item/item.py
+++ b/erpnext/stock/doctype/item/item.py
@@ -127,7 +127,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`
@@ -807,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()
@@ -895,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}):
@@ -953,7 +953,7 @@ class Item(WebsiteGenerator):
 				d.variant_of = self.variant_of
 
 	def cant_change(self):
-		if self.get("__islocal"):
+		if self.is_new():
 			return
 
 		fields = ("has_serial_no", "is_stock_item", "valuation_method", "has_batch_no")

From c229ac932288189366ec6dd57f74db6f34248b1f Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@iwebnotes.com>
Date: Sat, 22 May 2021 23:15:32 +0530
Subject: [PATCH 08/27] refactor: add guard clause for readability

Both functions only execute based on a condition. In such cases
condition should immediately exit the function, this is called "guard
clause" and helps in readability (less indent, and easy to "exit" when
reading the code.
---
 erpnext/stock/doctype/item/item.py | 94 ++++++++++++++++--------------
 1 file changed, 49 insertions(+), 45 deletions(-)

diff --git a/erpnext/stock/doctype/item/item.py b/erpnext/stock/doctype/item/item.py
index 7906923e6f7..f7856be4ae6 100644
--- a/erpnext/stock/doctype/item/item.py
+++ b/erpnext/stock/doctype/item/item.py
@@ -359,47 +359,49 @@ class Item(WebsiteGenerator):
 				context.update(get_slideshow(self))
 
 	def set_attribute_context(self, context):
-		if self.has_variants:
-			attribute_values_available = {}
-			context.attribute_values = {}
-			context.selected_attributes = {}
+		if not self.has_variants:
+			return
 
-			# load attributes
-			for v in context.variants:
-				v.attributes = frappe.get_all("Item Variant Attribute",
-					  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
+		attribute_values_available = {}
+		context.attribute_values = {}
+		context.selected_attributes = {}
 
-				for attr in v.attributes:
-					values = attribute_values_available.setdefault(attr.attribute, [])
-					if attr.attribute_value not in values:
-						values.append(attr.attribute_value)
+		# load attributes
+		for v in context.variants:
+			v.attributes = frappe.get_all("Item Variant Attribute",
+				  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:
-						context.selected_attributes[attr.attribute] = attr.attribute_value
+			for attr in v.attributes:
+				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
-			for attr in self.attributes:
-				values = context.attribute_values.setdefault(attr.attribute, [])
+				if v.name == context.variant.name:
+					context.selected_attributes[attr.attribute] = attr.attribute_value
 
-				if cint(frappe.db.get_value("Item Attribute", attr.attribute, "numeric_values")):
-					for val in sorted(attribute_values_available.get(attr.attribute, []), key=flt):
-						values.append(val)
+		# filter attributes, order based on attribute table
+		for attr in self.attributes:
+			values = context.attribute_values.setdefault(attr.attribute, [])
 
-				else:
-					# 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"):
+			if cint(frappe.db.get_value("Item Attribute", attr.attribute, "numeric_values")):
+				for val in sorted(attribute_values_available.get(attr.attribute, []), key=flt):
+					values.append(val)
 
-						if attr_value.attribute_value in attribute_values_available.get(attr.attribute, []):
-							values.append(attr_value.attribute_value)
+			else:
+				# 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"""
@@ -736,20 +738,22 @@ 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 self.show_in_website:
-				self.show_variant_in_website = 1
-				self.show_in_website = 0
+		if not self.variant_of:
+			return
 
-			if self.show_variant_in_website:
-				# show template
-				template_item = frappe.get_doc("Item", self.variant_of)
+		if self.show_in_website:
+			self.show_variant_in_website = 1
+			self.show_in_website = 0
 
-				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()
+		if self.show_variant_in_website:
+			# show template
+			template_item = frappe.get_doc("Item", self.variant_of)
+
+			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 = {row.company for row in self.item_defaults}

From 44c489223b855f837a279c65c989f30500ea70e8 Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@iwebnotes.com>
Date: Sat, 22 May 2021 23:43:28 +0530
Subject: [PATCH 09/27] chore: remove py2 compat code

---
 erpnext/stock/doctype/item/item.py      | 4 +---
 erpnext/stock/doctype/item/test_item.py | 9 ++++-----
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/erpnext/stock/doctype/item/item.py b/erpnext/stock/doctype/item/item.py
index f7856be4ae6..a6f5160b5c3 100644
--- a/erpnext/stock/doctype/item/item.py
+++ b/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
diff --git a/erpnext/stock/doctype/item/test_item.py b/erpnext/stock/doctype/item/test_item.py
index e0b89d8e451..c300132ad0f 100644
--- a/erpnext/stock/doctype/item/test_item.py
+++ b/erpnext/stock/doctype/item/test_item.py
@@ -14,7 +14,6 @@ from erpnext.stock.doctype.item.item import get_uom_conv_factor
 from erpnext.stock.doctype.stock_entry.stock_entry_utils import make_stock_entry
 from erpnext.stock.get_item_details import get_item_details
 
-from six import iteritems
 
 test_ignore = ["BOM"]
 test_dependencies = ["Warehouse", "Item Group", "Item Tax Template", "Brand"]
@@ -98,7 +97,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 +193,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 +214,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):
@@ -464,7 +463,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

From ccbde0efa07306710676d144fb7faf29635639db Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@iwebnotes.com>
Date: Sat, 22 May 2021 23:52:00 +0530
Subject: [PATCH 10/27] refactor: use enumerate instead of trackign index

also removed dead code
---
 erpnext/stock/doctype/item/item.py | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/erpnext/stock/doctype/item/item.py b/erpnext/stock/doctype/item/item.py
index a6f5160b5c3..9a52fb4fc1a 100644
--- a/erpnext/stock/doctype/item/item.py
+++ b/erpnext/stock/doctype/item/item.py
@@ -72,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'''
@@ -1277,14 +1275,13 @@ def get_item_attribute(parent, attribute_value=''):
 		filters = {'parent': parent, 'attribute_value': ("like", "%%%s%%" % attribute_value)})
 
 def update_variants(variants, template, publish_progress=True):
-	count=0
-	for d in variants:
+	total = len(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

From 958d485ba49d47af46324395519d9683cbde4674 Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@iwebnotes.com>
Date: Sat, 22 May 2021 23:58:38 +0530
Subject: [PATCH 11/27] refactor: msgprint(raise_exception)->frappe.throw

---
 erpnext/stock/doctype/item/item.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/erpnext/stock/doctype/item/item.py b/erpnext/stock/doctype/item/item.py
index 9a52fb4fc1a..e865bda5c16 100644
--- a/erpnext/stock/doctype/item/item.py
+++ b/erpnext/stock/doctype/item/item.py
@@ -516,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
@@ -1269,7 +1269,7 @@ def get_uom_conv_factor(uom, stock_uom):
 @frappe.whitelist()
 def get_item_attribute(parent, attribute_value=''):
 	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)})

From 297dc5e345b494c6f9cdba12653cdf45721a2af3 Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@iwebnotes.com>
Date: Sun, 23 May 2021 00:17:08 +0530
Subject: [PATCH 12/27] perf: add basic optimisation for uom conversion

---
 erpnext/stock/doctype/item/item.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/erpnext/stock/doctype/item/item.py b/erpnext/stock/doctype/item/item.py
index e865bda5c16..2c862dcfb7b 100644
--- a/erpnext/stock/doctype/item/item.py
+++ b/erpnext/stock/doctype/item/item.py
@@ -1244,6 +1244,9 @@ def get_item_details(item_code, company=None):
 
 @frappe.whitelist()
 def get_uom_conv_factor(uom, stock_uom):
+	if uom == stock_uom:
+		return 1.0
+
 	uoms = [uom, stock_uom]
 	value = ""
 	uom_details = frappe.db.sql("""select to_uom, from_uom, value from `tabUOM Conversion Factor`\

From 0d7f54c6c22578797f1e55eb4c29fbc452c591ce Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@iwebnotes.com>
Date: Sun, 23 May 2021 00:30:27 +0530
Subject: [PATCH 13/27] refactor: simplify UOM conversion logic

- Remove unnecessary sql query
- Remove convoluted matching logic and be explcitiy while querying.
- better variable names for understanding matching cases
---
 erpnext/stock/doctype/item/item.py | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/erpnext/stock/doctype/item/item.py b/erpnext/stock/doctype/item/item.py
index 2c862dcfb7b..ef855c7db56 100644
--- a/erpnext/stock/doctype/item/item.py
+++ b/erpnext/stock/doctype/item/item.py
@@ -1247,27 +1247,22 @@ def get_uom_conv_factor(uom, stock_uom):
 	if uom == stock_uom:
 		return 1.0
 
-	uoms = [uom, stock_uom]
-	value = ""
-	uom_details = frappe.db.sql("""select to_uom, from_uom, value from `tabUOM Conversion Factor`\
-		where to_uom in ({0})
-		""".format(', '.join([frappe.db.escape(i, percent=False) for i in uoms])), as_dict=True)
+	exact_match = frappe.db.get_value("UOM Conversion Factor", {"to_uom": stock_uom, "from_uom": uom}, ["value"], as_dict=1)
+	if exact_match:
+		return exact_match.value
 
-	for d in uom_details:
-		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
+	inverse_match = frappe.db.get_value("UOM Conversion Factor", {"to_uom": uom, "from_uom": stock_uom}, ["value"], as_dict=1)
+	if inverse_match:
+		 return 1 / inverse_match.value
 
-	if not value:
-		uom_stock = frappe.db.get_value("UOM Conversion Factor", {"to_uom": stock_uom}, ["from_uom", "value"], as_dict=1)
-		uom_row = frappe.db.get_value("UOM Conversion Factor", {"to_uom": uom}, ["from_uom", "value"], as_dict=1)
+	# This attempts to try and get conversion from intermediate UOM. E.g. mg <=> g <=> kg
+	uom_stock = frappe.db.get_value("UOM Conversion Factor", {"to_uom": stock_uom}, ["from_uom", "value"], as_dict=1)
+	uom_row = frappe.db.get_value("UOM Conversion Factor", {"to_uom": uom}, ["from_uom", "value"], as_dict=1)
 
-		if uom_stock and uom_row:
-			if uom_stock.from_uom == uom_row.from_uom:
-				value = flt(uom_stock.value) * 1/flt(uom_row.value)
+	if uom_stock and uom_row:
+		if uom_stock.from_uom == uom_row.from_uom:
+			return  flt(uom_stock.value) * 1/flt(uom_row.value)
 
-	return value
 
 @frappe.whitelist()
 def get_item_attribute(parent, attribute_value=''):

From 019be66b7b1d137e686ca9b8189f638abdd5f47d Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@iwebnotes.com>
Date: Sun, 23 May 2021 01:12:47 +0530
Subject: [PATCH 14/27] fix: consider all UOMs for intermediate conversion

- Using `get_value` will restrict intermediate UOM to first UOM that is
  found.
- A self join is required to truly capture the required behaviour.
- Add explanation and examples.
---
 erpnext/stock/doctype/item/item.py | 32 ++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/erpnext/stock/doctype/item/item.py b/erpnext/stock/doctype/item/item.py
index ef855c7db56..a5bc4924227 100644
--- a/erpnext/stock/doctype/item/item.py
+++ b/erpnext/stock/doctype/item/item.py
@@ -1244,24 +1244,40 @@ def get_item_details(item_code, company=None):
 
 @frappe.whitelist()
 def get_uom_conv_factor(uom, stock_uom):
+	""" Get UOM conversion factor from uom to stock_uom
+		e.g. uom = "Kg", stock_uom = "Gram" then returns 1000.0
+	"""
 	if uom == stock_uom:
 		return 1.0
 
-	exact_match = frappe.db.get_value("UOM Conversion Factor", {"to_uom": stock_uom, "from_uom": uom}, ["value"], as_dict=1)
+	from_uom, to_uom = uom, stock_uom   # renaming for readability
+
+	exact_match = frappe.db.get_value("UOM Conversion Factor", {"to_uom": to_uom, "from_uom": from_uom}, ["value"], as_dict=1)
 	if exact_match:
 		return exact_match.value
 
-	inverse_match = frappe.db.get_value("UOM Conversion Factor", {"to_uom": uom, "from_uom": stock_uom}, ["value"], as_dict=1)
+	inverse_match = frappe.db.get_value("UOM Conversion Factor", {"to_uom": from_uom, "from_uom": to_uom}, ["value"], as_dict=1)
 	if inverse_match:
 		 return 1 / inverse_match.value
 
-	# This attempts to try and get conversion from intermediate UOM. E.g. mg <=> g <=> kg
-	uom_stock = frappe.db.get_value("UOM Conversion Factor", {"to_uom": stock_uom}, ["from_uom", "value"], as_dict=1)
-	uom_row = frappe.db.get_value("UOM Conversion Factor", {"to_uom": uom}, ["from_uom", "value"], as_dict=1)
+	# 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 uom_stock and uom_row:
-		if uom_stock.from_uom == uom_row.from_uom:
-			return  flt(uom_stock.value) * 1/flt(uom_row.value)
+	if intermediate_match:
+		return intermediate_match[0].value
 
 
 @frappe.whitelist()

From b9fa12d5721dddd00291fca1e9ef157527fb5905 Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@iwebnotes.com>
Date: Sun, 23 May 2021 12:26:01 +0530
Subject: [PATCH 15/27] test: add tests for uom conversion function

---
 erpnext/stock/doctype/item/test_item.py | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/erpnext/stock/doctype/item/test_item.py b/erpnext/stock/doctype/item/test_item.py
index c300132ad0f..2366f06f6d4 100644
--- a/erpnext/stock/doctype/item/test_item.py
+++ b/erpnext/stock/doctype/item/test_item.py
@@ -374,6 +374,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)

From f5a937bc45e2fe8dd4a18a3b804a86e3caa84cad Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@iwebnotes.com>
Date: Sun, 23 May 2021 12:38:15 +0530
Subject: [PATCH 16/27] test: check index creation on item table

---
 erpnext/stock/doctype/item/test_item.py | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/erpnext/stock/doctype/item/test_item.py b/erpnext/stock/doctype/item/test_item.py
index 2366f06f6d4..d9d1e5a44d3 100644
--- a/erpnext/stock/doctype/item/test_item.py
+++ b/erpnext/stock/doctype/item/test_item.py
@@ -487,6 +487,20 @@ class TestItem(unittest.TestCase):
 		new_barcode.barcode_type = 'EAN'
 		self.assertRaises(InvalidBarcode, item_doc.save)
 
+	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 set_item_variant_settings(fields):
 	doc = frappe.get_doc('Item Variant Settings')
 	doc.set('fields', fields)

From eb177328767c940857f46ca2345e972e9915eda2 Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@iwebnotes.com>
Date: Sun, 23 May 2021 13:12:16 +0530
Subject: [PATCH 17/27] test: add test for item attribute completion

---
 erpnext/stock/doctype/item/item.py      |  5 +++--
 erpnext/stock/doctype/item/test_item.py | 19 +++++++++++++++++--
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/erpnext/stock/doctype/item/item.py b/erpnext/stock/doctype/item/item.py
index a5bc4924227..ec46f60f2b3 100644
--- a/erpnext/stock/doctype/item/item.py
+++ b/erpnext/stock/doctype/item/item.py
@@ -1281,12 +1281,13 @@ def get_uom_conv_factor(uom, stock_uom):
 
 
 @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.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):
 	total = len(variants)
diff --git a/erpnext/stock/doctype/item/test_item.py b/erpnext/stock/doctype/item/test_item.py
index d9d1e5a44d3..7cd6050c027 100644
--- a/erpnext/stock/doctype/item/test_item.py
+++ b/erpnext/stock/doctype/item/test_item.py
@@ -10,13 +10,13 @@ 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
 from erpnext.stock.doctype.stock_entry.stock_entry_utils import make_stock_entry
 from erpnext.stock.get_item_details import get_item_details
 
 
 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):
@@ -500,6 +500,21 @@ class TestItem(unittest.TestCase):
 		if expected_columns:
 			self.fail(f"Expected db index on these columns: {', '.join(expected_columns)}")
 
+	def test_attribute_completions(self):
+		expected_attrs = [{'attribute_value': 'Small'},
+		 {'attribute_value': 'Extra Small'},
+		 {'attribute_value': 'Extra Large'},
+		 {'attribute_value': 'Large'},
+		 {'attribute_value': '2XL'},
+		 {'attribute_value': 'Medium'}]
+
+		attrs = get_item_attribute("Test Size")
+		self.assertEqual(attrs, expected_attrs)
+
+		attrs = get_item_attribute("Test Size", attribute_value="extra")
+		self.assertEqual(attrs, [{'attribute_value': 'Extra Small'}, {'attribute_value': 'Extra Large'}])
+
+
 
 def set_item_variant_settings(fields):
 	doc = frappe.get_doc('Item Variant Settings')

From a11a8e8ab2e30eaa9f3b1cd80c27dc9ad8f13aeb Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@iwebnotes.com>
Date: Sun, 23 May 2021 13:19:22 +0530
Subject: [PATCH 18/27] chore: add blame ignore file

---
 .git-blame-ignore-revs | 12 ++++++++++++
 1 file changed, 12 insertions(+)
 create mode 100644 .git-blame-ignore-revs

diff --git a/.git-blame-ignore-revs b/.git-blame-ignore-revs
new file mode 100644
index 00000000000..be425ec2d9d
--- /dev/null
+++ b/.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

From 4e360f805f5cb4f7ed500316aa97ca7e52b2f9bf Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@iwebnotes.com>
Date: Sun, 23 May 2021 14:48:03 +0530
Subject: [PATCH 19/27] test: hoist defaults to function signature

---
 erpnext/stock/doctype/item/test_item.py | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/erpnext/stock/doctype/item/test_item.py b/erpnext/stock/doctype/item/test_item.py
index 7cd6050c027..9694927914d 100644
--- a/erpnext/stock/doctype/item/test_item.py
+++ b/erpnext/stock/doctype/item/test_item.py
@@ -530,23 +530,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,
-	customer=None, is_purchase_item=None, opening_stock=None, company=None):
+def create_item(item_code, is_stock_item=1, valuation_rate=0, warehouse="_Test Warehouse - _TC",
+		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.opening_stock = opening_stock or 0
-		item.valuation_rate = valuation_rate or 0.0
+		item.is_stock_item = is_stock_item
+		item.opening_stock = opening_stock
+		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',
-			"company": company or "_Test Company"
+			"default_warehouse": warehouse,
+			"company": company
 		})
 		item.save()
 	else:

From fc54cf68ac33b213dd44821c10e01f84a6c4727a Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@iwebnotes.com>
Date: Sun, 23 May 2021 15:04:41 +0530
Subject: [PATCH 20/27] test: add tests for checking stock_uom with bin

---
 erpnext/stock/doctype/item/test_item.py | 29 +++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/erpnext/stock/doctype/item/test_item.py b/erpnext/stock/doctype/item/test_item.py
index 9694927914d..8df12a3f16f 100644
--- a/erpnext/stock/doctype/item/test_item.py
+++ b/erpnext/stock/doctype/item/test_item.py
@@ -514,6 +514,35 @@ class TestItem(unittest.TestCase):
 		attrs = get_item_attribute("Test Size", attribute_value="extra")
 		self.assertEqual(attrs, [{'attribute_value': 'Extra Small'}, {'attribute_value': '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 set_item_variant_settings(fields):

From 57266a7343edd1fb963d20db28593bed3f80ae50 Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@iwebnotes.com>
Date: Sun, 23 May 2021 14:21:24 +0530
Subject: [PATCH 21/27] refactor: check_stock_uom_with_bin

---
 erpnext/stock/doctype/item/item.py | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/erpnext/stock/doctype/item/item.py b/erpnext/stock/doctype/item/item.py
index ec46f60f2b3..a0bd49543eb 100644
--- a/erpnext/stock/doctype/item/item.py
+++ b/erpnext/stock/doctype/item/item.py
@@ -1183,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
-	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
+			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))
 
-		if matched and bin_list:
-			frappe.db.sql("""update tabBin set stock_uom=%s where item_code=%s""", (stock_uom, item))
+	bin_list = frappe.db.sql("""
+			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)

From e971b4592e3bb1894294f561a522f2b06336908b Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@iwebnotes.com>
Date: Sun, 23 May 2021 15:33:06 +0530
Subject: [PATCH 22/27] test: add test for is_stock_item

---
 erpnext/stock/doctype/item/test_item.py | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/erpnext/stock/doctype/item/test_item.py b/erpnext/stock/doctype/item/test_item.py
index 8df12a3f16f..d9c77efc24b 100644
--- a/erpnext/stock/doctype/item/test_item.py
+++ b/erpnext/stock/doctype/item/test_item.py
@@ -10,7 +10,8 @@ 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, get_item_attribute
+from erpnext.stock.doctype.item.item import (get_uom_conv_factor, get_item_attribute,
+	validate_is_stock_item)
 from erpnext.stock.doctype.stock_entry.stock_entry_utils import make_stock_entry
 from erpnext.stock.get_item_details import get_item_details
 
@@ -544,6 +545,13 @@ class TestItem(unittest.TestCase):
 		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}")
 
 def set_item_variant_settings(fields):
 	doc = frappe.get_doc('Item Variant Settings')

From 42e057d079c1807393d376d347762e97100b6883 Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@iwebnotes.com>
Date: Sun, 23 May 2021 15:45:25 +0530
Subject: [PATCH 23/27] test: add test for get_timeline_data in item

---
 erpnext/stock/doctype/item/test_item.py | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/erpnext/stock/doctype/item/test_item.py b/erpnext/stock/doctype/item/test_item.py
index d9c77efc24b..234a9132c29 100644
--- a/erpnext/stock/doctype/item/test_item.py
+++ b/erpnext/stock/doctype/item/test_item.py
@@ -11,7 +11,7 @@ from erpnext.controllers.item_variant import (create_variant, ItemVariantExistsE
 	InvalidItemAttributeValueError, get_variant)
 from erpnext.stock.doctype.item.item import StockExistsForTemplate, InvalidBarcode
 from erpnext.stock.doctype.item.item import (get_uom_conv_factor, get_item_attribute,
-	validate_is_stock_item)
+	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
 
@@ -488,6 +488,20 @@ 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

From 76dd6e904682a1bec1ff21d66c45e164fd26a47b Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@iwebnotes.com>
Date: Sun, 23 May 2021 16:19:48 +0530
Subject: [PATCH 24/27] test: contextmanager to change settings

---
 erpnext/tests/utils.py | 40 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/erpnext/tests/utils.py b/erpnext/tests/utils.py
index 16ecd5180b2..11eb6afc1fa 100644
--- a/erpnext/tests/utils.py
+++ b/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()

From c15fef571fda7fa6bf2ae7f43380b29098775d87 Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@iwebnotes.com>
Date: Sun, 23 May 2021 16:26:37 +0530
Subject: [PATCH 25/27] test: item naming series behaviour

---
 erpnext/stock/doctype/item/test_item.py | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/erpnext/stock/doctype/item/test_item.py b/erpnext/stock/doctype/item/test_item.py
index 234a9132c29..9adacdfb782 100644
--- a/erpnext/stock/doctype/item/test_item.py
+++ b/erpnext/stock/doctype/item/test_item.py
@@ -14,6 +14,7 @@ from erpnext.stock.doctype.item.item import (get_uom_conv_factor, get_item_attri
 	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
 
 
 test_ignore = ["BOM"]
@@ -567,6 +568,13 @@ class TestItem(unittest.TestCase):
 		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)

From 3aed662f4690ad6fb5fda680aad4246103eded81 Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@iwebnotes.com>
Date: Sun, 23 May 2021 16:41:03 +0530
Subject: [PATCH 26/27] chore: translation / semgrep / sider fixes

---
 erpnext/stock/doctype/item/item.py                     |  6 +++---
 erpnext/stock/doctype/item/test_item.py                | 10 +++++-----
 .../stock_reconciliation/stock_reconciliation.py       |  4 ++--
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/erpnext/stock/doctype/item/item.py b/erpnext/stock/doctype/item/item.py
index a0bd49543eb..dd815404fae 100644
--- a/erpnext/stock/doctype/item/item.py
+++ b/erpnext/stock/doctype/item/item.py
@@ -365,8 +365,8 @@ class Item(WebsiteGenerator):
 		# load attributes
 		for v in context.variants:
 			v.attributes = frappe.get_all("Item Variant Attribute",
-				  fields=["attribute", "attribute_value"],
-				  filters={"parent": v.name})
+				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:
@@ -1256,7 +1256,7 @@ def get_uom_conv_factor(uom, stock_uom):
 
 	inverse_match = frappe.db.get_value("UOM Conversion Factor", {"to_uom": from_uom, "from_uom": to_uom}, ["value"], as_dict=1)
 	if inverse_match:
-		 return 1 / inverse_match.value
+		return 1 / inverse_match.value
 
 	# This attempts to try and get conversion from intermediate UOM.
 	# case:
diff --git a/erpnext/stock/doctype/item/test_item.py b/erpnext/stock/doctype/item/test_item.py
index 9adacdfb782..406039dc589 100644
--- a/erpnext/stock/doctype/item/test_item.py
+++ b/erpnext/stock/doctype/item/test_item.py
@@ -518,11 +518,11 @@ class TestItem(unittest.TestCase):
 
 	def test_attribute_completions(self):
 		expected_attrs = [{'attribute_value': 'Small'},
-		 {'attribute_value': 'Extra Small'},
-		 {'attribute_value': 'Extra Large'},
-		 {'attribute_value': 'Large'},
-		 {'attribute_value': '2XL'},
-		 {'attribute_value': 'Medium'}]
+			{'attribute_value': 'Extra Small'},
+			{'attribute_value': 'Extra Large'},
+			{'attribute_value': 'Large'},
+			{'attribute_value': '2XL'},
+			{'attribute_value': 'Medium'}]
 
 		attrs = get_item_attribute("Test Size")
 		self.assertEqual(attrs, expected_attrs)
diff --git a/erpnext/stock/doctype/stock_reconciliation/stock_reconciliation.py b/erpnext/stock/doctype/stock_reconciliation/stock_reconciliation.py
index 96b1cadaaf2..b9f91906c6d 100644
--- a/erpnext/stock/doctype/stock_reconciliation/stock_reconciliation.py
+++ b/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 = []
@@ -182,7 +182,7 @@ class StockReconciliation(StockController):
 			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

From 15f8a0fb22addd730b1ebfb43635cfb29f1ddb90 Mon Sep 17 00:00:00 2001
From: Ankush Menat <ankush@iwebnotes.com>
Date: Sun, 23 May 2021 18:10:21 +0530
Subject: [PATCH 27/27] test: fix flaky test

---
 erpnext/stock/doctype/item/test_item.py | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/erpnext/stock/doctype/item/test_item.py b/erpnext/stock/doctype/item/test_item.py
index 406039dc589..c7467a5a0f5 100644
--- a/erpnext/stock/doctype/item/test_item.py
+++ b/erpnext/stock/doctype/item/test_item.py
@@ -517,18 +517,15 @@ class TestItem(unittest.TestCase):
 			self.fail(f"Expected db index on these columns: {', '.join(expected_columns)}")
 
 	def test_attribute_completions(self):
-		expected_attrs = [{'attribute_value': 'Small'},
-			{'attribute_value': 'Extra Small'},
-			{'attribute_value': 'Extra Large'},
-			{'attribute_value': 'Large'},
-			{'attribute_value': '2XL'},
-			{'attribute_value': 'Medium'}]
+		expected_attrs = {"Small", "Extra Small", "Extra Large", "Large", "2XL", "Medium"}
 
 		attrs = get_item_attribute("Test Size")
-		self.assertEqual(attrs, expected_attrs)
+		received_attrs = {attr.attribute_value for attr in attrs}
+		self.assertEqual(received_attrs, expected_attrs)
 
 		attrs = get_item_attribute("Test Size", attribute_value="extra")
-		self.assertEqual(attrs, [{'attribute_value': 'Extra Small'}, {'attribute_value': 'Extra Large'}])
+		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.