nsinnovations/erpnext
2
0
Fork 0
mirror of https://github.com/frappe/erpnext.git synced 2026-08-14 07:01:56 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(controllers): source trend report labels from the master (#57724)

Browse Source 
* fix(controllers): source trend report labels from the master

item_name, customer_name, territory and supplier_name are stored on each
transaction and editable, so they are not functionally dependent on the grouped
key and historical documents can hold different values for the same item,
customer or supplier. Aggregating them with Max() is a text sort, and MariaDB
folds case while PostgreSQL orders by byte value, so the two engines can label
the same row differently.

Read each from its master instead. Those values ARE dependent on the grouped
key, so they can be grouped without splitting rows and agree on both engines by
construction rather than by an assumption about the data. Supplier needed no new
join -- the Supplier master was already joined as t3 for supplier_group.

A Quotation's party_name is a dynamic link to either a Customer or a Lead, so
neither master can be joined without dropping the other; there the values come
from correlated subqueries over both, keyed only on the grouped party_name.

Row counts and every numeric total are unchanged. What changes is that a
renamed record now shows its current name rather than whichever historical
snapshot happened to sort highest.

* test(selling): assert which label the trends report returns

The existing tests assert the customer stays one row but never which territory
or name comes back, so a divergence between engines passes unnoticed. Asserts
both equal the Customer master's values while an order stores a different
territory.

* fix(controllers): resolve a Quotation's party label through quotation_to

party_name is a dynamic link, so looking it up in Customer and Lead alone was
wrong twice over: a Quotation raised against a Prospect or a CRM Deal got a
blank label, and when a Lead shared its name with a Customer the Customer-first
lookup returned the wrong record's name and territory.

Resolve through the quotation_to discriminator instead, mirroring
Quotation.set_customer_name -- Customer, Lead (company_name falling back to
lead_name), Prospect, and CRM Deal. The CRM Deal branch is emitted only when its
table exists, since it ships with the CRM app.

quotation_to joins the GROUP BY as well: two parties of different types can
share a name, and merging them into one row was never right.

* style(controllers): name the quotation CASE branches

semgrep's string-concat-in-list flags adjacent string literals inside a list,
since that shape is usually a missing comma rather than deliberate. Bind each
branch to a name first so the concatenation is unambiguous.
This commit is contained in:
Mihir Kandoi
2026-08-03 12:22:25 +05:30
committed by GitHub
parent 8154c45bf0
commit f03c1311cd
3 changed files with 136 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files

95
erpnext/controllers/trends.py
View File

@@ -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"

31
erpnext/selling/report/quotation_trends/test_quotation_trends.py
View File

@@ -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

36
erpnext/selling/report/sales_order_trends/test_sales_order_trends.py
View File

@@ -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