Merge pull request #1940 from lukasolson/issue-1933

Don't allow sorting on unsortable field types
elastic · Nov 19, 2014 · bb906f0 · bb906f0
2 parents b8a068a + 1baa52c
commit bb906f0
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 15 deletions.
diff --git a/src/kibana/plugins/discover/directives/table_header.js b/src/kibana/plugins/discover/directives/table_header.js
@@ -14,9 +14,14 @@ define(function (require) {
       },
       template: headerHtml,
       controller: function ($scope) {
+        var unsortableFields = ['geo_point', 'geo_shape', 'attachment'];
+        var sortableField = function (field) {
+          var mapping = $scope.mapping[field];
+          return mapping && mapping.indexed && !_.contains(unsortableFields, mapping.type);
+        };
+
         $scope.headerClass = function (column) {
-          if (!$scope.mapping) return;
-          if ($scope.mapping[column] && !$scope.mapping[column].indexed) return;
+          if (!sortableField(column)) return;
 
           var sorting = $scope.sorting;
           var defaultClass = ['fa', 'fa-sort', 'table-header-sortchange'];
@@ -45,7 +50,7 @@ define(function (require) {
         };
 
         $scope.sort = function (column) {
-          if ($scope.mapping[column] && !$scope.mapping[column].indexed) return;
+          if (!sortableField(column)) return;
           var sorting = $scope.sorting || [];
           $scope.sorting = [column, sorting[1] === 'asc' ? 'desc' : 'asc'];
         };

diff --git a/test/unit/specs/apps/discover/directives/table.js b/test/unit/specs/apps/discover/directives/table.js
@@ -13,7 +13,7 @@ define(function (require) {
 
   var $parentScope, $scope, config;
 
-  // Stub out a minimal mapping of 3 fields
+  // Stub out a minimal mapping of 4 fields
   var mapping = {
     bytes: {
       indexed: true,
@@ -26,6 +26,10 @@ define(function (require) {
     timestamp: {
       indexed: true,
       type: 'date'
+    },
+    geo: {
+      indexed: true,
+      type: 'geo_point'
     }
   };
 
@@ -156,6 +160,11 @@ define(function (require) {
           done();
         });
 
+        it('should NOT sort geo_point fields', function (done) {
+          $scope.sort('geo');
+          expect($scope.sorting).to.be(undefined);
+          done();
+        });
       });
 
       describe('moving columns', function () {
@@ -174,10 +183,10 @@ define(function (require) {
         });
 
         it('shouldnt move the last column to the right', function () {
-          expect($scope.columns[2]).to.be('timestamp');
+          expect($scope.columns[3]).to.be('geo');
 
-          $scope.moveRight('timestamp');
-          expect($scope.columns[2]).to.be('timestamp');
+          $scope.moveRight('geo');
+          expect($scope.columns[3]).to.be('geo');
         });
 
         it('should move columns to the left', function () {
@@ -334,13 +343,7 @@ define(function (require) {
           it('should have a row for each field', function () {
             var rows = $details.find('tr');
             var row = $scope.row;
-            expect($details.find('tr').length).to.be(3);
-          });
-
-          it('should have a row for each field', function () {
-            var rows = $details.find('tr');
-            var row = $scope.row;
-            expect($details.find('tr').length).to.be(3);
+            expect($details.find('tr').length).to.be(_.keys(mapping).length);
           });
 
           describe('filtering', function () {
@@ -397,7 +400,7 @@ define(function (require) {
       });
 
       it('should render even when the row source contains a field with the same name as a meta field', function () {
-        expect($details.find('tr').length).to.be(4);
+        expect($details.find('tr').length).to.be(_.keys(mapping).length);
       });
     });