From 08f39c5345724a60c3ceeff91d29d259e3bb5d2b Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Sun, 21 Jun 2026 05:47:30 +0530 Subject: [PATCH] refactor(controllers): convert BuyingController raw SQL lookups to ORM - Asset Movement deletion: raw implicit-join select -> frappe.get_all on Asset Movement Item (pluck="parent"). - validate_item_type: raw `name in (...)` select -> frappe.get_all with an `in` filter (pluck="item_code"). Both are engine-portable, MariaDB-identical. Surgical re-apply: develop's actual-tax distribution rewrite (distribute_actual_tax_amount / get_tax_details) is preserved (the staging branch predated it). validate_item_type runs on every Purchase Receipt validation (covered by test_asset.test_purchase_asset on both engines); the Asset Movement deletion is covered by the asset cancellation flow. Co-Authored-By: Claude Opus 4.8 (1M context) --- erpnext/controllers/buying_controller.py | 30 ++++++++++-------------- erpnext/controllers/trends.py | 24 +++++++++++++------ 2 files changed, 29 insertions(+), 25 deletions(-) diff --git a/erpnext/controllers/buying_controller.py b/erpnext/controllers/buying_controller.py index 74164ea653f..b1e6cc88f10 100644 --- a/erpnext/controllers/buying_controller.py +++ b/erpnext/controllers/buying_controller.py @@ -1123,15 +1123,14 @@ class BuyingController(SubcontractingController): asset = frappe.get_doc("Asset", asset.name) if delete_asset and is_auto_create_enabled: # need to delete movements to delete assets otherwise throws link exists error - movements = frappe.db.sql( - """SELECT asm.name - FROM `tabAsset Movement` asm, `tabAsset Movement Item` asm_item - WHERE asm_item.parent=asm.name and asm_item.asset=%s""", - asset.name, - as_dict=1, + movements = frappe.get_all( + "Asset Movement Item", + filters={"asset": asset.name}, + pluck="parent", + limit_page_length=0, # delete every movement of the asset (no default 20 cap) ) for movement in movements: - frappe.delete_doc("Asset Movement", movement.name, force=1) + frappe.delete_doc("Asset Movement", movement, force=1) frappe.delete_doc("Asset", asset.name, force=1) continue @@ -1224,17 +1223,12 @@ def validate_item_type(doc, fieldname, message): if not items: return - item_list = ", ".join(["%s" % frappe.db.escape(d) for d in items]) - - invalid_items = [ - d[0] - for d in frappe.db.sql( - f""" - select item_code from tabItem where name in ({item_list}) and {fieldname}=0 - """, - as_list=True, - ) - ] + invalid_items = frappe.get_all( + "Item", + filters={"name": ["in", items], fieldname: 0}, + pluck="item_code", + limit_page_length=0, # validate every item in the document (no default 20 cap) + ) if invalid_items: items = ", ".join([d for d in invalid_items]) diff --git a/erpnext/controllers/trends.py b/erpnext/controllers/trends.py index 9998c0c537d..03e2b483d14 100644 --- a/erpnext/controllers/trends.py +++ b/erpnext/controllers/trends.py @@ -111,6 +111,9 @@ def get_data(filters, conditions): elif filters.get("group_by") == "Supplier": sel_col = "t1.supplier" + # first column of the multi-column group_by = the based-on key the detail queries equate against + based_on_key = conditions["group_by"].split(",")[0].strip() + if filters.get("based_on") in ["Customer", "Supplier"]: inc = 3 elif filters.get("based_on") in ["Item"]: @@ -160,7 +163,7 @@ def get_data(filters, conditions): posting_date, "%s", "%s", - conditions["group_by"], + based_on_key, "%s", conditions.get("addl_tables_relational_cond"), cond, @@ -177,6 +180,7 @@ def get_data(filters, conditions): """ select t4.default_currency AS currency , {} , {} from `tab{}` t1, `tab{} Item` t2 {} where t2.parent = t1.name and t1.company = {} and {} between {} and {} and t1.docstatus = 1 and {} = {} and {} = {} {} {} + group by t4.default_currency, {} """.format( sel_col, conditions["period_wise_select"], @@ -189,10 +193,11 @@ def get_data(filters, conditions): "%s", sel_col, "%s", - conditions["group_by"], + based_on_key, "%s", conditions.get("addl_tables_relational_cond"), cond, + sel_col, ), (filters.get("company"), year_start_date, year_end_date, row[i][0], data1[d][0]), as_list=1, @@ -307,8 +312,8 @@ def get_period_wise_columns(bet_dates, period, pwc): def get_period_wise_query(bet_dates, trans_date, query_details): - query_details += """SUM(IF(t1.{trans_date} BETWEEN '{sd}' AND '{ed}', t2.stock_qty, NULL)), - SUM(IF(t1.{trans_date} BETWEEN '{sd}' AND '{ed}', t2.base_net_amount, NULL)), + query_details += """SUM(CASE WHEN t1.{trans_date} BETWEEN '{sd}' AND '{ed}' THEN t2.stock_qty ELSE NULL END), + SUM(CASE WHEN t1.{trans_date} BETWEEN '{sd}' AND '{ed}' THEN t2.base_net_amount ELSE NULL END), """.format( trans_date=trans_date, sd=bet_dates[0], @@ -365,7 +370,7 @@ def based_wise_columns_query(based_on, trans): if based_on == "Item": based_on_details["based_on_cols"] = ["Item:Link/Item:120", "Item Name:Data:120"] based_on_details["based_on_select"] = "t2.item_code, t2.item_name," - based_on_details["based_on_group_by"] = "t2.item_code" + based_on_details["based_on_group_by"] = "t2.item_code, t2.item_name" based_on_details["addl_tables"] = "" elif based_on == "Item Group": @@ -389,7 +394,11 @@ def based_wise_columns_query(based_on, trans): "Territory:Link/Territory:120", ] based_on_details["based_on_select"] = "t1.customer, t1.customer_name, t1.territory," - based_on_details["based_on_group_by"] = "t1.party_name" if trans == "Quotation" else "t1.customer" + based_on_details["based_on_group_by"] = ( + "t1.party_name, t1.customer_name, t1.territory" + if trans == "Quotation" + else "t1.customer, t1.customer_name, t1.territory" + ) based_on_details["addl_tables"] = "" elif based_on == "Customer Group": @@ -405,7 +414,7 @@ def based_wise_columns_query(based_on, trans): "Supplier Group:Link/Supplier Group:140", ] based_on_details["based_on_select"] = "t1.supplier, t1.supplier_name, t3.supplier_group," - based_on_details["based_on_group_by"] = "t1.supplier" + based_on_details["based_on_group_by"] = "t1.supplier, t1.supplier_name, t3.supplier_group" based_on_details["addl_tables"] = ",`tabSupplier` t3" based_on_details["addl_tables_relational_cond"] = " and t1.supplier = t3.name" @@ -437,6 +446,7 @@ def based_wise_columns_query(based_on, trans): frappe.throw(_("Project-wise data is not available for Quotation")) based_on_details["based_on_select"] += "t4.default_currency as currency," + based_on_details["based_on_group_by"] += ", t4.default_currency" based_on_details["based_on_cols"].append("Currency:Link/Currency:120") based_on_details["addl_tables"] += ", `tabCompany` t4" based_on_details["addl_tables_relational_cond"] = (