chore: Less hacky tests, versioning (replace bom) and clearing log da…

…ta (update cost) - Remove `auto_commit_on_many_writes` in `update_cost_in_level()` as commits happen every N BOMs - Auto commit every 50 BOMs - test: Remove hacky `frappe.flags.in_test` returns - test: Enqueue `now` if in tests (for update cost and replace bom) - Replace BOM: Copy bom object to `_doc_before_save` so that version.py finds a difference between the two - Replace BOM: Add reference to version - Update Cost: Unset `processed_boms` if Log is completed (useless after completion) - test: `update_cost_in_all_boms_in_test` works close to actual prod implementation (only call Cron job manually) - Test: use `enqueue_replace_bom` so that test works closest to production behaviour Co-authored-by: Ankush Menat <[email protected]>
frappe · Jun 9, 2022 · 51b6a50 · 51b6a50
1 parent a6edce2
commit 51b6a50
Show file tree

Hide file tree

Showing 4 changed files with 23 additions and 80 deletions.
diff --git a/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.py b/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.py
@@ -67,16 +67,14 @@ def validate_bom_cost_update_in_progress(self):
 			)
 
 	def on_submit(self):
-		if frappe.flags.in_test:
-			return
-
 		if self.update_type == "Replace BOM":
 			boms = {"current_bom": self.current_bom, "new_bom": self.new_bom}
 			frappe.enqueue(
 				method="erpnext.manufacturing.doctype.bom_update_log.bom_update_log.run_replace_bom_job",
 				doc=self,
 				boms=boms,
 				timeout=40000,
+				now=frappe.flags.in_test,
 			)
 		else:
 			process_boms_cost_level_wise(self)
