From cff91558d4f380cc7566d009ea85ccba36976f69 Mon Sep 17 00:00:00 2001 From: marination Date: Thu, 17 Mar 2022 12:32:37 +0530 Subject: [PATCH] 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 --- .../bom_update_log/bom_update_log.json | 8 +- .../doctype/bom_update_log/bom_update_log.py | 146 ++++++++++++------ .../bom_update_tool/bom_update_tool.js | 43 +++++- .../bom_update_tool/bom_update_tool.py | 108 ++++--------- 4 files changed, 171 insertions(+), 134 deletions(-) diff --git a/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.json b/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.json index 222168be8c..d89427edc0 100644 --- a/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.json +++ b/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", - "reqd": 1 + "options": "BOM" }, { "fieldname": "new_bom", "fieldtype": "Link", "in_list_view": 1, "label": "New BOM", - "options": "BOM", - "reqd": 1 + "options": "BOM" }, { "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", diff --git a/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.py b/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.py index 10db0de9a1..b08d6f906c 100644 --- a/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.py +++ b/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 erpnext.manufacturing.doctype.bom.bom import get_boms_in_bottom_up_order - +from frappe.utils import cstr, flt 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() - self.validate_same_bom() - self.validate_bom_items() + if self.update_type == "Replace BOM": + self.validate_boms_are_specified() + 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", - boms=boms, doc=self, timeout=40000 + method="erpnext.manufacturing.doctype.bom_update_log.bom_update_log.run_bom_job", + doc=self, boms=boms, timeout=40000 ) else: frappe.enqueue( - method="erpnext.manufacturing.doctype.bom_update_tool.bom_update_tool.update_cost_queue", - doc=self, timeout=40000 + method="erpnext.manufacturing.doctype.bom_update_log.bom_update_log.run_bom_job", + 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) - doc = frappe.get_doc("BOM Update Tool") - doc.current_bom = args.current_bom - doc.new_bom = args.new_bom - doc.replace_bom() + boms = frappe._dict(boms or {}) + + if update_type == "Replace BOM": + replace_bom(boms) + 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) \ No newline at end of file diff --git a/erpnext/manufacturing/doctype/bom_update_tool/bom_update_tool.js b/erpnext/manufacturing/doctype/bom_update_tool/bom_update_tool.js index bf5fe2e18d..ec6a76d61c 100644 --- a/erpnext/manufacturing/doctype/bom_update_tool/bom_update_tool.js +++ b/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() { - frappe.msgprint(__("Latest price updated in all BOMs")); + callback: result => { + 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" + }); } }); diff --git a/erpnext/manufacturing/doctype/bom_update_tool/bom_update_tool.py b/erpnext/manufacturing/doctype/bom_update_tool/bom_update_tool.py index fad53f0568..16add4fc11 100644 --- a/erpnext/manufacturing/doctype/bom_update_tool/bom_update_tool.py +++ b/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): - 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 + pass @frappe.whitelist() -def enqueue_replace_bom(args): - if isinstance(args, str): - args = json.loads(args) - - create_bom_update_log(boms=args) - frappe.msgprint(_("Queued for replacing the BOM. It may take a few minutes.")) +def enqueue_replace_bom(boms: Optional[Union[Dict, str]] = None, args: Optional[Union[Dict, str]] = None) -> "BOMUpdateLog": + """Returns a BOM Update Log (that queues a job) for BOM Replacement.""" + boms = boms or args + if isinstance(boms, str): + boms = json.loads(boms) + update_log = create_bom_update_log(boms=boms) + return update_log @frappe.whitelist() -def enqueue_update_cost(): - create_bom_update_log(update_type="Update Cost") - frappe.msgprint(_("Queued for updating latest price in all Bill of Materials. It may take a few minutes.")) +def enqueue_update_cost() -> "BOMUpdateLog": + """Returns a BOM Update Log (that queues a job) for BOM Cost Updation.""" + update_log = create_bom_update_log(update_type="Update Cost") + return update_log -def auto_update_latest_price_in_all_boms(): - "Called via hooks.py." +def auto_update_latest_price_in_all_boms() -> None: + """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"): - "Creates a BOM Update Log that handles the background job." - current_bom = boms.get("current_bom") if boms else None - new_bom = boms.get("new_bom") if boms else None - log_doc = frappe.get_doc({ +def update_cost() -> None: + """Updates Cost for all BOMs from bottom to top.""" + 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) + +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 - }) - log_doc.submit() \ No newline at end of file + "update_type": update_type, + }).submit() \ No newline at end of file