From f828d853e3051d9cd9b15f54538954c83b5a8afb Mon Sep 17 00:00:00 2001
From: Rucha Mahabal <ruchamahabal2@gmail.com>
Date: Wed, 1 Sep 2021 23:07:26 +0530
Subject: [PATCH] fix: Org Chart fixes (#27290)

* fix(org chart): multiple root nodes not expanding on clicking Expand All

* fix: unstable filter inserts duplicate nodes
---
 .../organizational_chart.js                   |  2 ++
 .../hierarchy_chart_desktop.js                | 26 +++++++++++--------
 .../hierarchy_chart/hierarchy_chart_mobile.js |  2 --
 erpnext/utilities/hierarchy_chart.py          | 12 ++++++---
 4 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/erpnext/hr/page/organizational_chart/organizational_chart.js b/erpnext/hr/page/organizational_chart/organizational_chart.js
index 81162a4c9a..b0e41e0090 100644
--- a/erpnext/hr/page/organizational_chart/organizational_chart.js
+++ b/erpnext/hr/page/organizational_chart/organizational_chart.js
@@ -15,6 +15,8 @@ frappe.pages['organizational-chart'].on_page_load = function(wrapper) {
 			} else {
 				organizational_chart = new erpnext.HierarchyChart('Employee', wrapper, method);
 			}
+
+			frappe.breadcrumbs.add('HR');
 			organizational_chart.show();
 		});
 	});
diff --git a/erpnext/public/js/hierarchy_chart/hierarchy_chart_desktop.js b/erpnext/public/js/hierarchy_chart/hierarchy_chart_desktop.js
index 23ec2fdb84..6286732753 100644
--- a/erpnext/public/js/hierarchy_chart/hierarchy_chart_desktop.js
+++ b/erpnext/public/js/hierarchy_chart/hierarchy_chart_desktop.js
@@ -67,8 +67,6 @@ erpnext.HierarchyChart = class {
 	}
 
 	show() {
-		frappe.breadcrumbs.add('HR');
-
 		this.setup_actions();
 		if ($(`[data-fieldname="company"]`).length) return;
 		let me = this;
@@ -83,8 +81,9 @@ erpnext.HierarchyChart = class {
 			reqd: 1,
 			change: () => {
 				me.company = undefined;
+				$('#hierarchy-chart-wrapper').remove();
 
-				if (company.get_value() && me.company != company.get_value()) {
+				if (company.get_value()) {
 					me.company = company.get_value();
 
 					// svg for connectors
@@ -92,6 +91,8 @@ erpnext.HierarchyChart = class {
 					me.setup_hierarchy();
 					me.render_root_nodes();
 					me.all_nodes_expanded = false;
+				} else {
+					frappe.throw(__('Please select a company first.'));
 				}
 			}
 		});
@@ -172,11 +173,11 @@ erpnext.HierarchyChart = class {
 			</ul>`);
 
 		this.page.main
-			.find('#hierarchy-chart-wrapper')
+			.find('#hierarchy-chart')
+			.empty()
 			.append(this.$hierarchy);
 
 		this.nodes = {};
-		this.all_nodes_expanded = false;
 	}
 
 	make_svg_markers() {
@@ -203,6 +204,8 @@ erpnext.HierarchyChart = class {
 					<g id="connectors" fill="none">
 					</g>
 				</svg>
+				<div id="hierarchy-chart">
+				</div>
 			</div>`);
 	}
 
@@ -219,7 +222,10 @@ erpnext.HierarchyChart = class {
 				let expand_node = undefined;
 				let node = undefined;
 
-				$.each(r.message, (i, data) => {
+				$.each(r.message, (_i, data) => {
+					if ($(`#${data.id}`).length)
+						return;
+
 					node = new me.Node({
 						id: data.id,
 						parent: $('<li class="child-node"></li>').appendTo(me.$hierarchy.find('.node-children')),
@@ -290,7 +296,7 @@ erpnext.HierarchyChart = class {
 				() => frappe.dom.freeze(),
 				() => this.setup_hierarchy(),
 				() => this.render_root_nodes(true),
-				() => this.get_all_nodes(node.id, node.name),
+				() => this.get_all_nodes(),
 				(data_list) => this.render_children_of_all_nodes(data_list),
 				() => frappe.dom.unfreeze()
 			]);
@@ -341,15 +347,13 @@ erpnext.HierarchyChart = class {
 		node.expanded = true;
 	}
 
-	get_all_nodes(node_id, node_name) {
+	get_all_nodes() {
 		return new Promise(resolve => {
 			frappe.call({
 				method: 'erpnext.utilities.hierarchy_chart.get_all_nodes',
 				args: {
 					method: this.method,
-					company: this.company,
-					parent: node_id,
-					parent_name: node_name
+					company: this.company
 				},
 				callback: (r) => {
 					resolve(r.message);
diff --git a/erpnext/public/js/hierarchy_chart/hierarchy_chart_mobile.js b/erpnext/public/js/hierarchy_chart/hierarchy_chart_mobile.js
index b1b78c0517..b1a8879557 100644
--- a/erpnext/public/js/hierarchy_chart/hierarchy_chart_mobile.js
+++ b/erpnext/public/js/hierarchy_chart/hierarchy_chart_mobile.js
@@ -59,8 +59,6 @@ erpnext.HierarchyChartMobile = class {
 	}
 
 	show() {
-		frappe.breadcrumbs.add('HR');
-
 		let me = this;
 		if ($(`[data-fieldname="company"]`).length) return;
 
diff --git a/erpnext/utilities/hierarchy_chart.py b/erpnext/utilities/hierarchy_chart.py
index 384d84194b..cc506e5cfd 100644
--- a/erpnext/utilities/hierarchy_chart.py
+++ b/erpnext/utilities/hierarchy_chart.py
@@ -6,17 +6,21 @@ import frappe
 from frappe import _
 
 @frappe.whitelist()
-def get_all_nodes(parent, parent_name, method, company):
+def get_all_nodes(method, company):
 	'''Recursively gets all data from nodes'''
 	method = frappe.get_attr(method)
 
 	if method not in frappe.whitelisted:
 		frappe.throw(_('Not Permitted'), frappe.PermissionError)
 
-	data = method(parent, company)
-	result = [dict(parent=parent, parent_name=parent_name, data=data)]
+	root_nodes = method(company=company)
+	result = []
+	nodes_to_expand = []
 
-	nodes_to_expand = [{'id': d.get('id'), 'name': d.get('name')} for d in data if d.get('expandable')]
+	for root in root_nodes:
+		data = method(root.id, company)
+		result.append(dict(parent=root.id, parent_name=root.name, data=data))
+		nodes_to_expand.extend([{'id': d.get('id'), 'name': d.get('name')} for d in data if d.get('expandable')])
 
 	while nodes_to_expand:
 		parent = nodes_to_expand.pop(0)