Merge ~ltrager/maas:2.3_1752754 into maas:2.3

[Lee Trager]

LP: #1752754 - Request historic results over the websocket instead of always sending them.

Backport of 9e35f1a for LP: #1830365

[Blake Rouse (blake-rouse)]

Looks good

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b 2.3_1752754 lp:~ltrager/maas/+git/maas into -b 2.3 lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/5805/consoleText

2bd8056... by Lee Trager on 2019-05-31

Merge branch '2.3' into 2.3_1752754

diff --git a/src/maasserver/static/js/angular/controllers/node_results.js b/src/maasserver/static/js/angular/controllers/node_results.js
index 101e500..59f3f6f 100644
--- a/src/maasserver/static/js/angular/controllers/node_results.js
+++ b/src/maasserver/static/js/angular/controllers/node_results.js
@@ -1,4 +1,4 @@
-/* Copyright 2017 Canonical Ltd.  This software is licensed under the
+/* Copyright 2017-2018 Canonical Ltd.  This software is licensed under the
  * GNU Affero General Public License version 3 (see the file LICENSE).
  *
  * MAAS Node Results Controller
@@ -135,17 +135,12 @@ angular.module('MAAS').controller('NodeResultsController', [
                     });
             } else {
                 var result = null;
-                var i, j;
+                var i;
                 // Find the installation result to be displayed.
                 for(i = 0; i < $scope.installation_results.length; i++) {
-                    var hlist = $scope.installation_results[i].history_list;
-                    for(j = 0; j < hlist.length; j++) {
-                        if(hlist[j].id === $scope.logs.option.id) {
-                            result = hlist[j];
-                            break;
-                        }
-                    }
-                    if(result) {
+                    if($scope.installation_results[i].id ===
+                       $scope.logs.option.id) {
+                        result = $scope.installation_results[i];
                         break;
                     }
                 }
@@ -195,6 +190,22 @@ angular.module('MAAS').controller('NodeResultsController', [
             }
         };
 
+        $scope.loadHistory = function(result) {
+            result.showing_results = false;
+            // History has already been loaded, no need to request it.
+            if(angular.isArray(result.history_list)) {
+                result.showing_history = true;
+                return;
+            }
+            result.loading_history = true;
+            $scope.nodeResultsManager.get_history(result.id).then(
+                function(history) {
+                    result.history_list = history;
+                    result.loading_history = false;
+                    result.showing_history = true;
+                });
+        };
+
         // Destroy the NodeResultsManager when the scope is destroyed. This is
         // so the client will not recieve any more notifications about results
         // from this node.
diff --git a/src/maasserver/static/js/angular/controllers/tests/test_node_results.js b/src/maasserver/static/js/angular/controllers/tests/test_node_results.js
index f2f44d7..fbeb5a1 100644
--- a/src/maasserver/static/js/angular/controllers/tests/test_node_results.js
+++ b/src/maasserver/static/js/angular/controllers/tests/test_node_results.js
@@ -433,4 +433,50 @@ describe("NodeResultsController", function() {
                 "BUG: Unknown log status " + installation_result.status);
         });
     });
+
+    describe("loadHistory", function() {
+        it("loads results", function() {
+            var defer = $q.defer();
+            var controller = makeController();
+            var result = {
+                id: makeInteger(0, 100)
+            };
+            var history_list = [
+                {id: makeInteger(0, 100)},
+                {id: makeInteger(0, 100)},
+                {id: makeInteger(0, 100)}
+            ];
+            $scope.node = node;
+            $scope.nodeResultsManager = NodeResultsManagerFactory.getManager(
+                $scope.node);
+            spyOn($scope.nodeResultsManager, "get_history").and.returnValue(
+                defer.promise);
+
+            $scope.loadHistory(result);
+            defer.resolve(history_list);
+            $rootScope.$digest();
+
+            expect(result.history_list).toBe(history_list);
+            expect(result.loading_history).toBe(false);
+            expect(result.showing_history).toBe(true);
+        });
+
+        it("doesnt reload", function() {
+            var controller = makeController();
+            var result = {
+                id: makeInteger(0, 100),
+                history_list: [{id: makeInteger(0, 100)}]
+            };
+            $scope.node = node;
+            $scope.nodeResultsManager = NodeResultsManagerFactory.getManager(
+                $scope.node);
+            spyOn($scope.nodeResultsManager, "get_history");
+
+            $scope.loadHistory(result);
+
+            expect(result.showing_history).toBe(true);
+            expect(
+                $scope.nodeResultsManager.get_history).not.toHaveBeenCalled();
+        });
+    });
 });
diff --git a/src/maasserver/static/js/angular/factories/node_results.js b/src/maasserver/static/js/angular/factories/node_results.js
index 1a0a5de..94a52f7 100644
--- a/src/maasserver/static/js/angular/factories/node_results.js
+++ b/src/maasserver/static/js/angular/factories/node_results.js
@@ -1,4 +1,4 @@
-/* Copyright 2017 Canonical Ltd.  This software is licensed under the
+/* Copyright 2017-2018 Canonical Ltd.  This software is licensed under the
  * GNU Affero General Public License version 3 (see the file LICENSE).
  *
  * MAAS NodeResultsManager Manager
@@ -266,6 +266,15 @@ angular.module('MAAS').factory(
             return RegionConnection.callMethod(method, params);
         };
 
+        // Get historic data.
+        NodeResultsManager.prototype.get_history = function(script_id) {
+            var method = this._handler + ".get_history";
+            var params = {
+                id: script_id,
+            };
+            return RegionConnection.callMethod(method, params);
+        };
+
         // Factory that holds all created NodeResultsManagers.
         function NodeResultsManagerFactory() {
             // Holds a list of all NodeResultsManagers that have been created.
diff --git a/src/maasserver/static/js/angular/factories/tests/test_node_results.js b/src/maasserver/static/js/angular/factories/tests/test_node_results.js
index 859dda3..2508969 100644
--- a/src/maasserver/static/js/angular/factories/tests/test_node_results.js
+++ b/src/maasserver/static/js/angular/factories/tests/test_node_results.js
@@ -1,4 +1,4 @@
-/* Copyright 2017 Canonical Ltd.  This software is licensed under the
+/* Copyright 2017-2018 Canonical Ltd.  This software is licensed under the
  * GNU Affero General Public License version 3 (see the file LICENSE).
  *
  * Unit tests for NodeResultsManagerFactory.
@@ -532,6 +532,27 @@ describe("NodeResultsManagerFactory", function() {
         });
     });
 
+    describe("get_history", function() {
+
+        it("calls NodeResultHandler.get_history", function(done) {
+            var node = makenode();
+            var output = [{
+                id: makeInteger(0, 100),
+                name: makeName("output"),
+                status: makeInteger(0, 100)
+            }];
+            var id = makeInteger(0, 100);
+            webSocket.returnData.push(makeFakeResponse(output));
+            NodeResultsManager = NodeResultsManagerFactory.getManager(node);
+            NodeResultsManager.get_history(id).then(function() {
+                var sentObject = angular.fromJson(webSocket.sentData[0]);
+                expect(sentObject.method).toBe("noderesult.get_history");
+                expect(sentObject.params.id).toEqual(id);
+                done();
+            });
+        });
+    });
+
     describe("destroyManager", function() {
 
         it("removes manager from _managers", function() {
diff --git a/src/maasserver/static/partials/script-results-list.html b/src/maasserver/static/partials/script-results-list.html
index 913978c..67ca3ab 100644
--- a/src/maasserver/static/partials/script-results-list.html
+++ b/src/maasserver/static/partials/script-results-list.html
@@ -44,7 +44,7 @@
                                     <div class="table__controls-menu ng-hide" role="menu" data-ng-show="isToggled">
                                         <button class="table__controls-action" aria-label="View metrics" data-ng-if="!result.showing_results" data-ng-click="toggleMenu(); result.showing_history = false; result.showing_results = true">View metrics</button>
                                         <button class="table__controls-action" aria-label="Hide metrics" data-ng-if="result.showing_results" data-ng-click="toggleMenu(); result.showing_history = false; result.showing_results = false">Hide metrics</button>
-                                        <button class="table__controls-action" aria-label="View previous {$ result.result_section $}" data-ng-if="!result.showing_history" data-ng-click="toggleMenu(); result.showing_results = false; result.showing_history = true">View previous {$ result.result_section $}</button>
+                                        <button class="table__controls-action" aria-label="View previous {$ result.result_section $}" data-ng-if="!result.showing_history" data-ng-click="toggleMenu(); loadHistory(result)">View previous {$ result.result_section $}</button>
                                         <button class="table__controls-action" aria-label="Hide previous {$ result.result_section $}" data-ng-if="result.showing_history" data-ng-click="toggleMenu(); result.showing_results = false; result.showing_history = false">Hide previous {$ result.result_section $}</button>
                                     </div>
                                 </div>
@@ -60,6 +60,11 @@
                                 </div>
                             </div>
 
+			    <div class="p-table-expanding__panel col-12" aria-label="loading history" data-ng-if="result.loading_history">
+			        <div class="col-12">
+				    <p class="u-text--loading"><i class="p-icon--spinner u-animation--spin"></i>&nbsp;&nbsp;Loading...</p>
+				</div>
+			    </div>
                             <div class="table__dropdown" aria-label="history" data-ng-if="result.showing_history">
                                 <div class="table__row is-active">
                                     <div class="table__data table-col--100">
@@ -83,6 +88,7 @@
                                         <p class="u-align--center u-margin--bottom">
                                             <button class="button--secondary button--inline" data-ng-click="result.showing_history = false">Hide previous {$ result.result_section $}</button>
                                         </p>
+
                                     </div>
                                 </div>
                             </div>
diff --git a/src/maasserver/websockets/handlers/node_result.py b/src/maasserver/websockets/handlers/node_result.py
index e6e516e..cf04e58 100644
--- a/src/maasserver/websockets/handlers/node_result.py
+++ b/src/maasserver/websockets/handlers/node_result.py
@@ -32,6 +32,7 @@ class NodeResultHandler(TimestampedModelHandler):
             'clear',
             'get',
             'get_result_data',
+            'get_history',
             'list',
         ]
         listen_channels = ['scriptresult']
@@ -88,18 +89,6 @@ class NodeResultHandler(TimestampedModelHandler):
             # Script object.
             data["hardware_type"] = HARDWARE_TYPE.NODE
             data["tags"] = 'commissioning'
-        data["history_list"] = [
-            {
-                "id": history.id,
-                "updated": dehydrate_datetime(history.updated),
-                "status": history.status,
-                "status_name": history.status_name,
-                "runtime": history.runtime,
-                "starttime": history.starttime,
-                "endtime": history.endtime,
-                "estimated_runtime": history.estimated_runtime,
-            } for history in obj.history
-        ]
         try:
             results = obj.read_results()
         except ValidationError as e:
@@ -213,6 +202,26 @@ class NodeResultHandler(TimestampedModelHandler):
         else:
             return "Unknown data_type %s" % data_type
 
+    def get_history(self, params):
+        """Return a list of historic results."""
+        id = params.get('id')
+        script_result = ScriptResult.objects.filter(id=id).only(
+            'script_id', 'script_set_id', 'physical_blockdevice_id',
+            'script_name').first()
+        history_qs = script_result.history.only(
+            'id', 'updated', 'status', 'started', 'ended', 'script_id',
+            'script_name', 'script_set_id', 'physical_blockdevice_id')
+        return [{
+            'id': history.id,
+            'updated': dehydrate_datetime(history.updated),
+            'status': history.status,
+            'status_name': history.status_name,
+            'runtime': history.runtime,
+            'starttime': history.starttime,
+            'endtime': history.endtime,
+            'estimated_runtime': history.estimated_runtime,
+        } for history in history_qs]
+
     def clear(self, params):
         """Clears the current node for events.
 
diff --git a/src/maasserver/websockets/handlers/tests/test_node_result.py b/src/maasserver/websockets/handlers/tests/test_node_result.py
index 5b3fc37..a8fe7fe 100644
--- a/src/maasserver/websockets/handlers/tests/test_node_result.py
+++ b/src/maasserver/websockets/handlers/tests/test_node_result.py
@@ -1,4 +1,4 @@
-# Copyright 2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2017-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for `maasserver.websockets.handlers.node_result`"""
@@ -16,6 +16,7 @@ from maasserver.websockets.base import (
     HandlerPKError,
 )
 from maasserver.websockets.handlers.node_result import NodeResultHandler
+from maastesting.djangotestcase import CountQueries
 from metadataserver.enum import (
     HARDWARE_TYPE,
     HARDWARE_TYPE_CHOICES,
@@ -49,16 +50,6 @@ class TestNodeResultHandler(MAASServerTestCase):
             "result_type": script_result.script_set.result_type,
             "hardware_type": script_result.script.hardware_type,
             "tags": ", ".join(script_result.script.tags),
-            "history_list": [{
-                "id": history.id,
-                "updated": dehydrate_datetime(history.updated),
-                "status": history.status,
-                "status_name": history.status_name,
-                "runtime": history.runtime,
-                "starttime": history.starttime,
-                "endtime": history.endtime,
-                "estimated_runtime": history.estimated_runtime,
-                } for history in script_result.history],
             "results": [{
                 "name": key,
                 "title": key,
@@ -333,6 +324,36 @@ class TestNodeResultHandler(MAASServerTestCase):
                 'data_type': unknown_data_type,
                 }))
 
+    def test_get_history(self):
+        user = factory.make_User()
+        handler = NodeResultHandler(user, {})
+        node = factory.make_Node(owner=user)
+        script = factory.make_Script()
+        script_results = []
+        for _ in range(10):
+            script_set = factory.make_ScriptSet(node=node)
+            script_results.append(factory.make_ScriptResult(
+                script=script, script_set=script_set,
+                status=SCRIPT_STATUS.PASSED))
+        latest_script_result = script_results[-1]
+        script_results = sorted(
+            script_results, key=lambda i: i.id, reverse=True)
+        queries = CountQueries()
+        with queries:
+            ret = handler.get_history({'id': latest_script_result.id})
+        self.assertEqual(4, queries.num_queries)
+        for script_result, out in zip(script_results, ret):
+            self.assertDictEqual({
+                'id': script_result.id,
+                'updated': dehydrate_datetime(script_result.updated),
+                'status': script_result.status,
+                'status_name': script_result.status_name,
+                'runtime': script_result.runtime,
+                'starttime': script_result.starttime,
+                'endtime': script_result.endtime,
+                'estimated_runtime': script_result.estimated_runtime,
+            }, out)
+
     def test_clear_removes_system_id_from_cache(self):
         user = factory.make_User()
         handler = NodeResultHandler(user, {})
diff --git a/src/metadataserver/models/scriptresult.py b/src/metadataserver/models/scriptresult.py
index 741c6db..7f0ea1b 100644
--- a/src/metadataserver/models/scriptresult.py
+++ b/src/metadataserver/models/scriptresult.py
@@ -1,4 +1,4 @@
-# Copyright 2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2017-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 __all__ = [
     'ScriptResult',
@@ -139,7 +139,9 @@ class ScriptResult(CleanSave, TimestampedModel):
             return self.runtime
         runtime = None
         # Get an estimated runtime from previous runs.
-        for script_result in self.history:
+        for script_result in self.history.only(
+                'status', 'started', 'ended', 'script_id', 'script_name',
+                'script_set_id', 'physical_blockdevice_id'):
             # Only look at passed results when calculating an estimated
             # runtime. Failed results may take longer or shorter than
             # average. Don't use self.history.filter for this as the now
diff --git a/src/metadataserver/models/tests/test_scriptresult.py b/src/metadataserver/models/tests/test_scriptresult.py
index 11269d9..5930fae 100644
--- a/src/metadataserver/models/tests/test_scriptresult.py
+++ b/src/metadataserver/models/tests/test_scriptresult.py
@@ -1,4 +1,4 @@
-# Copyright 2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2017-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __all__ = []
@@ -19,6 +19,7 @@ from maasserver.models import (
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
 from maasserver.utils.orm import reload_object
+from maastesting.djangotestcase import CountQueries
 from maastesting.matchers import (
     DocTestMatches,
     MockCalledOnceWith,
@@ -28,7 +29,10 @@ from metadataserver.enum import (
     SCRIPT_STATUS,
     SCRIPT_STATUS_CHOICES,
 )
-from metadataserver.models import scriptresult as scriptresult_module
+from metadataserver.models import (
+    ScriptResult,
+    scriptresult as scriptresult_module,
+)
 from provisioningserver.events import EVENT_TYPES
 import yaml
 
@@ -624,6 +628,26 @@ class TestScriptResult(MAASServerTestCase):
         script_result = script_results[-1]
         self.assertItemsEqual(script_results, script_result.history)
 
+    def test_history_query_count(self):
+        script_name = factory.make_name('script_name')
+        script_set = factory.make_ScriptSet()
+        script_results = [
+            factory.make_ScriptResult(
+                script_name=script_name, script_set=script_set)
+            for _ in range(10)
+        ]
+        queries_one = CountQueries()
+        script_result_one = ScriptResult.objects.get(id=script_results[0].id)
+        with queries_one:
+            script_result_one.history
+        queries_many = CountQueries()
+        script_result_many = ScriptResult.objects.get(
+            id=script_results[-1].id)
+        with queries_many:
+            script_result_many.history
+        self.assertEquals(1, queries_one.num_queries)
+        self.assertEquals(1, queries_many.num_queries)
+
     def test_history_storage_device(self):
         # Regression test for LP: #1721524
         script = factory.make_Script()