chore: Polish error handling and code sepration

- Added Typing
- Moved all job business logic to bom update log
- Added `run_bom_job` that handles errors and runs either of two methods
- UX: Replace button disabled until both inputs are filled
- Show log creation message on UI for correctness
- APIs return log document as result
- Converted raw sql to QB

erpnext/manufacturing/doctype/bom_update_log/bom_update_log.json

@@ -20,16 +20,14 @@
    "fieldtype": "Link",
    "in_list_view": 1,
    "label": "Current BOM",
-   "options": "BOM",
+   "options": "BOM"
-   "reqd": 1
   },
   {
    "fieldname": "new_bom",
    "fieldtype": "Link",
    "in_list_view": 1,
    "label": "New BOM",
-   "options": "BOM",
+   "options": "BOM"
-   "reqd": 1
   },
   {
    "fieldname": "column_break_3",
@@ -61,7 +59,7 @@
  "index_web_pages_for_search": 1,
  "is_submittable": 1,
  "links": [],
- "modified": "2022-03-16 18:25:49.833836",
+ "modified": "2022-03-17 12:21:16.156437",
  "modified_by": "Administrator",
  "module": "Manufacturing",
  "name": "BOM Update Log",

erpnext/manufacturing/doctype/bom_update_log/bom_update_log.py

@@ -1,23 +1,27 @@
 # Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and contributors
 # For license information, please see license.txt
+from typing import Dict, List, Optional
+import click
 
 import frappe
 from frappe import _
 from frappe.model.document import Document
-from frappe.utils import cstr
+from frappe.utils import cstr, flt
-
-from erpnext.manufacturing.doctype.bom.bom import get_boms_in_bottom_up_order
-
 from rq.timeouts import JobTimeoutException
 
+from erpnext.manufacturing.doctype.bom_update_tool.bom_update_tool import update_cost
 
-class BOMMissingError(frappe.ValidationError): pass
+
+class BOMMissingError(frappe.ValidationError):
+	pass
 
 class BOMUpdateLog(Document):
 	def validate(self):
-		self.validate_boms_are_specified()
+		if self.update_type == "Replace BOM":
-		self.validate_same_bom()
+			self.validate_boms_are_specified()
-		self.validate_bom_items()
+			self.validate_same_bom()
+			self.validate_bom_items()
+
 		self.status = "Queued"
 
 	def validate_boms_are_specified(self):
@@ -48,16 +52,88 @@ class BOMUpdateLog(Document):
 				"new_bom": self.new_bom
 			}
 			frappe.enqueue(
-				method="erpnext.manufacturing.doctype.bom_update_tool.bom_update_tool.replace_bom",
+				method="erpnext.manufacturing.doctype.bom_update_log.bom_update_log.run_bom_job",
-				boms=boms, doc=self, timeout=40000
+				doc=self, boms=boms, timeout=40000
 			)
 		else:
 			frappe.enqueue(
-				method="erpnext.manufacturing.doctype.bom_update_tool.bom_update_tool.update_cost_queue",
+				method="erpnext.manufacturing.doctype.bom_update_log.bom_update_log.run_bom_job",
-				doc=self, timeout=40000
+				doc=self, update_type="Update Cost", timeout=40000
 			)
 
