UI: Faceted search on the jobs list page #5236
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,9 +2,28 @@ import { inject as service } from '@ember/service'; | |
| import { alias } from '@ember/object/computed'; | ||
| 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 Searchable from 'nomad-ui/mixins/searchable'; | ||
|
|
||
| // An unattractive but robust way to encode query params | ||
| const qpSerialize = arr => (arr.length ? JSON.stringify(arr) : ''); | ||
| const qpDeserialize = str => { | ||
| try { | ||
| return JSON.parse(str) | ||
| .compact() | ||
| .without(''); | ||
| } catch (e) { | ||
| return []; | ||
| } | ||
| }; | ||
|
|
||
| const qpSelection = qpKey => | ||
| computed(qpKey, function() { | ||
| return qpDeserialize(this.get(qpKey)); | ||
| }); | ||
|
|
||
| export default Controller.extend(Sortable, Searchable, { | ||
| system: service(), | ||
| jobsController: controller('jobs'), | ||
|
|
@@ -16,6 +35,10 @@ export default Controller.extend(Sortable, Searchable, { | |
| searchTerm: 'search', | ||
| sortProperty: 'sort', | ||
| sortDescending: 'desc', | ||
| qpType: 'type', | ||
| qpStatus: 'status', | ||
| qpDatacenter: 'dc', | ||
| qpPrefix: 'prefix', | ||
| }, | ||
|
|
||
| currentPage: 1, | ||
|
|
@@ -28,11 +51,95 @@ export default Controller.extend(Sortable, Searchable, { | |
| fuzzySearchProps: computed(() => ['name']), | ||
| fuzzySearchEnabled: true, | ||
|
|
||
| optionsType: computed(() => [ | ||
| { key: 'batch', label: 'Batch' }, | ||
| { key: 'parameterized', label: 'Parameterized' }, | ||
| { key: 'periodic', label: 'Periodic' }, | ||
| { key: 'service', label: 'Service' }, | ||
| { key: 'system', label: 'System' }, | ||
| ]), | ||
|
|
||
| optionsStatus: computed(() => [ | ||
| { key: 'pending', label: 'Pending' }, | ||
| { key: 'running', label: 'Running' }, | ||
| { key: 'dead', label: 'Dead' }, | ||
| ]), | ||
|
|
||
| optionsDatacenter: computed('visibleJobs.[]', function() { | ||
| const flatten = (acc, val) => acc.concat(val); | ||
| const allDatacenters = new Set( | ||
| this.get('visibleJobs') | ||
| .mapBy('datacenters') | ||
| .reduce(flatten, []) | ||
| ); | ||
|
|
||
| // Remove any invalid datacenters from the query param/selection | ||
| const availableDatacenters = Array.from(allDatacenters).compact(); | ||
| scheduleOnce('actions', () => { | ||
| this.set( | ||
| 'qpDatacenter', | ||
| qpSerialize(intersection(availableDatacenters, this.get('selectionDatacenter'))) | ||
|
There was a problem hiding this comment. Can you use There was a problem hiding this comment. That'd work fine, but it wouldn't save me a dependency since I'm already using There was a problem hiding this comment. Ah cool, I think it's just here: nomad/ui/app/models/allocation.js Line 73 in c12bdcd You could remove lodash there also? I suppose it depends what side of the fence you are on, but it would save a dependency, no biggie either way. |
||
| ); | ||
| }); | ||
|
|
||
| return availableDatacenters.sort().map(dc => ({ key: dc, label: dc })); | ||
| }), | ||
|
|
||
| optionsPrefix: computed('visibleJobs.[]', function() { | ||
| // A prefix is defined as the start of a job name up to the first - or . | ||
| // ex: mktg-analytics -> mktg, ds.supermodel.classifier -> ds | ||
| const hasPrefix = /.[-._]/; | ||
|
|
||
| // Collect and count all the prefixes | ||
| const allNames = this.get('visibleJobs').mapBy('name'); | ||
| const nameHistogram = allNames.reduce((hist, name) => { | ||
| if (hasPrefix.test(name)) { | ||
| const prefix = name.match(/(.+?)[-.]/)[1]; | ||
| hist[prefix] = hist[prefix] ? hist[prefix] + 1 : 1; | ||
| } | ||
| return hist; | ||
| }, {}); | ||
|
There was a problem hiding this comment. I'm rubbish at RegEx so not sure on this one, but is there a way to test and match the prefix in one shot? There was a problem hiding this comment. Yes. The regex could be changed to capture up to the delimiter and also require characters after the delimiter. Then if there's no match or the capture group is empty, that'd mean there was no prefix. I'm not sure how large of an impact that would make on performance here, but I suppose it's worth investigating. There was a problem hiding this comment. Up to you. It wasn't really the performance angle that I was coming from (maybe it's more performant to use a I think it was more of a 'tool for the job' thing. Actually, if anything from a performance angle, I'd hoist your regex definitions to the top scope maybe? I don't think it matters here though really, I don't think this will ever be that hot, but I could be wrong, don't have enough context there. |
||
|
|
||
| // Convert to an array | ||
| const nameTable = Object.keys(nameHistogram).map(key => ({ | ||
| prefix: key, | ||
| count: nameHistogram[key], | ||
| })); | ||
|
|
||
| // Only consider prefixes that match more than one name | ||
| const prefixes = nameTable.filter(name => name.count > 1); | ||
|
|
||
| // Remove any invalid prefixes from the query param/selection | ||
| const availablePrefixes = prefixes.mapBy('prefix'); | ||
| scheduleOnce('actions', () => { | ||
| this.set( | ||
| 'qpPrefix', | ||
| qpSerialize(intersection(availablePrefixes, this.get('selectionPrefix'))) | ||
| ); | ||
| }); | ||
|
|
||
| // Sort, format, and include the count in the label | ||
| return prefixes.sortBy('prefix').map(name => ({ | ||
| key: name.prefix, | ||
| label: `${name.prefix} (${name.count})`, | ||
| })); | ||
| }), | ||
|
|
||
| qpType: '', | ||
| qpStatus: '', | ||
| qpDatacenter: '', | ||
| qpPrefix: '', | ||
|
|
||
| selectionType: qpSelection('qpType'), | ||
| selectionStatus: qpSelection('qpStatus'), | ||
| selectionDatacenter: qpSelection('qpDatacenter'), | ||
| selectionPrefix: qpSelection('qpPrefix'), | ||
|
|
||
|
There was a problem hiding this comment. Should all these go up at the top of the class definition with your other variables? |
||
| /** | ||
| Filtered jobs are those that match the selected namespace and aren't children | ||
| of periodic or parameterized jobs. | ||
| */ | ||
| visibleJobs: computed('model.[]', '[email protected]', function() { | ||
|
There was a problem hiding this comment. If you are changing the name of the property here, maybe change the comment also? Or maybe the comment refers to the code under this computedProperty now? Not sure |
||
| // Namespace related properties are ommitted from the dependent keys | ||
| // due to a prop invalidation bug caused by region switching. | ||
| const hasNamespaces = this.get('system.namespaces.length'); | ||
|
|
@@ -44,12 +151,60 @@ export default Controller.extend(Sortable, Searchable, { | |
| .filter(job => !job.get('parent.content')); | ||
| }), | ||
|
|
||
| filteredJobs: computed( | ||
| 'visibleJobs.[]', | ||
| 'selectionType', | ||
| 'selectionStatus', | ||
| 'selectionDatacenter', | ||
| 'selectionPrefix', | ||
| function() { | ||
| const { | ||
| selectionType: types, | ||
| selectionStatus: statuses, | ||
| selectionDatacenter: datacenters, | ||
| selectionPrefix: prefixes, | ||
| } = this.getProperties( | ||
| 'selectionType', | ||
| 'selectionStatus', | ||
| 'selectionDatacenter', | ||
| 'selectionPrefix' | ||
| ); | ||
|
|
||
| // A job must match ALL filter facets, but it can match ANY selection within a facet | ||
| // Always return early to prevent unnecessary facet predicates. | ||
| return this.get('visibleJobs').filter(job => { | ||
| if (types.length && !types.includes(job.get('displayType'))) { | ||
| return false; | ||
| } | ||
|
|
||
| if (statuses.length && !statuses.includes(job.get('status'))) { | ||
| return false; | ||
| } | ||
|
|
||
| if (datacenters.length && !job.get('datacenters').find(dc => datacenters.includes(dc))) { | ||
| return false; | ||
| } | ||
|
|
||
| const name = job.get('name'); | ||
| if (prefixes.length && !prefixes.find(prefix => name.startsWith(prefix))) { | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| }); | ||
| } | ||
|
There was a problem hiding this comment. For me, I'd consider using some curly brackets instead of one line conditionals here, especially at the end here it gets a little "hold on, what's happening here". I never use this type of one line conditionals though so maybe its just me. There was a problem hiding this comment. Yeah, it's definitely dense, but a bunch of curlies seems noisy. I had to try really hard to avoid overengineering this 😄 I'll poke at it some more. There was a problem hiding this comment. Cool, this is more verbose but to my eyes its easier to read so thanks for that! I realized after my comment that your original code might have been more readable originally but maybe I think this is more just a readability/syntax/style thing rather than an engineering thing, you could maybe do the engineering differently, but I don't think it matters too much here (see below) Just a last note here if it helps, I use a switch(true) {
case types.length && !types.includes(job.get('displayType')):
case statuses.length && !statuses.includes(job.get('status')):
case datacenters.length && !job.get('datacenters').find(dc => datacenters.includes(dc)):
case prefixes.length && !prefixes.find(prefix => job.get('name').startsWith(prefix))
return false;
default:
return true;
}Comments can fit in really nicely between the Little engineering offshoot, but why all the |
||
| ), | ||
|
|
||
| listToSort: alias('filteredJobs'), | ||
| listToSearch: alias('listSorted'), | ||
| sortedJobs: alias('listSearched'), | ||
|
|
||
| isShowingDeploymentDetails: false, | ||
|
|
||
| setFacetQueryParam(queryParam, selection) { | ||
| this.set(queryParam, qpSerialize(selection)); | ||
| }, | ||
|
|
||
| actions: { | ||
| gotoJob(job) { | ||
| this.transitionToRoute('jobs.job', job.get('plainId')); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -142,4 +142,10 @@ | |
| } | ||
| } | ||
| } | ||
|
|
||
| .dropdown-empty { | ||
| display: block; | ||
| padding: 8px 12px; | ||
| color: $grey-light; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hehe I see what you mean here by unattractive! So my URL right now has:
Generally I've seen these things done with
status[]=thing, I'm not sure how ember treats that though, I did notice something the other day related to this, I'll try and dig it out again in a bit incase it helps.I'm not entirely sure that
"should be in URLs, and also that,shouldn't - probably worth having a look to see what is 'usual' here. Do you need the"'s? Are you expecting to havetrueand"true"at the same time?Anyway, if you take a look lemme know what you find out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the thing I was looking at the other day which is ever so slightly related to this
https://github.com/tildeio/route-recognizer/blob/0940966757104ea5297717102b1ae3dc260ee8ee/lib/route-recognizer.ts#L564-L574
Looks like ember is aware of the
key[]=valuething, just be careful as I'm not sure it encodes keys properly when you do that. I don't think that would effect you here anyway.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was honestly expecting Ember to use the
status[]=&status[]=style when I bound an array to a query param, but that's not what happened. Instead it basically does what I'm doing now: JSON stringifying.As for encoding characters, it's curious that you are seeing
"and,in the URL, what browser are you using? In Chrome, they were automatically encoded. Not sure if that's a property of Ember or Chrome.Yes-ish. Ideally I would change
qpSerializeto justarr.join(','), but commas (and basically everything) is a legal job name and datacenter character. So there are other possible encoding schemes thanJSON.stringify, but it's not worth spinning my wheels on that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh weird were you not getting
"s? I was in Chrome. I did find it strange that it seemed to be encoding one but not the other. I'm gonna check this branch out again in a bit and double check. It was definitely showing me actual"s when I did this first pass.Yeah for sure, this is why I was thinking a
filter[status][]=url%20encoded%2Cjob-name&filter[status][]=another%22url%20encoded%2Cjob-nameseemed to make more sense to me? I'm not entirely sure what ember does though, so I'm not sure how easily it is to make ember produce/consume that - I might have a bit more of a dig in a bit myself later.So my question would be, what's the reason for doing it here if ember does the same? I don't have enough practical info on what ember does here though.
I think I'd still be tempted to use URL encoding to encode strings for the URL - but there's also an argument to say 'use the framework', and if it uses a javascript encoder to encode the entire data blob into a single string parameter and then URL encode it then maybe there is a reason for that I'm not aware of/haven't thought of. It might just be 'because it's more straightforward'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely getting
"s in Chrome