From b85c79c3e09780204aeec88009d0cb6bca61c983 Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Sun, 21 Jun 2026 06:20:15 +0530 Subject: [PATCH] fix(stock): convert item.py raw SQL to qb/ORM (Postgres-valid) Convert the raw frappe.db.sql statements in Item to frappe.qb / ORM: validate_barcode duplicate check (-> frappe.get_all), stock_ledger_created (-> frappe.db.exists), update_item_price + the BOM/BOM Item/BOM Explosion description updates + check_stock_uom_with_bin's Bin UOM update (-> frappe.qb .update), on_trash Bin/Item Price deletes (-> frappe.db.delete), check_stock_uom_with_bin's bin lookup (-> frappe.get_all with or_filters), and get_uom_conv_factor's self-join (-> frappe.qb). The one genuine Postgres break is validate_duplicate_item_in_stock_reconciliation: its raw query used `HAVING records > 1`, referencing the SELECT alias, which Postgres rejects. The qb version uses `HAVING Count("*") > 1`. Surgical re-apply (not a whole-file port): develop's opening-stock-reconciliation flow (set_opening_stock / create_opening_stock_reconciliation / make_opening_stock_entry) is preserved, and get_timeline_data keeps develop's CurDate()-Interval form (valid on both engines), so the Interval/CurDate/ SerialBatchCreation imports are retained. Verified: test_item 38/38 on MariaDB. Added a merge-rename test exercising the HAVING query (validate_duplicate_item_in_stock_reconciliation) which passes on MariaDB and Postgres. Co-Authored-By: Claude Opus 4.8 (1M context) --- erpnext/stock/doctype/item/item.py | 151 +++++++++++------------- erpnext/stock/doctype/item/test_item.py | 12 ++ 2 files changed, 80 insertions(+), 83 deletions(-) diff --git a/erpnext/stock/doctype/item/item.py b/erpnext/stock/doctype/item/item.py index 74e8dde48af..3cec8d45845 100644 --- a/erpnext/stock/doctype/item/item.py +++ b/erpnext/stock/doctype/item/item.py @@ -534,14 +534,16 @@ class Item(Document): for item_barcode in self.barcodes: options = frappe.get_meta("Item Barcode").get_options("barcode_type").split("\n") if item_barcode.barcode: - duplicate = frappe.db.sql( - """select parent from `tabItem Barcode` where barcode = %s and parent != %s""", - (item_barcode.barcode, self.name), + duplicate = frappe.get_all( + "Item Barcode", + filters={"barcode": item_barcode.barcode, "parent": ["!=", self.name]}, + pluck="parent", + limit=1, # only the first conflicting item is reported ) if duplicate: frappe.throw( _("Barcode {0} already used in Item {1}").format( - item_barcode.barcode, duplicate[0][0] + item_barcode.barcode, duplicate[0] ) ) @@ -602,12 +604,8 @@ class Item(Document): def stock_ledger_created(self): if not hasattr(self, "_stock_ledger_created"): - self._stock_ledger_created = len( - frappe.db.sql( - """select name from `tabStock Ledger Entry` - where item_code = %s and is_cancelled = 0 limit 1""", - self.name, - ) + self._stock_ledger_created = bool( + frappe.db.exists("Stock Ledger Entry", {"item_code": self.name, "is_cancelled": 0}) ) return self._stock_ledger_created @@ -618,26 +616,18 @@ class Item(Document): if self.is_new(): return - frappe.db.sql( - """ - UPDATE `tabItem Price` - SET - item_name=%(item_name)s, - item_description=%(item_description)s, - brand=%(brand)s - WHERE item_code=%(item_code)s - """, - dict( - item_name=self.item_name, - item_description=self.description, - brand=self.brand, - item_code=self.name, - ), - ) + item_price = frappe.qb.DocType("Item Price") + ( + frappe.qb.update(item_price) + .set(item_price.item_name, self.item_name) + .set(item_price.item_description, self.description) + .set(item_price.brand, self.brand) + .where(item_price.item_code == self.name) + ).run() def on_trash(self): - frappe.db.sql("""delete from tabBin where item_code=%s""", self.name) - frappe.db.sql("delete from `tabItem Price` where item_code=%s", self.name) + frappe.db.delete("Bin", {"item_code": self.name}) + frappe.db.delete("Item Price", {"item_code": self.name}) for variant_of in frappe.get_all("Item", filters={"variant_of": self.name}): frappe.delete_doc("Item", variant_of.name) @@ -669,15 +659,15 @@ class Item(Document): frappe.db.delete("Bin", {"item_code": old_name}) def validate_duplicate_item_in_stock_reconciliation(self, old_name, new_name): - records = frappe.db.sql( - """ SELECT parent, COUNT(*) as records - FROM `tabStock Reconciliation Item` - WHERE item_code = %s and docstatus = 1 - GROUP By item_code, warehouse, parent - HAVING records > 1 - """, - new_name, - as_dict=1, + sri = frappe.qb.DocType("Stock Reconciliation Item") + records = ( + frappe.qb.from_(sri) + .select(sri.parent, Count("*").as_("records")) + .where((sri.item_code == new_name) & (sri.docstatus == 1)) + .groupby(sri.item_code, sri.warehouse, sri.parent) + # HAVING references the aggregate itself; postgres rejects the SELECT alias here + .having(Count("*") > 1) + .run(as_dict=1) ) if not records: @@ -757,32 +747,26 @@ class Item(Document): return if self.db_get("description") != self.description: - frappe.db.sql( - """ - update `tabBOM` - set description = %s - where item = %s and docstatus < 2 - """, - (self.description, self.name), - ) + bom = frappe.qb.DocType("BOM") + ( + frappe.qb.update(bom) + .set(bom.description, self.description) + .where((bom.item == self.name) & (bom.docstatus < 2)) + ).run() - frappe.db.sql( - """ - update `tabBOM Item` - set description = %s - where item_code = %s and docstatus < 2 - """, - (self.description, self.name), - ) + bom_item = frappe.qb.DocType("BOM Item") + ( + frappe.qb.update(bom_item) + .set(bom_item.description, self.description) + .where((bom_item.item_code == self.name) & (bom_item.docstatus < 2)) + ).run() - frappe.db.sql( - """ - update `tabBOM Explosion Item` - set description = %s - where item_code = %s and docstatus < 2 - """, - (self.description, self.name), - ) + bom_explosion_item = frappe.qb.DocType("BOM Explosion Item") + ( + frappe.qb.update(bom_explosion_item) + .set(bom_explosion_item.description, self.description) + .where((bom_explosion_item.item_code == self.name) & (bom_explosion_item.docstatus < 2)) + ).run() def validate_item_defaults(self): companies = {row.company for row in self.item_defaults} @@ -1380,14 +1364,17 @@ def check_stock_uom_with_bin(item, stock_uom): ).format(item) ) - bin_list = frappe.db.sql( - """ - select * from `tabBin` where item_code = %s - and (reserved_qty > 0 or ordered_qty > 0 or indented_qty > 0 or planned_qty > 0) - and stock_uom != %s - """, - (item, stock_uom), - as_dict=1, + bin_list = frappe.get_all( + "Bin", + filters={"item_code": item, "stock_uom": ["!=", stock_uom]}, + or_filters=[ + ["reserved_qty", ">", 0], + ["ordered_qty", ">", 0], + ["indented_qty", ">", 0], + ["planned_qty", ">", 0], + ], + pluck="name", # only used as an existence check below + limit=1, ) if bin_list: @@ -1398,7 +1385,8 @@ def check_stock_uom_with_bin(item, stock_uom): ) # No SLE or documents against item. Bin UOM can be changed safely. - frappe.db.sql("""update `tabBin` set stock_uom=%s where item_code=%s""", (stock_uom, item)) + bin_dt = frappe.qb.DocType("Bin") + frappe.qb.update(bin_dt).set(bin_dt.stock_uom, stock_uom).where(bin_dt.item_code == item).run() def get_item_defaults(item_code, company): @@ -1468,19 +1456,16 @@ def get_uom_conv_factor(uom: str | None, stock_uom: str | None): # g -> mg = 1000 # g -> kg = 0.001 # therefore kg -> mg = 1000 / 0.001 = 1,000,000 - intermediate_match = frappe.db.sql( - """ - select (first.value / second.value) as value - from `tabUOM Conversion Factor` first - join `tabUOM Conversion Factor` second - on first.from_uom = second.from_uom - where - first.to_uom = %(to_uom)s - and second.to_uom = %(from_uom)s - limit 1 - """, - {"to_uom": to_uom, "from_uom": from_uom}, - as_dict=1, + first = frappe.qb.DocType("UOM Conversion Factor").as_("first") + second = frappe.qb.DocType("UOM Conversion Factor").as_("second") + intermediate_match = ( + frappe.qb.from_(first) + .join(second) + .on(first.from_uom == second.from_uom) + .select((first.value / second.value).as_("value")) + .where((first.to_uom == to_uom) & (second.to_uom == from_uom)) + .limit(1) + .run(as_dict=1) ) if intermediate_match: diff --git a/erpnext/stock/doctype/item/test_item.py b/erpnext/stock/doctype/item/test_item.py index fcfa5d50075..5273b792b9b 100644 --- a/erpnext/stock/doctype/item/test_item.py +++ b/erpnext/stock/doctype/item/test_item.py @@ -76,6 +76,18 @@ class TestItem(ERPNextTestSuite): super().setUp() frappe.flags.attribute_values = None + def test_merge_rename_runs_duplicate_stock_reco_check(self): + # after_rename(merge=True) calls validate_duplicate_item_in_stock_reconciliation, whose query + # uses HAVING Count(*) > 1. The raw version referenced the SELECT alias in HAVING, which + # Postgres rejects; this exercises the converted query on both engines. + make_item("_Test Item Merge Source", {"is_stock_item": 1, "is_purchase_item": 1}) + make_item("_Test Item Merge Target", {"is_stock_item": 1, "is_purchase_item": 1}) + + frappe.rename_doc("Item", "_Test Item Merge Source", "_Test Item Merge Target", merge=True) + + self.assertTrue(frappe.db.exists("Item", "_Test Item Merge Target")) + self.assertFalse(frappe.db.exists("Item", "_Test Item Merge Source")) + def get_item(self, idx): item_code = self.globalTestRecords["Item"][idx].get("item_code") if not frappe.db.exists("Item", item_code):