From 646cf54679c68422aeff7c4a9aba0cccb9d7f852 Mon Sep 17 00:00:00 2001 From: Vishakh Desai <78500008+vishakhdesai@users.noreply.github.com> Date: Mon, 31 Mar 2025 11:25:41 +0530 Subject: [PATCH] fix: multiple Bank Reconciliation Tool issues (#46644) * fix: bank reconciliation tool issue * refactor: separate Bank Transaction linking from other logic * fix: delink old pe on update_after_submit in bank transaction * fix: failing test case fixed * fix: changes as per review * refactor: rename `gles` to `gl_entries` --------- Co-authored-by: Sagar Vora --- .../bank_reconciliation_tool.py | 43 +- .../bank_transaction/bank_transaction.js | 31 +- .../bank_transaction/bank_transaction.py | 402 +++++++++--------- erpnext/accounts/test/accounts_mixin.py | 1 + 4 files changed, 244 insertions(+), 233 deletions(-) diff --git a/erpnext/accounts/doctype/bank_reconciliation_tool/bank_reconciliation_tool.py b/erpnext/accounts/doctype/bank_reconciliation_tool/bank_reconciliation_tool.py index 6c57ea42367..d47cc7b5968 100644 --- a/erpnext/accounts/doctype/bank_reconciliation_tool/bank_reconciliation_tool.py +++ b/erpnext/accounts/doctype/bank_reconciliation_tool/bank_reconciliation_tool.py @@ -8,6 +8,7 @@ import frappe from frappe import _ from frappe.model.document import Document from frappe.query_builder.custom import ConstantColumn +from frappe.query_builder.functions import Sum from frappe.utils import cint, flt from erpnext import get_default_cost_center @@ -517,16 +518,23 @@ def subtract_allocations(gl_account, vouchers): voucher_allocated_amounts = get_total_allocated_amount(voucher_docs) for voucher in vouchers: - rows = voucher_allocated_amounts.get((voucher.get("doctype"), voucher.get("name"))) or [] - filtered_row = list(filter(lambda row: row.get("gl_account") == gl_account, rows)) - - if amount := None if not filtered_row else filtered_row[0]["total"]: + if amount := get_allocated_amount(voucher_allocated_amounts, voucher, gl_account): voucher["paid_amount"] -= amount copied.append(voucher) return copied +def get_allocated_amount(voucher_allocated_amounts, voucher, gl_account): + if not (voucher_details := voucher_allocated_amounts.get((voucher.get("doctype"), voucher.get("name")))): + return + + if not (row := voucher_details.get(gl_account)): + return + + return row.get("total") + + def check_matching( bank_account, company, @@ -796,26 +804,20 @@ def get_je_matching_query( je = frappe.qb.DocType("Journal Entry") jea = frappe.qb.DocType("Journal Entry Account") - ref_condition = je.cheque_no == transaction.reference_number - ref_rank = frappe.qb.terms.Case().when(ref_condition, 1).else_(0) - amount_field = f"{cr_or_dr}_in_account_currency" - amount_equality = getattr(jea, amount_field) == transaction.unallocated_amount - amount_rank = frappe.qb.terms.Case().when(amount_equality, 1).else_(0) filter_by_date = je.posting_date.between(from_date, to_date) if cint(filter_by_reference_date): filter_by_date = je.cheque_date.between(from_reference_date, to_reference_date) - query = ( + subquery = ( frappe.qb.from_(jea) .join(je) .on(jea.parent == je.name) .select( - (ref_rank + amount_rank + 1).as_("rank"), + Sum(getattr(jea, amount_field)).as_("paid_amount"), ConstantColumn("Journal Entry").as_("doctype"), je.name, - getattr(jea, amount_field).as_("paid_amount"), je.cheque_no.as_("reference_no"), je.cheque_date.as_("reference_date"), je.pay_to_recd_from.as_("party"), @@ -827,13 +829,26 @@ def get_je_matching_query( .where(je.voucher_type != "Opening Entry") .where(je.clearance_date.isnull()) .where(jea.account == common_filters.bank_account) - .where(amount_equality if exact_match else getattr(jea, amount_field) > 0.0) .where(filter_by_date) + .groupby(je.name) .orderby(je.cheque_date if cint(filter_by_reference_date) else je.posting_date) ) if frappe.flags.auto_reconcile_vouchers is True: - query = query.where(ref_condition) + subquery = subquery.where(je.cheque_no == transaction.reference_number) + + ref_rank = frappe.qb.terms.Case().when(subquery.reference_no == transaction.reference_number, 1).else_(0) + amount_equality = subquery.paid_amount == transaction.unallocated_amount + amount_rank = frappe.qb.terms.Case().when(amount_equality, 1).else_(0) + + query = ( + frappe.qb.from_(subquery) + .select( + "*", + (ref_rank + amount_rank + 1).as_("rank"), + ) + .where(amount_equality if exact_match else subquery.paid_amount > 0.0) + ) return query diff --git a/erpnext/accounts/doctype/bank_transaction/bank_transaction.js b/erpnext/accounts/doctype/bank_transaction/bank_transaction.js index d899d429178..5c5d9ff3469 100644 --- a/erpnext/accounts/doctype/bank_transaction/bank_transaction.js +++ b/erpnext/accounts/doctype/bank_transaction/bank_transaction.js @@ -31,6 +31,12 @@ frappe.ui.form.on("Bank Transaction", { }, }; }); + + frm.set_query("bank_account", function () { + return { + filters: { is_company_account: 1 }, + }; + }); }, get_payment_doctypes: function () { @@ -39,31 +45,6 @@ frappe.ui.form.on("Bank Transaction", { }, }); -frappe.ui.form.on("Bank Transaction Payments", { - payment_entries_remove: function (frm, cdt, cdn) { - update_clearance_date(frm, cdt, cdn); - }, -}); - -const update_clearance_date = (frm, cdt, cdn) => { - if (frm.doc.docstatus === 1) { - frappe - .xcall("erpnext.accounts.doctype.bank_transaction.bank_transaction.unclear_reference_payment", { - doctype: cdt, - docname: cdn, - bt_name: frm.doc.name, - }) - .then((e) => { - if (e == "success") { - frappe.show_alert({ - message: __("Document {0} successfully uncleared", [e]), - indicator: "green", - }); - } - }); - } -}; - function set_bank_statement_filter(frm) { frm.set_query("bank_statement", function () { return { diff --git a/erpnext/accounts/doctype/bank_transaction/bank_transaction.py b/erpnext/accounts/doctype/bank_transaction/bank_transaction.py index 7e509896fec..39ea5fde777 100644 --- a/erpnext/accounts/doctype/bank_transaction/bank_transaction.py +++ b/erpnext/accounts/doctype/bank_transaction/bank_transaction.py @@ -5,7 +5,7 @@ import frappe from frappe import _ from frappe.model.docstatus import DocStatus from frappe.model.document import Document -from frappe.utils import flt +from frappe.utils import flt, getdate class BankTransaction(Document): @@ -84,16 +84,16 @@ class BankTransaction(Document): if not self.payment_entries: return - pe = [] + references = set() for row in self.payment_entries: reference = (row.payment_document, row.payment_entry) - if reference in pe: + if reference in references: frappe.throw( _("{0} {1} is allocated twice in this Bank Transaction").format( row.payment_document, row.payment_entry ) ) - pe.append(reference) + references.add(reference) def update_allocated_amount(self): allocated_amount = ( @@ -104,6 +104,19 @@ class BankTransaction(Document): self.allocated_amount = flt(allocated_amount, self.precision("allocated_amount")) self.unallocated_amount = flt(unallocated_amount, self.precision("unallocated_amount")) + def delink_old_payment_entries(self): + if self.flags.updating_linked_bank_transaction: + return + + old_doc = self.get_doc_before_save() + payment_entry_names = set(pe.name for pe in self.payment_entries) + + for old_pe in old_doc.payment_entries: + if old_pe.name in payment_entry_names: + continue + + self.delink_payment_entry(old_pe) + def before_submit(self): self.allocate_payment_entries() self.set_status() @@ -113,13 +126,14 @@ class BankTransaction(Document): def before_update_after_submit(self): self.validate_duplicate_references() - self.allocate_payment_entries() self.update_allocated_amount() + self.delink_old_payment_entries() + self.allocate_payment_entries() self.set_status() def on_cancel(self): for payment_entry in self.payment_entries: - self.clear_linked_payment_entry(payment_entry, for_cancel=True) + self.delink_payment_entry(payment_entry) self.set_status() @@ -152,43 +166,55 @@ class BankTransaction(Document): - 0 > a: Error: already over-allocated - clear means: set the latest transaction date as clearance date """ + if self.flags.updating_linked_bank_transaction or not self.payment_entries: + return + remaining_amount = self.unallocated_amount - to_remove = [] payment_entry_docs = [(pe.payment_document, pe.payment_entry) for pe in self.payment_entries] pe_bt_allocations = get_total_allocated_amount(payment_entry_docs) + gl_entries = get_related_bank_gl_entries(payment_entry_docs) + gl_bank_account = frappe.db.get_value("Bank Account", self.bank_account, "account") - for payment_entry in self.payment_entries: - if payment_entry.allocated_amount == 0.0: - unallocated_amount, should_clear, latest_transaction = get_clearance_details( - self, - payment_entry, - pe_bt_allocations.get((payment_entry.payment_document, payment_entry.payment_entry)) - or [], + for payment_entry in list(self.payment_entries): + if payment_entry.allocated_amount != 0: + continue + + allocable_amount, should_clear, clearance_date = get_clearance_details( + self, + payment_entry, + pe_bt_allocations.get((payment_entry.payment_document, payment_entry.payment_entry)) or {}, + gl_entries.get((payment_entry.payment_document, payment_entry.payment_entry)) or {}, + gl_bank_account, + ) + + if allocable_amount < 0: + frappe.throw(_("Voucher {0} is over-allocated by {1}").format(allocable_amount)) + + if remaining_amount <= 0: + self.remove(payment_entry) + continue + + if allocable_amount == 0: + if should_clear: + self.clear_linked_payment_entry(payment_entry, clearance_date=clearance_date) + self.remove(payment_entry) + continue + + should_clear = should_clear and allocable_amount <= remaining_amount + payment_entry.allocated_amount = min(allocable_amount, remaining_amount) + remaining_amount = flt( + remaining_amount - payment_entry.allocated_amount, + self.precision("unallocated_amount"), + ) + + if payment_entry.payment_document == "Bank Transaction": + self.update_linked_bank_transaction( + payment_entry.payment_entry, payment_entry.allocated_amount ) + elif should_clear: + self.clear_linked_payment_entry(payment_entry, clearance_date=clearance_date) - if 0.0 == unallocated_amount: - if should_clear: - latest_transaction.clear_linked_payment_entry(payment_entry) - to_remove.append(payment_entry) - - elif remaining_amount <= 0.0: - to_remove.append(payment_entry) - - elif 0.0 < unallocated_amount <= remaining_amount: - payment_entry.allocated_amount = unallocated_amount - remaining_amount -= unallocated_amount - if should_clear: - latest_transaction.clear_linked_payment_entry(payment_entry) - - elif 0.0 < unallocated_amount: - payment_entry.allocated_amount = remaining_amount - remaining_amount = 0.0 - - elif 0.0 > unallocated_amount: - frappe.throw(_("Voucher {0} is over-allocated by {1}").format(unallocated_amount)) - - for payment_entry in to_remove: - self.remove(payment_entry) + self.update_allocated_amount() @frappe.whitelist() def remove_payment_entries(self): @@ -199,14 +225,64 @@ class BankTransaction(Document): def remove_payment_entry(self, payment_entry): "Clear payment entry and clearance" - self.clear_linked_payment_entry(payment_entry, for_cancel=True) + self.delink_payment_entry(payment_entry) self.remove(payment_entry) - def clear_linked_payment_entry(self, payment_entry, for_cancel=False): - clearance_date = None if for_cancel else self.date - set_voucher_clearance( - payment_entry.payment_document, payment_entry.payment_entry, clearance_date, self - ) + def delink_payment_entry(self, payment_entry): + if payment_entry.payment_document == "Bank Transaction": + self.update_linked_bank_transaction(payment_entry.payment_entry, allocated_amount=None) + else: + self.clear_linked_payment_entry(payment_entry, clearance_date=None) + + def clear_linked_payment_entry(self, payment_entry, clearance_date=None): + doctype = payment_entry.payment_document + docname = payment_entry.payment_entry + + # might be a bank transaction + if doctype not in get_doctypes_for_bank_reconciliation(): + return + + if doctype == "Sales Invoice": + frappe.db.set_value( + "Sales Invoice Payment", + dict(parenttype=doctype, parent=docname), + "clearance_date", + clearance_date, + ) + return + + frappe.db.set_value(doctype, docname, "clearance_date", clearance_date) + + def update_linked_bank_transaction(self, bank_transaction_name, allocated_amount=None): + """For when a second bank transaction has fixed another, e.g. refund""" + + bt = frappe.get_doc(self.doctype, bank_transaction_name) + if allocated_amount: + bt.append( + "payment_entries", + { + "payment_document": self.doctype, + "payment_entry": self.name, + "allocated_amount": allocated_amount, + }, + ) + + else: + pe = next( + ( + pe + for pe in bt.payment_entries + if pe.payment_document == self.doctype and pe.payment_entry == self.name + ), + None, + ) + if not pe: + return + + bt.flags.updating_linked_bank_transaction = True + bt.remove(pe) + + bt.save() def auto_set_party(self): from erpnext.accounts.doctype.bank_transaction.auto_match_party import AutoMatchParty @@ -238,71 +314,107 @@ def get_doctypes_for_bank_reconciliation(): return frappe.get_hooks("bank_reconciliation_doctypes") -def get_clearance_details(transaction, payment_entry, bt_allocations): +def get_clearance_details(transaction, payment_entry, bt_allocations, gl_entries, gl_bank_account): """ - There should only be one bank gle for a voucher. - Could be none for a Bank Transaction. - But if a JE, could affect two banks. - Should only clear the voucher if all bank gles are allocated. + There should only be one bank gl entry for a voucher, except for JE. + For JE, there can be multiple bank gl entries for the same account. + In this case, the allocable_amount will be the sum of amounts of all gl entries of the account. + There will be no gl entry for a Bank Transaction so return the unallocated amount. + Should only clear the voucher if all bank gl entries are allocated. """ - gl_bank_account = frappe.db.get_value("Bank Account", transaction.bank_account, "account") - gles = get_related_bank_gl_entries(payment_entry.payment_document, payment_entry.payment_entry) - unallocated_amount = min( - transaction.unallocated_amount, - get_paid_amount(payment_entry, transaction.currency, gl_bank_account), - ) - unmatched_gles = len(gles) - latest_transaction = transaction - for gle in gles: - if gle["gl_account"] == gl_bank_account: - if gle["amount"] <= 0.0: - frappe.throw( - _("Voucher {0} value is broken: {1}").format(payment_entry.payment_entry, gle["amount"]) + transaction_date = getdate(transaction.date) + + if payment_entry.payment_document == "Bank Transaction": + bt = frappe.db.get_value( + "Bank Transaction", + payment_entry.payment_entry, + ("unallocated_amount", "bank_account"), + as_dict=True, + ) + + if bt.bank_account != gl_bank_account: + frappe.throw( + _("Bank Account {} in Bank Transaction {} is not matching with Bank Account {}").format( + bt.bank_account, payment_entry.payment_entry, gl_bank_account ) + ) - unmatched_gles -= 1 - unallocated_amount = gle["amount"] - for a in bt_allocations: - if a["gl_account"] == gle["gl_account"]: - unallocated_amount = gle["amount"] - a["total"] - if frappe.utils.getdate(transaction.date) < a["latest_date"]: - latest_transaction = frappe.get_doc("Bank Transaction", a["latest_name"]) - else: - # Must be a Journal Entry affecting more than one bank - for a in bt_allocations: - if a["gl_account"] == gle["gl_account"] and a["total"] == gle["amount"]: - unmatched_gles -= 1 + return abs(bt.unallocated_amount), True, transaction_date - return unallocated_amount, unmatched_gles == 0, latest_transaction + if gl_bank_account not in gl_entries: + frappe.throw( + _("{} {} is not affecting bank account {}").format( + payment_entry.payment_document, payment_entry.payment_entry, gl_bank_account + ) + ) + + allocable_amount = gl_entries.pop(gl_bank_account) or 0 + if allocable_amount <= 0.0: + frappe.throw( + _("Invalid amount in accounting entries of {} {} for Account {}: {}").format( + payment_entry.payment_document, payment_entry.payment_entry, gl_bank_account, allocable_amount + ) + ) + + matching_bt_allocaion = bt_allocations.pop(gl_bank_account, {}) + + allocable_amount = flt( + allocable_amount - matching_bt_allocaion.get("total", 0), transaction.precision("unallocated_amount") + ) + + should_clear = all( + gl_entries[gle_account] == bt_allocations.get(gle_account, {}).get("total", 0) + for gle_account in gl_entries + ) + + bt_allocation_date = matching_bt_allocaion.get("latest_date", None) + clearance_date = transaction_date if not bt_allocation_date else max(transaction_date, bt_allocation_date) + + return allocable_amount, should_clear, clearance_date -def get_related_bank_gl_entries(doctype, docname): +def get_related_bank_gl_entries(docs): # nosemgrep: frappe-semgrep-rules.rules.frappe-using-db-sql - return frappe.db.sql( + if not docs: + return {} + + result = frappe.db.sql( """ - SELECT - ABS(gle.credit_in_account_currency - gle.debit_in_account_currency) AS amount, - gle.account AS gl_account - FROM - `tabGL Entry` gle - LEFT JOIN - `tabAccount` ac ON ac.name=gle.account - WHERE - ac.account_type = 'Bank' - AND gle.voucher_type = %(doctype)s - AND gle.voucher_no = %(docname)s - AND is_cancelled = 0 - """, - dict(doctype=doctype, docname=docname), + SELECT + gle.voucher_type AS doctype, + gle.voucher_no AS docname, + gle.account AS gl_account, + SUM(ABS(gle.credit_in_account_currency - gle.debit_in_account_currency)) AS amount + FROM + `tabGL Entry` gle + LEFT JOIN + `tabAccount` ac ON ac.name = gle.account + WHERE + ac.account_type = 'Bank' + AND (gle.voucher_type, gle.voucher_no) IN %(docs)s + AND gle.is_cancelled = 0 + GROUP BY + gle.voucher_type, gle.voucher_no, gle.account + """, + {"docs": docs}, as_dict=True, ) + entries = {} + for row in result: + key = (row["doctype"], row["docname"]) + if key not in entries: + entries[key] = {} + entries[key][row["gl_account"]] = row["amount"] + + return entries + def get_total_allocated_amount(docs): """ Gets the sum of allocations for a voucher on each bank GL account - along with the latest bank transaction name & date + along with the latest bank transaction date NOTE: query may also include just saved vouchers/payments but with zero allocated_amount """ if not docs: @@ -311,11 +423,10 @@ def get_total_allocated_amount(docs): # nosemgrep: frappe-semgrep-rules.rules.frappe-using-db-sql result = frappe.db.sql( """ - SELECT total, latest_name, latest_date, gl_account, payment_document, payment_entry FROM ( + SELECT total, latest_date, gl_account, payment_document, payment_entry FROM ( SELECT ROW_NUMBER() OVER w AS rownum, SUM(btp.allocated_amount) OVER(PARTITION BY ba.account, btp.payment_document, btp.payment_entry) AS total, - FIRST_VALUE(bt.name) OVER w AS latest_name, FIRST_VALUE(bt.date) OVER w AS latest_date, ba.account AS gl_account, btp.payment_document, @@ -338,104 +449,14 @@ def get_total_allocated_amount(docs): payment_allocation_details = {} for row in result: - # Why is this *sometimes* a byte string? - if isinstance(row["latest_name"], bytes): - row["latest_name"] = row["latest_name"].decode() - row["latest_date"] = frappe.utils.getdate(row["latest_date"]) - payment_allocation_details.setdefault((row["payment_document"], row["payment_entry"]), []).append(row) + row["latest_date"] = getdate(row["latest_date"]) + payment_allocation_details.setdefault((row["payment_document"], row["payment_entry"]), {})[ + row["gl_account"] + ] = row return payment_allocation_details -def get_paid_amount(payment_entry, currency, gl_bank_account): - if payment_entry.payment_document in ["Payment Entry", "Sales Invoice", "Purchase Invoice"]: - paid_amount_field = "paid_amount" - if payment_entry.payment_document == "Payment Entry": - doc = frappe.get_doc("Payment Entry", payment_entry.payment_entry) - - if doc.payment_type == "Receive": - paid_amount_field = ( - "received_amount" if doc.paid_to_account_currency == currency else "base_received_amount" - ) - elif doc.payment_type == "Pay": - paid_amount_field = ( - "paid_amount" if doc.paid_from_account_currency == currency else "base_paid_amount" - ) - - return frappe.db.get_value( - payment_entry.payment_document, payment_entry.payment_entry, paid_amount_field - ) - - elif payment_entry.payment_document == "Journal Entry": - return abs( - frappe.db.get_value( - "Journal Entry Account", - {"parent": payment_entry.payment_entry, "account": gl_bank_account}, - "sum(debit_in_account_currency-credit_in_account_currency)", - ) - or 0 - ) - - elif payment_entry.payment_document == "Expense Claim": - return frappe.db.get_value( - payment_entry.payment_document, payment_entry.payment_entry, "total_amount_reimbursed" - ) - - elif payment_entry.payment_document == "Loan Disbursement": - return frappe.db.get_value( - payment_entry.payment_document, payment_entry.payment_entry, "disbursed_amount" - ) - - elif payment_entry.payment_document == "Loan Repayment": - return frappe.db.get_value(payment_entry.payment_document, payment_entry.payment_entry, "amount_paid") - - elif payment_entry.payment_document == "Bank Transaction": - dep, wth = frappe.db.get_value( - "Bank Transaction", payment_entry.payment_entry, ("deposit", "withdrawal") - ) - return abs(flt(wth) - flt(dep)) - - else: - frappe.throw( - f"Please reconcile {payment_entry.payment_document}: {payment_entry.payment_entry} manually" - ) - - -def set_voucher_clearance(doctype, docname, clearance_date, self): - if doctype in get_doctypes_for_bank_reconciliation(): - if ( - doctype == "Payment Entry" - and frappe.db.get_value("Payment Entry", docname, "payment_type") == "Internal Transfer" - and len(get_reconciled_bank_transactions(doctype, docname)) < 2 - ): - return - - if doctype == "Sales Invoice": - frappe.db.set_value( - "Sales Invoice Payment", - dict(parenttype=doctype, parent=docname), - "clearance_date", - clearance_date, - ) - return - - frappe.db.set_value(doctype, docname, "clearance_date", clearance_date) - - elif doctype == "Bank Transaction": - # For when a second bank transaction has fixed another, e.g. refund - bt = frappe.get_doc(doctype, docname) - if clearance_date: - vouchers = [{"payment_doctype": "Bank Transaction", "payment_name": self.name}] - bt.add_payment_entries(vouchers) - bt.save() - else: - for pe in bt.payment_entries: - if pe.payment_document == self.doctype and pe.payment_entry == self.name: - bt.remove(pe) - bt.save() - break - - def get_reconciled_bank_transactions(doctype, docname): return frappe.get_all( "Bank Transaction Payments", @@ -444,13 +465,6 @@ def get_reconciled_bank_transactions(doctype, docname): ) -@frappe.whitelist() -def unclear_reference_payment(doctype, docname, bt_name): - bt = frappe.get_doc("Bank Transaction", bt_name) - set_voucher_clearance(doctype, docname, None, bt) - return docname - - def remove_from_bank_transaction(doctype, docname): """Remove a (cancelled) voucher from all Bank Transactions.""" for bt_name in get_reconciled_bank_transactions(doctype, docname): diff --git a/erpnext/accounts/test/accounts_mixin.py b/erpnext/accounts/test/accounts_mixin.py index ee651ee9f4c..d4de7626272 100644 --- a/erpnext/accounts/test/accounts_mixin.py +++ b/erpnext/accounts/test/accounts_mixin.py @@ -91,6 +91,7 @@ class AccountsTestMixin: "attribute_name": "bank", "account_name": "HDFC", "parent_account": "Bank Accounts - " + abbr, + "account_type": "Bank", } ), frappe._dict(