From dbd1388b404bafcfc9a0fbef6bda9cdac0759482 Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Sun, 21 Jun 2026 22:38:30 +0530 Subject: [PATCH 1/2] fix(controllers): keep Sales/Purchase Trends one row per based-on key (MariaDB parity) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #56192 made the trends queries Postgres-strict-GROUP-BY-valid by widening based_on_group_by to include the selected descriptive columns. For Item it added t2.item_name, for Customer t1.territory (and customer_name) — but item_name is an editable per-line field and territory an editable per-document field, not functionally dependent on the item_code/customer key. On MariaDB (ONLY_FULL_GROUP_BY off) this SPLITS the single row per key into one row per distinct (key, item_name)/(key, territory), so a customer transacting across two territories (or an item with an edited item_name) now shows duplicate rows with fractured per-period subtotals. Group by the KEY only and aggregate the non-key descriptive columns with Max(): one row per based-on key (identical to the pre-#56192 MariaDB output) and still Postgres-valid. Supplier columns are master-joined / fetch-locked (functionally dependent) so they stay unchanged. --- erpnext/controllers/trends.py | 23 +++++++++++-------- .../test_sales_order_trends.py | 5 ++-- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/erpnext/controllers/trends.py b/erpnext/controllers/trends.py index 03e2b483d14..82b8bd30088 100644 --- a/erpnext/controllers/trends.py +++ b/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": diff --git a/erpnext/selling/report/sales_order_trends/test_sales_order_trends.py b/erpnext/selling/report/sales_order_trends/test_sales_order_trends.py index 9fbae8f2e31..61d768d410c 100644 --- a/erpnext/selling/report/sales_order_trends/test_sales_order_trends.py +++ b/erpnext/selling/report/sales_order_trends/test_sales_order_trends.py @@ -7,8 +7,9 @@ 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 From 8f1c703871edd0630a7448b0cdf6614e38c567d6 Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Mon, 22 Jun 2026 00:58:56 +0530 Subject: [PATCH 2/2] fix(controllers): keep Supplier trends one row per supplier + add regression tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The earlier parity fix aggregated the non-key descriptive columns for the Item and Customer based-on paths but left Supplier grouping by all three selected columns (supplier, supplier_name, supplier_group). supplier_name is a stored per-transaction field, so historical purchase docs holding a divergent value for the same supplier would split one supplier into multiple rows — diverging from the original MariaDB output, which grouped by t1.supplier only. Aggregate supplier_name with Max() and keep only supplier + the FD master column supplier_group in GROUP BY, restoring one row per supplier on both engines. Add regression tests for the Supplier (purchase) and Customer (sales) paths that assert a single row per key even when stored descriptive fields diverge; both fail on the pre-fix multi-column GROUP BY and pass after the fix, on MariaDB and Postgres. --- .../test_purchase_order_trends.py | 32 +++++++++++++++++++ erpnext/controllers/trends.py | 10 ++++-- .../test_sales_order_trends.py | 26 +++++++++++++++ 3 files changed, 66 insertions(+), 2 deletions(-) create mode 100644 erpnext/buying/report/purchase_order_trends/test_purchase_order_trends.py diff --git a/erpnext/buying/report/purchase_order_trends/test_purchase_order_trends.py b/erpnext/buying/report/purchase_order_trends/test_purchase_order_trends.py new file mode 100644 index 00000000000..90d84447cb7 --- /dev/null +++ b/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) diff --git a/erpnext/controllers/trends.py b/erpnext/controllers/trends.py index 82b8bd30088..92ff6adc5af 100644 --- a/erpnext/controllers/trends.py +++ b/erpnext/controllers/trends.py @@ -418,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" diff --git a/erpnext/selling/report/sales_order_trends/test_sales_order_trends.py b/erpnext/selling/report/sales_order_trends/test_sales_order_trends.py index 61d768d410c..46f856a6f03 100644 --- a/erpnext/selling/report/sales_order_trends/test_sales_order_trends.py +++ b/erpnext/selling/report/sales_order_trends/test_sales_order_trends.py @@ -1,6 +1,8 @@ # 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 @@ -25,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)