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

[Blake Rouse]

Save search filters for nodes and devices when changing the current view in angular.

This will keep the current search when going back and forth between the node listing and node details view. Now if you refresh the page the filters will reset.

[Mike Pontillo (mpontillo)]

Looks generally good. I have a minor comment below about some code that looks to bit a bit boilerplate. I think we might be able to do better with some slight refactoring; right now it seems like we have objects and constants scattered about for the nodes and devices; maybe if a single object knew about all these, it would be cleaner?

review: Approve

=== modified file 'src/maasserver/static/js/angular/controllers/nodes_list.js'
--- src/maasserver/static/js/angular/controllers/nodes_list.js	2015-05-26 13:52:53 +0000
+++ src/maasserver/static/js/angular/controllers/nodes_list.js	2015-06-03 20:11:57 +0000
@@ -45,7 +45,7 @@
         $scope.tabs.nodes.predicate = 'fqdn';
         $scope.tabs.nodes.allViewableChecked = false;
         $scope.tabs.nodes.metadata = NodesManager.getMetadata();
-        $scope.tabs.nodes.filters = SearchService.emptyFilter;
+        $scope.tabs.nodes.filters = SearchService.getEmptyFilter();
         $scope.tabs.nodes.column = 'fqdn';
         $scope.tabs.nodes.actionOption = null;
         $scope.tabs.nodes.takeActionOptions = GeneralManager.getData(
@@ -75,7 +75,7 @@
         $scope.tabs.devices.predicate = 'fqdn';
         $scope.tabs.devices.allViewableChecked = false;
         $scope.tabs.devices.metadata = DevicesManager.getMetadata();
-        $scope.tabs.devices.filters = SearchService.emptyFilter;
+        $scope.tabs.devices.filters = SearchService.getEmptyFilter();
         $scope.tabs.devices.column = 'fqdn';
         $scope.tabs.devices.actionOption = null;
         $scope.tabs.devices.takeActionOptions = GeneralManager.getData(
@@ -289,7 +289,7 @@
             var filters = SearchService.getCurrentFilters(
                 $scope.tabs[tab].search);
             if(filters === null) {
-                $scope.tabs[tab].filters = SearchService.emptyFilter;
+                $scope.tabs[tab].filters = SearchService.getEmptyFilter();
                 $scope.tabs[tab].searchValid = false;
             } else {
                 $scope.tabs[tab].filters = filters;
@@ -477,11 +477,27 @@
                 $scope.loading = false;
             });
 
-        // Stop polling when the scope is destroyed.
+        // Stop polling and save the current filter when the scope is destroyed.
         $scope.$on("$destroy", function() {
             GeneralManager.stopPolling("osinfo");
+            SearchService.storeFilters("nodes", $scope.tabs.nodes.filters);
+            SearchService.storeFilters("devices", $scope.tabs.devices.filters);
         });
 
+        // Restore the filters if any saved.
+        var nodesFilter = SearchService.retrieveFilters("nodes");
+        if(angular.isObject(nodesFilter)) {
+            $scope.tabs.nodes.search = SearchService.filtersToString(
+                nodesFilter);
+            $scope.updateFilters("nodes");
+        }
+        var devicesFilter = SearchService.retrieveFilters("devices");
+        if(angular.isObject(devicesFilter)) {
+            $scope.tabs.devices.search = SearchService.filtersToString(
+                devicesFilter);
+            $scope.updateFilters("devices");
+        }
+
         // Switch to the specified tab, if specified.
         if($routeParams.tab === "nodes" || $routeParams.tab === "devices") {
             $scope.toggleTab($routeParams.tab);

=== modified file 'src/maasserver/static/js/angular/controllers/tests/test_nodes_list.js'
--- src/maasserver/static/js/angular/controllers/tests/test_nodes_list.js	2015-05-26 13:52:53 +0000
+++ src/maasserver/static/js/angular/controllers/tests/test_nodes_list.js	2015-06-03 20:11:57 +0000
@@ -149,6 +149,18 @@
             "osinfo");
     });
 
+    it("saves current filters for nodes and devices when scope destroyed",
+        function() {
+            var controller = makeController();
+            var nodesFilters = {}, devicesFilters = {};
+            $scope.tabs.nodes.filters = nodesFilters;
+            $scope.tabs.devices.filters = devicesFilters;
+            $scope.$destroy();
+            expect(SearchService.retrieveFilters("nodes")).toBe(nodesFilters);
+            expect(SearchService.retrieveFilters("devices")).toBe(
+                devicesFilters);
+        });
+
     it("calls loadManagers with NodesManager, DevicesManager," +
         "GeneralManager, UsersManager",
         function() {
@@ -166,6 +178,24 @@
         expect($scope.loading).toBe(false);
     });
 
