diff --git a/erpnext/accounts/doctype/payment_reconciliation/test_payment_reconciliation.py b/erpnext/accounts/doctype/payment_reconciliation/test_payment_reconciliation.py index 874b8c78cbf..dff218e77d1 100644 --- a/erpnext/accounts/doctype/payment_reconciliation/test_payment_reconciliation.py +++ b/erpnext/accounts/doctype/payment_reconciliation/test_payment_reconciliation.py @@ -187,6 +187,103 @@ class TestPaymentReconciliation(ERPNextTestSuite): ) return je + def test_voucher_outstanding_metadata_comes_from_one_ledger_entry(self): + """cost_center and remarks must describe the same Payment Ledger Entry. + + A voucher can post several ledger entries for one party with different cost centers and + remarks. Aggregating each column on its own can pair one entry's cost center with another's + remarks -- a row that was never posted -- and because Max() over text is a sort, MariaDB and + PostgreSQL can pick differently on top of that. + """ + from erpnext.accounts.utils import QueryPaymentLedger + + je = frappe.new_doc("Journal Entry") + je.posting_date = nowdate() + je.company = self.company + je.user_remark = "aaa base remark" + for cost_center, remark, amount in ( + (self.main_cc, "aaa main line", 100), + (self.sub_cc, "zzz sub line", 50), + ): + je.append( + "accounts", + { + "account": self.debit_to, + "party_type": "Customer", + "party": self.customer, + "cost_center": cost_center, + "user_remark": remark, + "debit_in_account_currency": amount, + }, + ) + je.append( + "accounts", {"account": self.cash, "cost_center": self.main_cc, "credit_in_account_currency": 150} + ) + je.save() + je.submit() + + posted = { + (row.cost_center, row.remarks) + for row in frappe.get_all( + "Payment Ledger Entry", + filters={"voucher_no": je.name, "delinked": 0}, + fields=["cost_center", "remarks"], + ) + } + self.assertGreater(len(posted), 1, "fixture must post more than one ledger entry to be meaningful") + + ledger = QueryPaymentLedger() + rows = ledger.get_voucher_outstandings( + vouchers=[frappe._dict(voucher_type="Journal Entry", voucher_no=je.name)] + ) + self.assertTrue(rows) + + for row in rows: + self.assertIn((row.cost_center, row.remarks), posted) + + def test_voucher_outstanding_splits_by_party_account(self): + """A voucher posting to two party accounts must report each account separately. + + account is the join key between the amount and outstanding CTEs. Selecting Max(account) + while grouping without it made that key an aggregate over two different row sets, so the two + sides could pick different accounts, the join would miss and the outstanding come back NULL. + It also summed amounts across accounts that need not share a currency. + """ + from erpnext.accounts.utils import QueryPaymentLedger + + second_receivable = "_Test Receivable - _TC" + je = frappe.new_doc("Journal Entry") + je.posting_date = nowdate() + je.company = self.company + je.user_remark = "two receivable accounts" + for account, amount in ((self.debit_to, 100), (second_receivable, 60)): + je.append( + "accounts", + { + "account": account, + "party_type": "Customer", + "party": self.customer, + "cost_center": self.main_cc, + "debit_in_account_currency": amount, + }, + ) + je.append( + "accounts", {"account": self.cash, "cost_center": self.main_cc, "credit_in_account_currency": 160} + ) + je.save() + je.submit() + + rows = QueryPaymentLedger().get_voucher_outstandings( + vouchers=[frappe._dict(voucher_type="Journal Entry", voucher_no=je.name)] + ) + by_account = {row.account: row for row in rows} + + self.assertEqual(set(by_account), {self.debit_to, second_receivable}) + self.assertEqual(flt(by_account[self.debit_to].invoice_amount), 100) + self.assertEqual(flt(by_account[second_receivable].invoice_amount), 60) + for row in rows: + self.assertIsNotNone(row.outstanding) + def test_filter_min_max(self): # check filter condition minimum and maximum amount self.create_sales_invoice(qty=1, rate=300) diff --git a/erpnext/accounts/utils.py b/erpnext/accounts/utils.py index 8ec0c053038..8e6197dd8b5 100644 --- a/erpnext/accounts/utils.py +++ b/erpnext/accounts/utils.py @@ -2379,13 +2379,16 @@ class QueryPaymentLedger: ) # build query for voucher amount - query_voucher_amount = ( + # account is grouped, not aggregated: it is a join key against the outstanding CTE below, and + # it fixes the currency the amounts are summed in. The two CTEs aggregate over different row + # sets, so two Max() picks could disagree and the join would silently miss, leaving the + # outstanding NULL. posting_date/due_date are dates, so Max() there cannot depend on + # collation. cost_center and remarks are free text that genuinely varies per row, so they + # come off one real row instead -- see representative below. + grouped_voucher_amount = ( qb.from_(ple) .select( - # columns that are constant per (voucher_type, voucher_no, party_type, party) are - # wrapped in Max() so the query is valid on postgres (which, unlike MariaDB, requires - # every non-aggregated column to be grouped or aggregated) - Max(ple.account).as_("account"), + ple.account, ple.voucher_type, ple.voucher_no, ple.party_type, @@ -2393,25 +2396,47 @@ class QueryPaymentLedger: Max(ple.posting_date).as_("posting_date"), Max(ple.due_date).as_("due_date"), Max(ple.account_currency).as_("currency"), - Max(ple.cost_center).as_("cost_center"), Sum(ple.amount).as_("amount"), Sum(ple.amount_in_account_currency).as_("amount_in_account_currency"), - Max(ple.remarks).as_("remarks"), + Min(ple.name).as_("representative"), ) .where(ple.delinked == 0) .where(Criterion.all(filter_on_voucher_no)) .where(Criterion.all(self.common_filter)) .where(Criterion.all(self.dimensions_filter)) .where(Criterion.all(self.voucher_posting_date)) - .groupby(ple.voucher_type, ple.voucher_no, ple.party_type, ple.party) + .groupby(ple.account, ple.voucher_type, ple.voucher_no, ple.party_type, ple.party) + ).as_("grouped") + + # Payment Ledger Entry has no autoname rule, so frappe names it by hash -- lower-case, which + # keeps Min(name) free of the collation divergence that picking Max() over free text has. + representative_ple = qb.DocType("Payment Ledger Entry").as_("representative_ple") + query_voucher_amount = ( + qb.from_(grouped_voucher_amount) + .inner_join(representative_ple) + .on(representative_ple.name == grouped_voucher_amount.representative) + .select( + grouped_voucher_amount.account, + grouped_voucher_amount.voucher_type, + grouped_voucher_amount.voucher_no, + grouped_voucher_amount.party_type, + grouped_voucher_amount.party, + grouped_voucher_amount.posting_date, + grouped_voucher_amount.due_date, + grouped_voucher_amount.currency, + grouped_voucher_amount.amount, + grouped_voucher_amount.amount_in_account_currency, + representative_ple.cost_center.as_("cost_center"), + representative_ple.remarks.as_("remarks"), + ) ) # build query for voucher outstanding query_voucher_outstanding = ( qb.from_(ple) .select( - # Max() on columns constant per group keeps this valid on postgres (see above) - Max(ple.account).as_("account"), + # grouped, not aggregated: this is the other side of the join key -- see above + ple.account, ple.against_voucher_type.as_("voucher_type"), ple.against_voucher_no.as_("voucher_no"), ple.party_type, @@ -2425,7 +2450,7 @@ class QueryPaymentLedger: .where(ple.delinked == 0) .where(Criterion.all(filter_on_against_voucher_no)) .where(Criterion.all(self.common_filter)) - .groupby(ple.against_voucher_type, ple.against_voucher_no, ple.party_type, ple.party) + .groupby(ple.account, ple.against_voucher_type, ple.against_voucher_no, ple.party_type, ple.party) ) # build CTE for combining voucher amount and outstanding