From 465446bb790a6624a01d625b8a84b5e948ad3102 Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Sun, 21 Jun 2026 05:23:31 +0530 Subject: [PATCH 1/2] refactor(controllers): convert sales/purchase return lookups to qb/ORM validate_returned_items used a raw frappe.db.sql with a string-built column list (and a separate Packed Item select); get_already_returned_items used a raw GROUP BY sum. Convert both to frappe.get_all / frappe.qb (Sum(Abs(...)) with an explicit groupby). The qb GROUP BY mirrors the original `group by item_code, `, so it is parity-preserving (not a behaviour change) and valid on Postgres. Surgical re-apply: develop's `is_debit_note = 0` credit-note fix in make_return_doc is preserved (the staging branch predated and would have reverted it). Adds a test (Delivery Note -> sales return) exercising validate_returned_items and get_already_returned_items on both engines. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../controllers/sales_and_purchase_return.py | 65 ++++++++++--------- erpnext/controllers/stock_controller.py | 42 +++++------- .../tests/test_sales_and_purchase_return.py | 39 +++++++++++ 3 files changed, 93 insertions(+), 53 deletions(-) create mode 100644 erpnext/controllers/tests/test_sales_and_purchase_return.py diff --git a/erpnext/controllers/sales_and_purchase_return.py b/erpnext/controllers/sales_and_purchase_return.py index 866b2aa38c9..d84c8bd2192 100644 --- a/erpnext/controllers/sales_and_purchase_return.py +++ b/erpnext/controllers/sales_and_purchase_return.py @@ -7,7 +7,7 @@ import frappe from frappe import _, bold from frappe.model.meta import get_field_precision from frappe.query_builder import DocType -from frappe.query_builder.functions import Abs +from frappe.query_builder.functions import Abs, Sum from frappe.utils import cint, flt, format_datetime, get_datetime import erpnext @@ -86,26 +86,27 @@ def validate_return_against(doc): def validate_returned_items(doc): valid_items = frappe._dict() - select_fields = "item_code, qty, stock_qty, rate, parenttype, conversion_factor, name" + select_fields = ["item_code", "qty", "stock_qty", "rate", "parenttype", "conversion_factor", "name"] if doc.doctype != "Purchase Invoice": - select_fields += ",serial_no, batch_no" + select_fields += ["serial_no", "batch_no"] if doc.doctype in ["Purchase Invoice", "Purchase Receipt", "Subcontracting Receipt"]: - select_fields += ",rejected_qty, received_qty" + select_fields += ["rejected_qty", "received_qty"] - for d in frappe.db.sql( - f"""select {select_fields} from `tab{doc.doctype} Item` where parent = %s""", - doc.return_against, - as_dict=1, + for d in frappe.get_all( + f"{doc.doctype} Item", + filters={"parent": doc.return_against}, + fields=select_fields, + limit_page_length=0, # all item rows of the reference document are needed (no default 20 cap) ): valid_items = get_ref_item_dict(valid_items, d) if doc.doctype in ("Delivery Note", "Sales Invoice"): - for d in frappe.db.sql( - """select item_code, qty, serial_no, batch_no from `tabPacked Item` - where parent = %s""", - doc.return_against, - as_dict=1, + for d in frappe.get_all( + "Packed Item", + filters={"parent": doc.return_against}, + fields=["item_code", "qty", "serial_no", "batch_no"], + limit_page_length=0, # all packed-item rows are needed (no default 20 cap) ): valid_items = get_ref_item_dict(valid_items, d) @@ -271,29 +272,35 @@ def get_ref_item_dict(valid_items, ref_item_row): def get_already_returned_items(doc): - column = "child.item_code, sum(abs(child.qty)) as qty, sum(abs(child.stock_qty)) as stock_qty" - if doc.doctype in ["Purchase Invoice", "Purchase Receipt", "Subcontracting Receipt"]: - column += """, sum(abs(child.rejected_qty) * child.conversion_factor) as rejected_qty, - sum(abs(child.received_qty) * child.conversion_factor) as received_qty""" + child = DocType(f"{doc.doctype} Item") + par = DocType(doc.doctype) field = ( frappe.scrub(doc.doctype) + "_item" if doc.doctype in ["Purchase Invoice", "Purchase Receipt", "Sales Invoice", "POS Invoice"] else "dn_detail" ) - data = frappe.db.sql( - f""" - select {column}, child.{field} - from - `tab{doc.doctype} Item` child, `tab{doc.doctype}` par - where - child.parent = par.name and par.docstatus = 1 - and par.is_return = 1 and par.return_against = %s - group by item_code, {field} - """, - doc.return_against, - as_dict=1, + + query = ( + frappe.qb.from_(child) + .inner_join(par) + .on(child.parent == par.name) + .select( + child.item_code, + Sum(Abs(child.qty)).as_("qty"), + Sum(Abs(child.stock_qty)).as_("stock_qty"), + child[field], + ) + .where((par.docstatus == 1) & (par.is_return == 1) & (par.return_against == doc.return_against)) + .groupby(child.item_code, child[field]) ) + if doc.doctype in ["Purchase Invoice", "Purchase Receipt", "Subcontracting Receipt"]: + query = query.select( + Sum(Abs(child.rejected_qty) * child.conversion_factor).as_("rejected_qty"), + Sum(Abs(child.received_qty) * child.conversion_factor).as_("received_qty"), + ) + + data = query.run(as_dict=1) items = {} diff --git a/erpnext/controllers/stock_controller.py b/erpnext/controllers/stock_controller.py index b987a9d2946..46fbbbf484e 100644 --- a/erpnext/controllers/stock_controller.py +++ b/erpnext/controllers/stock_controller.py @@ -5,6 +5,8 @@ import json import frappe from frappe import _, bold +from frappe.query_builder import Criterion +from frappe.query_builder.functions import Count from frappe.utils import cint, cstr, flt, get_link_to_form, getdate import erpnext @@ -279,11 +281,7 @@ class StockController(AccountsController): def make_gl_entries_on_cancel(self, from_repost=False): if not from_repost: cancel_exchange_gain_loss_journal(frappe._dict(doctype=self.doctype, name=self.name)) - if frappe.db.sql( - """select name from `tabGL Entry` where voucher_type=%s - and voucher_no=%s""", - (self.doctype, self.name), - ): + if frappe.db.exists("GL Entry", {"voucher_type": self.doctype, "voucher_no": self.name}): self.make_gl_entries() def validate_warehouse(self): @@ -632,7 +630,7 @@ def check_item_quality_inspection(doctype: str, docstatus: str | int, items: str inspection_fieldname = INSPECTION_FIELDNAME_MAP.get(doctype) if inspection_fieldname is None: - return items if doctype == "Stock Entry" else [] + return [] allow_after_transaction = cint(docstatus) == 1 and frappe.get_single_value( "Stock Settings", "allow_to_make_quality_inspection_after_purchase_or_delivery" @@ -725,20 +723,18 @@ def future_sle_exists(args, sl_entries=None): args["posting_datetime"] = get_combine_datetime(args["posting_date"], args["posting_time"]) - data = frappe.db.sql( - """ - select item_code, warehouse, count(name) as total_row - from `tabStock Ledger Entry` - where - ({}) - and posting_datetime >= %(posting_datetime)s - and voucher_no != %(voucher_no)s - and is_cancelled = 0 - GROUP BY - item_code, warehouse - """.format(" or ".join(or_conditions)), - args, - as_dict=1, + sle = frappe.qb.DocType("Stock Ledger Entry") + data = ( + frappe.qb.from_(sle) + .select(sle.item_code, sle.warehouse, Count(sle.name).as_("total_row")) + .where( + Criterion.any(or_conditions) + & (sle.posting_datetime >= args["posting_datetime"]) + & (sle.voucher_no != args["voucher_no"]) + & (sle.is_cancelled == 0) + ) + .groupby(sle.item_code, sle.warehouse) + .run(as_dict=1) ) for d in data: @@ -792,12 +788,10 @@ def get_conditions_to_validate_future_sle(sl_entries): warehouse_items_map[entry.warehouse].add(entry.item_code) + sle = frappe.qb.DocType("Stock Ledger Entry") or_conditions = [] for warehouse, items in warehouse_items_map.items(): - or_conditions.append( - f"""warehouse = {frappe.db.escape(warehouse)} - and item_code in ({", ".join(frappe.db.escape(item) for item in items)})""" - ) + or_conditions.append((sle.warehouse == warehouse) & sle.item_code.isin(list(items))) return or_conditions diff --git a/erpnext/controllers/tests/test_sales_and_purchase_return.py b/erpnext/controllers/tests/test_sales_and_purchase_return.py new file mode 100644 index 00000000000..97a33281cc0 --- /dev/null +++ b/erpnext/controllers/tests/test_sales_and_purchase_return.py @@ -0,0 +1,39 @@ +# Copyright (c) 2025, Frappe Technologies Pvt. Ltd. and Contributors +# See license.txt + +import frappe + +from erpnext.tests.utils import ERPNextTestSuite + + +class TestSalesAndPurchaseReturn(ERPNextTestSuite): + @staticmethod + def _cancel_and_delete(doctype, name): + if not frappe.db.exists(doctype, name): + return + doc = frappe.get_doc(doctype, name) + if doc.docstatus == 1: + doc.cancel() + frappe.delete_doc(doctype, name, force=1) + + def test_sales_return_validates_against_original(self): + # Submitting a return Delivery Note runs validate_returned_items (Item / Packed Item lookups + # via frappe.get_all) and get_already_returned_items (qb GROUP BY of the returned qty) -- both + # converted from raw SQL here. Exercises them on both engines. + from erpnext.stock.doctype.delivery_note.mapper import make_sales_return + from erpnext.stock.doctype.delivery_note.test_delivery_note import create_delivery_note + from erpnext.stock.doctype.stock_entry.stock_entry_utils import make_stock_entry + + se = make_stock_entry(item_code="_Test Item", target="_Test Warehouse - _TC", qty=20, basic_rate=100) + self.addCleanup(self._cancel_and_delete, "Stock Entry", se.name) + + dn = create_delivery_note(qty=5) + self.addCleanup(self._cancel_and_delete, "Delivery Note", dn.name) + + return_dn = make_sales_return(dn.name) + return_dn.insert() + return_dn.submit() + self.addCleanup(self._cancel_and_delete, "Delivery Note", return_dn.name) + + self.assertEqual(return_dn.is_return, 1) + self.assertEqual(return_dn.items[0].qty, -5) From 8138f5aecddd3d4537531eee0056ac880ad01653 Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Sun, 21 Jun 2026 05:23:32 +0530 Subject: [PATCH 2/2] refactor(controllers): convert StockController future-SLE/GL checks to qb/ORM - make_gl_entries_on_cancel: raw GL Entry existence select -> frappe.db.exists. - future_sle_exists: raw GROUP BY count -> frappe.qb Count with Criterion.any, and get_conditions_to_validate_future_sle builds qb Criterion objects (warehouse == x & item_code.isin(...)) instead of escaped SQL strings. Parity-preserving and valid on Postgres. Surgical re-apply: develop's check_item_quality_inspection fix (`return items if doctype == "Stock Entry" else []`) is preserved (the staging branch predated and would have reverted it). Adds a test asserting future_sle_exists detects a later SLE for the same item/warehouse on both engines. Co-Authored-By: Claude Opus 4.8 (1M context) --- erpnext/controllers/stock_controller.py | 2 +- .../tests/test_stock_controller.py | 42 +++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) create mode 100644 erpnext/controllers/tests/test_stock_controller.py diff --git a/erpnext/controllers/stock_controller.py b/erpnext/controllers/stock_controller.py index 46fbbbf484e..e350f2d950c 100644 --- a/erpnext/controllers/stock_controller.py +++ b/erpnext/controllers/stock_controller.py @@ -630,7 +630,7 @@ def check_item_quality_inspection(doctype: str, docstatus: str | int, items: str inspection_fieldname = INSPECTION_FIELDNAME_MAP.get(doctype) if inspection_fieldname is None: - return [] + return items if doctype == "Stock Entry" else [] allow_after_transaction = cint(docstatus) == 1 and frappe.get_single_value( "Stock Settings", "allow_to_make_quality_inspection_after_purchase_or_delivery" diff --git a/erpnext/controllers/tests/test_stock_controller.py b/erpnext/controllers/tests/test_stock_controller.py new file mode 100644 index 00000000000..beb0976b9e7 --- /dev/null +++ b/erpnext/controllers/tests/test_stock_controller.py @@ -0,0 +1,42 @@ +# Copyright (c) 2025, Frappe Technologies Pvt. Ltd. and Contributors +# See license.txt + +import frappe +from frappe.utils import add_days, today + +from erpnext.tests.utils import ERPNextTestSuite + + +class TestStockControllerConversions(ERPNextTestSuite): + @staticmethod + def _cancel_and_delete(doctype, name): + if not frappe.db.exists(doctype, name): + return + doc = frappe.get_doc(doctype, name) + if doc.docstatus == 1: + doc.cancel() + frappe.delete_doc(doctype, name, force=1) + + def test_future_sle_exists_detects_later_entries(self): + # future_sle_exists / get_conditions_to_validate_future_sle were converted to query builder + # (Count + Criterion.any). A later SLE for the same item+warehouse must be detected, which + # exercises the converted GROUP BY query on both engines. + from erpnext.controllers.stock_controller import future_sle_exists + from erpnext.stock.doctype.item.test_item import make_item + from erpnext.stock.doctype.stock_entry.stock_entry_utils import make_stock_entry + + item = make_item("_Test Future SLE Item", {"is_stock_item": 1}).name + se = make_stock_entry(item_code=item, target="_Test Warehouse - _TC", qty=10, basic_rate=100) + self.addCleanup(self._cancel_and_delete, "Stock Entry", se.name) + + # Pretend a different voucher posts a day earlier for the same item/warehouse: the existing + # (later) SLE must be reported as a future entry. + args = frappe._dict( + voucher_type="Stock Entry", + voucher_no="_TEST-NONEXISTENT-SE", + posting_date=add_days(today(), -1), + posting_time="00:00:00", + ) + sl_entries = [frappe._dict(item_code=item, warehouse="_Test Warehouse - _TC")] + + self.assertTrue(future_sle_exists(args, sl_entries))