Merge pull request #56217 from mihir-kandoi/pg-controllers-buying-itemvariant-trends

refactor(controllers): buying_controller + item_variant + trends Postgres validity

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])

erpnext/controllers/item_variant.py

@@ -443,13 +443,21 @@ def make_variant_item_code(template_item_code, template_item_name, variant):
 
 	abbreviations = []
 	for attr in variant.attributes:
-		item_attribute = frappe.db.sql(
-			"""select i.numeric_values, v.abbr
-			from `tabItem Attribute` i left join `tabItem Attribute Value` v
-				on (i.name=v.parent)
-			where i.name=%(attribute)s and (v.attribute_value=%(attribute_value)s or i.numeric_values = 1)""",
-			{"attribute": attr.attribute, "attribute_value": attr.attribute_value},
-			as_dict=True,
+		ia = frappe.qb.DocType("Item Attribute")
+		iav = frappe.qb.DocType("Item Attribute Value")
+		item_attribute = (
+			frappe.qb.from_(ia)
+			.left_join(iav)
+			.on(ia.name == iav.parent)
+			.select(ia.numeric_values, iav.abbr)
+			.where(
+				(ia.name == attr.attribute)
+				# attribute_value is a varchar column; cast the param to str so postgres doesn't choke on
+				# `varchar = numeric` for numeric attributes (where this side is irrelevant anyway, since
+				# numeric_values == 1 already satisfies the OR). Non-numeric values are already strings.
+				& ((iav.attribute_value == cstr(attr.attribute_value)) | (ia.numeric_values == 1))
+			)
+			.run(as_dict=True)
 		)
 
 		if not item_attribute:

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"] = (

erpnext/selling/report/sales_order_trends/test_sales_order_trends.py

@@ -0,0 +1,26 @@
+# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors
+# License: GNU General Public License v3. See license.txt
+
+from erpnext.tests.utils import ERPNextTestSuite
+
+
+class TestSalesOrderTrends(ERPNextTestSuite):
+	def test_report_executes_with_group_by(self):
+		# trends.get_data builds per-period SUM(CASE ...) aggregates (converted from MySQL SUM(IF)),
+		# with a GROUP BY widened to every selected non-aggregated column and a based_on_key for the
+		# group-by detail subqueries. Setting group_by exercises that full path on both engines.
+		from erpnext.selling.doctype.sales_order.test_sales_order import make_sales_order
+		from erpnext.selling.report.sales_order_trends.sales_order_trends import execute
+
+		make_sales_order(item_code="_Test Item", qty=3, rate=100)
+
+		filters = {
+			"company": "_Test Company",
+			"period": "Monthly",
+			"based_on": "Item",
+			"group_by": "Customer",
+		}
+		columns, data, _chart_none, _chart = execute(filters)
+
+		self.assertTrue(columns)
+		self.assertTrue(any("_Test Item" in [str(cell) for cell in row] for row in data))