diff --git a/erpnext/controllers/trends.py b/erpnext/controllers/trends.py index 2563d3b7fd7..4ced0129bda 100644 --- a/erpnext/controllers/trends.py +++ b/erpnext/controllers/trends.py @@ -376,6 +376,41 @@ def get_period_month_ranges(period, fiscal_year): return period_month_ranges +def quotation_party_name_expr(): + """Resolve a Quotation's party label from its dynamic link, mirroring set_customer_name().""" + customer_branch = ( + "when t1.quotation_to = 'Customer' then " + "(select c.customer_name from `tabCustomer` c where c.name = t1.party_name)" + ) + lead_branch = ( + "when t1.quotation_to = 'Lead' then " + "(select coalesce(nullif(l.company_name, ''), l.lead_name) from `tabLead` l " + "where l.name = t1.party_name)" + ) + prospect_branch = "when t1.quotation_to = 'Prospect' then t1.party_name" + branches = [customer_branch, lead_branch, prospect_branch] + # CRM Deal ships with the CRM app; skip the branch when its table is absent + if frappe.db.table_exists("CRM Deal"): + branches.append( + "when t1.quotation_to = 'CRM Deal' then " + "(select d.organization from `tabCRM Deal` d where d.name = t1.party_name)" + ) + + return "case " + " ".join(branches) + " end" + + +def quotation_territory_expr(): + """Only Customer and Lead carry a territory; other party types have none.""" + return ( + "case " + "when t1.quotation_to = 'Customer' then " + "(select c.territory from `tabCustomer` c where c.name = t1.party_name) " + "when t1.quotation_to = 'Lead' then " + "(select l.territory from `tabLead` l where l.name = t1.party_name) " + "end" + ) + + def based_wise_columns_query(based_on, trans): based_on_details = {} @@ -385,12 +420,14 @@ def based_wise_columns_query(based_on, trans): {"label": _("Item"), "fieldtype": "Link", "options": "Item", "width": 120, "fieldname": "item"}, {"label": _("Item Name"), "fieldtype": "Data", "width": 120, "fieldname": "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"] = "" + # item_name is stored per line and editable, so it is not functionally dependent on item_code + # and Max() over it is a sort -- which MariaDB and PostgreSQL resolve differently. Read it + # from the Item master instead: that IS functionally dependent on the grouped item_code, so + # it can be grouped without splitting rows and is identical on both engines by construction. + based_on_details["based_on_select"] = "t2.item_code, item_master.item_name as item_name," + based_on_details["based_on_group_by"] = "t2.item_code, item_master.item_name" + based_on_details["addl_tables"] = ",`tabItem` item_master" + based_on_details["addl_tables_relational_cond"] = " and t2.item_code = item_master.name" elif based_on == "Item Group": based_on_details["based_on_cols"] = [ @@ -425,9 +462,17 @@ def based_wise_columns_query(based_on, trans): "fieldname": "territory", }, ] - based_on_details[ - "based_on_select" - ] = "t1.party_name, Max(t1.customer_name) as customer_name, Max(t1.territory) as territory," + # a Quotation's party_name is a dynamic link, so no single master can be joined. Resolve + # it through the quotation_to discriminator, mirroring Quotation.set_customer_name, and + # group by it too: two parties of different types can share a name, and merging them + # under one row was never right. Correlated only on grouped columns, so the query stays + # valid under GROUP BY and free of any text sort. + based_on_details["based_on_select"] = ( + f"t1.party_name, {quotation_party_name_expr()} as customer_name, " + f"{quotation_territory_expr()} as territory," + ) + based_on_details["based_on_group_by"] = "t1.party_name, t1.quotation_to" + based_on_details["addl_tables"] = "" else: based_on_details["based_on_cols"] = [ { @@ -451,13 +496,19 @@ def based_wise_columns_query(based_on, trans): "fieldname": "territory", }, ] + # customer_name and territory are stored per transaction and editable, so they are not + # functionally dependent on the customer and Max() over them is a text sort, which the + # engines resolve differently. The Customer master's values ARE dependent on the grouped + # key, so they can be grouped without splitting rows and agree on both engines. + based_on_details["based_on_select"] = ( + "t1.customer, customer_master.customer_name as customer_name, " + "customer_master.territory as 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"] = "" + "based_on_group_by" + ] = "t1.customer, customer_master.customer_name, customer_master.territory" + based_on_details["addl_tables"] = ",`tabCustomer` customer_master" + based_on_details["addl_tables_relational_cond"] = " and t1.customer = customer_master.name" elif based_on == "Customer Group": based_on_details["based_on_cols"] = [ @@ -490,14 +541,12 @@ def based_wise_columns_query(based_on, trans): "fieldname": "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" + # supplier_name is stored per transaction and editable, so Max() over it is a text sort that + # the engines resolve differently. The Supplier master is already joined here as t3 and its + # columns are functionally dependent on the grouped supplier, so both can simply be grouped: + # no row split, and identical on both engines by construction. + based_on_details["based_on_select"] = "t1.supplier, t3.supplier_name, t3.supplier_group," + based_on_details["based_on_group_by"] = "t1.supplier, t3.supplier_name, 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/quotation_trends/test_quotation_trends.py b/erpnext/selling/report/quotation_trends/test_quotation_trends.py index 95ba6dd50bc..8128195f7ab 100644 --- a/erpnext/selling/report/quotation_trends/test_quotation_trends.py +++ b/erpnext/selling/report/quotation_trends/test_quotation_trends.py @@ -88,6 +88,37 @@ class TestQuotationTrends(ERPNextTestSuite): labels, after = self.run_report(based_on="Customer") self.assertEqual(self._cell(after, "Party", "_Test Customer", amt_col, labels) - before_amt, 300) + def test_lead_quotation_label_resolves_through_quotation_to(self): + """party_name is a dynamic link, so the label must be resolved via quotation_to. + + Looking the party up in Customer alone leaves a Lead's row blank, and looking in Customer + first returns the wrong record when a Lead and a Customer share a name. + """ + lead_name = "_Test Trends Lead Party" + if not frappe.db.exists("Lead", {"lead_name": lead_name}): + frappe.get_doc({"doctype": "Lead", "lead_name": lead_name}).insert() + lead = frappe.db.get_value("Lead", {"lead_name": lead_name}, ["name", "company_name"], as_dict=True) + + quotation = frappe.new_doc("Quotation") + quotation.company = "_Test Company" + quotation.transaction_date = TXN_DATE + quotation.currency = "INR" + quotation.quotation_to = "Lead" + quotation.party_name = lead.name + quotation.append( + "items", + {"item_code": "_Test Item", "qty": 1, "rate": 100, "warehouse": "_Test Warehouse - _TC"}, + ) + quotation.insert() + quotation.submit() + + labels, rows = self.run_report(based_on="Customer") + party_idx, name_idx = labels.index("Party"), labels.index("Party Name") + lead_rows = [row for row in rows if row[party_idx] == lead.name] + + self.assertEqual(len(lead_rows), 1) + self.assertEqual(lead_rows[0][name_idx], lead.company_name or lead_name) + def test_group_by_chart_matches_table_total_with_mixed_group_sizes(self): # _Test Item is quoted to two customers -> two detail rows under one header row. # _Test Item 2 is quoted to only one customer -> exactly one detail row under its 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 47a1c9679f8..5abe913491d 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 @@ -31,11 +31,41 @@ 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_labels_come_from_the_master_not_a_stored_snapshot(self): + """territory and customer_name must be the Customer master's, not one order's snapshot. + + Both are stored per transaction and editable, so historical orders can hold different values + for one customer. Aggregating them with Max() is a text sort, and MariaDB (case-folding) and + PostgreSQL (byte order) resolve it differently, so the two engines could label the same row + differently. The master's values are functionally dependent on the grouped customer, so they + are the same on both engines by construction. + """ + 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) + frappe.db.set_value("Sales Order", so2.name, "territory", "_Test Territory Rest Of The World") + + master_territory, master_name = frappe.db.get_value( + "Customer", "_Test Customer", ["territory", "customer_name"] + ) + + columns, data, _chart_none, _chart = execute( + {"company": "_Test Company", "period": "Monthly", "based_on": "Customer"} + ) + + self.assertTrue(columns) + customer_rows = [row for row in data if row[0] == "_Test Customer"] + self.assertEqual(len(customer_rows), 1) + self.assertEqual(customer_rows[0][1], master_name) + self.assertEqual(customer_rows[0][2], master_territory) + 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. + # different value for the same customer. The report reads both from the Customer master, so it stays + # one row per customer on both MariaDB and Postgres. Grouping by the stored territory 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