+    it("sets nodes search from SearchService",
+        function() {
+            var query = makeName("query");
+            SearchService.storeFilters(
+                "nodes", SearchService.getCurrentFilters(query));
+            var controller = makeController();
+            expect($scope.tabs.nodes.search).toBe(query);
+        });
+
+    it("sets devices search from SearchService",
+        function() {
+            var query = makeName("query");
+            SearchService.storeFilters(
+                "devices", SearchService.getCurrentFilters(query));
+            var controller = makeController();
+            expect($scope.tabs.devices.search).toBe(query);
+        });
+
     it("sets nodes search from $routeParams.query",
         function() {
             var query = makeName("query");
@@ -228,7 +258,8 @@
                 expect(tabScope.selectedItems).toBe(
                     tabScope.manager.getSelectedItems());
                 expect(tabScope.metadata).toBe(tabScope.manager.getMetadata());
-                expect(tabScope.filters).toBe(SearchService.emptyFilter);
+                expect(tabScope.filters).toEqual(
+                    SearchService.getEmptyFilter());
                 expect(tabScope.column).toBe("fqdn");
                 expect(tabScope.actionOption).toBeNull();
                 expect(tabScope.takeActionOptions).toEqual([]);
@@ -440,7 +471,7 @@
                 it("calls SearchService.toggleFilter", function() {
                     var controller = makeController();
                     spyOn(SearchService, "toggleFilter").and.returnValue(
-                        SearchService.emptyFilter);
+                        SearchService.getEmptyFilter());
                     $scope.toggleFilter("hostname", "test", tab);
                     expect(SearchService.toggleFilter).toHaveBeenCalled();
                 });
@@ -507,8 +538,8 @@
                         $scope.tabs[tab].search = "test hostname:(name";
                         $scope.updateFilters(tab);
                         expect(
-                            $scope.tabs[tab].filters).toBe(
-                                SearchService.emptyFilter);
+                            $scope.tabs[tab].filters).toEqual(
+                                SearchService.getEmptyFilter());
                         expect($scope.tabs[tab].searchValid).toBe(false);
                     });
             });

=== modified file 'src/maasserver/static/js/angular/services/search.js'
--- src/maasserver/static/js/angular/services/search.js	2015-01-29 21:35:52 +0000
+++ src/maasserver/static/js/angular/services/search.js	2015-06-03 20:11:57 +0000
@@ -6,13 +6,21 @@
 
 angular.module('MAAS').service('SearchService', function() {
 
+    // Holds an empty filter object.
+    var emptyFilter = { _: [] };
+
+    // Return a new empty filter;
+    this.getEmptyFilter = function() {
+        return angular.copy(emptyFilter);
+    };
+
     // Splits the search string into different terms based on white space.
     // This handles the ability for whitespace to be inside of '(', ')'.
     //
     // XXX blake_r 28-01-15: This could be improved with a regex, but was
     // unable to come up with one that would allow me to validate the end
     // ')' in the string.
-    function getSplitSearch(search) {
+    this.getSplitSearch = function(search) {
         var terms = search.split(' ');
         var fixedTerms = [];
         var spanningParentheses = false;
@@ -46,15 +54,15 @@
             return null;
         }
         return fixedTerms;
-    }
+    };
 
     // Return all of the currently active filters for the given search.
-    function getCurrentFilters(search) {
-        var filters = { _: [] };
+    this.getCurrentFilters = function(search) {
+        var filters = this.getEmptyFilter();
         if(search.length === 0) {
             return filters;
         }
-        var searchTerms = getSplitSearch(search);
+        var searchTerms = this.getSplitSearch(search);
         if(!searchTerms) {
             return null;
         }
@@ -94,10 +102,10 @@
             }
         });
         return filters;
-    }
+    };
 
     // Convert "filters" into a search string.
-    function filtersToString(filters) {
+    this.filtersToString = function(filters) {
         var search = "";
         angular.forEach(filters, function(terms, type) {
             // Skip empty and skip "_" as it gets appended at the
@@ -111,19 +119,19 @@
             search = filters._.join(" ") + " " + search;
         }
         return search.trim();
-    }
+    };
 
     // Return true if the type and value are in the filters.
-    function isFilterActive(filters, type, value) {
+    this.isFilterActive = function(filters, type, value) {
         var values = filters[type];
         if(angular.isUndefined(values)) {
             return false;
         }
         return values.indexOf(value) !== -1;
-    }
+    };
 
     // Toggles a filter on or off based on type and value.
-    function toggleFilter(filters, type, value) {
+    this.toggleFilter = function(filters, type, value) {
         if(angular.isUndefined(filters[type])) {
             filters[type] = [];
         }
@@ -134,14 +142,18 @@
             filters[type].splice(idx, 1);
         }
         return filters;
-    }
-
-    return {
-        emptyFilter: { _: [] },
-        getSplitSearch: getSplitSearch,
-        getCurrentFilters: getCurrentFilters,
-        filtersToString: filtersToString,
-        isFilterActive: isFilterActive,
-        toggleFilter: toggleFilter
+    };
+
+    // Holds all stored filters.
+    var storedFilters = {};
+
+    // Store a filter for later.
+    this.storeFilters = function(name, filters) {
+        storedFilters[name] = filters;
+    };
+
+    // Retrieve a stored fitler.
+    this.retrieveFilters = function(name) {
+        return storedFilters[name];
     };
 });

=== modified file 'src/maasserver/static/js/angular/services/tests/test_search.js'
--- src/maasserver/static/js/angular/services/tests/test_search.js	2015-01-29 21:35:52 +0000
+++ src/maasserver/static/js/angular/services/tests/test_search.js	2015-06-03 20:11:57 +0000
@@ -146,10 +146,33 @@
         });
     });
 
-    describe("emptyFilter", function() {
+    describe("getEmptyFilter", function() {
 
         it("includes _ empty list", function() {
-            expect(SearchService.emptyFilter).toEqual({ _: [] });
+            expect(SearchService.getEmptyFilter()).toEqual({ _: [] });
+        });
+
+        it("returns different object on each call", function() {
+            var one = SearchService.getEmptyFilter();
+            var two = SearchService.getEmptyFilter();
+            expect(one).not.toBe(two);
+        });
+    });
+
+    describe("storeFilters/retrieveFilters", function() {
+
+        it("stores and retrieves the same object", function() {
+            var i, names = [], objects = [];
+            for(i = 0; i < 3; i++) {
+                names.push(makeName("name"));
+                objects.push({});
+            }
+            angular.forEach(names, function(name, idx) {
+                SearchService.storeFilters(name, objects[idx]);
+            });
+            angular.forEach(names, function(name, idx) {
+                expect(SearchService.retrieveFilters(name)).toBe(objects[idx]);
+            });
         });
     });
 });