UI: Fix client sorting (#6817)

There are two changes here, and some caveats/commentary:

1. The “State“ table column was actually sorting only by status. The state was not an actual property, just something calculated in each client row, as a product of status, isEligible, and isDraining. This PR adds isDraining as a component of compositeState so it can be used for sorting.

2. The Sortable mixin declares dependent keys that cause the sort to be live-updating, but only if the members of the array change, such as if a new client is added, but not if any of the sortable properties change. This PR adds a SortableFactory function that generates a mixin whose listSorted computed property includes dependent keys for the sortable properties, so the table will live-update if any of the sortable properties change, not just the array members. There’s a warning if you use SortableFactory without dependent keys and via the original Sortable interface, so we can eventually migrate away from it.

ui/app/components/client-node-row.js

@@ -3,6 +3,7 @@ import Component from '@ember/component';
 import { lazyClick } from '../helpers/lazy-click';
 import { watchRelationship } from 'nomad-ui/utils/properties/watch';
 import WithVisibilityDetection from 'nomad-ui/mixins/with-component-visibility-detection';
+import { computed } from '@ember/object';
 
 export default Component.extend(WithVisibilityDetection, {
   store: service(),
@@ -45,4 +46,16 @@ export default Component.extend(WithVisibilityDetection, {
   },
 
   watch: watchRelationship('allocations'),
+
+  compositeStatusClass: computed('node.compositeStatus', function() {
+    let compositeStatus = this.get('node.compositeStatus');
+
+    if (compositeStatus === 'draining') {
+      return 'status-text is-info';
+    } else if (compositeStatus === 'ineligible') {
+      return 'status-text is-warning';
+    } else {
+      return '';
+    }
+  }),
 });

ui/app/controllers/clients/index.js

@@ -3,116 +3,120 @@ import Controller, { inject as controller } from '@ember/controller';
 import { computed } from '@ember/object';
 import { scheduleOnce } from '@ember/runloop';
 import intersection from 'lodash.intersection';
-import Sortable from 'nomad-ui/mixins/sortable';
+import SortableFactory from 'nomad-ui/mixins/sortable-factory';
 import Searchable from 'nomad-ui/mixins/searchable';
 import { serialize, deserializedQueryParam as selection } from 'nomad-ui/utils/qp-serialize';
 
-export default Controller.extend(Sortable, Searchable, {
-  clientsController: controller('clients'),
+export default Controller.extend(
+  SortableFactory(['id', 'name', 'compositeStatus', 'datacenter']),
+  Searchable,
+  {
+    clientsController: controller('clients'),
 
-  nodes: alias('model.nodes'),
-  agents: alias('model.agents'),
+    nodes: alias('model.nodes'),
+    agents: alias('model.agents'),
 
-  queryParams: {
-    currentPage: 'page',
-    searchTerm: 'search',
-    sortProperty: 'sort',
-    sortDescending: 'desc',
-    qpClass: 'class',
-    qpState: 'state',
-    qpDatacenter: 'dc',
-  },
-
-  currentPage: 1,
-  pageSize: 8,
-
-  sortProperty: 'modifyIndex',
-  sortDescending: true,
-
-  searchProps: computed(() => ['id', 'name', 'datacenter']),
-
-  qpClass: '',
-  qpState: '',
-  qpDatacenter: '',
-
-  selectionClass: selection('qpClass'),
-  selectionState: selection('qpState'),
-  selectionDatacenter: selection('qpDatacenter'),
-
-  optionsClass: computed('nodes.[]', function() {
-    const classes = Array.from(new Set(this.nodes.mapBy('nodeClass'))).compact();
-
-    // Remove any invalid node classes from the query param/selection
-    scheduleOnce('actions', () => {
-      this.set('qpClass', serialize(intersection(classes, this.selectionClass)));
-    });
-
-    return classes.sort().map(dc => ({ key: dc, label: dc }));
-  }),
-
-  optionsState: computed(() => [
-    { key: 'initializing', label: 'Initializing' },
-    { key: 'ready', label: 'Ready' },
-    { key: 'down', label: 'Down' },
-    { key: 'ineligible', label: 'Ineligible' },
-    { key: 'draining', label: 'Draining' },
-  ]),
-
-  optionsDatacenter: computed('nodes.[]', function() {
-    const datacenters = Array.from(new Set(this.nodes.mapBy('datacenter'))).compact();
-
-    // Remove any invalid datacenters from the query param/selection
-    scheduleOnce('actions', () => {
-      this.set('qpDatacenter', serialize(intersection(datacenters, this.selectionDatacenter)));
-    });
-
-    return datacenters.sort().map(dc => ({ key: dc, label: dc }));
-  }),
-
-  filteredNodes: computed(
-    'nodes.[]',
-    'selectionClass',
-    'selectionState',
-    'selectionDatacenter',
-    function() {
-      const {
-        selectionClass: classes,
-        selectionState: states,
-        selectionDatacenter: datacenters,
-      } = this;
-
-      const onlyIneligible = states.includes('ineligible');
-      const onlyDraining = states.includes('draining');
-
-      // states is a composite of node status and other node states
-      const statuses = states.without('ineligible').without('draining');
-
-      return this.nodes.filter(node => {
-        if (classes.length && !classes.includes(node.get('nodeClass'))) return false;
-        if (statuses.length && !statuses.includes(node.get('status'))) return false;
-        if (datacenters.length && !datacenters.includes(node.get('datacenter'))) return false;
-
-        if (onlyIneligible && node.get('isEligible')) return false;
-        if (onlyDraining && !node.get('isDraining')) return false;
-
-        return true;
-      });
-    }
-  ),
-
-  listToSort: alias('filteredNodes'),
-  listToSearch: alias('listSorted'),
-  sortedNodes: alias('listSearched'),
-
-  isForbidden: alias('clientsController.isForbidden'),
-
-  setFacetQueryParam(queryParam, selection) {
-    this.set(queryParam, serialize(selection));
-  },
-
-  actions: {
-    gotoNode(node) {
-      this.transitionToRoute('clients.client', node);
+    queryParams: {
+      currentPage: 'page',
+      searchTerm: 'search',
+      sortProperty: 'sort',
+      sortDescending: 'desc',
+      qpClass: 'class',
+      qpState: 'state',
+      qpDatacenter: 'dc',
     },
-  },
-});
+
+    currentPage: 1,
+    pageSize: 8,
+
+    sortProperty: 'modifyIndex',
+    sortDescending: true,
+
+    searchProps: computed(() => ['id', 'name', 'datacenter']),
+
+    qpClass: '',
+    qpState: '',
+    qpDatacenter: '',
+
+    selectionClass: selection('qpClass'),
+    selectionState: selection('qpState'),
+    selectionDatacenter: selection('qpDatacenter'),
+
+    optionsClass: computed('nodes.[]', function() {
+      const classes = Array.from(new Set(this.nodes.mapBy('nodeClass'))).compact();
+
+      // Remove any invalid node classes from the query param/selection
+      scheduleOnce('actions', () => {
+        this.set('qpClass', serialize(intersection(classes, this.selectionClass)));
+      });
+
+      return classes.sort().map(dc => ({ key: dc, label: dc }));
+    }),
+
+    optionsState: computed(() => [
+      { key: 'initializing', label: 'Initializing' },
+      { key: 'ready', label: 'Ready' },
+      { key: 'down', label: 'Down' },
+      { key: 'ineligible', label: 'Ineligible' },
+      { key: 'draining', label: 'Draining' },
+    ]),
+
+    optionsDatacenter: computed('nodes.[]', function() {
+      const datacenters = Array.from(new Set(this.nodes.mapBy('datacenter'))).compact();
+
+      // Remove any invalid datacenters from the query param/selection
+      scheduleOnce('actions', () => {
+        this.set('qpDatacenter', serialize(intersection(datacenters, this.selectionDatacenter)));
+      });
+
+      return datacenters.sort().map(dc => ({ key: dc, label: dc }));
+    }),
+
+    filteredNodes: computed(
+      'nodes.[]',
+      'selectionClass',
+      'selectionState',
+      'selectionDatacenter',
+      function() {
+        const {
+          selectionClass: classes,
+          selectionState: states,
+          selectionDatacenter: datacenters,
+        } = this;
+
+        const onlyIneligible = states.includes('ineligible');
+        const onlyDraining = states.includes('draining');
+
+        // states is a composite of node status and other node states
+        const statuses = states.without('ineligible').without('draining');
+
+        return this.nodes.filter(node => {
+          if (classes.length && !classes.includes(node.get('nodeClass'))) return false;
+          if (statuses.length && !statuses.includes(node.get('status'))) return false;
+          if (datacenters.length && !datacenters.includes(node.get('datacenter'))) return false;
+
+          if (onlyIneligible && node.get('isEligible')) return false;
+          if (onlyDraining && !node.get('isDraining')) return false;
+
+          return true;
+        });
+      }
+    ),
+
+    listToSort: alias('filteredNodes'),
+    listToSearch: alias('listSorted'),
+    sortedNodes: alias('listSearched'),
+
+    isForbidden: alias('clientsController.isForbidden'),
+
+    setFacetQueryParam(queryParam, selection) {
+      this.set(queryParam, serialize(selection));
+    },
+
+    actions: {
+      gotoNode(node) {
+        this.transitionToRoute('clients.client', node);
+      },
+    },
+  }
+);

ui/app/mixins/sortable-factory.js

@@ -0,0 +1,58 @@
+import Mixin from '@ember/object/mixin';
+import Ember from 'ember';
+import { computed } from '@ember/object';
+import { warn } from '@ember/debug';
+
+/**
+  Sortable mixin factory
+
+  Simple sorting behavior for a list of objects. Pass the list of properties
+  you want the list to be live-sorted based on, or use the generic sortable.js
+  if you don’t need that.
+
+  Properties to override:
+    - sortProperty: the property to sort by
+    - sortDescending: when true, the list is reversed
+    - listToSort: the list of objects to sort
+
+  Properties provided:
+    - listSorted: a copy of listToSort that has been sorted
+*/
+export default function sortableFactory(properties, fromSortableMixin) {
+  const eachProperties = properties.map(property => `listToSort.@each.${property}`);
+
+  return Mixin.create({
+    // Override in mixin consumer
+    sortProperty: null,
+    sortDescending: true,
+    listToSort: computed(() => []),
+
+    _sortableFactoryWarningPrinted: false,
+
+    listSorted: computed(
+      ...eachProperties,
+      'listToSort.[]',
+      'sortProperty',
+      'sortDescending',
+      function() {
+        if (!this._sortableFactoryWarningPrinted && !Ember.testing) {
+          let message =
+            'Using SortableFactory without property keys means the list will only sort when the members change, not when any of their properties change.';
+
+          if (fromSortableMixin) {
+            message += ' The Sortable mixin is deprecated in favor of SortableFactory.';
+          }
+
+          warn(message, properties.length > 0, { id: 'nomad.no-sortable-properties' });
+          this.set('_sortableFactoryWarningPrinted', true);
+        }
+
+        const sorted = this.listToSort.compact().sortBy(this.sortProperty);
+        if (this.sortDescending) {
+          return sorted.reverse();
+        }
+        return sorted;
+      }
+    ),
+  });
+}

ui/app/mixins/sortable.js

@@ -1,32 +1,5 @@
-import Mixin from '@ember/object/mixin';
-import { computed } from '@ember/object';
+import SortableFactory from 'nomad-ui/mixins/sortable-factory';
 
-/**
-  Sortable mixin
+// A generic version of SortableFactory with no sort property dependent keys.
 
-  Simple sorting behavior for a list of objects.
-
-  Properties to override:
-    - sortProperty: the property to sort by
-    - sortDescending: when true, the list is reversed
-    - listToSort: the list of objects to sort
-
-  Properties provided:
-    - listSorted: a copy of listToSort that has been sorted
-*/
-export default Mixin.create({
-  // Override in mixin consumer
-  sortProperty: null,
-  sortDescending: true,
-  listToSort: computed(() => []),
-
-  listSorted: computed('listToSort.[]', 'sortProperty', 'sortDescending', function() {
-    const sorted = this.listToSort
-      .compact()
-      .sortBy(this.sortProperty);
-    if (this.sortDescending) {
-      return sorted.reverse();
-    }
-    return sorted;
-  }),
-});
+export default SortableFactory([], true);

ui/app/models/node.js

@@ -61,7 +61,13 @@ export default Model.extend({
 
   // A status attribute that includes states not included in node status.
   // Useful for coloring and sorting nodes
-  compositeStatus: computed('status', 'isEligible', function() {
-    return this.isEligible ? this.status : 'ineligible';
+  compositeStatus: computed('isDraining', 'isEligible', 'status', function() {
+    if (this.isDraining) {
+      return 'draining';
+    } else if (!this.isEligible) {
+      return 'ineligible';
+    } else {
+      return this.status;
+    }
   }),
 });

ui/app/styles/components/node-status-light.scss

@@ -27,7 +27,8 @@ $size: 0.75em;
     );
   }
 
-  &.ineligible {
+  &.ineligible,
+  &.draining {
     background: $warning;
   }
 }

ui/app/templates/clients/index.hbs

@@ -49,7 +49,7 @@
             <th class="is-narrow"></th>
             {{#t.sort-by prop="id"}}ID{{/t.sort-by}}
             {{#t.sort-by class="is-200px is-truncatable" prop="name"}}Name{{/t.sort-by}}
-            {{#t.sort-by prop="status"}}State{{/t.sort-by}}
+            {{#t.sort-by prop="compositeStatus"}}State{{/t.sort-by}}
             <th>Address</th>
             {{#t.sort-by prop="datacenter"}}Datacenter{{/t.sort-by}}
             <th># Allocs</th>

ui/app/templates/components/client-node-row.hbs

@@ -7,15 +7,9 @@
 </td>
 <td data-test-client-id>{{#link-to "clients.client" node.id class="is-primary"}}{{node.shortId}}{{/link-to}}</td>
 <td data-test-client-name class="is-200px is-truncatable" title="{{node.name}}">{{node.name}}</td>
-<td data-test-client-state>
+<td data-test-client-composite-status>
   <span class="tooltip" aria-label="{{node.status}} / {{if node.isDraining "draining" "not draining"}} / {{if node.isEligible "eligible" "not eligible"}}">
-    {{#if node.isDraining}}
-      <span class="status-text is-info">draining</span>
-    {{else if (not node.isEligible)}}
-      <span class="status-text is-warning">ineligible</span>
-    {{else}}
-      {{node.status}}
-    {{/if}}
+    <span class="{{compositeStatusClass}}">{{node.compositeStatus}}</span>
   </span>
 </td>
 <td data-test-client-address>{{node.httpAddr}}</td>

ui/tests/acceptance/client-detail-test.js

@@ -59,11 +59,6 @@ module('Acceptance | client detail', function(hooks) {
 
     assert.ok(ClientDetail.title.includes(node.name), 'Title includes name');
     assert.ok(ClientDetail.title.includes(node.id), 'Title includes id');
-    assert.equal(
-      ClientDetail.statusLight.objectAt(0).id,
-      node.status,
-      'Title includes status light'
-    );
   });
 
   test('/clients/:id should list additional detail for the node below the title', async function(assert) {

ui/tests/acceptance/clients-list-test.js

@@ -1,4 +1,4 @@
-import { currentURL } from '@ember/test-helpers';
+import { currentURL, settled } from '@ember/test-helpers';
 import { module, test } from 'qunit';
 import { setupApplicationTest } from 'ember-qunit';
 import { setupMirage } from 'ember-cli-mirage/test-support';
@@ -41,13 +41,17 @@ module('Acceptance | clients list', function(hooks) {
 
     assert.equal(nodeRow.id, node.id.split('-')[0], 'ID');
     assert.equal(nodeRow.name, node.name, 'Name');
-    assert.equal(nodeRow.state.text, 'draining', 'Combined status, draining, and eligbility');
+    assert.equal(
+      nodeRow.compositeStatus.text,
+      'draining',
+      'Combined status, draining, and eligbility'
+    );
     assert.equal(nodeRow.address, node.httpAddr);
     assert.equal(nodeRow.datacenter, node.datacenter, 'Datacenter');
     assert.equal(nodeRow.allocations, allocations.length, '# Allocations');
   });
 
-  test('client status, draining, and eligibility are collapsed into one column', async function(assert) {
+  test('client status, draining, and eligibility are collapsed into one column that stays sorted', async function(assert) {
     server.createList('agent', 1);
 
     server.create('node', {
@@ -81,20 +85,47 @@ module('Acceptance | clients list', function(hooks) {
 
     await ClientsList.visit();
 
-    ClientsList.nodes[0].state.as(readyClient => {
+    ClientsList.nodes[0].compositeStatus.as(readyClient => {
       assert.equal(readyClient.text, 'ready');
       assert.ok(readyClient.isUnformatted, 'expected no status class');
       assert.equal(readyClient.tooltip, 'ready / not draining / eligible');
     });
 
-    assert.equal(ClientsList.nodes[1].state.text, 'initializing');
-    assert.equal(ClientsList.nodes[2].state.text, 'down');
+    assert.equal(ClientsList.nodes[1].compositeStatus.text, 'initializing');
+    assert.equal(ClientsList.nodes[2].compositeStatus.text, 'down');
 
-    assert.equal(ClientsList.nodes[3].state.text, 'ineligible');
-    assert.ok(ClientsList.nodes[3].state.isWarning, 'expected warning class');
+    assert.equal(ClientsList.nodes[3].compositeStatus.text, 'ineligible');
+    assert.ok(ClientsList.nodes[3].compositeStatus.isWarning, 'expected warning class');
 
-    assert.equal(ClientsList.nodes[4].state.text, 'draining');
-    assert.ok(ClientsList.nodes[4].state.isInfo, 'expected info class');
+    assert.equal(ClientsList.nodes[4].compositeStatus.text, 'draining');
+    assert.ok(ClientsList.nodes[4].compositeStatus.isInfo, 'expected info class');
+
+    await ClientsList.sortBy('compositeStatus');
+
+    assert.deepEqual(ClientsList.nodes.mapBy('compositeStatus.text'), [
+      'ready',
+      'initializing',
+      'ineligible',
+      'draining',
+      'down',
+    ]);
+
+    // Simulate a client state change arriving through polling
+    let readyClient = this.owner
+      .lookup('service:store')
+      .peekAll('node')
+      .findBy('modifyIndex', 4);
+    readyClient.set('schedulingEligibility', 'ineligible');
+
+    await settled();
+
+    assert.deepEqual(ClientsList.nodes.mapBy('compositeStatus.text'), [
+      'initializing',
+      'ineligible',
+      'ineligible',
+      'draining',
+      'down',
+    ]);
   });
 
   test('each client should link to the client detail page', async function(assert) {

ui/tests/pages/clients/list.js

@@ -18,12 +18,24 @@ export default create({
 
   search: fillable('.search-box input'),
 
+  sortOptions: collection('[data-test-sort-by]', {
+    id: attribute('data-test-sort-by'),
+    sort: clickable(),
+  }),
+
+  sortBy(id) {
+    return this.sortOptions
+      .toArray()
+      .findBy('id', id)
+      .sort();
+  },
+
   nodes: collection('[data-test-client-node-row]', {
     id: text('[data-test-client-id]'),
     name: text('[data-test-client-name]'),
 
-    state: {
-      scope: '[data-test-client-state]',
+    compositeStatus: {
+      scope: '[data-test-client-composite-status]',
 
       tooltip: attribute('aria-label', '.tooltip'),