From b6e46eea80da79d57bfcebdfbe5831f68b7e60e3 Mon Sep 17 00:00:00 2001 From: marination Date: Wed, 18 May 2022 13:00:00 +0530 Subject: [PATCH 01/20] perf: `get_boms_in_bottom_up_order` - Create child-parent map once and fetch value from child key to get parents - Get parents recursively for a leaf node (get all ancestors) - Approx. 44 secs for 4lakh 70k boms --- erpnext/manufacturing/doctype/bom/bom.py | 90 +++++++++++++++++------- 1 file changed, 66 insertions(+), 24 deletions(-) diff --git a/erpnext/manufacturing/doctype/bom/bom.py b/erpnext/manufacturing/doctype/bom/bom.py index 220ce1dbd89e..a828869c36ad 100644 --- a/erpnext/manufacturing/doctype/bom/bom.py +++ b/erpnext/manufacturing/doctype/bom/bom.py @@ -1,11 +1,11 @@ -# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors +# Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and Contributors # License: GNU General Public License v3. See license.txt import functools import re -from collections import deque +from collections import defaultdict, deque from operator import itemgetter -from typing import List +from typing import List, Optional import frappe from frappe import _ @@ -1130,35 +1130,77 @@ def get_children(parent=None, is_root=False, **filters): return bom_items -def get_boms_in_bottom_up_order(bom_no=None): - def _get_parent(bom_no): +def get_boms_in_bottom_up_order(bom_no: Optional[str] = None) -> List: + def _generate_child_parent_map(): + bom = frappe.qb.DocType("BOM") + bom_item = frappe.qb.DocType("BOM Item") + + bom_parents = ( + frappe.qb.from_(bom_item) + .join(bom) + .on(bom_item.parent == bom.name) + .select(bom_item.bom_no, bom_item.parent) + .where( + (bom_item.bom_no.isnotnull()) + & (bom_item.bom_no != "") + & (bom.docstatus == 1) + & (bom.is_active == 1) + & (bom_item.parenttype == "BOM") + ) + ).run(as_dict=True) + + child_parent_map = defaultdict(list) + for bom in bom_parents: + child_parent_map[bom.bom_no].append(bom.parent) + + return child_parent_map + + def _get_flat_parent_map(leaf, child_parent_map): + parents_list = [] + + def _get_parents(node, parents_list): + "Returns updated ancestors list." + first_parents = child_parent_map.get(node) # immediate parents of node + if not first_parents: # top most node + return parents_list + + parents_list.extend(first_parents) + parents_list = list(dict.fromkeys(parents_list).keys()) # remove duplicates + + for nth_node in first_parents: + # recursively find parents + parents_list = _get_parents(nth_node, parents_list) + + return parents_list + + parents_list = _get_parents(leaf, parents_list) + return parents_list + + def _get_leaf_boms(): return frappe.db.sql_list( - """ - select distinct bom_item.parent from `tabBOM Item` bom_item - where bom_item.bom_no = %s and bom_item.docstatus=1 and bom_item.parenttype='BOM' - and exists(select bom.name from `tabBOM` bom where bom.name=bom_item.parent and bom.is_active=1) - """, - bom_no, + """select name from `tabBOM` bom + where docstatus=1 and is_active=1 + and not exists(select bom_no from `tabBOM Item` + where parent=bom.name and ifnull(bom_no, '')!='')""" ) - count = 0 bom_list = [] if bom_no: bom_list.append(bom_no) else: - # get all leaf BOMs - bom_list = frappe.db.sql_list( - """select name from `tabBOM` bom - where docstatus=1 and is_active=1 - and not exists(select bom_no from `tabBOM Item` - where parent=bom.name and ifnull(bom_no, '')!='')""" - ) + bom_list = _get_leaf_boms() + + child_parent_map = _generate_child_parent_map() + + for leaf_bom in bom_list: + # generate list recursively bottom to top + parent_list = _get_flat_parent_map(leaf_bom, child_parent_map) + + if not parent_list: + continue - while count < len(bom_list): - for child_bom in _get_parent(bom_list[count]): - if child_bom not in bom_list: - bom_list.append(child_bom) - count += 1 + bom_list.extend(parent_list) + bom_list = list(dict.fromkeys(bom_list).keys()) # remove duplicates return bom_list From 5932e9d78a70fc53f9f1c57ed201815f80960384 Mon Sep 17 00:00:00 2001 From: marination Date: Thu, 19 May 2022 20:22:13 +0530 Subject: [PATCH 02/20] fix: DB update child items, remove redundancy, fix perf MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Move `get_boms_in_bottom_up_order` in bom update tool’s file - Remove repeated rm cost update from `update_cost`. `calculate_cost` handles RM cost update - db_update children in `calculate_cost` optionally - Don’t call `update_exploded_items` and regenerate exploded items in `update_cost`. They will stay the same (except cost) --- erpnext/manufacturing/doctype/bom/bom.py | 121 ++---------------- .../bom_update_tool/bom_update_tool.py | 97 +++++++++++++- 2 files changed, 105 insertions(+), 113 deletions(-) diff --git a/erpnext/manufacturing/doctype/bom/bom.py b/erpnext/manufacturing/doctype/bom/bom.py index a828869c36ad..047bcc5239ce 100644 --- a/erpnext/manufacturing/doctype/bom/bom.py +++ b/erpnext/manufacturing/doctype/bom/bom.py @@ -3,9 +3,9 @@ import functools import re -from collections import defaultdict, deque +from collections import deque from operator import itemgetter -from typing import List, Optional +from typing import List import frappe from frappe import _ @@ -383,35 +383,9 @@ def update_cost(self, update_parent=True, from_child_bom=False, update_hour_rate existing_bom_cost = self.total_cost - for d in self.get("items"): - if not d.item_code: - continue - - rate = self.get_rm_rate( - { - "company": self.company, - "item_code": d.item_code, - "bom_no": d.bom_no, - "qty": d.qty, - "uom": d.uom, - "stock_uom": d.stock_uom, - "conversion_factor": d.conversion_factor, - "sourced_by_supplier": d.sourced_by_supplier, - } - ) - - if rate: - d.rate = rate - d.amount = flt(d.rate) * flt(d.qty) - d.base_rate = flt(d.rate) * flt(self.conversion_rate) - d.base_amount = flt(d.amount) * flt(self.conversion_rate) - - if save: - d.db_update() - if self.docstatus == 1: self.flags.ignore_validate_update_after_submit = True - self.calculate_cost(update_hour_rate) + self.calculate_cost(save_updates=save, update_hour_rate=update_hour_rate) if save: self.db_update() @@ -613,11 +587,11 @@ def _get_children(bom_no): bom_list.reverse() return bom_list - def calculate_cost(self, update_hour_rate=False): + def calculate_cost(self, save_update=False, update_hour_rate=False): """Calculate bom totals""" self.calculate_op_cost(update_hour_rate) - self.calculate_rm_cost() - self.calculate_sm_cost() + self.calculate_rm_cost(save=save_update) + self.calculate_sm_cost(save=save_update) self.total_cost = self.operating_cost + self.raw_material_cost - self.scrap_material_cost self.base_total_cost = ( self.base_operating_cost + self.base_raw_material_cost - self.base_scrap_material_cost @@ -659,7 +633,7 @@ def update_rate_and_time(self, row, update_hour_rate=False): if update_hour_rate: row.db_update() - def calculate_rm_cost(self): + def calculate_rm_cost(self, save=False): """Fetch RM rate as per today's valuation rate and calculate totals""" total_rm_cost = 0 base_total_rm_cost = 0 @@ -674,11 +648,13 @@ def calculate_rm_cost(self): total_rm_cost += d.amount base_total_rm_cost += d.base_amount + if save: + d.db_update() self.raw_material_cost = total_rm_cost self.base_raw_material_cost = base_total_rm_cost - def calculate_sm_cost(self): + def calculate_sm_cost(self, save=False): """Fetch RM rate as per today's valuation rate and calculate totals""" total_sm_cost = 0 base_total_sm_cost = 0 @@ -693,6 +669,8 @@ def calculate_sm_cost(self): ) total_sm_cost += d.amount base_total_sm_cost += d.base_amount + if save: + d.db_update() self.scrap_material_cost = total_sm_cost self.base_scrap_material_cost = base_total_sm_cost @@ -1130,81 +1108,6 @@ def get_children(parent=None, is_root=False, **filters): return bom_items -def get_boms_in_bottom_up_order(bom_no: Optional[str] = None) -> List: - def _generate_child_parent_map(): - bom = frappe.qb.DocType("BOM") - bom_item = frappe.qb.DocType("BOM Item") - - bom_parents = ( - frappe.qb.from_(bom_item) - .join(bom) - .on(bom_item.parent == bom.name) - .select(bom_item.bom_no, bom_item.parent) - .where( - (bom_item.bom_no.isnotnull()) - & (bom_item.bom_no != "") - & (bom.docstatus == 1) - & (bom.is_active == 1) - & (bom_item.parenttype == "BOM") - ) - ).run(as_dict=True) - - child_parent_map = defaultdict(list) - for bom in bom_parents: - child_parent_map[bom.bom_no].append(bom.parent) - - return child_parent_map - - def _get_flat_parent_map(leaf, child_parent_map): - parents_list = [] - - def _get_parents(node, parents_list): - "Returns updated ancestors list." - first_parents = child_parent_map.get(node) # immediate parents of node - if not first_parents: # top most node - return parents_list - - parents_list.extend(first_parents) - parents_list = list(dict.fromkeys(parents_list).keys()) # remove duplicates - - for nth_node in first_parents: - # recursively find parents - parents_list = _get_parents(nth_node, parents_list) - - return parents_list - - parents_list = _get_parents(leaf, parents_list) - return parents_list - - def _get_leaf_boms(): - return frappe.db.sql_list( - """select name from `tabBOM` bom - where docstatus=1 and is_active=1 - and not exists(select bom_no from `tabBOM Item` - where parent=bom.name and ifnull(bom_no, '')!='')""" - ) - - bom_list = [] - if bom_no: - bom_list.append(bom_no) - else: - bom_list = _get_leaf_boms() - - child_parent_map = _generate_child_parent_map() - - for leaf_bom in bom_list: - # generate list recursively bottom to top - parent_list = _get_flat_parent_map(leaf_bom, child_parent_map) - - if not parent_list: - continue - - bom_list.extend(parent_list) - bom_list = list(dict.fromkeys(bom_list).keys()) # remove duplicates - - return bom_list - - def add_additional_cost(stock_entry, work_order): # Add non stock items cost in the additional cost stock_entry.additional_costs = [] 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 b0e7da12017b..5b073b7539e3 100644 --- a/erpnext/manufacturing/doctype/bom_update_tool/bom_update_tool.py +++ b/erpnext/manufacturing/doctype/bom_update_tool/bom_update_tool.py @@ -2,7 +2,8 @@ # For license information, please see license.txt import json -from typing import TYPE_CHECKING, Dict, Literal, Optional, Union +from collections import defaultdict +from typing import TYPE_CHECKING, Dict, List, Literal, Optional, Union if TYPE_CHECKING: from erpnext.manufacturing.doctype.bom_update_log.bom_update_log import BOMUpdateLog @@ -10,8 +11,6 @@ import frappe from frappe.model.document import Document -from erpnext.manufacturing.doctype.bom.bom import get_boms_in_bottom_up_order - class BOMUpdateTool(Document): pass @@ -47,7 +46,10 @@ 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) + bom_doc = frappe.get_doc("BOM", bom) + bom_doc.calculate_cost(save_updates=True, update_hour_rate=True) + # bom_doc.update_exploded_items(save=True) #TODO: edit exploded items rate + bom_doc.db_update() def create_bom_update_log( @@ -67,3 +69,90 @@ def create_bom_update_log( "update_type": update_type, } ).submit() + + +def get_boms_in_bottom_up_order(bom_no: Optional[str] = None) -> List: + """ + Eg: Main BOM + |- Sub BOM 1 + |- Leaf BOM 1 + |- Sub BOM 2 + |- Leaf BOM 2 + Result: [Leaf BOM 1, Leaf BOM 2, Sub BOM 1, Sub BOM 2, Main BOM] + """ + leaf_boms = [] + if bom_no: + leaf_boms.append(bom_no) + else: + leaf_boms = _get_leaf_boms() + + child_parent_map = _generate_child_parent_map() + bom_list = leaf_boms.copy() + + for leaf_bom in leaf_boms: + parent_list = _get_flat_parent_map(leaf_bom, child_parent_map) + + if not parent_list: + continue + + bom_list.extend(parent_list) + bom_list = list(dict.fromkeys(bom_list).keys()) # remove duplicates + + return bom_list + + +def _generate_child_parent_map(): + bom = frappe.qb.DocType("BOM") + bom_item = frappe.qb.DocType("BOM Item") + + bom_parents = ( + frappe.qb.from_(bom_item) + .join(bom) + .on(bom_item.parent == bom.name) + .select(bom_item.bom_no, bom_item.parent) + .where( + (bom_item.bom_no.isnotnull()) + & (bom_item.bom_no != "") + & (bom.docstatus == 1) + & (bom.is_active == 1) + & (bom_item.parenttype == "BOM") + ) + ).run(as_dict=True) + + child_parent_map = defaultdict(list) + for bom in bom_parents: + child_parent_map[bom.bom_no].append(bom.parent) + + return child_parent_map + + +def _get_flat_parent_map(leaf, child_parent_map): + "Get ancestors at all levels of a leaf BOM." + parents_list = [] + + def _get_parents(node, parents_list): + "Returns recursively updated ancestors list." + first_parents = child_parent_map.get(node) # immediate parents of node + if not first_parents: # top most node + return parents_list + + parents_list.extend(first_parents) + parents_list = list(dict.fromkeys(parents_list).keys()) # remove duplicates + + for nth_node in first_parents: + # recursively find parents + parents_list = _get_parents(nth_node, parents_list) + + return parents_list + + parents_list = _get_parents(leaf, parents_list) + return parents_list + + +def _get_leaf_boms(): + return frappe.db.sql_list( + """select name from `tabBOM` bom + where docstatus=1 and is_active=1 + and not exists(select bom_no from `tabBOM Item` + where parent=bom.name and ifnull(bom_no, '')!='')""" + ) From 9dc30830887c3b6e303df6bb92ba1148799afea6 Mon Sep 17 00:00:00 2001 From: marination Date: Thu, 19 May 2022 20:33:48 +0530 Subject: [PATCH 03/20] fix: Call `calculate_cost` for Draft BOM and typo in argument --- erpnext/manufacturing/doctype/bom/bom.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/erpnext/manufacturing/doctype/bom/bom.py b/erpnext/manufacturing/doctype/bom/bom.py index 047bcc5239ce..399eb5a08796 100644 --- a/erpnext/manufacturing/doctype/bom/bom.py +++ b/erpnext/manufacturing/doctype/bom/bom.py @@ -385,7 +385,9 @@ def update_cost(self, update_parent=True, from_child_bom=False, update_hour_rate if self.docstatus == 1: self.flags.ignore_validate_update_after_submit = True - self.calculate_cost(save_updates=save, update_hour_rate=update_hour_rate) + + self.calculate_cost(save_updates=save, update_hour_rate=update_hour_rate) + if save: self.db_update() @@ -587,11 +589,11 @@ def _get_children(bom_no): bom_list.reverse() return bom_list - def calculate_cost(self, save_update=False, update_hour_rate=False): + def calculate_cost(self, save_updates=False, update_hour_rate=False): """Calculate bom totals""" self.calculate_op_cost(update_hour_rate) - self.calculate_rm_cost(save=save_update) - self.calculate_sm_cost(save=save_update) + self.calculate_rm_cost(save=save_updates) + self.calculate_sm_cost(save=save_updates) self.total_cost = self.operating_cost + self.raw_material_cost - self.scrap_material_cost self.base_total_cost = ( self.base_operating_cost + self.base_raw_material_cost - self.base_scrap_material_cost From 9a7e9d902d2b2960034cf2d7ad0d084dee01c1b4 Mon Sep 17 00:00:00 2001 From: marination Date: Thu, 19 May 2022 21:24:31 +0530 Subject: [PATCH 04/20] perf: Use cached doc instead of `get_doc` - Doc is only used to iterate over items(which wont change) and change rate/amount of rows - These changes are inserted in db via `db_update`, so no harm - Tested locally: refetching cached doc after db update, reflects fresh data. --- .../manufacturing/doctype/bom_update_tool/bom_update_tool.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 5b073b7539e3..e76572534081 100644 --- a/erpnext/manufacturing/doctype/bom_update_tool/bom_update_tool.py +++ b/erpnext/manufacturing/doctype/bom_update_tool/bom_update_tool.py @@ -46,7 +46,7 @@ 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: - bom_doc = frappe.get_doc("BOM", bom) + bom_doc = frappe.get_cached_doc("BOM", bom) bom_doc.calculate_cost(save_updates=True, update_hour_rate=True) # bom_doc.update_exploded_items(save=True) #TODO: edit exploded items rate bom_doc.db_update() From dd99c00eb64dc16b0f54a0cbf8e2c3b7f0f7e5fe Mon Sep 17 00:00:00 2001 From: marination Date: Thu, 19 May 2022 21:48:24 +0530 Subject: [PATCH 05/20] fix: Get fresh RM rate in `calculate_rm_cost` --- erpnext/manufacturing/doctype/bom/bom.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/erpnext/manufacturing/doctype/bom/bom.py b/erpnext/manufacturing/doctype/bom/bom.py index 399eb5a08796..560019a86d55 100644 --- a/erpnext/manufacturing/doctype/bom/bom.py +++ b/erpnext/manufacturing/doctype/bom/bom.py @@ -641,6 +641,18 @@ def calculate_rm_cost(self, save=False): base_total_rm_cost = 0 for d in self.get("items"): + d.rate = self.get_rm_rate( + { + "company": self.company, + "item_code": d.item_code, + "bom_no": d.bom_no, + "qty": d.qty, + "uom": d.uom, + "stock_uom": d.stock_uom, + "conversion_factor": d.conversion_factor, + "sourced_by_supplier": d.sourced_by_supplier, + } + ) d.base_rate = flt(d.rate) * flt(self.conversion_rate) d.amount = flt(d.rate, d.precision("rate")) * flt(d.qty, d.precision("qty")) d.base_amount = d.amount * flt(self.conversion_rate) From 90d4dc0cd6428d2befa0805c9903446e668c9ba8 Mon Sep 17 00:00:00 2001 From: marination Date: Fri, 20 May 2022 01:02:56 +0530 Subject: [PATCH 06/20] fix: `test_work_order_with_non_stock_item` - Use the right price list and currency to avoid rate conversion (1000/62.9), since rates are reset correctly now - Use RM rate based on Price List in BOM. Non stock item has no valuation --- .../production_plan/test_production_plan.py | 1 - .../doctype/work_order/test_work_order.py | 15 ++++++++++++--- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/erpnext/manufacturing/doctype/production_plan/test_production_plan.py b/erpnext/manufacturing/doctype/production_plan/test_production_plan.py index 891a4978789f..e88049d810d5 100644 --- a/erpnext/manufacturing/doctype/production_plan/test_production_plan.py +++ b/erpnext/manufacturing/doctype/production_plan/test_production_plan.py @@ -798,7 +798,6 @@ def make_bom(**args): for item in args.raw_materials: item_doc = frappe.get_doc("Item", item) - bom.append( "items", { diff --git a/erpnext/manufacturing/doctype/work_order/test_work_order.py b/erpnext/manufacturing/doctype/work_order/test_work_order.py index 2aba48231be8..27e7e24a823e 100644 --- a/erpnext/manufacturing/doctype/work_order/test_work_order.py +++ b/erpnext/manufacturing/doctype/work_order/test_work_order.py @@ -417,7 +417,7 @@ def test_work_order_with_non_stock_item(self): "doctype": "Item Price", "item_code": "_Test FG Non Stock Item", "price_list_rate": 1000, - "price_list": "Standard Buying", + "price_list": "_Test Price List India", } ).insert(ignore_permissions=True) @@ -426,8 +426,17 @@ def test_work_order_with_non_stock_item(self): item_code="_Test FG Item", target="_Test Warehouse - _TC", qty=1, basic_rate=100 ) - if not frappe.db.get_value("BOM", {"item": fg_item}): - make_bom(item=fg_item, rate=1000, raw_materials=["_Test FG Item", "_Test FG Non Stock Item"]) + if not frappe.db.get_value("BOM", {"item": fg_item, "docstatus": 1}): + bom = make_bom( + item=fg_item, + rate=1000, + raw_materials=["_Test FG Item", "_Test FG Non Stock Item"], + do_not_save=True, + ) + bom.rm_cost_as_per = "Price List" # non stock item won't have valuation rate + bom.buying_price_list = "_Test Price List India" + bom.currency = "INR" + bom.save() wo = make_wo_order_test_record(production_item=fg_item) From ab2d95a74d8beda1d751f7d795f37058826fff18 Mon Sep 17 00:00:00 2001 From: marination Date: Mon, 23 May 2022 13:00:00 +0530 Subject: [PATCH 07/20] feat: Level-wise BOM cost updation - Process BOMs level wise and Pause after level is complete - Cron job will resume Paused jobs, which will again process the new level and pause at the end - This will go on until all BOMs are updated - Added Progress section with fields to track updated BOMs in Log - Cleanup: Add BOM Updation utils file to contain helper functions/sub-functions - Cleanup: BOM Update Log file will only contain functions that are in direct context of the Log Co-authored-by: Gavin D'souza --- erpnext/hooks.py | 5 +- .../bom_update_log/bom_update_log.json | 29 ++- .../doctype/bom_update_log/bom_update_log.py | 169 ++++++------- .../bom_update_log/bom_updation_utils.py | 223 ++++++++++++++++++ .../bom_update_log/test_bom_update_log.py | 6 +- .../bom_update_tool/bom_update_tool.py | 102 +------- 6 files changed, 335 insertions(+), 199 deletions(-) create mode 100644 erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py diff --git a/erpnext/hooks.py b/erpnext/hooks.py index 813ac17ca01e..05f06b3bda20 100644 --- a/erpnext/hooks.py +++ b/erpnext/hooks.py @@ -392,9 +392,12 @@ scheduler_events = { "cron": { + "0/5 * * * *": [ + "erpnext.manufacturing.doctype.bom_update_log.bom_update_log.resume_bom_cost_update_jobs", + ], "0/30 * * * *": [ "erpnext.utilities.doctype.video.video.update_youtube_data", - ] + ], }, "all": [ "erpnext.projects.doctype.project.project.project_status_update_reminder", 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 98c1acb71cee..3455b8665735 100644 --- a/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.json +++ b/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.json @@ -13,6 +13,10 @@ "update_type", "status", "error_log", + "progress_section", + "current_boms", + "parent_boms", + "processed_boms", "amended_from" ], "fields": [ @@ -47,7 +51,7 @@ "fieldname": "status", "fieldtype": "Select", "label": "Status", - "options": "Queued\nIn Progress\nCompleted\nFailed" + "options": "Queued\nIn Progress\nPaused\nCompleted\nFailed" }, { "fieldname": "amended_from", @@ -63,13 +67,34 @@ "fieldtype": "Link", "label": "Error Log", "options": "Error Log" + }, + { + "fieldname": "progress_section", + "fieldtype": "Section Break", + "label": "Progress" + }, + { + "fieldname": "current_boms", + "fieldtype": "Text", + "label": "Current BOMs" + }, + { + "description": "Immediate parent BOMs", + "fieldname": "parent_boms", + "fieldtype": "Text", + "label": "Parent BOMs" + }, + { + "fieldname": "processed_boms", + "fieldtype": "Text", + "label": "Processed BOMs" } ], "in_create": 1, "index_web_pages_for_search": 1, "is_submittable": 1, "links": [], - "modified": "2022-03-31 12:51:44.885102", + "modified": "2022-05-23 14:42:14.725914", "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 c0770fac90a3..639628ac3837 100644 --- a/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.py +++ b/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.py @@ -1,13 +1,19 @@ # Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and contributors # For license information, please see license.txt -from typing import Dict, List, Literal, Optional +import json +from typing import Dict, Optional import frappe from frappe import _ from frappe.model.document import Document -from frappe.utils import cstr, flt +from frappe.utils import cstr -from erpnext.manufacturing.doctype.bom_update_tool.bom_update_tool import update_cost +from erpnext.manufacturing.doctype.bom_update_log.bom_updation_utils import ( + get_leaf_boms, + handle_exception, + replace_bom, + set_values_in_log, +) class BOMMissingError(frappe.ValidationError): @@ -49,116 +55,93 @@ def on_submit(self): 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_bom_job", + method="erpnext.manufacturing.doctype.bom_update_log.bom_update_log.run_replace_bom_job", doc=self, boms=boms, timeout=40000, ) else: - frappe.enqueue( - 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: 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_in_bom_items(unit_cost, current_bom, new_bom) - - frappe.cache().delete_key("bom_children") - parent_boms = get_parent_boms(new_bom) - - for bom in parent_boms: - bom_obj = frappe.get_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_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_in_bom_items(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() - ) + process_boms_cost_level_wise(self) - return flt(new_bom_unitcost[0][0]) - -def run_bom_job( +def run_replace_bom_job( doc: "BOMUpdateLog", boms: Optional[Dict[str, str]] = None, - update_type: Literal["Replace BOM", "Update Cost"] = "Replace BOM", ) -> None: try: doc.db_set("status", "In Progress") + if not frappe.flags.in_test: frappe.db.commit() frappe.db.auto_commit_on_many_writes = 1 - boms = frappe._dict(boms or {}) - - if update_type == "Replace BOM": - replace_bom(boms) - else: - update_cost() + replace_bom(boms) doc.db_set("status", "Completed") - except Exception: - frappe.db.rollback() - error_log = doc.log_error("BOM Update Tool Error") - - doc.db_set("status", "Failed") - doc.db_set("error_log", error_log.name) - + handle_exception(doc) finally: frappe.db.auto_commit_on_many_writes = 0 frappe.db.commit() # nosemgrep + + +def process_boms_cost_level_wise(update_doc: "BOMUpdateLog") -> None: + "Queue jobs at the start of new BOM Level in 'Update Cost' Jobs." + + current_boms, parent_boms = {}, [] + values = {} + + if update_doc.status == "Queued": + # First level yet to process. On Submit. + current_boms = {bom: False for bom in get_leaf_boms()} + values = { + "current_boms": json.dumps(current_boms), + "parent_boms": "[]", + "processed_boms": json.dumps({}), + "status": "In Progress", + } + else: + # status is Paused, resume. via Cron Job. + current_boms, parent_boms = json.loads(update_doc.current_boms), json.loads( + update_doc.parent_boms + ) + if not current_boms: + # Process the next level BOMs. Stage parents as current BOMs. + current_boms = {bom: False for bom in parent_boms} + values = { + "current_boms": json.dumps(current_boms), + "parent_boms": "[]", + "status": "In Progress", + } + + set_values_in_log(update_doc.name, values, commit=True) + queue_bom_cost_jobs(current_boms, update_doc) + + +def queue_bom_cost_jobs(current_boms: Dict, update_doc: "BOMUpdateLog") -> None: + "Queue batches of 20k BOMs of the same level to process parallelly" + current_boms_list = [bom for bom in current_boms] + + while current_boms_list: + boms_to_process = current_boms_list[:20000] # slice out batch of 20k BOMs + + # update list to exclude 20K (queued) BOMs + current_boms_list = current_boms_list[20000:] if len(current_boms_list) > 20000 else [] + frappe.enqueue( + method="erpnext.manufacturing.doctype.bom_update_log.bom_updation_utils.update_cost_in_level", + doc=update_doc, + bom_list=boms_to_process, + timeout=40000, + ) + + +def resume_bom_cost_update_jobs(): + "Called every 10 minutes via Cron job." + paused_jobs = frappe.db.get_all("BOM Update Log", {"status": "Paused"}) + if not paused_jobs: + return + + for job in paused_jobs: + # resume from next level + process_boms_cost_level_wise(update_doc=frappe.get_doc("BOM Update Log", job.name)) diff --git a/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py b/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py new file mode 100644 index 000000000000..b5964cec9d4b --- /dev/null +++ b/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py @@ -0,0 +1,223 @@ +# Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and contributors +# For license information, please see license.txt + +import json +from collections import defaultdict +from typing import TYPE_CHECKING, Dict, List, Optional + +if TYPE_CHECKING: + from erpnext.manufacturing.doctype.bom_update_log.bom_update_log import BOMUpdateLog + +import frappe +from frappe import _ + + +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_bom_unit_cost(new_bom) + update_new_bom_in_bom_items(unit_cost, current_bom, new_bom) + + frappe.cache().delete_key("bom_children") + parent_boms = get_ancestor_boms(new_bom) + + for bom in parent_boms: + bom_obj = frappe.get_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_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_cost_in_level(doc: "BOMUpdateLog", bom_list: List[str]) -> None: + "Updates Cost for BOMs within a given level. Runs via background jobs." + try: + status = frappe.db.get_value("BOM Update Log", doc.name, "status") + if status == "Failed": + return + + frappe.db.auto_commit_on_many_writes = 1 + # main updation logic + job_data = update_cost_in_boms(bom_list=bom_list, docname=doc.name) + + set_values_in_log( + doc.name, + values={ + "current_boms": json.dumps(job_data.get("current_boms")), + "processed_boms": json.dumps(job_data.get("processed_boms")), + }, + commit=True, + ) + + process_if_level_is_complete(doc.name, job_data["current_boms"], job_data["processed_boms"]) + except Exception: + handle_exception(doc) + finally: + frappe.db.auto_commit_on_many_writes = 0 + frappe.db.commit() # nosemgrep + + +def get_ancestor_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_ancestor_boms(d.parent, bom_list) + + return list(set(bom_list)) + + +def update_new_bom_in_bom_items(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_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 frappe.utils.flt(new_bom_unitcost[0][0]) + + +def update_cost_in_boms(bom_list: List[str], docname: str) -> Dict: + "Updates cost in given BOMs. Returns current and total updated BOMs." + updated_boms = {} # current boms that have been updated + + for bom in bom_list: + bom_doc = frappe.get_cached_doc("BOM", bom) + bom_doc.calculate_cost(save_updates=True, update_hour_rate=True) + # bom_doc.update_exploded_items(save=True) #TODO: edit exploded items rate + bom_doc.db_update() + updated_boms[bom] = True + + # Update processed BOMs in Log + log_data = frappe.db.get_values( + "BOM Update Log", docname, ["current_boms", "processed_boms"], as_dict=True + )[0] + + for field in ("current_boms", "processed_boms"): + log_data[field] = json.loads(log_data.get(field)) + log_data[field].update(updated_boms) + + return log_data + + +def process_if_level_is_complete(docname: str, current_boms: Dict, processed_boms: Dict) -> None: + "Prepare and set higher level BOMs in Log if current level is complete." + processing_complete = all(current_boms.get(bom) for bom in current_boms) + if not processing_complete: + return + + parent_boms = get_next_higher_level_boms(child_boms=current_boms, processed_boms=processed_boms) + set_values_in_log( + docname, + values={ + "current_boms": json.dumps({}), + "parent_boms": json.dumps(parent_boms), + "status": "Completed" if not parent_boms else "Paused", + }, + commit=True, + ) + + +def get_next_higher_level_boms(child_boms: Dict, processed_boms: Dict): + "Generate immediate higher level dependants with no unresolved dependencies." + + def _all_children_are_processed(parent): + bom_doc = frappe.get_cached_doc("BOM", parent) + return all(processed_boms.get(row.bom_no) for row in bom_doc.items if row.bom_no) + + dependants_map = _generate_dependants_map() + dependants = set() + for bom in child_boms: + parents = dependants_map.get(bom) or [] + for parent in parents: + if _all_children_are_processed(parent): + dependants.add(parent) + + return list(dependants) + + +def get_leaf_boms(): + return frappe.db.sql_list( + """select name from `tabBOM` bom + where docstatus=1 and is_active=1 + and not exists(select bom_no from `tabBOM Item` + where parent=bom.name and ifnull(bom_no, '')!='')""" + ) + + +def _generate_dependants_map(): + bom = frappe.qb.DocType("BOM") + bom_item = frappe.qb.DocType("BOM Item") + + bom_parents = ( + frappe.qb.from_(bom_item) + .join(bom) + .on(bom_item.parent == bom.name) + .select(bom_item.bom_no, bom_item.parent) + .where( + (bom_item.bom_no.isnotnull()) + & (bom_item.bom_no != "") + & (bom.docstatus == 1) + & (bom.is_active == 1) + & (bom_item.parenttype == "BOM") + ) + ).run(as_dict=True) + + child_parent_map = defaultdict(list) + for bom in bom_parents: + child_parent_map[bom.bom_no].append(bom.parent) + + return child_parent_map + + +def set_values_in_log(log_name: str, values: Dict, commit: bool = False) -> None: + "Update BOM Update Log record." + if not values: + return + + bom_update_log = frappe.qb.DocType("BOM Update Log") + query = frappe.qb.update(bom_update_log).where(bom_update_log.name == log_name) + + for key, value in values.items(): + query = query.set(key, value) + query.run() + + if commit: + frappe.db.commit() + + +def handle_exception(doc: "BOMUpdateLog"): + frappe.db.rollback() + error_log = doc.log_error("BOM Update Tool Error") + set_values_in_log(doc.name, {"status": "Failed", "error_log": error_log.name}) 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 index 47efea961b4e..4f151334a2a0 100644 --- a/erpnext/manufacturing/doctype/bom_update_log/test_bom_update_log.py +++ b/erpnext/manufacturing/doctype/bom_update_log/test_bom_update_log.py @@ -6,7 +6,7 @@ from erpnext.manufacturing.doctype.bom_update_log.bom_update_log import ( BOMMissingError, - run_bom_job, + run_replace_bom_job, ) from erpnext.manufacturing.doctype.bom_update_tool.bom_update_tool import enqueue_replace_bom @@ -71,7 +71,7 @@ def test_bom_update_log_completion(self): # Explicitly commits log, new bom (setUp) and replacement impact. # Is run via background jobs IRL - run_bom_job( + run_replace_bom_job( doc=log, boms=self.boms, update_type="Replace BOM", @@ -88,7 +88,7 @@ def test_bom_update_log_completion(self): log2 = enqueue_replace_bom( boms=self.boms, ) - run_bom_job( # Explicitly commits + run_replace_bom_job( # Explicitly commits doc=log2, boms=boms, update_type="Replace BOM", 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 e76572534081..4a2e03fb188e 100644 --- a/erpnext/manufacturing/doctype/bom_update_tool/bom_update_tool.py +++ b/erpnext/manufacturing/doctype/bom_update_tool/bom_update_tool.py @@ -2,8 +2,7 @@ # For license information, please see license.txt import json -from collections import defaultdict -from typing import TYPE_CHECKING, Dict, List, Literal, Optional, Union +from typing import TYPE_CHECKING, Dict, Literal, Optional, Union if TYPE_CHECKING: from erpnext.manufacturing.doctype.bom_update_log.bom_update_log import BOMUpdateLog @@ -39,17 +38,7 @@ def enqueue_update_cost() -> "BOMUpdateLog": 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 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: - bom_doc = frappe.get_cached_doc("BOM", bom) - bom_doc.calculate_cost(save_updates=True, update_hour_rate=True) - # bom_doc.update_exploded_items(save=True) #TODO: edit exploded items rate - bom_doc.db_update() + create_bom_update_log(update_type="Update Cost") def create_bom_update_log( @@ -69,90 +58,3 @@ def create_bom_update_log( "update_type": update_type, } ).submit() - - -def get_boms_in_bottom_up_order(bom_no: Optional[str] = None) -> List: - """ - Eg: Main BOM - |- Sub BOM 1 - |- Leaf BOM 1 - |- Sub BOM 2 - |- Leaf BOM 2 - Result: [Leaf BOM 1, Leaf BOM 2, Sub BOM 1, Sub BOM 2, Main BOM] - """ - leaf_boms = [] - if bom_no: - leaf_boms.append(bom_no) - else: - leaf_boms = _get_leaf_boms() - - child_parent_map = _generate_child_parent_map() - bom_list = leaf_boms.copy() - - for leaf_bom in leaf_boms: - parent_list = _get_flat_parent_map(leaf_bom, child_parent_map) - - if not parent_list: - continue - - bom_list.extend(parent_list) - bom_list = list(dict.fromkeys(bom_list).keys()) # remove duplicates - - return bom_list - - -def _generate_child_parent_map(): - bom = frappe.qb.DocType("BOM") - bom_item = frappe.qb.DocType("BOM Item") - - bom_parents = ( - frappe.qb.from_(bom_item) - .join(bom) - .on(bom_item.parent == bom.name) - .select(bom_item.bom_no, bom_item.parent) - .where( - (bom_item.bom_no.isnotnull()) - & (bom_item.bom_no != "") - & (bom.docstatus == 1) - & (bom.is_active == 1) - & (bom_item.parenttype == "BOM") - ) - ).run(as_dict=True) - - child_parent_map = defaultdict(list) - for bom in bom_parents: - child_parent_map[bom.bom_no].append(bom.parent) - - return child_parent_map - - -def _get_flat_parent_map(leaf, child_parent_map): - "Get ancestors at all levels of a leaf BOM." - parents_list = [] - - def _get_parents(node, parents_list): - "Returns recursively updated ancestors list." - first_parents = child_parent_map.get(node) # immediate parents of node - if not first_parents: # top most node - return parents_list - - parents_list.extend(first_parents) - parents_list = list(dict.fromkeys(parents_list).keys()) # remove duplicates - - for nth_node in first_parents: - # recursively find parents - parents_list = _get_parents(nth_node, parents_list) - - return parents_list - - parents_list = _get_parents(leaf, parents_list) - return parents_list - - -def _get_leaf_boms(): - return frappe.db.sql_list( - """select name from `tabBOM` bom - where docstatus=1 and is_active=1 - and not exists(select bom_no from `tabBOM Item` - where parent=bom.name and ifnull(bom_no, '')!='')""" - ) From 9f5f18e94da4254255a32d792abc94407ca5fde0 Mon Sep 17 00:00:00 2001 From: marination Date: Tue, 24 May 2022 18:17:40 +0530 Subject: [PATCH 08/20] style: Update docstrings and fix/add type hints + Collapsible progress section in Log --- .../bom_update_log/bom_update_log.json | 3 +- .../bom_update_log/bom_updation_utils.py | 45 ++++++++++++++----- 2 files changed, 35 insertions(+), 13 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 3455b8665735..db5f58d04f5e 100644 --- a/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.json +++ b/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.json @@ -69,6 +69,7 @@ "options": "Error Log" }, { + "collapsible": 1, "fieldname": "progress_section", "fieldtype": "Section Break", "label": "Progress" @@ -94,7 +95,7 @@ "index_web_pages_for_search": 1, "is_submittable": 1, "links": [], - "modified": "2022-05-23 14:42:14.725914", + "modified": "2022-05-24 17:52:21.824710", "modified_by": "Administrator", "module": "Manufacturing", "name": "BOM Update Log", diff --git a/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py b/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py index b5964cec9d4b..d246d3064f5c 100644 --- a/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py +++ b/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py @@ -3,7 +3,7 @@ import json from collections import defaultdict -from typing import TYPE_CHECKING, Dict, List, Optional +from typing import TYPE_CHECKING, Any, Dict, List, Optional if TYPE_CHECKING: from erpnext.manufacturing.doctype.bom_update_log.bom_update_log import BOMUpdateLog @@ -13,7 +13,8 @@ def replace_bom(boms: Dict) -> None: - """Replace current BOM with new BOM in parent BOMs.""" + "Replace current BOM with new BOM in parent BOMs." + current_bom = boms.get("current_bom") new_bom = boms.get("new_bom") @@ -39,6 +40,7 @@ def replace_bom(boms: Dict) -> None: def update_cost_in_level(doc: "BOMUpdateLog", bom_list: List[str]) -> None: "Updates Cost for BOMs within a given level. Runs via background jobs." + try: status = frappe.db.get_value("BOM Update Log", doc.name, "status") if status == "Failed": @@ -66,6 +68,8 @@ def update_cost_in_level(doc: "BOMUpdateLog", bom_list: List[str]) -> None: def get_ancestor_boms(new_bom: str, bom_list: Optional[List] = None) -> List: + "Recursively get all ancestors of BOM." + bom_list = bom_list or [] bom_item = frappe.qb.DocType("BOM Item") @@ -99,17 +103,18 @@ def update_new_bom_in_bom_items(unit_cost: float, current_bom: str, new_bom: str ).run() -def get_bom_unit_cost(new_bom: str) -> float: +def get_bom_unit_cost(bom_name: 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() + frappe.qb.from_(bom).select(bom.total_cost / bom.quantity).where(bom.name == bom_name).run() ) return frappe.utils.flt(new_bom_unitcost[0][0]) -def update_cost_in_boms(bom_list: List[str], docname: str) -> Dict: +def update_cost_in_boms(bom_list: List[str], docname: str) -> Dict[str, Dict]: "Updates cost in given BOMs. Returns current and total updated BOMs." + updated_boms = {} # current boms that have been updated for bom in bom_list: @@ -131,8 +136,11 @@ def update_cost_in_boms(bom_list: List[str], docname: str) -> Dict: return log_data -def process_if_level_is_complete(docname: str, current_boms: Dict, processed_boms: Dict) -> None: - "Prepare and set higher level BOMs in Log if current level is complete." +def process_if_level_is_complete( + docname: str, current_boms: Dict[str, bool], processed_boms: Dict[str, bool] +) -> None: + "Prepare and set higher level BOMs/dependants in Log if current level is complete." + processing_complete = all(current_boms.get(bom) for bom in current_boms) if not processing_complete: return @@ -149,7 +157,9 @@ def process_if_level_is_complete(docname: str, current_boms: Dict, processed_bom ) -def get_next_higher_level_boms(child_boms: Dict, processed_boms: Dict): +def get_next_higher_level_boms( + child_boms: Dict[str, bool], processed_boms: Dict[str, bool] +) -> List[str]: "Generate immediate higher level dependants with no unresolved dependencies." def _all_children_are_processed(parent): @@ -167,7 +177,9 @@ def _all_children_are_processed(parent): return list(dependants) -def get_leaf_boms(): +def get_leaf_boms() -> List[str]: + "Get BOMs that have no dependencies." + return frappe.db.sql_list( """select name from `tabBOM` bom where docstatus=1 and is_active=1 @@ -176,7 +188,13 @@ def get_leaf_boms(): ) -def _generate_dependants_map(): +def _generate_dependants_map() -> defaultdict: + """ + Generate map such as: { BOM-1: [Dependant-BOM-1, Dependant-BOM-2, ..] }. + Here BOM-1 is the leaf/lower level node/dependency. + The list contains one level higher nodes/dependants that depend on BOM-1. + """ + bom = frappe.qb.DocType("BOM") bom_item = frappe.qb.DocType("BOM Item") @@ -201,8 +219,9 @@ def _generate_dependants_map(): return child_parent_map -def set_values_in_log(log_name: str, values: Dict, commit: bool = False) -> None: +def set_values_in_log(log_name: str, values: Dict[str, Any], commit: bool = False) -> None: "Update BOM Update Log record." + if not values: return @@ -217,7 +236,9 @@ def set_values_in_log(log_name: str, values: Dict, commit: bool = False) -> None frappe.db.commit() -def handle_exception(doc: "BOMUpdateLog"): +def handle_exception(doc: "BOMUpdateLog") -> None: + "Rolls back and fails BOM Update Log." + frappe.db.rollback() error_log = doc.log_error("BOM Update Tool Error") set_values_in_log(doc.name, {"status": "Failed", "error_log": error_log.name}) From eabd8290d40db9745a046c422ed17e024a685ae2 Mon Sep 17 00:00:00 2001 From: marination Date: Wed, 25 May 2022 15:32:42 +0530 Subject: [PATCH 09/20] feat: Only update exploded items rate and amount - Generate RM-Rate map from Items table (will include subassembly items with rate) - Function to reset exploded item rate from above map - `db_update` exploded item rate only if rate is changed - Via Update Cost, only update exploded items rate, do not regenerate table again - Exploded Items are regenerated on Save and Replace BOM job - `calculate_exploded_cost` is run only via non doc events (Update Cost button, Update BOMs Cost Job) --- erpnext/manufacturing/doctype/bom/bom.py | 39 +++++++++++++++++-- .../bom_explosion_item.json | 4 +- .../bom_update_log/bom_updation_utils.py | 1 - 3 files changed, 39 insertions(+), 5 deletions(-) diff --git a/erpnext/manufacturing/doctype/bom/bom.py b/erpnext/manufacturing/doctype/bom/bom.py index 560019a86d55..6d53cfe7070d 100644 --- a/erpnext/manufacturing/doctype/bom/bom.py +++ b/erpnext/manufacturing/doctype/bom/bom.py @@ -5,7 +5,7 @@ import re from collections import deque from operator import itemgetter -from typing import List +from typing import Dict, List import frappe from frappe import _ @@ -185,6 +185,7 @@ def validate(self): self.validate_transfer_against() self.set_routing_operations() self.validate_operations() + self.update_exploded_items(save=False) self.calculate_cost() self.update_stock_qty() self.update_cost(update_parent=False, from_child_bom=True, update_hour_rate=False, save=False) @@ -391,8 +392,6 @@ def update_cost(self, update_parent=True, from_child_bom=False, update_hour_rate if save: self.db_update() - self.update_exploded_items(save=save) - # update parent BOMs if self.total_cost != existing_bom_cost and update_parent: parent_boms = frappe.db.sql_list( @@ -594,6 +593,10 @@ def calculate_cost(self, save_updates=False, update_hour_rate=False): self.calculate_op_cost(update_hour_rate) self.calculate_rm_cost(save=save_updates) self.calculate_sm_cost(save=save_updates) + if save_updates: + # not via doc event, table is not regenerated and needs updation + self.calculate_exploded_cost() + self.total_cost = self.operating_cost + self.raw_material_cost - self.scrap_material_cost self.base_total_cost = ( self.base_operating_cost + self.base_raw_material_cost - self.base_scrap_material_cost @@ -689,6 +692,36 @@ def calculate_sm_cost(self, save=False): self.scrap_material_cost = total_sm_cost self.base_scrap_material_cost = base_total_sm_cost + def calculate_exploded_cost(self): + "Set exploded row cost from it's parent BOM." + rm_rate_map = self.get_rm_rate_map() + + for row in self.get("exploded_items"): + old_rate = flt(row.rate) + row.rate = rm_rate_map.get(row.item_code) + row.amount = flt(row.stock_qty) * row.rate + + if old_rate != row.rate: + # Only db_update if unchanged + row.db_update() + + def get_rm_rate_map(self) -> Dict[str, float]: + "Create Raw Material-Rate map for Exploded Items. Fetch rate from Items table or Subassembly BOM." + rm_rate_map = {} + + for item in self.get("items"): + if item.bom_no: + # Get Item-Rate from Subassembly BOM + explosion_items = frappe.db.get_all( + "BOM Explosion Item", filters={"parent": item.bom_no}, fields=["item_code", "rate"] + ) + explosion_item_rate = {item.item_code: flt(item.rate) for item in explosion_items} + rm_rate_map.update(explosion_item_rate) + else: + rm_rate_map[item.item_code] = flt(item.base_rate) / flt(item.conversion_factor or 1.0) + + return rm_rate_map + def update_exploded_items(self, save=True): """Update Flat BOM, following will be correct data""" self.get_exploded_items() diff --git a/erpnext/manufacturing/doctype/bom_explosion_item/bom_explosion_item.json b/erpnext/manufacturing/doctype/bom_explosion_item/bom_explosion_item.json index f01d856e72af..9b1db63494b5 100644 --- a/erpnext/manufacturing/doctype/bom_explosion_item/bom_explosion_item.json +++ b/erpnext/manufacturing/doctype/bom_explosion_item/bom_explosion_item.json @@ -169,13 +169,15 @@ "index_web_pages_for_search": 1, "istable": 1, "links": [], - "modified": "2020-10-08 16:21:29.386212", + "modified": "2022-05-27 13:42:23.305455", "modified_by": "Administrator", "module": "Manufacturing", "name": "BOM Explosion Item", + "naming_rule": "Random", "owner": "Administrator", "permissions": [], "sort_field": "modified", "sort_order": "DESC", + "states": [], "track_changes": 1 } \ No newline at end of file diff --git a/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py b/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py index d246d3064f5c..1ec15f0d3ae1 100644 --- a/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py +++ b/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py @@ -120,7 +120,6 @@ def update_cost_in_boms(bom_list: List[str], docname: str) -> Dict[str, Dict]: for bom in bom_list: bom_doc = frappe.get_cached_doc("BOM", bom) bom_doc.calculate_cost(save_updates=True, update_hour_rate=True) - # bom_doc.update_exploded_items(save=True) #TODO: edit exploded items rate bom_doc.db_update() updated_boms[bom] = True From 59499462650965b483a2b0f9f377d59b98f756e6 Mon Sep 17 00:00:00 2001 From: marination Date: Fri, 27 May 2022 17:04:21 +0530 Subject: [PATCH 10/20] chore: Change BOM Progress field types to Long Text --- erpnext/manufacturing/doctype/bom/bom.py | 2 +- .../doctype/bom_update_log/bom_update_log.json | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/erpnext/manufacturing/doctype/bom/bom.py b/erpnext/manufacturing/doctype/bom/bom.py index 6d53cfe7070d..15048ec99067 100644 --- a/erpnext/manufacturing/doctype/bom/bom.py +++ b/erpnext/manufacturing/doctype/bom/bom.py @@ -702,7 +702,7 @@ def calculate_exploded_cost(self): row.amount = flt(row.stock_qty) * row.rate if old_rate != row.rate: - # Only db_update if unchanged + # Only db_update if changed row.db_update() def get_rm_rate_map(self) -> Dict[str, float]: 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 db5f58d04f5e..bea3cf037339 100644 --- a/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.json +++ b/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.json @@ -76,18 +76,18 @@ }, { "fieldname": "current_boms", - "fieldtype": "Text", + "fieldtype": "Long Text", "label": "Current BOMs" }, { "description": "Immediate parent BOMs", "fieldname": "parent_boms", - "fieldtype": "Text", + "fieldtype": "Long Text", "label": "Parent BOMs" }, { "fieldname": "processed_boms", - "fieldtype": "Text", + "fieldtype": "Long Text", "label": "Processed BOMs" } ], @@ -95,7 +95,7 @@ "index_web_pages_for_search": 1, "is_submittable": 1, "links": [], - "modified": "2022-05-24 17:52:21.824710", + "modified": "2022-05-27 17:03:34.712010", "modified_by": "Administrator", "module": "Manufacturing", "name": "BOM Update Log", From 2de2491e1737a9935183ca8f1db953f77f6c1185 Mon Sep 17 00:00:00 2001 From: marination Date: Fri, 27 May 2022 20:33:14 +0530 Subject: [PATCH 11/20] perf: `get_next_higher_level_boms` - Separate getting dependants and checking if they are valid (loop within loop led to redundant processing that slowed down function) - Adding to above, the same dependant(parent) was repeatedly processed as many children shared it. Expensive. - Use a parent-child map similar to child-parent map to check if all children are resolved - `map.get()` reduced time: 10 mins -> 0.9s~1 second (as compared to `get_cached_doc` or query) - Total time: 17 seconds to process 6599 leaf boms and 4.2L parent boms - Previous Total time: >10 mins (I terminated it due to not wanting to waste time XD) --- .../bom_update_log/bom_updation_utils.py | 44 ++++++++++++------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py b/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py index 1ec15f0d3ae1..790a79b3333d 100644 --- a/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py +++ b/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py @@ -159,21 +159,29 @@ def process_if_level_is_complete( def get_next_higher_level_boms( child_boms: Dict[str, bool], processed_boms: Dict[str, bool] ) -> List[str]: - "Generate immediate higher level dependants with no unresolved dependencies." + "Generate immediate higher level dependants with no unresolved dependencies (children)." - def _all_children_are_processed(parent): - bom_doc = frappe.get_cached_doc("BOM", parent) - return all(processed_boms.get(row.bom_no) for row in bom_doc.items if row.bom_no) + def _all_children_are_processed(parent_bom): + child_boms = dependency_map.get(parent_bom) + return all(processed_boms.get(bom) for bom in child_boms) - dependants_map = _generate_dependants_map() - dependants = set() + dependants_map, dependency_map = _generate_dependence_map() + + dependants = [] for bom in child_boms: + # generate list of immediate dependants parents = dependants_map.get(bom) or [] - for parent in parents: - if _all_children_are_processed(parent): - dependants.add(parent) + dependants.extend(parents) + + dependants = set(dependants) # remove duplicates + resolved_dependants = set() - return list(dependants) + # consider only if children are all resolved + for parent_bom in dependants: + if _all_children_are_processed(parent_bom): + resolved_dependants.add(parent_bom) + + return list(resolved_dependants) def get_leaf_boms() -> List[str]: @@ -187,17 +195,19 @@ def get_leaf_boms() -> List[str]: ) -def _generate_dependants_map() -> defaultdict: +def _generate_dependence_map() -> defaultdict: """ - Generate map such as: { BOM-1: [Dependant-BOM-1, Dependant-BOM-2, ..] }. + Generate maps such as: { BOM-1: [Dependant-BOM-1, Dependant-BOM-2, ..] }. Here BOM-1 is the leaf/lower level node/dependency. The list contains one level higher nodes/dependants that depend on BOM-1. + + Generate and return the reverse as well. """ bom = frappe.qb.DocType("BOM") bom_item = frappe.qb.DocType("BOM Item") - bom_parents = ( + bom_items = ( frappe.qb.from_(bom_item) .join(bom) .on(bom_item.parent == bom.name) @@ -212,10 +222,12 @@ def _generate_dependants_map() -> defaultdict: ).run(as_dict=True) child_parent_map = defaultdict(list) - for bom in bom_parents: - child_parent_map[bom.bom_no].append(bom.parent) + parent_child_map = defaultdict(list) + for row in bom_items: + child_parent_map[row.bom_no].append(row.parent) + parent_child_map[row.parent].append(row.bom_no) - return child_parent_map + return child_parent_map, parent_child_map def set_values_in_log(log_name: str, values: Dict[str, Any], commit: bool = False) -> None: From 978ba5238fae64fe0e348091d42c3210b999df02 Mon Sep 17 00:00:00 2001 From: marination Date: Fri, 27 May 2022 21:59:59 +0530 Subject: [PATCH 12/20] fix: Safe cast `row.rate` (in case of faulty exploded items, edge case but oh well) --- erpnext/manufacturing/doctype/bom/bom.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/erpnext/manufacturing/doctype/bom/bom.py b/erpnext/manufacturing/doctype/bom/bom.py index 15048ec99067..7055efac289e 100644 --- a/erpnext/manufacturing/doctype/bom/bom.py +++ b/erpnext/manufacturing/doctype/bom/bom.py @@ -699,7 +699,7 @@ def calculate_exploded_cost(self): for row in self.get("exploded_items"): old_rate = flt(row.rate) row.rate = rm_rate_map.get(row.item_code) - row.amount = flt(row.stock_qty) * row.rate + row.amount = flt(row.stock_qty) * flt(row.rate) if old_rate != row.rate: # Only db_update if changed From a62bc9b6c920b0d39a16b348a65962fa6a0d9992 Mon Sep 17 00:00:00 2001 From: marination Date: Tue, 31 May 2022 15:53:34 +0530 Subject: [PATCH 13/20] chore: Limit Update Cost jobs & `db_update` only if changed values - If `Update Cost` job is ongoing, then block creation of new ones since all BOMs are updated - `db_update` in `calculate_rm_cost` only if changed values to reduce redundant row updates - Misc: Use variable for batch size --- erpnext/manufacturing/doctype/bom/bom.py | 4 +++- .../doctype/bom_update_log/bom_update_log.py | 22 +++++++++++++++++-- .../bom_update_tool/bom_update_tool.py | 8 ++++++- 3 files changed, 30 insertions(+), 4 deletions(-) diff --git a/erpnext/manufacturing/doctype/bom/bom.py b/erpnext/manufacturing/doctype/bom/bom.py index 7055efac289e..d4e0d4b81478 100644 --- a/erpnext/manufacturing/doctype/bom/bom.py +++ b/erpnext/manufacturing/doctype/bom/bom.py @@ -644,6 +644,7 @@ def calculate_rm_cost(self, save=False): base_total_rm_cost = 0 for d in self.get("items"): + old_rate = d.rate d.rate = self.get_rm_rate( { "company": self.company, @@ -656,6 +657,7 @@ def calculate_rm_cost(self, save=False): "sourced_by_supplier": d.sourced_by_supplier, } ) + d.base_rate = flt(d.rate) * flt(self.conversion_rate) d.amount = flt(d.rate, d.precision("rate")) * flt(d.qty, d.precision("qty")) d.base_amount = d.amount * flt(self.conversion_rate) @@ -665,7 +667,7 @@ def calculate_rm_cost(self, save=False): total_rm_cost += d.amount base_total_rm_cost += d.base_amount - if save: + if save and (old_rate != d.rate): d.db_update() self.raw_material_cost = total_rm_cost 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 639628ac3837..f61f863c10ab 100644 --- a/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.py +++ b/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.py @@ -26,6 +26,8 @@ def validate(self): self.validate_boms_are_specified() self.validate_same_bom() self.validate_bom_items() + else: + self.validate_bom_cost_update_in_progress() self.status = "Queued" @@ -48,6 +50,21 @@ def validate_bom_items(self): if current_bom_item != new_bom_item: frappe.throw(_("The selected BOMs are not for the same item")) + def validate_bom_cost_update_in_progress(self): + "If another Cost Updation Log is still in progress, dont make new ones." + + wip_log = frappe.get_all( + "BOM Update Log", + {"update_type": "Update Cost", "status": ["in", ["Queued", "In Progress", "Paused"]]}, + limit_page_length=1, + ) + if wip_log: + log_link = frappe.utils.get_link_to_form("BOM Update Log", wip_log[0].name) + frappe.throw( + _("BOM Updation already in progress. Please wait until {0} is complete.").format(log_link), + title=_("Note"), + ) + def on_submit(self): if frappe.flags.in_test: return @@ -124,10 +141,11 @@ def queue_bom_cost_jobs(current_boms: Dict, update_doc: "BOMUpdateLog") -> None: current_boms_list = [bom for bom in current_boms] while current_boms_list: - boms_to_process = current_boms_list[:20000] # slice out batch of 20k BOMs + batch_size = 20_000 + boms_to_process = current_boms_list[:batch_size] # slice out batch of 20k BOMs # update list to exclude 20K (queued) BOMs - current_boms_list = current_boms_list[20000:] if len(current_boms_list) > 20000 else [] + current_boms_list = current_boms_list[batch_size:] if len(current_boms_list) > batch_size else [] frappe.enqueue( method="erpnext.manufacturing.doctype.bom_update_log.bom_updation_utils.update_cost_in_level", doc=update_doc, 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 4a2e03fb188e..758d8ed0ef96 100644 --- a/erpnext/manufacturing/doctype/bom_update_tool/bom_update_tool.py +++ b/erpnext/manufacturing/doctype/bom_update_tool/bom_update_tool.py @@ -38,7 +38,13 @@ def enqueue_update_cost() -> "BOMUpdateLog": 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"): - create_bom_update_log(update_type="Update Cost") + wip_log = frappe.get_all( + "BOM Update Log", + {"update_type": "Update Cost", "status": ["in", ["Queued", "In Progress", "Paused"]]}, + limit_page_length=1, + ) + if not wip_log: + create_bom_update_log(update_type="Update Cost") def create_bom_update_log( From 62857e3e080b3888f40a09112be63238974dd175 Mon Sep 17 00:00:00 2001 From: marination Date: Thu, 2 Jun 2022 13:35:30 +0530 Subject: [PATCH 14/20] feat: Track progress in Log Batch/Job wise - This was done due to stale reads while the background jobs tried updating status of the log - Added a table where all bom jobs within log will be tracked with what level they are processing - Cron job will check if table jobs are all processed every 5 mins - If yes, it will prepare parents and call `process_boms_cost_level_wise` to start next level - If pending jobs, do nothing - Current BOM Level is being tracked that helps adding rows to the table - Individual bom cost jobs (that are queued) will process and update boms > will update BOM Update Batch table row with list of updated BOMs --- .../doctype/bom_update_batch/__init__.py | 0 .../bom_update_batch/bom_update_batch.json | 45 ++++++++ .../bom_update_batch/bom_update_batch.py | 9 ++ .../bom_update_log/bom_update_log.json | 21 ++-- .../doctype/bom_update_log/bom_update_log.py | 106 +++++++++++++----- .../bom_update_log/bom_updation_utils.py | 55 +-------- 6 files changed, 154 insertions(+), 82 deletions(-) create mode 100644 erpnext/manufacturing/doctype/bom_update_batch/__init__.py create mode 100644 erpnext/manufacturing/doctype/bom_update_batch/bom_update_batch.json create mode 100644 erpnext/manufacturing/doctype/bom_update_batch/bom_update_batch.py diff --git a/erpnext/manufacturing/doctype/bom_update_batch/__init__.py b/erpnext/manufacturing/doctype/bom_update_batch/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/erpnext/manufacturing/doctype/bom_update_batch/bom_update_batch.json b/erpnext/manufacturing/doctype/bom_update_batch/bom_update_batch.json new file mode 100644 index 000000000000..9938454ce4ef --- /dev/null +++ b/erpnext/manufacturing/doctype/bom_update_batch/bom_update_batch.json @@ -0,0 +1,45 @@ +{ + "actions": [], + "autoname": "autoincrement", + "creation": "2022-05-31 17:34:39.825537", + "doctype": "DocType", + "engine": "InnoDB", + "field_order": [ + "level", + "batch_no", + "boms_updated" + ], + "fields": [ + { + "fieldname": "level", + "fieldtype": "Int", + "in_list_view": 1, + "label": "Level" + }, + { + "fieldname": "batch_no", + "fieldtype": "Int", + "in_list_view": 1, + "label": "Batch No." + }, + { + "fieldname": "boms_updated", + "fieldtype": "Long Text", + "in_list_view": 1, + "label": "BOMs Updated" + } + ], + "index_web_pages_for_search": 1, + "istable": 1, + "links": [], + "modified": "2022-05-31 23:36:13.628391", + "modified_by": "Administrator", + "module": "Manufacturing", + "name": "BOM Update Batch", + "naming_rule": "Autoincrement", + "owner": "Administrator", + "permissions": [], + "sort_field": "modified", + "sort_order": "DESC", + "states": [] +} \ No newline at end of file diff --git a/erpnext/manufacturing/doctype/bom_update_batch/bom_update_batch.py b/erpnext/manufacturing/doctype/bom_update_batch/bom_update_batch.py new file mode 100644 index 000000000000..f952e435e67d --- /dev/null +++ b/erpnext/manufacturing/doctype/bom_update_batch/bom_update_batch.py @@ -0,0 +1,9 @@ +# Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and contributors +# For license information, please see license.txt + +# import frappe +from frappe.model.document import Document + + +class BOMUpdateBatch(Document): + pass 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 bea3cf037339..b1c24ab9954e 100644 --- a/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.json +++ b/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.json @@ -14,9 +14,10 @@ "status", "error_log", "progress_section", - "current_boms", + "current_level", "parent_boms", "processed_boms", + "bom_batches", "amended_from" ], "fields": [ @@ -70,15 +71,11 @@ }, { "collapsible": 1, + "depends_on": "eval: doc.update_type == \"Update Cost\"", "fieldname": "progress_section", "fieldtype": "Section Break", "label": "Progress" }, - { - "fieldname": "current_boms", - "fieldtype": "Long Text", - "label": "Current BOMs" - }, { "description": "Immediate parent BOMs", "fieldname": "parent_boms", @@ -89,13 +86,23 @@ "fieldname": "processed_boms", "fieldtype": "Long Text", "label": "Processed BOMs" + }, + { + "fieldname": "bom_batches", + "fieldtype": "Table", + "options": "BOM Update Batch" + }, + { + "fieldname": "current_level", + "fieldtype": "Int", + "label": "Current Level" } ], "in_create": 1, "index_web_pages_for_search": 1, "is_submittable": 1, "links": [], - "modified": "2022-05-27 17:03:34.712010", + "modified": "2022-05-31 20:20:06.370786", "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 f61f863c10ab..bfae76c2b2eb 100644 --- a/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.py +++ b/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.py @@ -1,15 +1,16 @@ # Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and contributors # For license information, please see license.txt import json -from typing import Dict, Optional +from typing import Any, Dict, List, Optional, Tuple import frappe from frappe import _ from frappe.model.document import Document -from frappe.utils import cstr +from frappe.utils import cint, cstr from erpnext.manufacturing.doctype.bom_update_log.bom_updation_utils import ( get_leaf_boms, + get_next_higher_level_boms, handle_exception, replace_bom, set_values_in_log, @@ -111,55 +112,110 @@ def process_boms_cost_level_wise(update_doc: "BOMUpdateLog") -> None: if update_doc.status == "Queued": # First level yet to process. On Submit. - current_boms = {bom: False for bom in get_leaf_boms()} + current_level = 0 + current_boms = get_leaf_boms() values = { - "current_boms": json.dumps(current_boms), "parent_boms": "[]", "processed_boms": json.dumps({}), "status": "In Progress", + "current_level": current_level, } else: - # status is Paused, resume. via Cron Job. - current_boms, parent_boms = json.loads(update_doc.current_boms), json.loads( - update_doc.parent_boms - ) - if not current_boms: - # Process the next level BOMs. Stage parents as current BOMs. - current_boms = {bom: False for bom in parent_boms} - values = { - "current_boms": json.dumps(current_boms), - "parent_boms": "[]", - "status": "In Progress", - } + # Resume next level. via Cron Job. + current_level = cint(update_doc.current_level) + 1 + parent_boms = json.loads(update_doc.parent_boms) + + # Process the next level BOMs. Stage parents as current BOMs. + current_boms = parent_boms.copy() + values = {"parent_boms": "[]", "current_level": current_level} set_values_in_log(update_doc.name, values, commit=True) - queue_bom_cost_jobs(current_boms, update_doc) + queue_bom_cost_jobs(current_boms, update_doc, current_level) -def queue_bom_cost_jobs(current_boms: Dict, update_doc: "BOMUpdateLog") -> None: +def queue_bom_cost_jobs( + current_boms_list: List, update_doc: "BOMUpdateLog", current_level: int +) -> None: "Queue batches of 20k BOMs of the same level to process parallelly" - current_boms_list = [bom for bom in current_boms] + batch_no = 0 while current_boms_list: + batch_no += 1 batch_size = 20_000 boms_to_process = current_boms_list[:batch_size] # slice out batch of 20k BOMs # update list to exclude 20K (queued) BOMs current_boms_list = current_boms_list[batch_size:] if len(current_boms_list) > batch_size else [] + + batch_row = update_doc.append("bom_batches", {"level": current_level, "batch_no": batch_no}) + batch_row.db_insert() + 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, timeout=40000, ) def resume_bom_cost_update_jobs(): - "Called every 10 minutes via Cron job." - paused_jobs = frappe.db.get_all("BOM Update Log", {"status": "Paused"}) - if not paused_jobs: + """ + 1. Checks for In Progress BOM Update Log. + 2. Checks if this job has completed the _current level_. + 3. If current level is complete, get parent BOMs and start next level. + 4. If no parents, mark as Complete. + 5. If current level is WIP, skip the Log. + + Called every 5 minutes via Cron job. + """ + + in_progress_logs = frappe.db.get_all( + "BOM Update Log", + {"update_type": "Update Cost", "status": "In Progress"}, + ["name", "processed_boms", "current_level"], + ) + if not in_progress_logs: return - for job in paused_jobs: - # resume from next level - process_boms_cost_level_wise(update_doc=frappe.get_doc("BOM Update Log", job.name)) + for log in in_progress_logs: + # check if all log batches of current level are processed + bom_batches = frappe.db.get_all( + "BOM Update Batch", {"parent": log.name, "level": log.current_level}, ["name", "boms_updated"] + ) + incomplete_level = any(not row.get("boms_updated") for row in bom_batches) + if not bom_batches or incomplete_level: + continue + + # Prep parent BOMs & updated processed BOMs for next level + 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), + "parent_boms": json.dumps(parent_boms), + "status": "Completed" if not parent_boms else "In Progress", + }, + commit=True, + ) + + if parent_boms: # there is a next level to process + process_boms_cost_level_wise(update_doc=frappe.get_doc("BOM Update Log", log.name)) + + +def get_processed_current_boms( + log: Dict[str, Any], bom_batches: Dict[str, Any] +) -> Tuple[List[str], Dict[str, Any]]: + "Aggregate all BOMs from BOM Update Batch rows into 'processed_boms' field and into current boms list." + processed_boms = json.loads(log.processed_boms) if log.processed_boms else {} + current_boms = [] + + for row in bom_batches: + boms_updated = json.loads(row.boms_updated) + current_boms.extend(boms_updated) + boms_updated_dict = {bom: True for bom in boms_updated} + processed_boms.update(boms_updated_dict) + + return current_boms, processed_boms diff --git a/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py b/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py index 790a79b3333d..2d6429b05049 100644 --- a/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py +++ b/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py @@ -38,7 +38,7 @@ def replace_bom(boms: Dict) -> None: bom_obj.save_version() -def update_cost_in_level(doc: "BOMUpdateLog", bom_list: List[str]) -> None: +def update_cost_in_level(doc: "BOMUpdateLog", bom_list: List[str], batch_name: int) -> None: "Updates Cost for BOMs within a given level. Runs via background jobs." try: @@ -47,19 +47,9 @@ def update_cost_in_level(doc: "BOMUpdateLog", bom_list: List[str]) -> None: return frappe.db.auto_commit_on_many_writes = 1 - # main updation logic - job_data = update_cost_in_boms(bom_list=bom_list, docname=doc.name) - - set_values_in_log( - doc.name, - values={ - "current_boms": json.dumps(job_data.get("current_boms")), - "processed_boms": json.dumps(job_data.get("processed_boms")), - }, - commit=True, - ) - process_if_level_is_complete(doc.name, job_data["current_boms"], job_data["processed_boms"]) + update_cost_in_boms(bom_list=bom_list) # main updation logic + frappe.db.set_value("BOM Update Batch", batch_name, "boms_updated", json.dumps(bom_list)) except Exception: handle_exception(doc) finally: @@ -112,48 +102,13 @@ def get_bom_unit_cost(bom_name: str) -> float: return frappe.utils.flt(new_bom_unitcost[0][0]) -def update_cost_in_boms(bom_list: List[str], docname: str) -> Dict[str, Dict]: +def update_cost_in_boms(bom_list: List[str]) -> None: "Updates cost in given BOMs. Returns current and total updated BOMs." - updated_boms = {} # current boms that have been updated - for bom in bom_list: bom_doc = frappe.get_cached_doc("BOM", bom) bom_doc.calculate_cost(save_updates=True, update_hour_rate=True) bom_doc.db_update() - updated_boms[bom] = True - - # Update processed BOMs in Log - log_data = frappe.db.get_values( - "BOM Update Log", docname, ["current_boms", "processed_boms"], as_dict=True - )[0] - - for field in ("current_boms", "processed_boms"): - log_data[field] = json.loads(log_data.get(field)) - log_data[field].update(updated_boms) - - return log_data - - -def process_if_level_is_complete( - docname: str, current_boms: Dict[str, bool], processed_boms: Dict[str, bool] -) -> None: - "Prepare and set higher level BOMs/dependants in Log if current level is complete." - - processing_complete = all(current_boms.get(bom) for bom in current_boms) - if not processing_complete: - return - - parent_boms = get_next_higher_level_boms(child_boms=current_boms, processed_boms=processed_boms) - set_values_in_log( - docname, - values={ - "current_boms": json.dumps({}), - "parent_boms": json.dumps(parent_boms), - "status": "Completed" if not parent_boms else "Paused", - }, - commit=True, - ) def get_next_higher_level_boms( @@ -244,7 +199,7 @@ def set_values_in_log(log_name: str, values: Dict[str, Any], commit: bool = Fals query.run() if commit: - frappe.db.commit() + frappe.db.commit() # nosemgrep def handle_exception(doc: "BOMUpdateLog") -> None: From 934db57fdd5346dde982e9022f33d61780175d07 Mon Sep 17 00:00:00 2001 From: marination Date: Mon, 6 Jun 2022 17:01:51 +0530 Subject: [PATCH 15/20] chore: Miscellanous fixes/enhancements - `get_valuation_rate`: if no bins are found return 0, SLEs do not exist either - `get_valuation_rate`: Compute average valuation rate via query - `get_rm_rate_map`: set order_by as None to avoid creating sort index (modified) each time query runs (seen in process list) - BOM Update Batch: add status field and hide `boms_updated` so that users can see progress without loading all updated boms (too much data) - BOM Update Batch: set batch row status to completed after job runs - BOM Update Log: remove `parent_boms` field (just pass parent boms to processing function) & remove Paused state (not used) - Move job to long queue to avoid choking default queue - `update_cost_in_boms`: use `get_doc` as each BOM is accessed only once. Use `for_update` to lock BOM row - Commit after every 100 BOMs --- erpnext/manufacturing/doctype/bom/bom.py | 32 ++++++++++------- .../bom_update_batch/bom_update_batch.json | 14 ++++++-- .../bom_update_log/bom_update_log.json | 12 ++----- .../doctype/bom_update_log/bom_update_log.py | 34 ++++++++++++------- .../bom_update_log/bom_updation_utils.py | 24 +++++++++---- .../bom_update_tool/bom_update_tool.py | 2 +- 6 files changed, 74 insertions(+), 44 deletions(-) diff --git a/erpnext/manufacturing/doctype/bom/bom.py b/erpnext/manufacturing/doctype/bom/bom.py index d4e0d4b81478..1fb00ef3fe23 100644 --- a/erpnext/manufacturing/doctype/bom/bom.py +++ b/erpnext/manufacturing/doctype/bom/bom.py @@ -714,8 +714,11 @@ def get_rm_rate_map(self) -> Dict[str, float]: for item in self.get("items"): if item.bom_no: # Get Item-Rate from Subassembly BOM - explosion_items = frappe.db.get_all( - "BOM Explosion Item", filters={"parent": item.bom_no}, fields=["item_code", "rate"] + explosion_items = frappe.get_all( + "BOM Explosion Item", + filters={"parent": item.bom_no}, + fields=["item_code", "rate"], + order_by=None, # to avoid sort index creation at db level (granular change) ) explosion_item_rate = {item.item_code: flt(item.rate) for item in explosion_items} rm_rate_map.update(explosion_item_rate) @@ -935,13 +938,17 @@ def get_bom_item_rate(args, bom_doc): def get_valuation_rate(args): - """Get weighted average of valuation rate from all warehouses""" - - total_qty, total_value, valuation_rate = 0.0, 0.0, 0.0 - item_bins = frappe.db.sql( + """ + 1) Get average valuation rate from all warehouses + 2) If no value, get last valuation rate from SLE + 3) If no value, get valuation rate from Item + """ + + valuation_rate = 0.0 + item_valuation = frappe.db.sql( """ select - bin.actual_qty, bin.stock_value + (sum(bin.stock_value) / sum(bin.actual_qty)) as valuation_rate from `tabBin` bin, `tabWarehouse` warehouse where @@ -950,14 +957,13 @@ def get_valuation_rate(args): and warehouse.company=%(company)s""", {"item": args["item_code"], "company": args["company"]}, as_dict=1, - ) + )[0] - for d in item_bins: - total_qty += flt(d.actual_qty) - total_value += flt(d.stock_value) + valuation_rate = item_valuation.get("valuation_rate") - if total_qty: - valuation_rate = total_value / total_qty + if valuation_rate is None: + # Explicit null value check. If null, Bins don't exist, neither does SLE + return valuation_rate if valuation_rate <= 0: last_valuation_rate = frappe.db.sql( diff --git a/erpnext/manufacturing/doctype/bom_update_batch/bom_update_batch.json b/erpnext/manufacturing/doctype/bom_update_batch/bom_update_batch.json index 9938454ce4ef..83b54d326cbb 100644 --- a/erpnext/manufacturing/doctype/bom_update_batch/bom_update_batch.json +++ b/erpnext/manufacturing/doctype/bom_update_batch/bom_update_batch.json @@ -7,7 +7,8 @@ "field_order": [ "level", "batch_no", - "boms_updated" + "boms_updated", + "status" ], "fields": [ { @@ -25,14 +26,23 @@ { "fieldname": "boms_updated", "fieldtype": "Long Text", + "hidden": 1, "in_list_view": 1, "label": "BOMs Updated" + }, + { + "fieldname": "status", + "fieldtype": "Select", + "in_list_view": 1, + "label": "Status", + "options": "Pending\nCompleted", + "read_only": 1 } ], "index_web_pages_for_search": 1, "istable": 1, "links": [], - "modified": "2022-05-31 23:36:13.628391", + "modified": "2022-06-06 14:50:35.161062", "modified_by": "Administrator", "module": "Manufacturing", "name": "BOM Update Batch", 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 b1c24ab9954e..c32e383b08a7 100644 --- a/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.json +++ b/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.json @@ -15,7 +15,6 @@ "error_log", "progress_section", "current_level", - "parent_boms", "processed_boms", "bom_batches", "amended_from" @@ -52,7 +51,7 @@ "fieldname": "status", "fieldtype": "Select", "label": "Status", - "options": "Queued\nIn Progress\nPaused\nCompleted\nFailed" + "options": "Queued\nIn Progress\nCompleted\nFailed" }, { "fieldname": "amended_from", @@ -76,15 +75,10 @@ "fieldtype": "Section Break", "label": "Progress" }, - { - "description": "Immediate parent BOMs", - "fieldname": "parent_boms", - "fieldtype": "Long Text", - "label": "Parent BOMs" - }, { "fieldname": "processed_boms", "fieldtype": "Long Text", + "hidden": 1, "label": "Processed BOMs" }, { @@ -102,7 +96,7 @@ "index_web_pages_for_search": 1, "is_submittable": 1, "links": [], - "modified": "2022-05-31 20:20:06.370786", + "modified": "2022-06-06 15:15:23.883251", "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 bfae76c2b2eb..d714b9d5fd01 100644 --- a/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.py +++ b/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.py @@ -56,7 +56,7 @@ def validate_bom_cost_update_in_progress(self): wip_log = frappe.get_all( "BOM Update Log", - {"update_type": "Update Cost", "status": ["in", ["Queued", "In Progress", "Paused"]]}, + {"update_type": "Update Cost", "status": ["in", ["Queued", "In Progress"]]}, limit_page_length=1, ) if wip_log: @@ -104,10 +104,12 @@ def run_replace_bom_job( frappe.db.commit() # nosemgrep -def process_boms_cost_level_wise(update_doc: "BOMUpdateLog") -> None: +def process_boms_cost_level_wise( + update_doc: "BOMUpdateLog", parent_boms: List[str] = None +) -> None: "Queue jobs at the start of new BOM Level in 'Update Cost' Jobs." - current_boms, parent_boms = {}, [] + current_boms = {} values = {} if update_doc.status == "Queued": @@ -115,26 +117,27 @@ def process_boms_cost_level_wise(update_doc: "BOMUpdateLog") -> None: current_level = 0 current_boms = get_leaf_boms() values = { - "parent_boms": "[]", "processed_boms": json.dumps({}), "status": "In Progress", "current_level": current_level, } else: # Resume next level. via Cron Job. + if not parent_boms: + return + current_level = cint(update_doc.current_level) + 1 - parent_boms = json.loads(update_doc.parent_boms) # Process the next level BOMs. Stage parents as current BOMs. current_boms = parent_boms.copy() - values = {"parent_boms": "[]", "current_level": current_level} + values = {"current_level": current_level} set_values_in_log(update_doc.name, values, commit=True) queue_bom_cost_jobs(current_boms, update_doc, current_level) def queue_bom_cost_jobs( - current_boms_list: List, update_doc: "BOMUpdateLog", current_level: int + current_boms_list: List[str], update_doc: "BOMUpdateLog", current_level: int ) -> None: "Queue batches of 20k BOMs of the same level to process parallelly" batch_no = 0 @@ -147,7 +150,9 @@ def queue_bom_cost_jobs( # update list to exclude 20K (queued) BOMs current_boms_list = current_boms_list[batch_size:] if len(current_boms_list) > batch_size else [] - batch_row = update_doc.append("bom_batches", {"level": current_level, "batch_no": batch_no}) + batch_row = update_doc.append( + "bom_batches", {"level": current_level, "batch_no": batch_no, "status": "Pending"} + ) batch_row.db_insert() frappe.enqueue( @@ -155,7 +160,7 @@ def queue_bom_cost_jobs( doc=update_doc, bom_list=boms_to_process, batch_name=batch_row.name, - timeout=40000, + queue="long", ) @@ -181,9 +186,11 @@ def resume_bom_cost_update_jobs(): for log in in_progress_logs: # check if all log batches of current level are processed bom_batches = frappe.db.get_all( - "BOM Update Batch", {"parent": log.name, "level": log.current_level}, ["name", "boms_updated"] + "BOM Update Batch", + {"parent": log.name, "level": log.current_level}, + ["name", "boms_updated", "status"], ) - incomplete_level = any(not row.get("boms_updated") for row in bom_batches) + incomplete_level = any(row.get("status") == "Pending" for row in bom_batches) if not bom_batches or incomplete_level: continue @@ -195,14 +202,15 @@ def resume_bom_cost_update_jobs(): log.name, values={ "processed_boms": json.dumps(processed_boms), - "parent_boms": json.dumps(parent_boms), "status": "Completed" if not parent_boms else "In Progress", }, commit=True, ) if parent_boms: # there is a next level to process - process_boms_cost_level_wise(update_doc=frappe.get_doc("BOM Update Log", log.name)) + process_boms_cost_level_wise( + update_doc=frappe.get_doc("BOM Update Log", log.name), parent_boms=parent_boms + ) def get_processed_current_boms( diff --git a/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py b/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py index 2d6429b05049..49e747c4bb96 100644 --- a/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py +++ b/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py @@ -3,7 +3,7 @@ import json from collections import defaultdict -from typing import TYPE_CHECKING, Any, Dict, List, Optional +from typing import TYPE_CHECKING, Any, Dict, List, Optional, Union if TYPE_CHECKING: from erpnext.manufacturing.doctype.bom_update_log.bom_update_log import BOMUpdateLog @@ -38,7 +38,9 @@ def replace_bom(boms: Dict) -> None: bom_obj.save_version() -def update_cost_in_level(doc: "BOMUpdateLog", bom_list: List[str], batch_name: int) -> None: +def update_cost_in_level( + doc: "BOMUpdateLog", bom_list: List[str], batch_name: Union[int, str] +) -> None: "Updates Cost for BOMs within a given level. Runs via background jobs." try: @@ -49,7 +51,14 @@ def update_cost_in_level(doc: "BOMUpdateLog", bom_list: List[str], batch_name: i frappe.db.auto_commit_on_many_writes = 1 update_cost_in_boms(bom_list=bom_list) # main updation logic - frappe.db.set_value("BOM Update Batch", batch_name, "boms_updated", json.dumps(bom_list)) + + bom_batch = frappe.qb.DocType("BOM Update Batch") + ( + frappe.qb.update(bom_batch) + .set(bom_batch.boms_updated, json.dumps(bom_list)) + .set(bom_batch.status, "Completed") + .where(bom_batch.name == batch_name) + ).run() except Exception: handle_exception(doc) finally: @@ -105,14 +114,17 @@ def get_bom_unit_cost(bom_name: str) -> float: def update_cost_in_boms(bom_list: List[str]) -> None: "Updates cost in given BOMs. Returns current and total updated BOMs." - for bom in bom_list: - bom_doc = frappe.get_cached_doc("BOM", bom) + for index, bom in enumerate(bom_list): + bom_doc = frappe.get_doc("BOM", bom, for_update=True) bom_doc.calculate_cost(save_updates=True, update_hour_rate=True) bom_doc.db_update() + if index % 100 == 0: + frappe.db.commit() + def get_next_higher_level_boms( - child_boms: Dict[str, bool], processed_boms: Dict[str, bool] + child_boms: List[str], processed_boms: Dict[str, bool] ) -> List[str]: "Generate immediate higher level dependants with no unresolved dependencies (children)." 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 758d8ed0ef96..d16fcd083264 100644 --- a/erpnext/manufacturing/doctype/bom_update_tool/bom_update_tool.py +++ b/erpnext/manufacturing/doctype/bom_update_tool/bom_update_tool.py @@ -40,7 +40,7 @@ def auto_update_latest_price_in_all_boms() -> None: if frappe.db.get_single_value("Manufacturing Settings", "update_bom_costs_automatically"): wip_log = frappe.get_all( "BOM Update Log", - {"update_type": "Update Cost", "status": ["in", ["Queued", "In Progress", "Paused"]]}, + {"update_type": "Update Cost", "status": ["in", ["Queued", "In Progress"]]}, limit_page_length=1, ) if not wip_log: From 15101190a6efb4a596824a1c7cba88d8363e5d17 Mon Sep 17 00:00:00 2001 From: marination Date: Mon, 6 Jun 2022 17:12:43 +0530 Subject: [PATCH 16/20] chore: `get_valuation_rate` in bom.py must always return float & goto Item master if no bins --- erpnext/manufacturing/doctype/bom/bom.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/erpnext/manufacturing/doctype/bom/bom.py b/erpnext/manufacturing/doctype/bom/bom.py index 1fb00ef3fe23..8bf124e058f6 100644 --- a/erpnext/manufacturing/doctype/bom/bom.py +++ b/erpnext/manufacturing/doctype/bom/bom.py @@ -961,11 +961,8 @@ def get_valuation_rate(args): valuation_rate = item_valuation.get("valuation_rate") - if valuation_rate is None: - # Explicit null value check. If null, Bins don't exist, neither does SLE - return valuation_rate - - if valuation_rate <= 0: + if (valuation_rate is not None) and valuation_rate <= 0: + # Explicit null value check. If None, Bins don't exist, neither does SLE last_valuation_rate = frappe.db.sql( """select valuation_rate from `tabStock Ledger Entry` From 6bde1bb5d2446c3ed08f566060c321664ad1d4e4 Mon Sep 17 00:00:00 2001 From: marination Date: Tue, 7 Jun 2022 14:44:00 +0530 Subject: [PATCH 17/20] test: Util to update cost in all BOMs - Utility to update cost in all BOMs without cron jobs or background jobs (run immediately) - Re-use util wherever all bom costs are to be updated - Skip explicit commits if in test - Specify company in test records (dirty data sometimes, company wh mismatch) - Skip background jobs queueing if in test --- erpnext/manufacturing/doctype/bom/test_bom.py | 6 +- .../doctype/bom/test_records.json | 1 + .../doctype/bom_update_log/bom_update_log.py | 21 +++- .../bom_update_log/bom_updation_utils.py | 10 +- .../bom_update_log/test_bom_update_log.py | 97 ++++++++++++------- .../bom_update_tool/test_bom_update_tool.py | 14 +-- 6 files changed, 97 insertions(+), 52 deletions(-) diff --git a/erpnext/manufacturing/doctype/bom/test_bom.py b/erpnext/manufacturing/doctype/bom/test_bom.py index 62fc0724e035..bc1bea7389e6 100644 --- a/erpnext/manufacturing/doctype/bom/test_bom.py +++ b/erpnext/manufacturing/doctype/bom/test_bom.py @@ -11,7 +11,9 @@ from erpnext.buying.doctype.purchase_order.test_purchase_order import create_purchase_order from erpnext.manufacturing.doctype.bom.bom import item_query, make_variant_bom -from erpnext.manufacturing.doctype.bom_update_tool.bom_update_tool import update_cost +from erpnext.manufacturing.doctype.bom_update_log.test_bom_update_log import ( + update_cost_in_all_boms_in_test, +) from erpnext.stock.doctype.item.test_item import make_item from erpnext.stock.doctype.stock_reconciliation.test_stock_reconciliation import ( create_stock_reconciliation, @@ -80,7 +82,7 @@ def test_update_bom_cost_in_all_boms(self): reset_item_valuation_rate(item_code="_Test Item 2", qty=200, rate=rm_rate + 10) # update cost of all BOMs based on latest valuation rate - update_cost() + update_cost_in_all_boms_in_test() # check if new valuation rate updated in all BOMs for d in frappe.db.sql( diff --git a/erpnext/manufacturing/doctype/bom/test_records.json b/erpnext/manufacturing/doctype/bom/test_records.json index 25730f9b9f4a..507d319b515a 100644 --- a/erpnext/manufacturing/doctype/bom/test_records.json +++ b/erpnext/manufacturing/doctype/bom/test_records.json @@ -32,6 +32,7 @@ "is_active": 1, "is_default": 1, "item": "_Test Item Home Desktop Manufactured", + "company": "_Test Company", "quantity": 1.0 }, { 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 d714b9d5fd01..71430bd57e42 100644 --- a/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.py +++ b/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.py @@ -1,7 +1,7 @@ # Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and contributors # For license information, please see license.txt import json -from typing import Any, Dict, List, Optional, Tuple +from typing import Any, Dict, List, Optional, Tuple, Union import frappe from frappe import _ @@ -101,12 +101,14 @@ def run_replace_bom_job( handle_exception(doc) finally: frappe.db.auto_commit_on_many_writes = 0 - frappe.db.commit() # nosemgrep + + if not frappe.flags.in_test: + frappe.db.commit() # nosemgrep def process_boms_cost_level_wise( update_doc: "BOMUpdateLog", parent_boms: List[str] = None -) -> None: +) -> Union[None, Tuple]: "Queue jobs at the start of new BOM Level in 'Update Cost' Jobs." current_boms = {} @@ -133,6 +135,10 @@ 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) @@ -155,6 +161,10 @@ 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, @@ -216,7 +226,10 @@ def resume_bom_cost_update_jobs(): def get_processed_current_boms( log: Dict[str, Any], bom_batches: Dict[str, Any] ) -> Tuple[List[str], Dict[str, Any]]: - "Aggregate all BOMs from BOM Update Batch rows into 'processed_boms' field and into current boms list." + """ + Aggregate all BOMs from BOM Update Batch rows into 'processed_boms' field + and into current boms list. + """ processed_boms = json.loads(log.processed_boms) if log.processed_boms else {} current_boms = [] diff --git a/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py b/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py index 49e747c4bb96..dde1e4ed7595 100644 --- a/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py +++ b/erpnext/manufacturing/doctype/bom_update_log/bom_updation_utils.py @@ -63,7 +63,9 @@ def update_cost_in_level( handle_exception(doc) finally: frappe.db.auto_commit_on_many_writes = 0 - frappe.db.commit() # nosemgrep + + if not frappe.flags.in_test: + frappe.db.commit() # nosemgrep def get_ancestor_boms(new_bom: str, bom_list: Optional[List] = None) -> List: @@ -119,8 +121,8 @@ 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: - frappe.db.commit() + if (index % 100 == 0) and not frappe.flags.in_test: + frappe.db.commit() # nosemgrep def get_next_higher_level_boms( @@ -210,7 +212,7 @@ def set_values_in_log(log_name: str, values: Dict[str, Any], commit: bool = Fals query = query.set(key, value) query.run() - if commit: + if commit 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 index 4f151334a2a0..d770f6c56a5d 100644 --- a/erpnext/manufacturing/doctype/bom_update_log/test_bom_update_log.py +++ b/erpnext/manufacturing/doctype/bom_update_log/test_bom_update_log.py @@ -1,14 +1,27 @@ # Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and Contributors # See license.txt +import json + import frappe from frappe.tests.utils import FrappeTestCase 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_tool.bom_update_tool import enqueue_replace_bom +from erpnext.manufacturing.doctype.bom_update_log.bom_updation_utils import ( + get_next_higher_level_boms, + set_values_in_log, + update_cost_in_level, +) +from erpnext.manufacturing.doctype.bom_update_tool.bom_update_tool import ( + enqueue_replace_bom, + enqueue_update_cost, +) test_records = frappe.get_test_records("BOM") @@ -31,17 +44,12 @@ def setUp(self): def tearDown(self): frappe.db.rollback() - if self._testMethodName == "test_bom_update_log_completion": - # clear logs and delete BOM created via setUp - frappe.db.delete("BOM Update Log") - self.new_bom_doc.cancel() - self.new_bom_doc.delete() - - # explicitly commit and restore to original state - frappe.db.commit() # nosemgrep - def test_bom_update_log_validate(self): - "Test if BOM presence is validated." + """ + 1) Test if BOM presence is validated. + 2) Test if same BOMs are validated. + 3) Test of non-existent BOM is validated. + """ with self.assertRaises(BOMMissingError): enqueue_replace_bom(boms={}) @@ -55,9 +63,7 @@ def test_bom_update_log_validate(self): def test_bom_update_log_queueing(self): "Test if BOM Update Log is created and queued." - log = enqueue_replace_bom( - boms=self.boms, - ) + log = enqueue_replace_bom(boms=self.boms) self.assertEqual(log.docstatus, 1) self.assertEqual(log.status, "Queued") @@ -65,32 +71,51 @@ def test_bom_update_log_queueing(self): def test_bom_update_log_completion(self): "Test if BOM Update Log handles job completion correctly." - log = enqueue_replace_bom( - boms=self.boms, - ) + log = enqueue_replace_bom(boms=self.boms) - # Explicitly commits log, new bom (setUp) and replacement impact. - # Is run via background jobs IRL - run_replace_bom_job( - doc=log, - boms=self.boms, - update_type="Replace BOM", - ) + # Is run via background job IRL + run_replace_bom_job(doc=log, boms=self.boms) log.reload() self.assertEqual(log.status, "Completed") - # teardown (undo replace impact) due to commit - boms = frappe._dict( - current_bom=self.boms.new_bom, - new_bom=self.boms.current_bom, - ) - log2 = enqueue_replace_bom( - boms=self.boms, + +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. + """ + 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"], ) - run_replace_bom_job( # Explicitly commits - doc=log2, - boms=boms, - update_type="Replace BOM", + 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", + }, ) - self.assertEqual(log2.status, "Completed") + 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 index fae72a0f6f71..d1882e56e954 100644 --- a/erpnext/manufacturing/doctype/bom_update_tool/test_bom_update_tool.py +++ b/erpnext/manufacturing/doctype/bom_update_tool/test_bom_update_tool.py @@ -1,11 +1,13 @@ -# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors +# Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and Contributors # License: GNU General Public License v3. See license.txt 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_tool.bom_update_tool import update_cost +from erpnext.manufacturing.doctype.bom_update_log.test_bom_update_log import ( + update_cost_in_all_boms_in_test, +) from erpnext.manufacturing.doctype.production_plan.test_production_plan import make_bom from erpnext.stock.doctype.item.test_item import create_item @@ -25,8 +27,8 @@ def test_replace_bom(self): boms = frappe._dict(current_bom=current_bom, new_bom=bom_doc.name) replace_bom(boms) - self.assertFalse(frappe.db.sql("select name from `tabBOM Item` where bom_no=%s", current_bom)) - self.assertTrue(frappe.db.sql("select name from `tabBOM Item` where bom_no=%s", bom_doc.name)) + 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 @@ -52,13 +54,13 @@ def test_bom_cost(self): self.assertEqual(doc.total_cost, 200) frappe.db.set_value("Item", "BOM Cost Test Item 2", "valuation_rate", 200) - update_cost() + update_cost_in_all_boms_in_test() doc.load_from_db() self.assertEqual(doc.total_cost, 300) frappe.db.set_value("Item", "BOM Cost Test Item 2", "valuation_rate", 100) - update_cost() + update_cost_in_all_boms_in_test() doc.load_from_db() self.assertEqual(doc.total_cost, 200) From 9f2793ccf1e185b28c0f6b480a304641923dbc30 Mon Sep 17 00:00:00 2001 From: marination Date: Tue, 7 Jun 2022 17:44:39 +0530 Subject: [PATCH 18/20] test: Fix `test_update_bom_cost_in_all_boms` - Use base_rate for assertions as rate is subject to change due to conversion factor (USD) --- erpnext/manufacturing/doctype/bom/test_bom.py | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/erpnext/manufacturing/doctype/bom/test_bom.py b/erpnext/manufacturing/doctype/bom/test_bom.py index f2731ec5ef79..04e937a7e323 100644 --- a/erpnext/manufacturing/doctype/bom/test_bom.py +++ b/erpnext/manufacturing/doctype/bom/test_bom.py @@ -71,26 +71,32 @@ def _get_default_bom_in_item(): def test_update_bom_cost_in_all_boms(self): # get current rate for '_Test Item 2' - rm_rate = frappe.db.sql( - """select rate from `tabBOM Item` - where parent='BOM-_Test Item Home Desktop Manufactured-001' - and item_code='_Test Item 2' and docstatus=1 and parenttype='BOM'""" + bom_rates = frappe.db.get_values( + "BOM Item", + { + "parent": "BOM-_Test Item Home Desktop Manufactured-001", + "item_code": "_Test Item 2", + "docstatus": 1, + }, + fieldname=["rate", "base_rate"], + as_dict=True, ) - rm_rate = rm_rate[0][0] if rm_rate else 0 + rm_base_rate = bom_rates[0].get("base_rate") if bom_rates else 0 + rm_rate = bom_rates[0].get("rate") if bom_rates else 0 # Reset item valuation rate - reset_item_valuation_rate(item_code="_Test Item 2", qty=200, rate=rm_rate + 10) + reset_item_valuation_rate(item_code="_Test Item 2", qty=200, rate=rm_base_rate + 10) # update cost of all BOMs based on latest valuation rate update_cost_in_all_boms_in_test() # check if new valuation rate updated in all BOMs for d in frappe.db.sql( - """select rate from `tabBOM Item` + """select base_rate from `tabBOM Item` where item_code='_Test Item 2' and docstatus=1 and parenttype='BOM'""", as_dict=1, ): - self.assertEqual(d.rate, rm_rate + 10) + self.assertEqual(d.base_rate, rm_base_rate + 10) def test_bom_cost(self): bom = frappe.copy_doc(test_records[2]) From 7e41d84a116f2acd03984c98ec4eaa8e50ddc1d3 Mon Sep 17 00:00:00 2001 From: marination Date: Wed, 8 Jun 2022 14:01:04 +0530 Subject: [PATCH 19/20] chore: `get_valuation_rate` sider fixes - Use qb instead of db.sql - Don't use `args` as argument for function - Cleaner variable names --- erpnext/manufacturing/doctype/bom/bom.py | 50 ++++++++++--------- erpnext/manufacturing/doctype/bom/test_bom.py | 1 - 2 files changed, 26 insertions(+), 25 deletions(-) diff --git a/erpnext/manufacturing/doctype/bom/bom.py b/erpnext/manufacturing/doctype/bom/bom.py index 3e2a2d13f180..631548b30993 100644 --- a/erpnext/manufacturing/doctype/bom/bom.py +++ b/erpnext/manufacturing/doctype/bom/bom.py @@ -932,44 +932,46 @@ def get_bom_item_rate(args, bom_doc): return flt(rate) -def get_valuation_rate(args): +def get_valuation_rate(data): """ 1) Get average valuation rate from all warehouses 2) If no value, get last valuation rate from SLE 3) If no value, get valuation rate from Item """ + from frappe.query_builder.functions import Sum + item_code, company = data.get("item_code"), data.get("company") valuation_rate = 0.0 - item_valuation = frappe.db.sql( - """ - select - (sum(bin.stock_value) / sum(bin.actual_qty)) as valuation_rate - from - `tabBin` bin, `tabWarehouse` warehouse - where - bin.item_code=%(item)s - and bin.warehouse = warehouse.name - and warehouse.company=%(company)s""", - {"item": args["item_code"], "company": args["company"]}, - as_dict=1, - )[0] + + bin_table = frappe.qb.DocType("Bin") + wh_table = frappe.qb.DocType("Warehouse") + item_valuation = ( + frappe.qb.from_(bin_table) + .join(wh_table) + .on(bin_table.warehouse == wh_table.name) + .select((Sum(bin_table.stock_value) / Sum(bin_table.actual_qty)).as_("valuation_rate")) + .where((bin_table.item_code == item_code) & (wh_table.company == company)) + ).run(as_dict=True)[0] valuation_rate = item_valuation.get("valuation_rate") if (valuation_rate is not None) and valuation_rate <= 0: # Explicit null value check. If None, Bins don't exist, neither does SLE - last_valuation_rate = frappe.db.sql( - """select valuation_rate - from `tabStock Ledger Entry` - where item_code = %s and valuation_rate > 0 and is_cancelled = 0 - order by posting_date desc, posting_time desc, creation desc limit 1""", - args["item_code"], - ) - - valuation_rate = flt(last_valuation_rate[0][0]) if last_valuation_rate else 0 + sle = frappe.qb.DocType("Stock Ledger Entry") + last_val_rate = ( + frappe.qb.from_(sle) + .select(sle.valuation_rate) + .where((sle.item_code == item_code) & (sle.valuation_rate > 0) & (sle.is_cancelled == 0)) + .orderby(sle.posting_date, order=frappe.qb.desc) + .orderby(sle.posting_time, order=frappe.qb.desc) + .orderby(sle.creation, order=frappe.qb.desc) + .limit(1) + ).run(as_dict=True) + + valuation_rate = flt(last_val_rate[0].get("valuation_rate")) if last_val_rate else 0 if not valuation_rate: - valuation_rate = frappe.db.get_value("Item", args["item_code"], "valuation_rate") + valuation_rate = frappe.db.get_value("Item", item_code, "valuation_rate") return flt(valuation_rate) diff --git a/erpnext/manufacturing/doctype/bom/test_bom.py b/erpnext/manufacturing/doctype/bom/test_bom.py index 04e937a7e323..182a20c6bb7e 100644 --- a/erpnext/manufacturing/doctype/bom/test_bom.py +++ b/erpnext/manufacturing/doctype/bom/test_bom.py @@ -82,7 +82,6 @@ def test_update_bom_cost_in_all_boms(self): as_dict=True, ) rm_base_rate = bom_rates[0].get("base_rate") if bom_rates else 0 - rm_rate = bom_rates[0].get("rate") if bom_rates else 0 # Reset item valuation rate reset_item_valuation_rate(item_code="_Test Item 2", qty=200, rate=rm_base_rate + 10) From 3fa0a46f39f7024c5d0b235a7725eaa9ad0f3869 Mon Sep 17 00:00:00 2001 From: marination Date: Thu, 9 Jun 2022 16:22:00 +0530 Subject: [PATCH 20/20] chore: Less hacky tests, versioning (replace bom) and clearing log data (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 --- .../doctype/bom_update_log/bom_update_log.py | 18 ++---- .../bom_update_log/bom_updation_utils.py | 19 ++++--- .../bom_update_log/test_bom_update_log.py | 56 +------------------ .../bom_update_tool/test_bom_update_tool.py | 12 ++-- 4 files changed, 23 insertions(+), 82 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 index 71430bd57e42..9c9c24044aae 100644 --- a/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.py +++ b/erpnext/manufacturing/doctype/bom_update_log/bom_update_log.py @@ -67,9 +67,6 @@ 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( @@ -77,6 +74,7 @@ def on_submit(self): 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 index dde1e4ed7595..af115e3e4210 100644 --- 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 index d770f6c56a5d..b38fc8976b2d 100644 --- a/erpnext/manufacturing/doctype/bom_update_log/test_bom_update_log.py +++ b/erpnext/manufacturing/doctype/bom_update_log/test_bom_update_log.py @@ -1,22 +1,12 @@ # Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and Contributors # See license.txt -import json - import frappe from frappe.tests.utils import FrappeTestCase 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 +50,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 index d1882e56e954..5dd557f8ab13 100644 --- 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)