From b13f8eecaa5ec564a5a8b51c1c26f770a96ad96c Mon Sep 17 00:00:00 2001 From: Deepesh Garg Date: Wed, 13 Sep 2023 20:10:31 +0530 Subject: [PATCH 1/3] fix: Don't allow merging accounts with different currency --- erpnext/accounts/doctype/account/account.js | 3 - erpnext/accounts/doctype/account/account.py | 31 +++-- .../accounts/doctype/account/test_account.py | 118 ++++++++++-------- .../doctype/ledger_merge/ledger_merge.py | 3 - 4 files changed, 88 insertions(+), 67 deletions(-) diff --git a/erpnext/accounts/doctype/account/account.js b/erpnext/accounts/doctype/account/account.js index 3c0eb8570187..bcf7efc98b67 100644 --- a/erpnext/accounts/doctype/account/account.js +++ b/erpnext/accounts/doctype/account/account.js @@ -137,9 +137,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 c1eca721b6f9..02e6c205728b 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" @@ -460,25 +464,34 @@ 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 new_account = frappe.get_cached_doc("Account", new) + old_account = frappe.get_cached_doc("Account", old) if not new_account: throw(_("Account {0} does not exist").format(new)) - if (new_account.is_group, new_account.root_type, new_account.company) != ( - 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 new_account.parent_account == old: + 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..e762ee2f4b40 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="Debtors INR", + parent_account="Current Assets - _TC", + company="_Test Company", + account_currency="INR", + ) + + create_account( + account_name="Debtors 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, + "Debtors INR - _TC", + "Debtors USD - _TC", ) def test_account_sync(self): @@ -400,11 +406,19 @@ 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() 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 381083bc3046..362d27342a68 100644 --- a/erpnext/accounts/doctype/ledger_merge/ledger_merge.py +++ b/erpnext/accounts/doctype/ledger_merge/ledger_merge.py @@ -48,9 +48,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() From 6f85ed53b2cc4a83a645c2d82cabf0dae309a621 Mon Sep 17 00:00:00 2001 From: Deepesh Garg Date: Thu, 14 Sep 2023 12:15:39 +0530 Subject: [PATCH 2/3] test: Update conflicting values --- erpnext/accounts/doctype/account/test_account.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/erpnext/accounts/doctype/account/test_account.py b/erpnext/accounts/doctype/account/test_account.py index e762ee2f4b40..8149a2328b58 100644 --- a/erpnext/accounts/doctype/account/test_account.py +++ b/erpnext/accounts/doctype/account/test_account.py @@ -79,14 +79,14 @@ def test_merge_account(self): ) create_account( - account_name="Debtors INR", + account_name="Receivable INR", parent_account="Current Assets - _TC", company="_Test Company", account_currency="INR", ) create_account( - account_name="Debtors USD", + account_name="Receivable USD", parent_account="Current Assets - _TC", company="_Test Company", account_currency="USD", @@ -126,8 +126,8 @@ def test_merge_account(self): self.assertRaises( InvalidAccountMergeError, merge_account, - "Debtors INR - _TC", - "Debtors USD - _TC", + "Receivable INR - _TC", + "Receivable USD - _TC", ) def test_account_sync(self): From 2274f08494bdcac604c4a246f7f64f457d91e44d Mon Sep 17 00:00:00 2001 From: Deepesh Garg Date: Thu, 14 Sep 2023 19:52:05 +0530 Subject: [PATCH 3/3] test: Update conflicting values --- erpnext/accounts/doctype/account/test_account.py | 1 + 1 file changed, 1 insertion(+) diff --git a/erpnext/accounts/doctype/account/test_account.py b/erpnext/accounts/doctype/account/test_account.py index 8149a2328b58..30eebef7fba4 100644 --- a/erpnext/accounts/doctype/account/test_account.py +++ b/erpnext/accounts/doctype/account/test_account.py @@ -414,6 +414,7 @@ def create_account(**kwargs): ) ) account.save() + return account.name else: account = frappe.get_doc( dict(