@@ -94,7 +92,7 @@ def run_replace_bom_job(
 
 		frappe.db.auto_commit_on_many_writes = 1
 		boms = frappe._dict(boms or {})
-		replace_bom(boms)
+		replace_bom(boms, doc.name)
 
 		doc.db_set("status", "Completed")
 	except Exception:
@@ -135,10 +133,6 @@ def process_boms_cost_level_wise(
 		values = {"current_level": current_level}
 
 	set_values_in_log(update_doc.name, values, commit=True)
-
-	if frappe.flags.in_test:
-		return current_boms, current_level
-
 	queue_bom_cost_jobs(current_boms, update_doc, current_level)
 
 
@@ -161,16 +155,13 @@ def queue_bom_cost_jobs(
 		)
 		batch_row.db_insert()
 
-		if frappe.flags.in_test:
-			# skip background jobs in test
-			return boms_to_process, batch_row.name
-
 		frappe.enqueue(
 			method="erpnext.manufacturing.doctype.bom_update_log.bom_updation_utils.update_cost_in_level",
 			doc=update_doc,
 			bom_list=boms_to_process,
 			batch_name=batch_row.name,
 			queue="long",
+			now=frappe.flags.in_test,
 		)
 
 
@@ -208,10 +199,11 @@ def resume_bom_cost_update_jobs():
 		current_boms, processed_boms = get_processed_current_boms(log, bom_batches)
 		parent_boms = get_next_higher_level_boms(child_boms=current_boms, processed_boms=processed_boms)
 
+		# Unset processed BOMs if log is complete, it is used for next level BOMs
 		set_values_in_log(
 			log.name,
 			values={
-				"processed_boms": json.dumps(processed_boms),
+				"processed_boms": json.dumps([] if not parent_boms else processed_boms),
 				"status": "Completed" if not parent_boms else "In Progress",
 			},
 			commit=True,

diff --git a/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py b/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py
@@ -1,6 +1,7 @@
 # Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and contributors
 # For license information, please see license.txt
 
+import copy
 import json
 from collections import defaultdict
 from typing import TYPE_CHECKING, Any, Dict, List, Optional, Union
@@ -12,7 +13,7 @@
 from frappe import _
 
 
-def replace_bom(boms: Dict) -> None:
+def replace_bom(boms: Dict, log_name: str) -> None:
 	"Replace current BOM with new BOM in parent BOMs."
 
 	current_bom = boms.get("current_bom")
@@ -29,13 +30,17 @@ def replace_bom(boms: Dict) -> None:
 		# 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._doc_before_save = copy.deepcopy(bom_obj)
 		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()
+		bom_obj.flags.updater_reference = {
+			"doctype": "BOM Update Log",
+			"docname": log_name,
+			"label": _("via BOM Update Tool"),
+		}
+		bom_obj.save_version()
 
 
 def update_cost_in_level(
@@ -48,8 +53,6 @@ def update_cost_in_level(
 		if status == "Failed":
 			return
 
-		frappe.db.auto_commit_on_many_writes = 1
-
 		update_cost_in_boms(bom_list=bom_list)  # main updation logic
 
 		bom_batch = frappe.qb.DocType("BOM Update Batch")
@@ -62,8 +65,6 @@ def update_cost_in_level(
 	except Exception:
 		handle_exception(doc)
 	finally:
-		frappe.db.auto_commit_on_many_writes = 0
-
 		if not frappe.flags.in_test:
 			frappe.db.commit()  # nosemgrep
 
@@ -121,7 +122,7 @@ def update_cost_in_boms(bom_list: List[str]) -> None:
 		bom_doc.calculate_cost(save_updates=True, update_hour_rate=True)
 		bom_doc.db_update()
 
-		if (index % 100 == 0) and not frappe.flags.in_test:
+		if (index % 50 == 0) and not frappe.flags.in_test:
 			frappe.db.commit()  # nosemgrep
 
 

diff --git a/erpnext/manufacturing/doctype/bom_update_log/test_bom_update_log.py b/erpnext/manufacturing/doctype/bom_update_log/test_bom_update_log.py
@@ -8,15 +8,7 @@
 
 from erpnext.manufacturing.doctype.bom_update_log.bom_update_log import (
 	BOMMissingError,
-	get_processed_current_boms,
-	process_boms_cost_level_wise,
-	queue_bom_cost_jobs,
-	run_replace_bom_job,
-)
-from erpnext.manufacturing.doctype.bom_update_log.bom_updation_utils import (
-	get_next_higher_level_boms,
-	set_values_in_log,
-	update_cost_in_level,
+	resume_bom_cost_update_jobs,
 )
 from erpnext.manufacturing.doctype.bom_update_tool.bom_update_tool import (
 	enqueue_replace_bom,
@@ -60,62 +52,22 @@ def test_bom_update_log_validate(self):
 		with self.assertRaises(frappe.ValidationError):
 			enqueue_replace_bom(boms=frappe._dict(current_bom=self.boms.new_bom, new_bom="Dummy BOM"))
 
-	def test_bom_update_log_queueing(self):
-		"Test if BOM Update Log is created and queued."
-
-		log = enqueue_replace_bom(boms=self.boms)
-
-		self.assertEqual(log.docstatus, 1)
-		self.assertEqual(log.status, "Queued")
-
 	def test_bom_update_log_completion(self):
 		"Test if BOM Update Log handles job completion correctly."
 
 		log = enqueue_replace_bom(boms=self.boms)
-
-		# Is run via background job IRL
-		run_replace_bom_job(doc=log, boms=self.boms)
 		log.reload()
-
 		self.assertEqual(log.status, "Completed")
 
 
 def update_cost_in_all_boms_in_test():
 	"""
-	Utility to run 'Update Cost' job in tests immediately without Cron job.
-	Run job for all levels (manually) until fully complete.
+	Utility to run 'Update Cost' job in tests without Cron job until fully complete.
 	"""
-	parent_boms = []
 	log = enqueue_update_cost()  # create BOM Update Log
 
 	while log.status != "Completed":
-		level_boms, current_level = process_boms_cost_level_wise(log, parent_boms)
-		log.reload()
-
-		boms, batch = queue_bom_cost_jobs(
-			level_boms, log, current_level
-		)  # adds rows in log for tracking
-		log.reload()
-
-		update_cost_in_level(log, boms, batch)  # business logic
-		log.reload()
-
-		# current level done, get next level boms
-		bom_batches = frappe.db.get_all(
-			"BOM Update Batch",
-			{"parent": log.name, "level": log.current_level},
-			["name", "boms_updated", "status"],
-		)
-		current_boms, processed_boms = get_processed_current_boms(log, bom_batches)
-		parent_boms = get_next_higher_level_boms(child_boms=current_boms, processed_boms=processed_boms)
-
-		set_values_in_log(
-			log.name,
-			values={
-				"processed_boms": json.dumps(processed_boms),
-				"status": "Completed" if not parent_boms else "In Progress",
-			},
-		)
+		resume_bom_cost_update_jobs()  # run cron job until complete
 		log.reload()
 
 	return log
diff --git a/erpnext/manufacturing/doctype/bom_update_tool/test_bom_update_tool.py b/erpnext/manufacturing/doctype/bom_update_tool/test_bom_update_tool.py
@@ -4,10 +4,10 @@
 import frappe
 from frappe.tests.utils import FrappeTestCase
 
-from erpnext.manufacturing.doctype.bom_update_log.bom_update_log import replace_bom
 from erpnext.manufacturing.doctype.bom_update_log.test_bom_update_log import (
 	update_cost_in_all_boms_in_test,
 )
+from erpnext.manufacturing.doctype.bom_update_tool.bom_update_tool import enqueue_replace_bom
 from erpnext.manufacturing.doctype.production_plan.test_production_plan import make_bom
 from erpnext.stock.doctype.item.test_item import create_item
 
@@ -17,6 +17,9 @@
 class TestBOMUpdateTool(FrappeTestCase):
 	"Test major functions run via BOM Update Tool."
 
+	def tearDown(self):
+		frappe.db.rollback()
+
 	def test_replace_bom(self):
 		current_bom = "BOM-_Test Item Home Desktop Manufactured-001"
 
@@ -25,16 +28,11 @@ def test_replace_bom(self):
 		bom_doc.insert()
 
 		boms = frappe._dict(current_bom=current_bom, new_bom=bom_doc.name)
-		replace_bom(boms)
+		enqueue_replace_bom(boms=boms)
 
 		self.assertFalse(frappe.db.exists("BOM Item", {"bom_no": current_bom, "docstatus": 1}))
 		self.assertTrue(frappe.db.exists("BOM Item", {"bom_no": bom_doc.name, "docstatus": 1}))
 
-		# reverse, as it affects other testcases
-		boms.current_bom = bom_doc.name
-		boms.new_bom = current_bom
-		replace_bom(boms)
-
 	def test_bom_cost(self):
 		for item in ["BOM Cost Test Item 1", "BOM Cost Test Item 2", "BOM Cost Test Item 3"]:
 			item_doc = create_item(item, valuation_rate=100)