Improvements to batch auto naming (#12496)

* refactor:
add new function - batch_uses_naming_series
use batch_uses_naming_series in autoname method

* properly update naming series on delete:
- add new functions - get_batch_prefix, get_batch_naming_series_key, get_batch_naming_series
- refactor get_name_from_naming_series
- add after_delete method

* add documentation and rename some functions

* PEP 8 compliance

* test

erpnext/stock/doctype/batch/batch.py

@@ -5,24 +5,31 @@ from __future__ import unicode_literals
 import frappe
 from frappe import _
 from frappe.model.document import Document
-from frappe.model.naming import make_autoname
+from frappe.model.naming import make_autoname, revert_series_if_last
 from frappe.utils import flt, cint
 
 
-class UnableToSelectBatchError(frappe.ValidationError): pass
+class UnableToSelectBatchError(frappe.ValidationError):
+	pass
 
 
 def get_name_from_naming_series():
-	naming_series_prefix = frappe.db.get_single_value('Stock Settings', 'naming_series_prefix')
+	"""
-	if not naming_series_prefix:
+	Get a name generated for a Batch from the Batch's naming series.
-		naming_series_prefix = 'BATCH-'
+	:return: The string that was generated.
-
+	"""
-	name = make_autoname(naming_series_prefix + '.#####')
+	naming_series_prefix = _get_batch_prefix()
+	key = _make_naming_series_key(naming_series_prefix)
+	name = make_autoname(key)
 
 	return name
 
 
 def get_name_from_hash():
+	"""
+	Get a name for a Batch by generating a unique hash.
+	:return: The hash that was generated.
+	"""
 	temp = None
 	while not temp:
 		temp = frappe.generate_hash()[:7].upper()
@@ -32,13 +39,66 @@ def get_name_from_hash():
 	return temp
 
 
+def batch_uses_naming_series():
+	"""
+	Verify if the Batch is to be named using a naming series
+	:return: bool
+	"""
+	use_naming_series = cint(frappe.db.get_single_value('Stock Settings', 'use_naming_series'))
+	return bool(use_naming_series)
+
+
+def _get_batch_prefix():
+	"""
+	Get the naming series prefix set in Stock Settings.
+
+	It does not do any sanity checks so make sure to use it after checking if the Batch
+	is set to use naming series.
+	:return: The naming series.
+	"""
+	naming_series_prefix = frappe.db.get_single_value('Stock Settings', 'naming_series_prefix')
+	if not naming_series_prefix:
+		naming_series_prefix = 'BATCH-'
+
+	return naming_series_prefix
+
+
+def _make_naming_series_key(prefix):
+	"""
+	Make naming series key for a Batch.
+
+	Naming series key is in the format [prefix].[#####]
+	:param prefix: Naming series prefix gotten from Stock Settings
+	:return: The derived key. If no prefix is given, an empty string is returned
+	"""
+	if not unicode(prefix):
+		return ''
+	else:
+		return prefix.upper() + '.#####'
+
+
+def get_batch_naming_series():
+	"""
+	Get naming series key for a Batch.
+
+	Naming series key is in the format [prefix].[#####]
+	:return: The naming series or empty string if not available
+	"""
+	series = ''
+	if batch_uses_naming_series():
+		prefix = _get_batch_prefix()
+		key = _make_naming_series_key(prefix)
+		series = key
+
+	return series
+
+
 class Batch(Document):
 	def autoname(self):
 		"""Generate random ID for batch if not specified"""
 		if not self.batch_id:
 			if frappe.db.get_value('Item', self.item, 'create_new_batch'):
-				use_naming_series = frappe.db.get_single_value('Stock Settings', 'use_naming_series')
+				if batch_uses_naming_series():
-				if use_naming_series:
 					self.batch_id = get_name_from_naming_series()
 				else:
 					self.batch_id = get_name_from_hash()
@@ -50,13 +110,17 @@ class Batch(Document):
 	def onload(self):
 		self.image = frappe.db.get_value('Item', self.item, 'image')
 
+	def after_delete(self):
+		revert_series_if_last(get_batch_naming_series(), self.name)
+
 	def validate(self):
 		self.item_has_batch_enabled()
 
 	def item_has_batch_enabled(self):
-		if frappe.db.get_value("Item",self.item,"has_batch_no") == 0:
+		if frappe.db.get_value("Item", self.item, "has_batch_no") == 0:
 			frappe.throw(_("The selected item cannot have Batch"))
 
+
 @frappe.whitelist()
 def get_batch_qty(batch_no=None, warehouse=None, item_code=None):
 	"""Returns batch actual qty if warehouse is passed,
@@ -89,16 +153,18 @@ def get_batch_qty(batch_no=None, warehouse=None, item_code=None):
 
 	return out
 
+
 @frappe.whitelist()
 def get_batches_by_oldest(item_code, warehouse):
 	"""Returns the oldest batch and qty for the given item_code and warehouse"""
-	batches = get_batch_qty(item_code = item_code, warehouse = warehouse)
+	batches = get_batch_qty(item_code=item_code, warehouse=warehouse)
 	batches_dates = [[batch, frappe.get_value('Batch', batch.batch_no, 'expiry_date')] for batch in batches]
 	batches_dates.sort(key=lambda tup: tup[1])
 	return batches_dates
 
+
 @frappe.whitelist()
-def split_batch(batch_no, item_code, warehouse, qty, new_batch_id = None):
+def split_batch(batch_no, item_code, warehouse, qty, new_batch_id=None):
 	"""Split the batch into a new batch"""
 	batch = frappe.get_doc(dict(doctype='Batch', item=item_code, batch_id=new_batch_id)).insert()
 	stock_entry = frappe.get_doc(dict(
@@ -106,16 +172,16 @@ def split_batch(batch_no, item_code, warehouse, qty, new_batch_id = None):
 		purpose='Repack',
 		items=[
 			dict(
-				item_code = item_code,
+				item_code=item_code,
-				qty = float(qty or 0),
+				qty=float(qty or 0),
-				s_warehouse = warehouse,
+				s_warehouse=warehouse,
-				batch_no = batch_no
+				batch_no=batch_no
 			),
 			dict(
-				item_code = item_code,
+				item_code=item_code,
-				qty = float(qty or 0),
+				qty=float(qty or 0),
-				t_warehouse = warehouse,
+				t_warehouse=warehouse,
-				batch_no = batch.name
+				batch_no=batch.name
 			),
 		]
 	))
@@ -124,7 +190,8 @@ def split_batch(batch_no, item_code, warehouse, qty, new_batch_id = None):
 
 	return batch.name
 
-def set_batch_nos(doc, warehouse_field, throw = False):
+
+def set_batch_nos(doc, warehouse_field, throw=False):
 	"""Automatically select `batch_no` for outgoing items in item table"""
 	for d in doc.items:
 		qty = d.get('stock_qty') or d.get('transfer_qty') or d.get('qty') or 0
@@ -138,6 +205,7 @@ def set_batch_nos(doc, warehouse_field, throw = False):
 				if flt(batch_qty) < flt(qty):
 					frappe.throw(_("Row #{0}: The batch {1} has only {2} qty. Please select another batch which has {3} qty available or split the row into multiple rows, to deliver/issue from multiple batches").format(d.idx, d.batch_no, batch_qty, d.qty))
 
+
 @frappe.whitelist()
 def get_batch_no(item_code, warehouse, qty=1, throw=False):
 	"""

erpnext/stock/doctype/batch/test_batch.py

@@ -7,6 +7,8 @@ from frappe.exceptions import ValidationError
 import unittest
 
 from erpnext.stock.doctype.batch.batch import get_batch_qty, UnableToSelectBatchError, get_batch_no
+from frappe.utils import cint
+
 
 class TestBatch(unittest.TestCase):
 
@@ -21,7 +23,7 @@ class TestBatch(unittest.TestCase):
 	def make_batch_item(cls, item_name):
 		from erpnext.stock.doctype.item.test_item import make_item
 		if not frappe.db.exists(item_name):
-			make_item(item_name, dict(has_batch_no = 1, create_new_batch = 1))
+			return make_item(item_name, dict(has_batch_no = 1, create_new_batch = 1))
 
 	def test_purchase_receipt(self, batch_qty = 100):
 		'''Test automated batch creation from Purchase Receipt'''
@@ -192,3 +194,37 @@ class TestBatch(unittest.TestCase):
 			]
 		)).insert()
 		stock_entry.submit()
+
+	def test_batch_name_with_naming_series(self):
+		stock_settings = frappe.get_single('Stock Settings')
+		use_naming_series = cint(stock_settings.use_naming_series)
+
+		if not use_naming_series:
+			frappe.set_value('Stock Settings', 'Stock Settings', 'use_naming_series', 1)
+
+		batch = self.make_new_batch('_Test Stock Item For Batch Test1')
+		batch_name = batch.name
+
+		self.assertTrue(batch_name.startswith('BATCH-'))
+
+		batch.delete()
+		batch = self.make_new_batch('_Test Stock Item For Batch Test2')
+
+		self.assertEqual(batch_name, batch.name)
+
+		# reset Stock Settings
+		if not use_naming_series:
+			frappe.set_value('Stock Settings', 'Stock Settings', 'use_naming_series', 0)
+
+	def make_new_batch(self, item_name, batch_id=None, do_not_insert=0):
+		batch = frappe.new_doc('Batch')
+		item = self.make_batch_item(item_name)
+		batch.item = item.name
+
+		if batch_id:
+			batch.batch_id = batch_id
+
+		if not do_not_insert:
+			batch.insert()
+
+		return batch