From 83d92251c54063f94a495e81fa31961b9644c794 Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Thu, 12 Dec 2019 13:06:54 -0600 Subject: [PATCH] UI: Fix client sorting (#6817) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 13 ++ ui/app/controllers/clients/index.js | 212 +++++++++--------- ui/app/mixins/sortable-factory.js | 58 +++++ ui/app/mixins/sortable.js | 33 +-- ui/app/models/node.js | 10 +- .../styles/components/node-status-light.scss | 3 +- ui/app/templates/clients/index.hbs | 2 +- .../templates/components/client-node-row.hbs | 10 +- ui/tests/acceptance/client-detail-test.js | 5 - ui/tests/acceptance/clients-list-test.js | 55 ++++- ui/tests/pages/clients/list.js | 16 +- 11 files changed, 252 insertions(+), 165 deletions(-) create mode 100644 ui/app/mixins/sortable-factory.js diff --git a/ui/app/components/client-node-row.js b/ui/app/components/client-node-row.js index 947ae6e9a823..07562358835e 100644 --- a/ui/app/components/client-node-row.js +++ b/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 ''; + } + }), }); diff --git a/ui/app/controllers/clients/index.js b/ui/app/controllers/clients/index.js index ea4bd6d2cc76..d37c23e5f2c7 100644 --- a/ui/app/controllers/clients/index.js +++ b/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'), - - 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; +export default Controller.extend( + SortableFactory(['id', 'name', 'compositeStatus', 'datacenter']), + Searchable, + { + clientsController: controller('clients'), + + 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))); }); - } - ), - listToSort: alias('filteredNodes'), - listToSearch: alias('listSorted'), - sortedNodes: alias('listSearched'), + 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' }, + ]), - isForbidden: alias('clientsController.isForbidden'), + 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))); + }); - setFacetQueryParam(queryParam, selection) { - this.set(queryParam, serialize(selection)); - }, + 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); + actions: { + gotoNode(node) { + this.transitionToRoute('clients.client', node); + }, }, - }, -}); + } +); diff --git a/ui/app/mixins/sortable-factory.js b/ui/app/mixins/sortable-factory.js new file mode 100644 index 000000000000..e65d18f9bc19 --- /dev/null +++ b/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; + } + ), + }); +} diff --git a/ui/app/mixins/sortable.js b/ui/app/mixins/sortable.js index 7a86d3c80d5b..95ebe0cb25a0 100644 --- a/ui/app/mixins/sortable.js +++ b/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); diff --git a/ui/app/models/node.js b/ui/app/models/node.js index 412548d3624b..c3efaff543bb 100644 --- a/ui/app/models/node.js +++ b/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; + } }), }); diff --git a/ui/app/styles/components/node-status-light.scss b/ui/app/styles/components/node-status-light.scss index 9077e333a8f0..50153ce2f7f7 100644 --- a/ui/app/styles/components/node-status-light.scss +++ b/ui/app/styles/components/node-status-light.scss @@ -27,7 +27,8 @@ $size: 0.75em; ); } - &.ineligible { + &.ineligible, + &.draining { background: $warning; } } diff --git a/ui/app/templates/clients/index.hbs b/ui/app/templates/clients/index.hbs index e7245c1bc943..f29105490dd7 100644 --- a/ui/app/templates/clients/index.hbs +++ b/ui/app/templates/clients/index.hbs @@ -49,7 +49,7 @@ {{#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}} Address {{#t.sort-by prop="datacenter"}}Datacenter{{/t.sort-by}} # Allocs diff --git a/ui/app/templates/components/client-node-row.hbs b/ui/app/templates/components/client-node-row.hbs index 35619aa19b77..bf7f5b848238 100644 --- a/ui/app/templates/components/client-node-row.hbs +++ b/ui/app/templates/components/client-node-row.hbs @@ -7,15 +7,9 @@ {{#link-to "clients.client" node.id class="is-primary"}}{{node.shortId}}{{/link-to}} {{node.name}} - + - {{#if node.isDraining}} - draining - {{else if (not node.isEligible)}} - ineligible - {{else}} - {{node.status}} - {{/if}} + {{node.compositeStatus}} {{node.httpAddr}} diff --git a/ui/tests/acceptance/client-detail-test.js b/ui/tests/acceptance/client-detail-test.js index 1be3287f38df..3fc65b40913c 100644 --- a/ui/tests/acceptance/client-detail-test.js +++ b/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) { diff --git a/ui/tests/acceptance/clients-list-test.js b/ui/tests/acceptance/clients-list-test.js index 409bf53ad3a5..4a1fe5514783 100644 --- a/ui/tests/acceptance/clients-list-test.js +++ b/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[3].state.text, 'ineligible'); - assert.ok(ClientsList.nodes[3].state.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[1].compositeStatus.text, 'initializing'); + assert.equal(ClientsList.nodes[2].compositeStatus.text, 'down'); + + assert.equal(ClientsList.nodes[3].compositeStatus.text, 'ineligible'); + assert.ok(ClientsList.nodes[3].compositeStatus.isWarning, 'expected warning 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) { diff --git a/ui/tests/pages/clients/list.js b/ui/tests/pages/clients/list.js index 7eb3c6c54b83..7a9c9b5421be 100644 --- a/ui/tests/pages/clients/list.js +++ b/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'),