-def replace_bom(boms, doc):
+def replace_bom(boms: Dict) -> None:
+	"""Replace current BOM with new BOM in parent BOMs."""
+	current_bom = boms.get("current_bom")
+	new_bom = boms.get("new_bom")
+
+	unit_cost = get_new_bom_unit_cost(new_bom)
+	update_new_bom(unit_cost, current_bom, new_bom)
+
+	frappe.cache().delete_key('bom_children')
+	parent_boms = get_parent_boms(new_bom)
+
+	with click.progressbar(parent_boms) as parent_boms:
+		pass
+	for bom in parent_boms:
+		bom_obj = frappe.get_cached_doc('BOM', bom)
+		# this is only used for versioning and we do not want
+		# to make separate db calls by using load_doc_before_save
+		# which proves to be expensive while doing bulk replace
+		bom_obj._doc_before_save = bom_obj
+		bom_obj.update_new_bom(unit_cost, current_bom, new_bom)
+		bom_obj.update_exploded_items()
+		bom_obj.calculate_cost()
+		bom_obj.update_parent_cost()
+		bom_obj.db_update()
+		if bom_obj.meta.get('track_changes') and not bom_obj.flags.ignore_version:
+			bom_obj.save_version()
+
+def update_new_bom(unit_cost: float, current_bom: str, new_bom: str) -> None:
+		bom_item = frappe.qb.DocType("BOM Item")
+		frappe.qb.update(bom_item).set(
+			bom_item.bom_no, new_bom
+		).set(
+			bom_item.rate, unit_cost
+		).set(
+			bom_item.amount, (bom_item.stock_qty * unit_cost)
+		).where(
+			(bom_item.bom_no == current_bom)
+			& (bom_item.docstatus < 2)
+			& (bom_item.parenttype == "BOM")
+		).run()
+
+def get_parent_boms(new_bom: str, bom_list: Optional[List] = None) -> List:
+		bom_list = bom_list or []
+		bom_item = frappe.qb.DocType("BOM Item")
+
+		parents = frappe.qb.from_(bom_item).select(
+			bom_item.parent
+		).where(
+			(bom_item.bom_no == new_bom)
+			& (bom_item.docstatus <2)
+			& (bom_item.parenttype == "BOM")
+		).run(as_dict=True)
+
+		for d in parents:
+			if new_bom == d.parent:
+				frappe.throw(_("BOM recursion: {0} cannot be child of {1}").format(new_bom, d.parent))
+
+			bom_list.append(d.parent)
+			get_parent_boms(d.parent, bom_list)
+
+		return list(set(bom_list))
+
+def get_new_bom_unit_cost(new_bom: str) -> float:
+	bom = frappe.qb.DocType("BOM")
+	new_bom_unitcost = frappe.qb.from_(bom).select(
+		bom.total_cost / bom.quantity
+	).where(
+		bom.name == new_bom
+	).run()
+
+	return flt(new_bom_unitcost[0][0])
+
+def run_bom_job(doc: "BOMUpdateLog", boms: Optional[Dict] = None, update_type: Optional[str] = "Replace BOM") -> None:
 	try:
 		doc.db_set("status", "In Progress")
 		if not frappe.flags.in_test:
@@ -65,18 +141,19 @@ def replace_bom(boms, doc):
 
 		frappe.db.auto_commit_on_many_writes = 1
 
-		args = frappe._dict(boms)
+		boms = frappe._dict(boms or {})
-		doc = frappe.get_doc("BOM Update Tool")
+
-		doc.current_bom = args.current_bom
+		if update_type == "Replace BOM":
-		doc.new_bom = args.new_bom
+			replace_bom(boms)
-		doc.replace_bom()
+		else:
+			update_cost()
 
 		doc.db_set("status", "Completed")
 
 	except (Exception, JobTimeoutException):
 		frappe.db.rollback()
 		frappe.log_error(
-			msg=frappe.get_traceback(),
+			message=frappe.get_traceback(),
 			title=_("BOM Update Tool Error")
 		)
 		doc.db_set("status", "Failed")
@@ -84,34 +161,3 @@ def replace_bom(boms, doc):
 	finally:
 		frappe.db.auto_commit_on_many_writes = 0
 		frappe.db.commit()
-
-def update_cost_queue(doc):
-	try:
-		doc.db_set("status", "In Progress")
-		if not frappe.flags.in_test:
-			frappe.db.commit()
-
-		frappe.db.auto_commit_on_many_writes = 1
-
-		bom_list = get_boms_in_bottom_up_order()
-		for bom in bom_list:
-			frappe.get_doc("BOM", bom).update_cost(update_parent=False, from_child_bom=True)
-
-		doc.db_set("status", "Completed")
-
-	except (Exception, JobTimeoutException):
-		frappe.db.rollback()
-		frappe.log_error(
-			msg=frappe.get_traceback(),
-			title=_("BOM Update Tool Error")
-		)
-		doc.db_set("status", "Failed")
-
-	finally:
-		frappe.db.auto_commit_on_many_writes = 0
-		frappe.db.commit()
-
-def update_cost():
-	bom_list = get_boms_in_bottom_up_order()
-	for bom in bom_list:
-		frappe.get_doc("BOM", bom).update_cost(update_parent=False, from_child_bom=True)

