Merge pull request #52331 from aerele/fix-validate-asset-category-accounts

fix(asset): validate depreciation when category has active depreciable assets

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:
+		if self.calculate_depreciation:
-			frappe.throw(
+			if non_depreciable_category:
-				_(
+				frappe.throw(
-					"This asset category is marked as non-depreciable. Please disable depreciation calculation or choose a different category."
+					_(
+						"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:

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

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 <b>{1}</b> for company <b>{2}</b>.").format(
+								acc_row.idx,
+								label,
+								get_link_to_form("Company", company),
+							)
+						)
+					else:
+						msg = _("Missing account configuration for company <b>{0}</b>.").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. <br><br>"
+			)
+			msg += _(
+				"You can either configure default depreciation accounts in the Company or set the required accounts in the following rows: <br><br>"
+			)
+			msg += "<br>".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

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)

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",
+		if not any(row.company_name == company for row in asset_category.accounts):
-			{
+			asset_category.append(
-				"company_name": "_Test Company with perpetual inventory",
+				"accounts",
-				"fixed_asset_account": "_Test Fixed Asset - TCP1",
+				{
-				"accumulated_depreciation_account": "_Test Accumulated Depreciations - TCP1",
+					"company_name": company,
-				"depreciation_expense_account": "_Test Depreciations - TCP1",
+					"fixed_asset_account": "_Test Fixed Asset - TCP1",
-				"capital_work_in_progress_account": "CWIP Account - 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_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",