From c41cb3930c76efb620cf8bc27672dab8c5302e6c Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Mon, 18 Sep 2023 13:03:08 +0530 Subject: [PATCH] fix: Don't allow merging accounts with different currency (#37074) * fix: Don't allow merging accounts with different currency (#37074) * fix: Don't allow merging accounts with different currency * test: Update conflicting values * test: Update conflicting values (cherry picked from commit 5e21e7cd1da12cad1d3491d92aafa2664c8518a8) # Conflicts: # erpnext/accounts/doctype/account/account.js # erpnext/accounts/doctype/account/account.py * chore: resolve conflicts --------- Co-authored-by: Deepesh Garg --- erpnext/accounts/doctype/account/account.js | 3 - erpnext/accounts/doctype/account/account.py | 39 ++++-- .../accounts/doctype/account/test_account.py | 119 ++++++++++-------- .../doctype/ledger_merge/ledger_merge.py | 3 - 4 files changed, 94 insertions(+), 70 deletions(-) diff --git a/erpnext/accounts/doctype/account/account.js b/erpnext/accounts/doctype/account/account.js index 320e1cab7c3d..7d63b257faff 100644 --- a/erpnext/accounts/doctype/account/account.js +++ b/erpnext/accounts/doctype/account/account.js @@ -117,9 +117,6 @@ frappe.ui.form.on('Account', { args: { old: frm.doc.name, new: data.name, - is_group: frm.doc.is_group, - root_type: frm.doc.root_type, - company: frm.doc.company }, callback: function(r) { if(!r.exc) { diff --git a/erpnext/accounts/doctype/account/account.py b/erpnext/accounts/doctype/account/account.py index bbe4c54a71ad..22ddc2ffae3b 100644 --- a/erpnext/accounts/doctype/account/account.py +++ b/erpnext/accounts/doctype/account/account.py @@ -18,6 +18,10 @@ class BalanceMismatchError(frappe.ValidationError): pass +class InvalidAccountMergeError(frappe.ValidationError): + pass + + class Account(NestedSet): nsm_parent_field = "parent_account" @@ -444,24 +448,35 @@ def update_account_number(name, account_name, account_number=None, from_descenda @frappe.whitelist() -def merge_account(old, new, is_group, root_type, company): +def merge_account(old, new): # Validate properties before merging - if not frappe.db.exists("Account", new): - throw(_("Account {0} does not exist").format(new)) + new_account = frappe.get_cached_doc("Account", new) + old_account = frappe.get_cached_doc("Account", old) - val = list(frappe.db.get_value("Account", new, ["is_group", "root_type", "company"])) + if not new_account: + throw(_("Account {0} does not exist").format(new)) - if val != [cint(is_group), root_type, company]: + if ( + cint(new_account.is_group), + new_account.root_type, + new_account.company, + cstr(new_account.account_currency), + ) != ( + cint(old_account.is_group), + old_account.root_type, + old_account.company, + cstr(old_account.account_currency), + ): throw( - _( - """Merging is only possible if following properties are same in both records. Is Group, Root Type, Company""" - ) + msg=_( + """Merging is only possible if following properties are same in both records. Is Group, Root Type, Company and Account Currency""" + ), + title=("Invalid Accounts"), + exc=InvalidAccountMergeError, ) - if is_group and frappe.db.get_value("Account", new, "parent_account") == old: - frappe.db.set_value( - "Account", new, "parent_account", frappe.db.get_value("Account", old, "parent_account") - ) + if old_account.is_group and new_account.parent_account == old: + new_account.db_set("parent_account", frappe.get_cached_value("Account", old, "parent_account")) frappe.rename_doc("Account", old, new, merge=1, force=1) diff --git a/erpnext/accounts/doctype/account/test_account.py b/erpnext/accounts/doctype/account/test_account.py index 62303bd723f6..30eebef7fba4 100644 --- a/erpnext/accounts/doctype/account/test_account.py +++ b/erpnext/accounts/doctype/account/test_account.py @@ -7,7 +7,11 @@ import frappe from frappe.test_runner import make_test_records -from erpnext.accounts.doctype.account.account import merge_account, update_account_number +from erpnext.accounts.doctype.account.account import ( + InvalidAccountMergeError, + merge_account, + update_account_number, +) from erpnext.stock import get_company_default_inventory_account, get_warehouse_account test_dependencies = ["Company"] @@ -47,49 +51,53 @@ def test_rename_account(self): frappe.delete_doc("Account", "1211-11-4 - 6 - Debtors 1 - Test - - _TC") def test_merge_account(self): - if not frappe.db.exists("Account", "Current Assets - _TC"): - acc = frappe.new_doc("Account") - acc.account_name = "Current Assets" - acc.is_group = 1 - acc.parent_account = "Application of Funds (Assets) - _TC" - acc.company = "_Test Company" - acc.insert() - if not frappe.db.exists("Account", "Securities and Deposits - _TC"): - acc = frappe.new_doc("Account") - acc.account_name = "Securities and Deposits" - acc.parent_account = "Current Assets - _TC" - acc.is_group = 1 - acc.company = "_Test Company" - acc.insert() - if not frappe.db.exists("Account", "Earnest Money - _TC"): - acc = frappe.new_doc("Account") - acc.account_name = "Earnest Money" - acc.parent_account = "Securities and Deposits - _TC" - acc.company = "_Test Company" - acc.insert() - if not frappe.db.exists("Account", "Cash In Hand - _TC"): - acc = frappe.new_doc("Account") - acc.account_name = "Cash In Hand" - acc.is_group = 1 - acc.parent_account = "Current Assets - _TC" - acc.company = "_Test Company" - acc.insert() - if not frappe.db.exists("Account", "Accumulated Depreciation - _TC"): - acc = frappe.new_doc("Account") - acc.account_name = "Accumulated Depreciation" - acc.parent_account = "Fixed Assets - _TC" - acc.company = "_Test Company" - acc.account_type = "Accumulated Depreciation" - acc.insert() + create_account( + account_name="Current Assets", + is_group=1, + parent_account="Application of Funds (Assets) - _TC", + company="_Test Company", + ) + + create_account( + account_name="Securities and Deposits", + is_group=1, + parent_account="Current Assets - _TC", + company="_Test Company", + ) + + create_account( + account_name="Earnest Money", + parent_account="Securities and Deposits - _TC", + company="_Test Company", + ) + + create_account( + account_name="Cash In Hand", + is_group=1, + parent_account="Current Assets - _TC", + company="_Test Company", + ) + + create_account( + account_name="Receivable INR", + parent_account="Current Assets - _TC", + company="_Test Company", + account_currency="INR", + ) + + create_account( + account_name="Receivable USD", + parent_account="Current Assets - _TC", + company="_Test Company", + account_currency="USD", + ) - doc = frappe.get_doc("Account", "Securities and Deposits - _TC") parent = frappe.db.get_value("Account", "Earnest Money - _TC", "parent_account") self.assertEqual(parent, "Securities and Deposits - _TC") - merge_account( - "Securities and Deposits - _TC", "Cash In Hand - _TC", doc.is_group, doc.root_type, doc.company - ) + merge_account("Securities and Deposits - _TC", "Cash In Hand - _TC") + parent = frappe.db.get_value("Account", "Earnest Money - _TC", "parent_account") # Parent account of the child account changes after merging @@ -98,30 +106,28 @@ def test_merge_account(self): # Old account doesn't exist after merging self.assertFalse(frappe.db.exists("Account", "Securities and Deposits - _TC")) - doc = frappe.get_doc("Account", "Current Assets - _TC") - # Raise error as is_group property doesn't match self.assertRaises( - frappe.ValidationError, + InvalidAccountMergeError, merge_account, "Current Assets - _TC", "Accumulated Depreciation - _TC", - doc.is_group, - doc.root_type, - doc.company, ) - doc = frappe.get_doc("Account", "Capital Stock - _TC") - # Raise error as root_type property doesn't match self.assertRaises( - frappe.ValidationError, + InvalidAccountMergeError, merge_account, "Capital Stock - _TC", "Softwares - _TC", - doc.is_group, - doc.root_type, - doc.company, + ) + + # Raise error as currency doesn't match + self.assertRaises( + InvalidAccountMergeError, + merge_account, + "Receivable INR - _TC", + "Receivable USD - _TC", ) def test_account_sync(self): @@ -400,11 +406,20 @@ def create_account(**kwargs): "Account", filters={"account_name": kwargs.get("account_name"), "company": kwargs.get("company")} ) if account: - return account + account = frappe.get_doc("Account", account) + account.update( + dict( + is_group=kwargs.get("is_group", 0), + parent_account=kwargs.get("parent_account"), + ) + ) + account.save() + return account.name else: account = frappe.get_doc( dict( doctype="Account", + is_group=kwargs.get("is_group", 0), account_name=kwargs.get("account_name"), account_type=kwargs.get("account_type"), parent_account=kwargs.get("parent_account"), diff --git a/erpnext/accounts/doctype/ledger_merge/ledger_merge.py b/erpnext/accounts/doctype/ledger_merge/ledger_merge.py index 18e5a1ac85bd..6e89d039932a 100644 --- a/erpnext/accounts/doctype/ledger_merge/ledger_merge.py +++ b/erpnext/accounts/doctype/ledger_merge/ledger_merge.py @@ -49,9 +49,6 @@ def start_merge(docname): merge_account( row.account, ledger_merge.account, - ledger_merge.is_group, - ledger_merge.root_type, - ledger_merge.company, ) row.db_set("merged", 1) frappe.db.commit()