diff --git a/erpnext/assets/doctype/asset/asset.py b/erpnext/assets/doctype/asset/asset.py index 241dd76b3a1..7190db400d9 100644 --- a/erpnext/assets/doctype/asset/asset.py +++ b/erpnext/assets/doctype/asset/asset.py @@ -426,12 +426,15 @@ class Asset(AccountsController): non_depreciable_category = frappe.db.get_value( "Asset Category", self.asset_category, "non_depreciable_category" ) - if self.calculate_depreciation and non_depreciable_category: - frappe.throw( - _( - "This asset category is marked as non-depreciable. Please disable depreciation calculation or choose a different category." + if self.calculate_depreciation: + if non_depreciable_category: + frappe.throw( + _( + "This asset category is marked as non-depreciable. Please disable depreciation calculation or choose a different category." + ) ) - ) + # validate accounts required for asset depreciation + get_depreciation_accounts(self.asset_category, self.company) def validate_precision(self): if self.net_purchase_amount: diff --git a/erpnext/assets/doctype/asset/test_asset.py b/erpnext/assets/doctype/asset/test_asset.py index 5c685ccdc71..4da21e3942b 100644 --- a/erpnext/assets/doctype/asset/test_asset.py +++ b/erpnext/assets/doctype/asset/test_asset.py @@ -829,6 +829,86 @@ class TestAsset(AssetSetup): asset.save().submit() self.assertEqual(asset.status, "Fully Depreciated") + def test_depreciation_accounts_is_set_for_depreciable_assets(self): + company_depreciation_accounts = frappe.db.get_value( + "Company", + "_Test Company", + [ + "accumulated_depreciation_account", + "depreciation_expense_account", + ], + as_dict=True, + ) + frappe.db.set_value( + "Company", + "_Test Company", + { + "accumulated_depreciation_account": "", + "depreciation_expense_account": "", + }, + ) + asset_category_name = "Computers" + asset_category_account = None + if frappe.db.exists("Asset Category", asset_category_name): + filters = { + "parent": asset_category_name, + "company_name": "_Test Company", + } + fieldname = [ + "name", + "accumulated_depreciation_account", + "depreciation_expense_account", + ] + asset_category_account = frappe.db.get_value( + "Asset Category Account", + filters=filters, + fieldname=fieldname, + as_dict=True, + ) + if asset_category_account and ( + asset_category_account.accumulated_depreciation_account + or asset_category_account.depreciation_expense_account + ): + frappe.db.set_value( + "Asset Category Account", + asset_category_account.name, + { + "accumulated_depreciation_account": "", + "depreciation_expense_account": "", + }, + ) + else: + asset_category = frappe.new_doc("Asset Category") + asset_category.asset_category_name = asset_category_name + asset_category.append( + "accounts", + { + "company_name": "_Test Company", + "fixed_asset_account": "_Test Fixed Asset - _TC", + }, + ) + asset_category.insert() + try: + asset = create_asset(asset_category=asset_category_name, calculate_depreciation=1, do_not_save=1) + with self.assertRaises(frappe.ValidationError) as err: + asset.save() + + self.assertTrue( + "Please set Depreciation related Accounts in Asset Category Computers or Company" + in str(err.exception) + ) + finally: + frappe.db.set_value("Company", "_Test Company", company_depreciation_accounts) + if asset_category_account: + frappe.db.set_value( + "Asset Category Account", + asset_category_account.name, + { + "accumulated_depreciation_account": asset_category_account.accumulated_depreciation_account, + "depreciation_expense_account": asset_category_account.depreciation_expense_account, + }, + ) + class TestDepreciationMethods(AssetSetup): @classmethod diff --git a/erpnext/assets/doctype/asset_category/asset_category.py b/erpnext/assets/doctype/asset_category/asset_category.py index 16e564ad1bd..7c0e0a50dad 100644 --- a/erpnext/assets/doctype/asset_category/asset_category.py +++ b/erpnext/assets/doctype/asset_category/asset_category.py @@ -31,7 +31,7 @@ class AssetCategory(Document): self.validate_finance_books() self.validate_account_types() self.validate_account_currency() - self.valide_cwip_account() + self.validate_accounts() def validate_finance_books(self): for d in self.finance_books: @@ -97,11 +97,21 @@ class AssetCategory(Document): title=_("Invalid Account"), ) - def valide_cwip_account(self): + def validate_accounts(self): + self.validate_duplicate_rows() + self.validate_cwip_accounts() + self.validate_depreciation_accounts() + + def validate_duplicate_rows(self): + companies = {row.company_name for row in self.accounts} + if len(companies) != len(self.accounts): + frappe.throw(_("Cannot set multiple account rows for the same company")) + + def validate_cwip_accounts(self): if self.enable_cwip_accounting: missing_cwip_accounts_for_company = [] for d in self.accounts: - if not d.capital_work_in_progress_account and not frappe.db.get_value( + if not d.capital_work_in_progress_account and not frappe.get_cached_value( "Company", d.company_name, "capital_work_in_progress_account" ): missing_cwip_accounts_for_company.append(get_link_to_form("Company", d.company_name)) @@ -115,6 +125,71 @@ class AssetCategory(Document): ) frappe.throw(msg, title=_("Missing Account")) + def validate_depreciation_accounts(self): + depreciation_account_map = { + "accumulated_depreciation_account": "Accumulated Depreciation Account", + "depreciation_expense_account": "Depreciation Expense Account", + } + + error_msg = [] + companies_with_accounts = set() + + def validate_company_accounts(company, acc_row=None): + default_accounts = frappe.get_cached_value( + "Company", + company, + ["accumulated_depreciation_account", "depreciation_expense_account"], + as_dict=True, + ) + for fieldname, label in depreciation_account_map.items(): + row_value = acc_row.get(fieldname) if acc_row else None + if not row_value and not default_accounts.get(fieldname): + if acc_row: + error_msg.append( + _("Row #{0}: Missing {1} for company {2}.").format( + acc_row.idx, + label, + get_link_to_form("Company", company), + ) + ) + else: + msg = _("Missing account configuration for company {0}.").format( + get_link_to_form("Company", company), + ) + if msg not in error_msg: + error_msg.append(msg) + + companies_with_assets = frappe.db.get_all( + "Asset", + { + "calculate_depreciation": 1, + "asset_category": self.name, + "status": ["in", ("Submitted", "Partially Depreciated")], + }, + pluck="company", + distinct=True, + ) + + for acc_row in self.accounts: + companies_with_accounts.add(acc_row.company_name) + if acc_row.company_name in companies_with_assets: + validate_company_accounts(acc_row.company_name, acc_row) + + for company in companies_with_assets: + if company not in companies_with_accounts: + validate_company_accounts(company) + + if error_msg: + msg = _( + "Since there are active depreciable assets under this category, the following accounts are required.

" + ) + msg += _( + "You can either configure default depreciation accounts in the Company or set the required accounts in the following rows:

" + ) + msg += "
".join(error_msg) + + frappe.throw(msg, title=_("Missing Accounts")) + def get_asset_category_account( fieldname, item=None, asset=None, account=None, asset_category=None, company=None diff --git a/erpnext/assets/doctype/asset_category/test_asset_category.py b/erpnext/assets/doctype/asset_category/test_asset_category.py index ea68875a552..995185fbf95 100644 --- a/erpnext/assets/doctype/asset_category/test_asset_category.py +++ b/erpnext/assets/doctype/asset_category/test_asset_category.py @@ -4,6 +4,8 @@ import frappe from frappe.tests import IntegrationTestCase +from erpnext.assets.doctype.asset.test_asset import create_asset + class TestAssetCategory(IntegrationTestCase): def test_mandatory_fields(self): @@ -50,3 +52,67 @@ class TestAssetCategory(IntegrationTestCase): ) self.assertRaises(frappe.ValidationError, asset_category.insert) + + def test_duplicate_company_accounts(self): + asset_category = frappe.get_doc( + { + "doctype": "Asset Category", + "asset_category_name": "Computers", + "accounts": [ + { + "company_name": "_Test Company", + "fixed_asset_account": "_Test Fixed Asset - _TC", + }, + { + "company_name": "_Test Company", + "fixed_asset_account": "_Test Fixed Asset - _TC", + }, + ], + } + ) + with self.assertRaises(frappe.ValidationError) as err: + asset_category.save() + self.assertTrue("Cannot set multiple account rows for the same company" in str(err.exception)) + + def test_depreciation_accounts_required_for_existing_depreciable_assets(self): + asset = create_asset( + asset_category="Computers", + calculate_depreciation=1, + company="_Test Company", + submit=1, + ) + company_acccount_depreciation = frappe.db.get_value( + "Company", + asset.company, + [ + "accumulated_depreciation_account", + "depreciation_expense_account", + ], + as_dict=True, + ) + frappe.db.set_value( + "Company", + asset.company, + { + "accumulated_depreciation_account": "", + "depreciation_expense_account": "", + }, + ) + try: + asset_category = frappe.get_doc("Asset Category", asset.asset_category) + asset_category.enable_cwip_accounting = 0 + for row in asset_category.accounts: + if row.company_name == asset.company and ( + row.accumulated_depreciation_account or row.depreciation_expense_account + ): + row.accumulated_depreciation_account = None + row.depreciation_expense_account = None + with self.assertRaises(frappe.ValidationError) as err: + asset_category.save() + + self.assertTrue( + "Since there are active depreciable assets under this category, the following accounts are required." + in str(err.exception) + ) + finally: + frappe.db.set_value("Company", asset.company, company_acccount_depreciation) diff --git a/erpnext/assets/doctype/asset_repair/test_asset_repair.py b/erpnext/assets/doctype/asset_repair/test_asset_repair.py index ebacf4d2215..012a3895768 100644 --- a/erpnext/assets/doctype/asset_repair/test_asset_repair.py +++ b/erpnext/assets/doctype/asset_repair/test_asset_repair.py @@ -210,26 +210,29 @@ class TestAssetRepair(IntegrationTestCase): self.assertRaises(frappe.ValidationError, asset_repair2.save) def test_gl_entries_with_perpetual_inventory(self): - set_depreciation_settings_in_company(company="_Test Company with perpetual inventory") + company = "_Test Company with perpetual inventory" + set_depreciation_settings_in_company(company) asset_category = frappe.get_doc("Asset Category", "Computers") - asset_category.append( - "accounts", - { - "company_name": "_Test Company with perpetual inventory", - "fixed_asset_account": "_Test Fixed Asset - TCP1", - "accumulated_depreciation_account": "_Test Accumulated Depreciations - TCP1", - "depreciation_expense_account": "_Test Depreciations - TCP1", - "capital_work_in_progress_account": "CWIP Account - TCP1", - }, - ) - asset_category.save() + + if not any(row.company_name == company for row in asset_category.accounts): + asset_category.append( + "accounts", + { + "company_name": company, + "fixed_asset_account": "_Test Fixed Asset - TCP1", + "accumulated_depreciation_account": "_Test Accumulated Depreciations - TCP1", + "depreciation_expense_account": "_Test Depreciations - TCP1", + "capital_work_in_progress_account": "CWIP Account - TCP1", + }, + ) + asset_category.save() asset_repair = create_asset_repair( capitalize_repair_cost=1, stock_consumption=1, warehouse="Stores - TCP1", - company="_Test Company with perpetual inventory", + company=company, pi_expense_account1="Administrative Expenses - TCP1", pi_expense_account2="Legal Expenses - TCP1", item="_Test Non Stock Item",