Merge lp:~blake-rouse/maas/fix-1462019 into lp:~maas-committers/maas/trunk

[Blake Rouse]

Add some defensive programming in the NodeDetailsController preventing the UI from becoming unresponsive when things go crazy.

[Raphaël Badin (rvb)]

Looks good. Might deserve a test or two.

review: Approve

[Blake Rouse (blake-rouse)]

Thanks for the review. I have added tests!

=== modified file 'src/maasserver/static/js/angular/controllers/node_details.js'
--- src/maasserver/static/js/angular/controllers/node_details.js	2015-06-03 20:22:58 +0000
+++ src/maasserver/static/js/angular/controllers/node_details.js	2015-06-05 13:16:02 +0000
@@ -155,12 +155,14 @@
 
             // Show the cluster disconnected error if the cluster is not
             // connected.
-            var cluster = ClustersManager.getItemFromList(
-                $scope.node.nodegroup.id);
-            if(!cluster.connected) {
-                showError("cluster_disconnected");
-            } else {
-                hideError("cluster_disconnected");
+            if(angular.isObject($scope.node.nodegroup)) {
+                var cluster = ClustersManager.getItemFromList(
+                    $scope.node.nodegroup.id);
+                if(!cluster.connected) {
+                    showError("cluster_disconnected");
+                } else {
+                    hideError("cluster_disconnected");
+                }
             }
         }
 
@@ -206,18 +208,21 @@
 
             // Always keep the available power types up-to-date even in
             // editing mode.
-            var cluster = ClustersManager.getItemFromList(
-                $scope.node.nodegroup.id);
+            var cluster;
+            if(angular.isObject($scope.node.nodegroup)) {
+                cluster = ClustersManager.getItemFromList(
+                    $scope.node.nodegroup.id);
+            }
             if(angular.isObject(cluster)) {
                 $scope.power.types = cluster.power_types;
             } else {
                 $scope.power.types = [];
             }
 
-            // If the cluster is disconnected then always force editing to
-            // false. Its not possible to stay editing the power section
-            // when the cluster is disconnected.
-            if(!cluster.connected) {
+            // If the no cluster or the cluster is disconnected then always
+            // force editing to false. Its not possible to stay editing the
+            // power section when the cluster is disconnected.
+            if(!angular.isObject(cluster) || !cluster.connected) {
                 $scope.power.editing = false;
             }
 
@@ -261,8 +266,10 @@
                 return;
             }
 
-            $scope.summary.cluster.selected = ClustersManager.getItemFromList(
-                $scope.node.nodegroup.id);
+            if(angular.isObject($scope.node.nodegroup)) {
+                $scope.summary.cluster.selected =
+                    ClustersManager.getItemFromList($scope.node.nodegroup.id);
+            }
             $scope.summary.zone.selected = ZonesManager.getItemFromList(
                 $scope.node.zone.id);
             $scope.summary.architecture.selected = $scope.node.architecture;
@@ -311,8 +318,10 @@
             $scope.machine_output.viewable = (
                 angular.isString($scope.node.summary_xml) ||
                 angular.isString($scope.node.summary_yaml) ||
-                $scope.node.commissioning_results.length > 0 ||
-                $scope.node.installation_results.length > 0);
+                (angular.isArray($scope.node.commissioning_results) &&
+                    $scope.node.commissioning_results.length > 0) ||
+                (angular.isArray($scope.node.installation_results) &&
+                    $scope.node.installation_results.length > 0));
 
             // Grab the selected view name, so it can be kept the same if
             // possible.
@@ -336,13 +345,15 @@
                     title: "Commissioning Summary"
                 });
             }
-            if($scope.node.commissioning_results.length > 0) {
+            if(angular.isArray($scope.node.commissioning_results) &&
+                $scope.node.commissioning_results.length > 0) {
                 $scope.machine_output.views.push({
                     name: "output",
                     title: "Commissioning Output"
                 });
             }
-            if($scope.node.installation_results.length > 0) {
+            if(angular.isArray($scope.node.installation_results) &&
+                $scope.node.installation_results.length > 0) {
                 $scope.machine_output.views.push({
                     name: "install",
                     title: "Installation Output"
@@ -923,6 +934,9 @@
             if(!angular.isObject($scope.node)) {
                 return false;
             }
+            if(!angular.isArray($scope.node.events)) {
+                return false;
+            }
             return (
                 $scope.node.events.length > 0 &&
                 $scope.node.events.length > $scope.events.limit &&
@@ -980,6 +994,9 @@
             // results, but it is unused. Only one installation result will
             // exists for a node. Grab the first one in the array.
             var results = $scope.node.installation_results;
+            if(!angular.isArray(results)) {
+                return "";
+            }
             if(results.length === 0) {
                 return "";
             } else {

=== modified file 'src/maasserver/static/js/angular/controllers/tests/test_node_details.js'
--- src/maasserver/static/js/angular/controllers/tests/test_node_details.js	2015-06-03 21:21:47 +0000
+++ src/maasserver/static/js/angular/controllers/tests/test_node_details.js	2015-06-05 13:16:02 +0000
@@ -347,6 +347,16 @@
             expect($scope.summary.editing).toBe(false);
         });
 
+    it("skips cluster_disconnected error if the nodegroup on node is invalid",
+        function() {
+            var cluster = ClustersManager.getItemFromList(node.nodegroup.id);
+            cluster.connected = false;
+            node.nodegroup = undefined;
+
+            var controller = makeControllerResolveSetActiveItem();
+            expect($scope.errors.cluster_disconnected.viewable).toBe(false);
+        });
+
     it("cluster_disconnected error visible if cluster disconnected",
         function() {
             var cluster = ClustersManager.getItemFromList(node.nodegroup.id);
@@ -509,12 +519,26 @@
         expect($scope.machine_output.viewable).toBe(true);
     });
 
+    it("machine output not visible if commissioning_results not an array",
+        function() {
+            node.commissioning_results = undefined;
+            var controller = makeControllerResolveSetActiveItem();
+            expect($scope.machine_output.viewable).toBe(false);
+        });
+
     it("machine output visible if installation_results", function() {
         node.installation_results.push({});
         var controller = makeControllerResolveSetActiveItem();
         expect($scope.machine_output.viewable).toBe(true);
     });
 
+    it("machine output not visible if installation_results not an array",
+        function() {
+            node.installation_results = undefined;
+            var controller = makeControllerResolveSetActiveItem();
+            expect($scope.machine_output.viewable).toBe(false);
+        });
+
     it("machine output summary view available if summary_xml and summary_yaml",
         function() {
             node.summary_xml = node.summary_yaml = "summary";
@@ -2035,6 +2059,13 @@
             expect($scope.allowShowMoreEvents()).toBe(false);
         });
 
+        it("returns false if node.events is not array", function() {
+            var controller = makeController();
+            $scope.node = node;
+            $scope.node.events = undefined;
+            expect($scope.allowShowMoreEvents()).toBe(false);
+        });
+
         it("returns false if node has no events", function() {
             var controller = makeController();
             $scope.node = node;
@@ -2165,6 +2196,14 @@
             expect($scope.getInstallationData()).toBe("");
         });
 
+        it("returns blank string if installation results not an array",
+            function() {
+                var controller = makeController();
+                $scope.node = makeNode();
+                $scope.node.installation_results = undefined;
+                expect($scope.getInstallationData()).toBe("");
+            });
+
         it("returns blank string if no installation results", function() {
             var controller = makeController();
             $scope.node = makeNode();