erpnext/manufacturing/doctype/bom_update_tool/bom_update_tool.js

@@ -20,30 +20,63 @@ frappe.ui.form.on('BOM Update Tool', {
 
 	refresh: function(frm) {
 		frm.disable_save();
+		frm.events.disable_button(frm, "replace");
 	},
 
-	replace: function(frm) {
+	disable_button: (frm, field, disable=true) => {
+		frm.get_field(field).input.disabled = disable;
+	},
+
+	current_bom: (frm) => {
+		if (frm.doc.current_bom && frm.doc.new_bom){
+			frm.events.disable_button(frm, "replace", false);
+		}
+	},
+
+	new_bom: (frm) => {
+		if (frm.doc.current_bom && frm.doc.new_bom){
+			frm.events.disable_button(frm, "replace", false);
+		}
+	},
+
+	replace: (frm) => {
 		if (frm.doc.current_bom && frm.doc.new_bom) {
 			frappe.call({
 				method: "erpnext.manufacturing.doctype.bom_update_tool.bom_update_tool.enqueue_replace_bom",
 				freeze: true,
 				args: {
-					args: {
+					boms: {
 						"current_bom": frm.doc.current_bom,
 						"new_bom": frm.doc.new_bom
 					}
+				},
+				callback: result => {
+					if (result && result.message && !result.exc) {
+						frm.events.confirm_job_start(frm, result.message);
+					}
 				}
 			});
 		}
 	},
 
-	update_latest_price_in_all_boms: function() {
+	update_latest_price_in_all_boms: (frm) => {
 		frappe.call({
 			method: "erpnext.manufacturing.doctype.bom_update_tool.bom_update_tool.enqueue_update_cost",
 			freeze: true,
-			callback: function() {
+			callback: result => {
-				frappe.msgprint(__("Latest price updated in all BOMs"));
+				if (result && result.message && !result.exc) {
+					frm.events.confirm_job_start(frm, result.message);
+				}
 			}
 		});
+	},
+
+	confirm_job_start: (frm, log_data) => {
+		let log_link = frappe.utils.get_form_link("BOM Update Log", log_data.name, true)
+		frappe.msgprint({
+			"message": __(`BOM Updation is queued and may take a few minutes. Check ${log_link} for progress.`),
+			"title": __("BOM Update Initiated"),
+			"indicator": "blue"
+		});
 	}
 });

erpnext/manufacturing/doctype/bom_update_tool/bom_update_tool.py

@@ -1,99 +1,59 @@
-# Copyright (c) 2017, Frappe Technologies Pvt. Ltd. and contributors
+# Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and contributors
 # For license information, please see license.txt
 
-
 import json
+from typing import Dict, List, Optional, TYPE_CHECKING, Union
+
+if TYPE_CHECKING:
+	from erpnext.manufacturing.doctype.bom_update_log.bom_update_log import BOMUpdateLog
 
-import click
 import frappe
 from frappe import _
 from frappe.model.document import Document
 from frappe.utils import cstr, flt
 
-from erpnext.manufacturing.doctype.bom_update_log.bom_update_log import update_cost
+from erpnext.manufacturing.doctype.bom.bom import get_boms_in_bottom_up_order
 
 
 class BOMUpdateTool(Document):
-	def replace_bom(self):
+	pass
-		unit_cost = get_new_bom_unit_cost(self.new_bom)
-		self.update_new_bom(unit_cost)
-
-		frappe.cache().delete_key('bom_children')
-		bom_list = self.get_parent_boms(self.new_bom)
-
-		with click.progressbar(bom_list) as bom_list:
-			pass
-		for bom in bom_list:
-			try:
-				bom_obj = frappe.get_cached_doc('BOM', bom)
-				# this is only used for versioning and we do not want
-				# to make separate db calls by using load_doc_before_save
-				# which proves to be expensive while doing bulk replace
-				bom_obj._doc_before_save = bom_obj
-				bom_obj.update_new_bom(self.current_bom, self.new_bom, unit_cost)
-				bom_obj.update_exploded_items()
-				bom_obj.calculate_cost()
-				bom_obj.update_parent_cost()
-				bom_obj.db_update()
-				if bom_obj.meta.get('track_changes') and not bom_obj.flags.ignore_version:
-					bom_obj.save_version()
-			except Exception:
-				frappe.log_error(frappe.get_traceback())
-
-	def update_new_bom(self, unit_cost):
-		frappe.db.sql("""update `tabBOM Item` set bom_no=%s,
-			rate=%s, amount=stock_qty*%s where bom_no = %s and docstatus < 2 and parenttype='BOM'""",
-			(self.new_bom, unit_cost, unit_cost, self.current_bom))
-
-	def get_parent_boms(self, bom, bom_list=None):
-		if bom_list is None:
-			bom_list = []
-		data = frappe.db.sql("""SELECT DISTINCT parent FROM `tabBOM Item`
-			WHERE bom_no = %s AND docstatus < 2 AND parenttype='BOM'""", bom)
-
-		for d in data:
-			if self.new_bom == d[0]:
-				frappe.throw(_("BOM recursion: {0} cannot be child of {1}").format(bom, self.new_bom))
-
-			bom_list.append(d[0])
-			self.get_parent_boms(d[0], bom_list)
-
-		return list(set(bom_list))
-
-def get_new_bom_unit_cost(bom):
-	new_bom_unitcost = frappe.db.sql("""SELECT `total_cost`/`quantity`
-		FROM `tabBOM` WHERE name = %s""", bom)
-
-	return flt(new_bom_unitcost[0][0]) if new_bom_unitcost else 0
 
 @frappe.whitelist()
-def enqueue_replace_bom(args):
+def enqueue_replace_bom(boms: Optional[Union[Dict, str]] = None, args: Optional[Union[Dict, str]] = None) -> "BOMUpdateLog":
-	if isinstance(args, str):
+	"""Returns a BOM Update Log (that queues a job) for BOM Replacement."""
-		args = json.loads(args)
+	boms = boms or args
-
+	if isinstance(boms, str):
-	create_bom_update_log(boms=args)
+		boms = json.loads(boms)
-	frappe.msgprint(_("Queued for replacing the BOM. It may take a few minutes."))
 
+	update_log = create_bom_update_log(boms=boms)
+	return update_log
 
 @frappe.whitelist()
-def enqueue_update_cost():
+def enqueue_update_cost() -> "BOMUpdateLog":
-	create_bom_update_log(update_type="Update Cost")
+	"""Returns a BOM Update Log (that queues a job) for BOM Cost Updation."""
-	frappe.msgprint(_("Queued for updating latest price in all Bill of Materials. It may take a few minutes."))
+	update_log = create_bom_update_log(update_type="Update Cost")
+	return update_log
 
 
-def auto_update_latest_price_in_all_boms():
+def auto_update_latest_price_in_all_boms() -> None:
-	"Called via hooks.py."
+	"""Called via hooks.py."""
 	if frappe.db.get_single_value("Manufacturing Settings", "update_bom_costs_automatically"):
 		update_cost()
 
-def create_bom_update_log(boms=None, update_type="Replace BOM"):
+def update_cost() -> None:
-	"Creates a BOM Update Log that handles the background job."
+	"""Updates Cost for all BOMs from bottom to top."""
-	current_bom = boms.get("current_bom") if boms else None
+	bom_list = get_boms_in_bottom_up_order()
-	new_bom = boms.get("new_bom") if boms else None
+	for bom in bom_list:
-	log_doc = frappe.get_doc({
+		frappe.get_doc("BOM", bom).update_cost(update_parent=False, from_child_bom=True)
+
+def create_bom_update_log(boms: Optional[Dict] = None, update_type: str = "Replace BOM") -> "BOMUpdateLog":
+	"""Creates a BOM Update Log that handles the background job."""
+	boms = boms or {}
+	current_bom = boms.get("current_bom")
+	new_bom = boms.get("new_bom")
+	return frappe.get_doc({
 		"doctype": "BOM Update Log",
 		"current_bom": current_bom,
 		"new_bom": new_bom,
-		"update_type": update_type
+		"update_type": update_type,
-	})
+	}).submit()
-	log_doc.submit()