Merge pull request #56265 from mihir-kandoi/pg-trends-rowcount

fix(controllers): keep Sales/Purchase Trends one row per based-on key (MariaDB parity)

erpnext/buying/report/purchase_order_trends/test_purchase_order_trends.py

@@ -0,0 +1,32 @@
+# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors
+# License: GNU General Public License v3. See license.txt
+
+import frappe
+
+from erpnext.tests.utils import ERPNextTestSuite
+
+
+class TestPurchaseOrderTrends(ERPNextTestSuite):
+	def test_supplier_with_divergent_stored_name_stays_one_row(self):
+		# supplier_name is a stored per-transaction field; historical purchase docs can hold a different
+		# value for the same supplier. trends groups by t1.supplier only and aggregates supplier_name with
+		# Max(), so the report stays one row per supplier on both MariaDB and Postgres. Grouping by
+		# supplier_name (the pre-fix behaviour) would split the supplier into two rows.
+		from erpnext.buying.doctype.purchase_order.test_purchase_order import create_purchase_order
+		from erpnext.buying.report.purchase_order_trends.purchase_order_trends import execute
+
+		create_purchase_order(supplier="_Test Supplier", qty=3, rate=100)
+		po2 = create_purchase_order(supplier="_Test Supplier", qty=2, rate=100)
+		# simulate a historical doc that stored a different supplier_name for the same supplier
+		frappe.db.set_value("Purchase Order", po2.name, "supplier_name", "_Test Supplier (renamed)")
+
+		filters = {
+			"company": "_Test Company",
+			"period": "Monthly",
+			"based_on": "Supplier",
+		}
+		columns, data, _chart_none, _chart = execute(filters)
+
+		self.assertTrue(columns)
+		supplier_rows = [row for row in data if row[0] == "_Test Supplier"]
+		self.assertEqual(len(supplier_rows), 1)

erpnext/controllers/trends.py

@@ -369,8 +369,11 @@ def based_wise_columns_query(based_on, trans):
 	# based_on_cols, based_on_select, based_on_group_by, addl_tables
 	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, t2.item_name"
+		# item_name is an editable per-line field, not functionally dependent on item_code, so it
+		# is aggregated (one row per item_code) rather than added to GROUP BY (which would split
+		# the row and change the MariaDB row count). See get_data's group-by query.
+		based_on_details["based_on_select"] = "t2.item_code, Max(t2.item_name) as item_name,"
+		based_on_details["based_on_group_by"] = "t2.item_code"
 		based_on_details["addl_tables"] = ""
 
 	elif based_on == "Item Group":
@@ -386,19 +389,21 @@ def based_wise_columns_query(based_on, trans):
 				"Party Name:Data:120",
 				"Territory:Link/Territory:120",
 			]
-			based_on_details["based_on_select"] = "t1.party_name, t1.customer_name, t1.territory,"
+			based_on_details[
+				"based_on_select"
+			] = "t1.party_name, Max(t1.customer_name) as customer_name, Max(t1.territory) as territory,"
 		else:
 			based_on_details["based_on_cols"] = [
 				"Customer:Link/Customer:120",
 				"Customer Name:Data:120",
 				"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, t1.customer_name, t1.territory"
-			if trans == "Quotation"
-			else "t1.customer, t1.customer_name, t1.territory"
-		)
+			based_on_details[
+				"based_on_select"
+			] = "t1.customer, Max(t1.customer_name) as customer_name, Max(t1.territory) as territory,"
+		# territory (and customer_name) are not functionally dependent on the customer key, so they
+		# are aggregated rather than grouped — one row per customer, matching the prior MariaDB output.
+		based_on_details["based_on_group_by"] = "t1.party_name" if trans == "Quotation" else "t1.customer"
 		based_on_details["addl_tables"] = ""
 
 	elif based_on == "Customer Group":
@@ -413,8 +418,14 @@ def based_wise_columns_query(based_on, trans):
 			"Supplier Name:Data:120",
 			"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, t1.supplier_name, t3.supplier_group"
+		# supplier_name is a stored per-transaction field (not functionally dependent on supplier), so
+		# it is aggregated to keep one row per supplier — matching the prior MariaDB output, which grouped
+		# by t1.supplier only. supplier_group comes from the joined master and is FD on supplier, so it
+		# stays in GROUP BY (postgres-valid, no row split).
+		based_on_details[
+			"based_on_select"
+		] = "t1.supplier, Max(t1.supplier_name) as supplier_name, t3.supplier_group,"
+		based_on_details["based_on_group_by"] = "t1.supplier, t3.supplier_group"
 		based_on_details["addl_tables"] = ",`tabSupplier` t3"
 		based_on_details["addl_tables_relational_cond"] = " and t1.supplier = t3.name"
 

erpnext/selling/report/sales_order_trends/test_sales_order_trends.py

@@ -1,14 +1,17 @@
 # Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors
 # License: GNU General Public License v3. See license.txt
 
+import frappe
+
 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.
+		# groups by the based-on KEY only (non-key descriptive columns like item_name/territory are
+		# MAX()-aggregated so the report stays one row per key on both engines), and uses 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
 
@@ -24,3 +27,27 @@ class TestSalesOrderTrends(ERPNextTestSuite):
 
 		self.assertTrue(columns)
 		self.assertTrue(any("_Test Item" in [str(cell) for cell in row] for row in data))
+
+	def test_customer_with_divergent_stored_territory_stays_one_row(self):
+		# territory (and customer_name) are stored per-transaction fields; historical sales docs can hold a
+		# different value for the same customer. trends groups by t1.customer only and aggregates these with
+		# Max(), so the report stays one row per customer on both MariaDB and Postgres. Grouping by territory
+		# (the pre-fix behaviour) would split the customer into two rows.
+		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(customer="_Test Customer", item_code="_Test Item", qty=3, rate=100)
+		so2 = make_sales_order(customer="_Test Customer", item_code="_Test Item", qty=2, rate=100)
+		# simulate a historical doc that stored a different territory for the same customer
+		frappe.db.set_value("Sales Order", so2.name, "territory", "_Test Territory Rest Of The World")
+
+		filters = {
+			"company": "_Test Company",
+			"period": "Monthly",
+			"based_on": "Customer",
+		}
+		columns, data, _chart_none, _chart = execute(filters)
+
+		self.assertTrue(columns)
+		customer_rows = [row for row in data if row[0] == "_Test Customer"]
+		self.assertEqual(len(customer_rows), 1)