Skip to content

Commit

Permalink
UI: Fix client sorting (#6817)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
backspace committed Dec 12, 2019
1 parent 630723c commit 83d9225
Show file tree
Hide file tree
Showing 11 changed files with 252 additions and 165 deletions.
13 changes: 13 additions & 0 deletions ui/app/components/client-node-row.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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 '';
}
}),
});
212 changes: 108 additions & 104 deletions ui/app/controllers/clients/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
},
},
},
});
}
);
58 changes: 58 additions & 0 deletions ui/app/mixins/sortable-factory.js
Original file line number Diff line number Diff line change
@@ -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;
}
),
});
}
33 changes: 3 additions & 30 deletions ui/app/mixins/sortable.js
Original file line number Diff line number Diff line change
@@ -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);
10 changes: 8 additions & 2 deletions ui/app/models/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}),
});
3 changes: 2 additions & 1 deletion ui/app/styles/components/node-status-light.scss
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ $size: 0.75em;
);
}

&.ineligible {
&.ineligible,
&.draining {
background: $warning;
}
}
2 changes: 1 addition & 1 deletion ui/app/templates/clients/index.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
10 changes: 2 additions & 8 deletions ui/app/templates/components/client-node-row.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
Loading

0 comments on commit 83d9225

Please sign in to comment.