fix(accounts): take the POS summary labels off one invoice, not a text sort (#59130)

erpnext/accounts/report/sales_payment_summary/sales_payment_summary.py

@@ -3,7 +3,7 @@
 
 import frappe
 from frappe import _
-from frappe.query_builder.functions import Coalesce, Max, Min, Sum
+from frappe.query_builder.functions import Coalesce, Min, Sum
 from frappe.utils import cstr
 
 
@@ -47,19 +47,23 @@ def get_columns(filters):
 
 def get_pos_sales_payment_data(filters):
 	sales_invoice_data = get_pos_invoice_data(filters)
-	data = [
-		[
-			row["posting_date"],
-			row["owner"],
-			row["mode_of_payment"],
-			row["net_total"],
-			row["total_taxes"],
-			row["paid_amount"],
-			row["warehouse"],
-			row["cost_center"],
-		]
-		for row in sales_invoice_data
-	]
+	labels = get_pos_row_labels(filters)
+
+	data = []
+	for row in sales_invoice_data:
+		label = labels.get(get_pos_row_key(row)) or frappe._dict()
+		data.append(
+			[
+				row["posting_date"],
+				row["owner"],
+				label.mode_of_payment,
+				row["net_total"],
+				row["total_taxes"],
+				row["paid_amount"],
+				row["warehouse"],
+				label.cost_center,
+			]
+		)
 
 	return data
 
@@ -123,25 +127,17 @@ def apply_conditions(query, a, filters):
 	return query
 
 
-def get_pos_invoice_data(filters):
+def get_invoice_item_totals():
+	"""One row per invoice: summed item base_total, plus warehouse and cost_center off its first line."""
 	sii = frappe.qb.DocType("Sales Invoice Item")
-	sip = frappe.qb.DocType("Sales Invoice Payment")
-	si = frappe.qb.DocType("Sales Invoice")
-
-	# t1: one row per invoice with the summed item base_total. warehouse and cost_center describe an
-	# item line, not the invoice, and an invoice may carry several. warehouse then becomes an outer
-	# grouping key below, so which line wins decides how rows are partitioned and what each row totals
-	# -- not merely which label is shown. Max() over text is a sort, and MariaDB (case-folding) and
-	# PostgreSQL (byte order) resolve it differently, so take both off one real line instead.
-	# The representative is the first line the user entered: Min(idx) is an integer, so the pick is
-	# free of collation and is meaningful, rather than turning on an unrelated hash-named row.
 	grouped_items = (
 		frappe.qb.from_(sii)
 		.select(sii.parent, Sum(sii.amount).as_("base_total"), Min(sii.idx).as_("representative_idx"))
 		.groupby(sii.parent)
 	).as_("grouped_items")
 	representative_item = frappe.qb.DocType("Sales Invoice Item").as_("representative_item")
-	t1 = (
+
+	return (
 		frappe.qb.from_(grouped_items)
 		.inner_join(representative_item)
 		.on(
@@ -156,24 +152,12 @@ def get_pos_invoice_data(filters):
 		)
 	)
 
-	# t3: mode_of_payment per invoice, from one real payment line for the same reason
-	grouped_payments = (
-		frappe.qb.from_(sip).select(sip.parent, Min(sip.idx).as_("representative_idx")).groupby(sip.parent)
-	).as_("grouped_payments")
-	representative_payment = frappe.qb.DocType("Sales Invoice Payment").as_("representative_payment")
-	t3 = (
-		frappe.qb.from_(grouped_payments)
-		.inner_join(representative_payment)
-		.on(
-			(representative_payment.parent == grouped_payments.parent)
-			& (representative_payment.idx == grouped_payments.representative_idx)
-		)
-		.select(grouped_payments.parent, representative_payment.mode_of_payment.as_("mode_of_payment"))
-	)
 
-	# a: invoice-level aggregates. Grouped by the primary key (si.name), so the other plain si columns
-	# (incl. customer, needed by the customer filter) are functionally dependent and valid on Postgres.
-	a = (
+def get_invoice_totals():
+	"""Invoice-level aggregates, grouped by the primary key so every plain column is dependent."""
+	si = frappe.qb.DocType("Sales Invoice")
+
+	return (
 		frappe.qb.from_(si)
 		.select(
 			si.docstatus,
@@ -183,6 +167,7 @@ def get_pos_invoice_data(filters):
 			si.name,
 			si.posting_date,
 			si.owner,
+			si.creation,
 			Sum(si.base_total).as_("base_total"),
 			Sum(si.net_total).as_("net_total"),
 			Sum(si.total_taxes_and_charges).as_("total_taxes"),
@@ -192,12 +177,76 @@ def get_pos_invoice_data(filters):
 		.groupby(si.name)
 	)
 
+
+def get_pos_row_key(row):
+	return (row.owner, row.posting_date, row.warehouse)
+
+
+def get_representative_payments():
+	"""One payment line per invoice: the first the user entered."""
+	sip = frappe.qb.DocType("Sales Invoice Payment")
+	grouped_payments = (
+		frappe.qb.from_(sip).select(sip.parent, Min(sip.idx).as_("representative_idx")).groupby(sip.parent)
+	).as_("grouped_payments")
+	representative_payment = frappe.qb.DocType("Sales Invoice Payment").as_("representative_payment")
+
+	return (
+		frappe.qb.from_(grouped_payments)
+		.inner_join(representative_payment)
+		.on(
+			(representative_payment.parent == grouped_payments.parent)
+			& (representative_payment.idx == grouped_payments.representative_idx)
+		)
+		.select(grouped_payments.parent, representative_payment.mode_of_payment.as_("mode_of_payment"))
+	)
+
+
+def get_pos_row_labels(filters):
+	"""cost_center and mode_of_payment off the earliest invoice in each row.
+
+	Ordered in Python rather than SQL, so no database collation applies to the tie-break.
+	"""
+	t1 = get_invoice_item_totals()
+	t3 = get_representative_payments()
+	a = get_invoice_totals()
+
 	query = (
 		frappe.qb.from_(t1)
 		.left_join(t3)
 		.on(t3.parent == t1.parent)
 		.join(a)
 		.on((t1.parent == a.name) & (t1.base_total == a.base_total))
+		.select(
+			a.owner,
+			a.posting_date,
+			a.creation,
+			a.name,
+			t1.warehouse,
+			t1.cost_center,
+			t3.mode_of_payment,
+		)
+		.where(a.docstatus == 1)
+	)
+	query = apply_conditions(query, a, filters)
+
+	labels = {}
+	for row in query.run(as_dict=True):
+		key = get_pos_row_key(row)
+		current = labels.get(key)
+		if current is None or (row.creation, row.name) < (current.creation, current.name):
+			labels[key] = row
+
+	return labels
+
+
+def get_pos_invoice_data(filters):
+	t1 = get_invoice_item_totals()
+	a = get_invoice_totals()
+
+	query = (
+		frappe.qb.from_(t1)
+		.join(a)
+		.on((t1.parent == a.name) & (t1.base_total == a.base_total))
 		.select(
 			a.posting_date,
 			a.owner,
@@ -205,10 +254,7 @@ def get_pos_invoice_data(filters):
 			Sum(a.total_taxes).as_("total_taxes"),
 			Sum(a.paid_amount).as_("paid_amount"),
 			Sum(a.outstanding_amount).as_("outstanding_amount"),
-			# mode_of_payment/cost_center are not in the outer GROUP BY -> Max() (deterministic, both engines)
-			Max(t3.mode_of_payment).as_("mode_of_payment"),
 			t1.warehouse,
-			Max(t1.cost_center).as_("cost_center"),
 		)
 		.where(a.docstatus == 1)
 		.groupby(a.owner, a.posting_date, t1.warehouse)

erpnext/accounts/report/sales_payment_summary/test_sales_payment_summary.py

@@ -9,6 +9,8 @@ from erpnext.accounts.report.sales_payment_summary.sales_payment_summary import
 	get_mode_of_payment_details,
 	get_mode_of_payments,
 	get_pos_invoice_data,
+	get_pos_row_key,
+	get_pos_row_labels,
 )
 from erpnext.tests.utils import ERPNextTestSuite
 
@@ -94,12 +96,43 @@ class TestSalesPaymentSummary(ERPNextTestSuite):
 		posted = {(row.warehouse, row.cost_center) for row in si.items}
 		self.assertGreater(len(posted), 1, "fixture must post more than one distinct pair")
 
+		labels = get_pos_row_labels(get_filters())
 		rows = get_pos_invoice_data(get_filters())
 		reported = [r for r in rows if r.get("warehouse") in {w for w, _ in posted}]
 		self.assertTrue(reported)
 
 		for row in reported:
-			self.assertIn((row["warehouse"], row["cost_center"]), posted)
+			label = labels[get_pos_row_key(row)]
+			self.assertIn((row["warehouse"], label.cost_center), posted)
+
+	def test_pos_row_labels_come_from_the_earliest_invoice(self):
+		"""The reported cost centre and payment mode must be one invoice's, and the same one's.
+
+		A row covers every invoice sharing an owner, date and warehouse, so neither column describes
+		it. Aggregating each independently sorts text -- which the two engines resolve differently --
+		and can pair one invoice's cost centre with another's payment mode.
+		"""
+		from erpnext.stock.doctype.warehouse.test_warehouse import create_warehouse
+
+		warehouse = create_warehouse("_Test POS Row Labels")
+		card = create_mode_of_payment("_Test POS Card", "_Test Bank - _TC")
+
+		# cross the two picks: the earlier invoice holds the lower cost centre and the higher mode
+		posted = [("Main - _TC", card, "_Test Bank - _TC"), ("Sub - _TC", "Cash", "_Test Cash - _TC")]
+		for cost_center, mode_of_payment, account in posted:
+			si = create_sales_invoice_record()
+			si.is_pos = 1
+			si.items[0].warehouse = warehouse
+			si.items[0].cost_center = cost_center
+			si.append("payments", {"mode_of_payment": mode_of_payment, "account": account, "amount": 10000})
+			si.insert()
+			si.submit()
+
+		rows = [row for row in get_pos_invoice_data(get_filters()) if row.get("warehouse") == warehouse]
+		self.assertEqual(len(rows), 1, "the reported row count must not change")
+
+		label = get_pos_row_labels(get_filters())[get_pos_row_key(rows[0])]
+		self.assertEqual((label.cost_center, label.mode_of_payment), ("Main - _TC", card))
 
 	def test_get_mode_of_payments_details(self):
 		filters = get_filters()
@@ -182,6 +215,21 @@ def get_filters():
 	return {"from_date": "1900-01-01", "to_date": today(), "company": "_Test Company"}
 
 
+def create_mode_of_payment(name, account, company="_Test Company"):
+	"""A POS payment row needs its mode to carry a default account for the company."""
+	if not frappe.db.exists("Mode of Payment", name):
+		frappe.get_doc(
+			{
+				"doctype": "Mode of Payment",
+				"mode_of_payment": name,
+				"type": "Bank",
+				"accounts": [{"company": company, "default_account": account}],
+			}
+		).insert()
+
+	return name
+
+
 def create_sales_invoice_record(qty=1):
 	# return sales invoice doc object
 	return frappe.get_doc(