fix(accounts): validate chart import rows and parent cycles

(cherry picked from commit 36ddeb4f5e)

erpnext/accounts/doctype/chart_of_accounts_importer/chart_of_accounts_importer.py

@@ -46,9 +46,7 @@ def validate_columns(data):
 	if not data:
 		frappe.throw(_("No data found. Seems like you uploaded a blank file"))
 
-	no_of_columns = max([len(d) for d in data])
-
-	if no_of_columns != 8:
+	if any(len(row) != 8 for row in data):
 		frappe.throw(
 			_(
 				"Columns are not according to template. Please compare the uploaded file with standard template"
@@ -99,13 +97,13 @@ def import_coa(file_name, company):
 		data = generate_data_from_excel(file_doc, extension)
 
 	validate_columns(data)
+	forest = build_forest(data)
 
 	validate_company(company)
 
 	unset_existing_data(company)
 
 	frappe.local.flags.ignore_root_company_validation = True
-	forest = build_forest(data)
 	create_charts(company, custom_chart=forest, from_coa_importer=True)
 
 	# trigger on_update for company to reset default accounts
@@ -135,25 +133,8 @@ def get_file(file_name):
 def generate_data_from_csv(file_doc, as_dict=False):
 	"""read csv file and return the generated nested tree"""
 
-	file_path = file_doc.get_full_path()
-
-	data = []
-	with open(file_path) as in_file:
-		csv_reader = list(csv.reader(in_file))
-		headers = csv_reader[0]
-		del csv_reader[0]  # delete top row and headers row
-
-		for row in csv_reader:
-			if as_dict:
-				data.append({frappe.scrub(header): row[index] for index, header in enumerate(headers)})
-			else:
-				if not row[1] and len(row) > 1:
-					row[1] = row[0]
-					row[3] = row[2]
-				data.append(row)
-
-	# convert csv data
-	return data
+	with open(file_doc.get_full_path()) as in_file:
+		return generate_data_from_rows(list(csv.reader(in_file)), as_dict)
 
 
 def generate_data_from_excel(file_doc, extension, as_dict=False):
@@ -164,13 +145,19 @@ def generate_data_from_excel(file_doc, extension, as_dict=False):
 	elif extension == "xls":
 		rows = read_xls_file_from_attached_file(content)
 
-	data = []
-	headers = rows[0]
-	del rows[0]
+	return generate_data_from_rows(rows, as_dict)
 
-	for row in rows:
+
+def generate_data_from_rows(rows, as_dict=False):
+	rows = [row for row in rows if any(cstr(value).strip() for value in row)]
+	validate_columns(rows[1:])
+	validate_columns(rows[:1])
+
+	headers = rows[0]
+	data = []
+	for row in rows[1:]:
 		if as_dict:
-			data.append({frappe.scrub(header): row[index] for index, header in enumerate(headers)})
+			data.append({frappe.scrub(cstr(header)): row[index] for index, header in enumerate(headers)})
 		else:
 			if not row[1]:
 				row[1] = row[0]
@@ -195,9 +182,9 @@ def get_coa(doctype, parent, is_root=False, file_name=None, for_validate=0):
 
 	validate_columns(data)
 	validate_accounts(file_doc, extension)
+	forest = build_forest(data)
 
 	if not for_validate:
-		forest = build_forest(data)
 		accounts = build_tree_from_json(
 			"", chart_data=forest, from_coa_importer=True
 		)  # returns a list of dict in a tree render-able form
@@ -225,6 +212,7 @@ def build_forest(data):
 	        }
 	}
 	"""
+	validate_columns(data)
 
 	# set the value of nested dictionary
 	def set_nested(d, path, value):
@@ -232,31 +220,25 @@ def build_forest(data):
 		return d
 
 	# returns the path of any node in list format
-	def return_parent(data, child):
-		from frappe import _
-
-		for row in data:
-			account_name, parent_account, account_number, parent_account_number = row[0:4]
-			if account_number:
-				account_number = cstr(account_number).strip()
-				account_name = f"{account_number} - {account_name}"
-			if parent_account_number:
-				parent_account_number = cstr(parent_account_number).strip()
-				parent_account = f"{parent_account_number} - {parent_account}"
-
-			if parent_account == account_name == child:
-				return [parent_account]
-			elif account_name == child:
-				parent_account_list = return_parent(data, parent_account)
-				if not parent_account_list and parent_account:
-					frappe.throw(
-						_("The parent account {0} does not exists in the uploaded template").format(
-							frappe.bold(parent_account)
-						)
+	def return_parent(parent_accounts, child):
+		path = []
+		while child:
+			if child in path:
+				frappe.throw(_("Circular reference found for account {0}").format(frappe.bold(child)))
+			path.append(child)
+			parent_account = parent_accounts[child]
+			if parent_account == child:
+				break
+			if parent_account and parent_account not in parent_accounts:
+				frappe.throw(
+					_("The parent account {0} does not exist in the uploaded template").format(
+						frappe.bold(parent_account)
 					)
-				return [child, *parent_account_list]
+				)
+			child = parent_account
+		return path
 
-	charts_map, paths = {}, []
+	charts_map, parent_accounts = {}, {}
 
 	line_no = 2
 	error_messages = []
@@ -274,12 +256,15 @@ def build_forest(data):
 		) = i
 
 		if not account_name:
-			error_messages.append(f"Row {line_no}: Please enter Account Name")
+			error_messages.append(_("Row {0}: Please enter Account Name").format(line_no))
 
 		name = account_name
 		if account_number:
 			account_number = cstr(account_number).strip()
 			account_name = f"{account_number} - {account_name}"
+		if parent_account_number:
+			parent_account = f"{cstr(parent_account_number).strip()} - {parent_account}"
+		parent_accounts[account_name] = parent_account
 
 		charts_map[account_name] = {}
 		charts_map[account_name]["account_name"] = name
@@ -293,15 +278,14 @@ def build_forest(data):
 			charts_map[account_name]["root_type"] = root_type
 		if account_currency:
 			charts_map[account_name]["account_currency"] = account_currency
-		path = return_parent(data, account_name)[::-1]
-		paths.append(path)  # List of path is created
 		line_no += 1
 
 	if error_messages:
 		frappe.throw("<br>".join(error_messages))
 
 	out = {}
-	for path in paths:
+	for account_name in charts_map:
+		path = return_parent(parent_accounts, account_name)[::-1]
 		for n, account_name in enumerate(path):
 			set_nested(
 				out, path[: n + 1], charts_map[account_name]
@@ -361,14 +345,14 @@ def get_template(template_type, company):
 
 	if template_type == "Blank Template":
 		for root_type in get_root_types():
-			writer.writerow(["", "", "", "", 1, "", root_type])
+			writer.writerow(["", "", "", "", 1, "", root_type, ""])
 
 		for account in get_mandatory_group_accounts():
-			writer.writerow(["", "", "", "", 1, account, "Asset"])
+			writer.writerow(["", "", "", "", 1, account, "Asset", ""])
 
 		for account_type in get_mandatory_account_types():
 			writer.writerow(
-				["", "", "", "", 0, account_type.get("account_type"), account_type.get("root_type")]
+				["", "", "", "", 0, account_type.get("account_type"), account_type.get("root_type"), ""]
 			)
 	else:
 		writer = get_sample_template(writer, company)
@@ -395,6 +379,8 @@ def validate_accounts(file_doc, extension):
 
 	accounts_dict = {}
 	for account in accounts:
+		if not account.get("account_name"):
+			frappe.throw(_("Please enter Account Name for every row in the uploaded template"))
 		accounts_dict.setdefault(account["account_name"], account)
 		if "parent_account" not in account:
 			msg = _(