From d74add35d46d2464cbff754cb47ce22519bffbd6 Mon Sep 17 00:00:00 2001 From: Mihir Kandoi Date: Mon, 3 Aug 2026 12:12:28 +0530 Subject: [PATCH] fix(accounts): take POS summary warehouse and cost centre from one item line (#57723) * fix(accounts): take POS summary warehouse and cost centre from one item line Both describe an item line, not the invoice, and an invoice can carry several. They were aggregated independently per invoice, so the report could show a warehouse from one line beside a cost centre from another -- a pair that was never posted. The warehouse then becomes an outer grouping key, so the pick is not merely a label: it decides how rows are partitioned across owner/date and therefore what each row totals. Max() over text is a sort, and MariaDB folds case while PostgreSQL orders by byte value, so the two engines can partition differently. Take both off one real line instead, and the mode of payment off one real payment line for the same reason. Sales Invoice Item is hash-named and Sales Invoice Payment declares no autoname rule, so frappe hash-names it too -- which keeps Min(name) free of the collation divergence that sorting text has. * test(accounts): cover POS summary warehouse/cost-centre coherence The existing tests post a single item line, so they cannot see this. Adds an invoice with two lines whose warehouse and cost centre are deliberately crossed: the higher warehouse sits on the line with the lower cost centre, so an independently aggregated pair belongs to neither line. * fix(accounts): pick the POS summary representative by idx, not by hash Min(name) selected whichever child row happened to have the lowest hash, which is arbitrary and turns on something unrelated to the data. Min(idx) selects the first line the user actually entered: an integer, so the pick is free of collation, and it is meaningful rather than incidental. The join moves to (parent, idx), which is unique per parent. --- .../sales_payment_summary.py | 52 +++++++++++++------ .../test_sales_payment_summary.py | 47 +++++++++++++++++ 2 files changed, 84 insertions(+), 15 deletions(-) diff --git a/erpnext/accounts/report/sales_payment_summary/sales_payment_summary.py b/erpnext/accounts/report/sales_payment_summary/sales_payment_summary.py index f5571f756bc..b26914e248b 100644 --- a/erpnext/accounts/report/sales_payment_summary/sales_payment_summary.py +++ b/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, Sum +from frappe.query_builder.functions import Coalesce, Max, Min, Sum from frappe.utils import cstr @@ -128,25 +128,47 @@ def get_pos_invoice_data(filters): 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/cost_center are line-level and - # not grouped, so they are arbitrary per invoice -- Max() makes that pick deterministic and valid on - # Postgres (item_code was selected but never consumed downstream, so it is dropped). - t1 = ( + # 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"), - Max(sii.warehouse).as_("warehouse"), - Max(sii.cost_center).as_("cost_center"), - ) + .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 = ( + frappe.qb.from_(grouped_items) + .inner_join(representative_item) + .on( + (representative_item.parent == grouped_items.parent) + & (representative_item.idx == grouped_items.representative_idx) + ) + .select( + grouped_items.parent, + grouped_items.base_total, + representative_item.warehouse, + representative_item.cost_center, + ) ) - # t3: mode_of_payment per invoice (arbitrary across an invoice's payment lines -> Max() to be valid) + # 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_(sip) - .select(sip.parent, Max(sip.mode_of_payment).as_("mode_of_payment")) - .groupby(sip.parent) + 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 diff --git a/erpnext/accounts/report/sales_payment_summary/test_sales_payment_summary.py b/erpnext/accounts/report/sales_payment_summary/test_sales_payment_summary.py index d2ee6f47939..dc7dc446c11 100644 --- a/erpnext/accounts/report/sales_payment_summary/test_sales_payment_summary.py +++ b/erpnext/accounts/report/sales_payment_summary/test_sales_payment_summary.py @@ -54,6 +54,53 @@ class TestSalesPaymentSummary(ERPNextTestSuite): self.assertIn("Credit Card", next(iter(mop.values()))) self.assertNotIn("Cash", next(iter(mop.values()))) + def test_pos_invoice_warehouse_and_cost_center_come_from_one_item(self): + """The reported warehouse and cost centre must belong to the same item line. + + They describe a line, not the invoice, and an invoice can carry several. Aggregating each + on its own can report a warehouse from one line beside a cost centre from another -- a pair + that was never posted. The warehouse is also an outer grouping key, so the pick decides how + rows are partitioned and what each one totals, not just what is displayed. + """ + from erpnext.stock.doctype.item.test_item import make_item + from erpnext.stock.doctype.warehouse.test_warehouse import create_warehouse + + low_warehouse = create_warehouse("_Test POS Summary AAA") + high_warehouse = create_warehouse("_Test POS Summary ZZZ") + second_item = make_item("_Test POS Summary Second Item", {"is_stock_item": 0}).name + + si = create_sales_invoice_record() + si.is_pos = 1 + # cross the two picks: the higher warehouse is on the line with the lower cost centre, so an + # independently aggregated pair cannot belong to either line + si.items[0].warehouse = high_warehouse + si.items[0].cost_center = "Main - _TC" + si.append( + "items", + { + "item_code": second_item, + "qty": 1, + "rate": 5000, + "income_account": "Sales - _TC", + "expense_account": "Cost of Goods Sold - _TC", + "warehouse": low_warehouse, + "cost_center": "Sub - _TC", + }, + ) + si.append("payments", {"mode_of_payment": "Cash", "account": "_Test Cash - _TC", "amount": 15000}) + si.insert() + si.submit() + + posted = {(row.warehouse, row.cost_center) for row in si.items} + self.assertGreater(len(posted), 1, "fixture must post more than one distinct pair") + + 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) + def test_get_mode_of_payments_details(self): filters = get_filters()