Factor out sorting in the doc table (#10997)

Backports PR #10924

src/core_plugins/kibana/public/dashboard/panel/panel.html

@@ -65,6 +65,8 @@
     data-description="{{savedObj.description}}"
     render-counter
     class="panel-content"
-    filter="filter">
+    filter="filter"
+    on-change-sort-order="setSortOrder"
+    >
   </doc-table>
 </div>

src/core_plugins/kibana/public/dashboard/panel/panel.js

@@ -114,9 +114,10 @@ uiModules
             $scope.saveState();
           });
 
-          $scope.$watchCollection('panel.sort', function () {
+          $scope.setSortOrder = function setSortOrder(columnName, direction) {
+            $scope.panel.sort = [columnName, direction];
             $scope.saveState();
-          });
+          };
         }
 
         $scope.filter = function (field, value, operator) {

src/core_plugins/kibana/public/discover/controllers/discover.js

@@ -475,6 +475,10 @@ function discoverController($scope, config, courier, $route, $window, Notifier,
     .set('filter', queryFilter.getFilters());
   });
 
+  $scope.setSortOrder = function setSortOrder(columnName, direction) {
+    $scope.state.sort = [columnName, direction];
+  };
+
   // TODO: On array fields, negating does not negate the combination, rather all terms
   $scope.filterQuery = function (field, values, operation) {
     $scope.indexPattern.popularizeField(field, 1);

src/core_plugins/kibana/public/discover/index.html

@@ -131,8 +131,9 @@ <h2>Searching</h2>
                 shared-item
                 data-title="{{opts.savedSearch.lastSavedTitle}}"
                 data-description="{{opts.savedSearch.description}}"
-                render-counter>
-              </doc-table>
+                render-counter
+                on-change-sort-order="setSortOrder"
+              ></doc-table>
 
               <div ng-if="rows.length == opts.sampleSize" class="discover-table-footer">
                 <center>

src/ui/public/doc_table/__tests__/lib/rows_headers.js

@@ -8,6 +8,10 @@ import $ from 'jquery';
 import 'plugins/kibana/discover/index';
 import FixturesStubbedLogstashIndexPatternProvider from 'fixtures/stubbed_logstash_index_pattern';
 
+
+const SORTABLE_FIELDS = ['bytes', '@timestamp'];
+const UNSORTABLE_FIELDS = ['request_body'];
+
 describe('Doc Table', function () {
 
   let $parentScope;
@@ -89,16 +93,24 @@ describe('Doc Table', function () {
 
   describe('kbnTableHeader', function () {
 
-    const $elem = angular.element(
-      '<thead kbn-table-header columns="columns" index-pattern="indexPattern" sort="sort"></thead>'
-    );
+    const $elem = angular.element(`
+      <thead
+        kbn-table-header
+        columns="columns"
+        index-pattern="indexPattern"
+        sort-order="sortOrder"
+        on-change-sort-order="onChangeSortOrder"
+      ></thead>
+    `);
 
     beforeEach(function () {
       init($elem, {
         columns: [],
-        sorting: [],
+        sortOrder: [],
+        onChangeSortOrder: sinon.stub(),
       });
     });
+
     afterEach(function () {
       destroy();
     });
@@ -107,57 +119,76 @@ describe('Doc Table', function () {
       columnTests('th', $elem);
     });
 
+    describe('cycleSortOrder function', function () {
+      it('should exist', function () {
+        expect($scope.cycleSortOrder).to.be.a(Function);
+      });
 
-    describe('sorting', function () {
-      it('should have a sort function that sets the elements of the sort array', function (done) {
-        expect($scope.sort).to.be.a(Function);
-        done();
+      it('should call onChangeSortOrder with ascending order for a sortable field without sort order', function () {
+        $scope.sortOrder = [];
+        $scope.cycleSortOrder(SORTABLE_FIELDS[0]);
+        expect($scope.onChangeSortOrder.callCount).to.be(1);
+        expect($scope.onChangeSortOrder.firstCall.args).to.eql([SORTABLE_FIELDS[0], 'asc']);
       });
 
-      it('should have a headClasser function that determines the css classes of the sort icons', function (done) {
-        expect($scope.headerClass).to.be.a(Function);
-        done();
+      it('should call onChangeSortOrder with ascending order for a sortable field already sorted by in descending order', function () {
+        $scope.sortOrder = [SORTABLE_FIELDS[0], 'desc'];
+        $scope.cycleSortOrder(SORTABLE_FIELDS[0]);
+        expect($scope.onChangeSortOrder.callCount).to.be(1);
+        expect($scope.onChangeSortOrder.firstCall.args).to.eql([SORTABLE_FIELDS[0], 'asc']);
       });
 
-      it('should sort asc by default, then by desc if already sorting', function (done) {
-        const fields = ['bytes', '@timestamp'];
+      it('should call onChangeSortOrder with ascending order for a sortable field when already sorted by an different field', function () {
+        $scope.sortOrder = [SORTABLE_FIELDS[1], 'asc'];
+        $scope.cycleSortOrder(SORTABLE_FIELDS[0]);
+        expect($scope.onChangeSortOrder.callCount).to.be(1);
+        expect($scope.onChangeSortOrder.firstCall.args).to.eql([SORTABLE_FIELDS[0], 'asc']);
+      });
 
-        // Should not be sorted at first
-        expect($scope.sorting).to.eql(undefined);
-        expect($scope.headerClass(fields[0])).to.contain('fa-sort-up');
+      it('should call onChangeSortOrder with descending order for a sortable field already sorted by in ascending order', function () {
+        $scope.sortOrder = [SORTABLE_FIELDS[0], 'asc'];
+        $scope.cycleSortOrder(SORTABLE_FIELDS[0]);
+        expect($scope.onChangeSortOrder.callCount).to.be(1);
+        expect($scope.onChangeSortOrder.firstCall.args).to.eql([SORTABLE_FIELDS[0], 'desc']);
+      });
 
-        $scope.sort(fields[0]);
-        expect($scope.sorting).to.eql([fields[0], 'asc']);
-        expect($scope.headerClass(fields[0])).to.contain('fa-sort-up');
+      it('should not call onChangeSortOrder for an unsortable field', function () {
+        $scope.sortOrder = [];
+        $scope.cycleSortOrder(UNSORTABLE_FIELDS[0]);
+        expect($scope.onChangeSortOrder.callCount).to.be(0);
+      });
 
-        $scope.sort(fields[0]);
-        expect($scope.sorting).to.eql([fields[0], 'desc']);
-        expect($scope.headerClass(fields[0])).to.contain('fa-sort-down');
+      it('should not try to call onChangeSortOrder when it is not defined', function () {
+        $scope.onChangeSortOrder = undefined;
+        expect(() => $scope.cycleSortOrder(SORTABLE_FIELDS[0])).to.not.throwException();
+      });
+    });
 
-        $scope.sort(fields[0]);
-        expect($scope.sorting).to.eql([fields[0], 'asc']);
-        expect($scope.headerClass(fields[0])).to.contain('fa-sort-up');
+    describe('headerClass function', function () {
+      it('should exist', function () {
+        expect($scope.headerClass).to.be.a(Function);
+      });
 
-        $scope.sort(fields[1]);
-        expect($scope.sorting).to.eql([fields[1], 'asc']);
-        expect($scope.headerClass(fields[1])).to.contain('fa-sort-up');
+      it('should return list including table-header-sortchange for a sortable field not currently sorted by', function () {
+        expect($scope.headerClass(SORTABLE_FIELDS[0])).to.contain('table-header-sortchange');
+      });
 
-        // Should show the default sort for any other fields[0]
-        expect($scope.headerClass(fields[0])).to.contain('fa-sort-up');
+      it('should return undefined for an unsortable field', function () {
+        expect($scope.headerClass(UNSORTABLE_FIELDS[0])).to.be(undefined);
+      });
 
-        done();
+      it('should return list including fa-sort-up for a sortable field not currently sorted by', function () {
+        expect($scope.headerClass(SORTABLE_FIELDS[0])).to.contain('fa-sort-up');
       });
 
-      it('should NOT sort unindexed fields', function (done) {
-        $scope.sort('request_body');
-        expect($scope.sorting).to.be(undefined);
-        done();
+      it('should return list including fa-sort-up for a sortable field currently sorted by in ascending order', function () {
+        $scope.sortOrder = [SORTABLE_FIELDS[0], 'asc'];
+        expect($scope.headerClass(SORTABLE_FIELDS[0])).to.contain('fa-sort-up');
       });
 
-      it('should NOT sort geo_point fields', function (done) {
-        $scope.sort('point');
-        expect($scope.sorting).to.be(undefined);
-        done();
+      it('should return list including fa-sort-down for a sortable field currently sorted by in descending order', function () {
+        $scope.sortOrder = [SORTABLE_FIELDS[0], 'desc'];
+        expect($scope.headerClass(SORTABLE_FIELDS[0])).to.contain('fa-sort-down');
       });
     });
 

src/ui/public/doc_table/components/table_header.html

@@ -1,11 +1,11 @@
 <tr>
   <th width="1%"></th>
   <th ng-if="indexPattern.timeFieldName" data-test-subj="docTableHeaderField">
-    <span>Time <i ng-class="headerClass(indexPattern.timeFieldName)" ng-click="sort(indexPattern.timeFieldName)" tooltip="Sort by time"></i></span>
+    <span>Time <i ng-class="headerClass(indexPattern.timeFieldName)" ng-click="cycleSortOrder(indexPattern.timeFieldName)" tooltip="Sort by time"></i></span>
   </th>
   <th ng-repeat="name in columns" data-test-subj="docTableHeaderField">
     <span class="table-header-name">
-      {{name | shortDots}} <i ng-class="headerClass(name)" ng-click="sort(name)" tooltip="{{tooltip(name)}}" tooltip-append-to-body="1"></i>
+      {{name | shortDots}} <i ng-class="headerClass(name)" ng-click="cycleSortOrder(name)" tooltip="{{tooltip(name)}}" tooltip-append-to-body="1"></i>
     </span>
     <span class="table-header-move">
       <i ng-click="toggleColumn(name)" ng-show="canRemove(name)" class="fa fa-remove" tooltip="Remove column" tooltip-append-to-body="1"></i>

src/ui/public/doc_table/components/table_header.js

@@ -10,20 +10,22 @@ module.directive('kbnTableHeader', function (shortDotsFilter) {
     restrict: 'A',
     scope: {
       columns: '=',
-      sorting: '=',
+      sortOrder: '=',
       indexPattern: '=',
+      onChangeSortOrder: '=?',
     },
     template: headerHtml,
     controller: function ($scope) {
-
-      const sortableField = function (field) {
-        if (!$scope.indexPattern) return;
-        const sortable = _.get($scope.indexPattern.fields.byName[field], 'sortable');
-        return sortable;
+      const isSortableColumn = function isSortableColumn(columnName) {
+        return (
+          !!$scope.indexPattern
+          && _.isFunction($scope.onChangeSortOrder)
+          && _.get($scope, ['indexPattern', 'fields', 'byName', columnName, 'sortable'], false)
+        );
       };
 
       $scope.tooltip = function (column) {
-        if (!sortableField(column)) return '';
+        if (!isSortableColumn(column)) return '';
         return 'Sort by ' + shortDotsFilter(column);
       };
 
@@ -32,13 +34,13 @@ module.directive('kbnTableHeader', function (shortDotsFilter) {
       };
 
       $scope.headerClass = function (column) {
-        if (!sortableField(column)) return;
+        if (!isSortableColumn(column)) return;
 
-        const sorting = $scope.sorting;
+        const sortOrder = $scope.sortOrder;
         const defaultClass = ['fa', 'fa-sort-up', 'table-header-sortchange'];
 
-        if (!sorting || column !== sorting[0]) return defaultClass;
-        return ['fa', sorting[1] === 'asc' ? 'fa-sort-up' : 'fa-sort-down'];
+        if (!sortOrder || column !== sortOrder[0]) return defaultClass;
+        return ['fa', sortOrder[1] === 'asc' ? 'fa-sort-up' : 'fa-sort-down'];
       };
 
       $scope.moveLeft = function (column) {
@@ -59,20 +61,19 @@ module.directive('kbnTableHeader', function (shortDotsFilter) {
         _.toggleInOut($scope.columns, fieldName);
       };
 
-      $scope.sort = function (column) {
-        if (!column || !sortableField(column)) return;
-
-        const sorting = $scope.sorting = $scope.sorting || [];
-
-        let direction = sorting[1] || 'asc';
-        if (sorting[0] !== column) {
-          direction = 'asc';
-        } else {
-          direction = sorting[1] === 'asc' ? 'desc' : 'asc';
+      $scope.cycleSortOrder = function cycleSortOrder(columnName) {
+        if (!isSortableColumn(columnName)) {
+          return;
         }
 
-        $scope.sorting[0] = column;
-        $scope.sorting[1] = direction;
+        const [currentColumnName, currentDirection = 'asc'] = $scope.sortOrder;
+        const newDirection = (
+          (columnName === currentColumnName && currentDirection === 'asc')
+          ? 'desc'
+          : 'asc'
+        );
+
+        $scope.onChangeSortOrder(columnName, newDirection);
       };
     }
   };

src/ui/public/doc_table/doc_table.html

@@ -8,16 +8,19 @@
         kbn-table-header
         columns="columns"
         index-pattern="indexPattern"
-        sorting="sorting">
-      </thead>
+        sort-order="sorting"
+        on-change-sort-order="onChangeSortOrder"
+      ></thead>
       <tbody>
         <tr ng-repeat="row in page|limitTo:limit track by row._index+row._type+row._id+row._score+row._version"
           kbn-table-row="row"
           columns="columns"
           sorting="sorting"
           index-pattern="indexPattern"
           filter="filter"
-          class="discover-table-row"></tr>
+          class="discover-table-row"
+          on-change-sort-order="onChangeSortOrder"
+        ></tr>
       </tbody>
     </table>
   </paginate>
@@ -27,8 +30,9 @@
       kbn-table-header
       columns="columns"
       index-pattern="indexPattern"
-      sorting="sorting">
-    </thead>
+      sort-order="sorting"
+      on-change-sort-order="onChangeSortOrder"
+    ></thead>
     <tbody>
       <tr ng-repeat="row in hits|limitTo:limit track by row._index+row._type+row._id+row._score+row._version"
         kbn-table-row="row"
@@ -39,6 +43,7 @@
         class="discover-table-row"
         ng-class="{'discover-table-row--highlight': row['$$_isAnchor']}"
         data-test-subj="docTableRow{{ row['$$_isAnchor'] ? ' docTableAnchorRow' : ''}}"
+        on-change-sort-order="onChangeSortOrder"
       ></tr>
     </tbody>
   </table>

src/ui/public/doc_table/doc_table.js

@@ -23,6 +23,7 @@ uiModules.get('kibana')
       searchSource: '=?',
       infiniteScroll: '=?',
       filter: '=?',
+      onChangeSortOrder: '=?',
     },
     link: function ($scope) {
       const notify = new Notifier();