Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

UI: Faceted search on the jobs list page #5236

Merged
merged 10 commits into from
Feb 11, 2019
Merged

UI: Faceted search on the jobs list page #5236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8ed8a85
Scaffold the facets and facet options for the jobs list page
DingoEatingFuzz Jan 17, 2019
3d87c47
Implement the dynamic facet lists (datacenters and prefixes)
DingoEatingFuzz Jan 17, 2019
3b235a1
Implement the job list filtering based on facet selection
DingoEatingFuzz Jan 17, 2019
5aef4d9
Support for the no options case
DingoEatingFuzz Jan 23, 2019
3b8187f
Introduce encode/decode for array query params
DingoEatingFuzz Jan 23, 2019
b20dc67
Model facets in the page object
DingoEatingFuzz Jan 24, 2019
0d8dd50
Test coverage for job list facets
DingoEatingFuzz Jan 24, 2019
a207fdd
Simplify options and selection names
DingoEatingFuzz Feb 1, 2019
2fdec7e
Make the facet filter function clearer with more brackets
DingoEatingFuzz Feb 1, 2019
b769365
Changes from code review
DingoEatingFuzz Feb 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
159 changes: 157 additions & 2 deletions ui/app/controllers/jobs/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 [];
}
};

Copy link
Contributor

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:

?status=%5B"pending"%2C"running"%2C"dead"%5D

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 have true and "true" at the same time?

Anyway, if you take a look lemme know what you find out.

Copy link
Contributor

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[]=value thing, 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.

Copy link
Contributor Author

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.

Do you need the "'s? Are you expecting to have true and "true" at the same time?

Yes-ish. Ideally I would change qpSerialize to just arr.join(','), but commas (and basically everything) is a legal job name and datacenter character. So there are other possible encoding schemes than JSON.stringify, but it's not worth spinning my wheels on that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for encoding characters, it's curious that you are seeing " and , in the URL

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.

but commas (and basically everything) is a legal job name and datacenter character

Yeah for sure, this is why I was thinking a filter[status][]=url%20encoded%2Cjob-name&filter[status][]=another%22url%20encoded%2Cjob-name seemed 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.

Instead it basically does what I'm doing now: JSON stringifying.

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'

Copy link
Contributor

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

screenshot 2019-02-05 at 16 34 08

const qpSelection = qpKey =>
computed(qpKey, function() {
return qpDeserialize(this.get(qpKey));
});

export default Controller.extend(Sortable, Searchable, {
system: service(),
jobsController: controller('jobs'),
Expand All @@ -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,
Expand All @@ -28,11 +51,95 @@ export default Controller.extend(Sortable, Searchable, {
fuzzySearchProps: computed(() => ['name']),
fuzzySearchEnabled: true,

qpType: '',
qpStatus: '',
qpDatacenter: '',
qpPrefix: '',

selectionType: qpSelection('qpType'),
selectionStatus: qpSelection('qpStatus'),
selectionDatacenter: qpSelection('qpDatacenter'),
selectionPrefix: qpSelection('qpPrefix'),

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')))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use a.filter(item => b.includes(item)) here? Will save you a dependency? Not sure if I've read this right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That'd work fine, but it wouldn't save me a dependency since I'm already using intersection elsewhere.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah cool, I think it's just here:

nomad/ui/app/models/allocation.js

Line 73 in c12bdcd

return intersection(taskGroupUnhealthyDrivers, nodeUnhealthyDrivers);

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;
}, {});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@johncowen johncowen Feb 5, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to you. It wasn't really the performance angle that I was coming from (maybe it's more performant to use a test to potentially avoid a match), I don't think performance is important here.

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})`,
}));
}),

/**
Filtered jobs are those that match the selected namespace and aren't children
Visible jobs are those that match the selected namespace and aren't children
of periodic or parameterized jobs.
*/
filteredJobs: computed('model.[]', 'model.@each.parent', function() {
visibleJobs: computed('model.[]', 'model.@each.parent', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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');
Expand All @@ -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;
});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@johncowen johncowen Feb 5, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 prettier wrapped it to make it 'prettier'? 🤷‍♂️

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 approach with things like this quite a lot, means you get the benefit of one-liners but readability is maintained, again maybe just a 'to my eyes' thing:

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 case statements there also if you ever need to get more wordy with what's happening. I generally put the most likely thing to happen at the top.

Little engineering offshoot, but why all the .length checks? Is that to ensure things aren't null/undefined. includes and find will return a falsey value on zero length arrays if I'm not mistaken?

),

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'));
Expand Down
6 changes: 6 additions & 0 deletions ui/app/styles/components/dropdown.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,4 +142,10 @@
}
}
}

.dropdown-empty {
display: block;
padding: 8px 12px;
color: $grey-light;
}
}
4 changes: 3 additions & 1 deletion ui/app/templates/components/multi-select-dropdown.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
{{#dd.content class="dropdown-options"}}
<ul role="listbox" data-test-dropdown-options>
{{#each options key="key" as |option|}}
<li data-test-dropdown-option class="dropdown-option" tabindex="1" onkeydown={{action "traverseList" option}}>
<li data-test-dropdown-option={{option.key}} class="dropdown-option" tabindex="1" onkeydown={{action "traverseList" option}}>
<label>
<input
type="checkbox"
Expand All @@ -28,6 +28,8 @@
{{option.label}}
</label>
</li>
{{else}}
<em data-test-dropdown-empty class="dropdown-empty">No options</em>
{{/each}}
</ul>
{{/dd.content}}
Expand Down
39 changes: 36 additions & 3 deletions ui/app/templates/jobs/index.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
{{else}}
<div class="columns">
{{#if filteredJobs.length}}
<div class="column">
<div class="column is-one-third">
{{search-box
data-test-jobs-search
searchTerm=(mut searchTerm)
Expand All @@ -13,7 +13,35 @@
</div>
{{/if}}
<div class="column is-centered">
{{#link-to "jobs.run" data-test-run-job class="button is-primary is-pulled-right"}}Run Job{{/link-to}}
<div class="button-bar is-pulled-right">
{{multi-select-dropdown
data-test-type-facet
label="Type"
options=optionsType
selection=selectionType
onSelect=(action setFacetQueryParam "qpType")}}
{{multi-select-dropdown
data-test-status-facet
label="Status"
options=optionsStatus
selection=selectionStatus
onSelect=(action setFacetQueryParam "qpStatus")}}
{{multi-select-dropdown
data-test-datacenter-facet
label="Datacenter"
options=optionsDatacenter
selection=selectionDatacenter
onSelect=(action setFacetQueryParam "qpDatacenter")}}
{{multi-select-dropdown
data-test-prefix-facet
label="Prefix"
options=optionsPrefix
selection=selectionPrefix
onSelect=(action setFacetQueryParam "qpPrefix")}}
</div>
</div>
<div class="column is-minimum is-centered">
{{#link-to "jobs.run" data-test-run-job class="button is-primary"}}Run Job{{/link-to}}
</div>
</div>
{{#list-pagination
Expand Down Expand Up @@ -52,11 +80,16 @@
</div>
{{else}}
<div data-test-empty-jobs-list class="empty-message">
{{#if (eq filteredJobs.length 0)}}
{{#if (eq visibleJobs.length 0)}}
<h3 data-test-empty-jobs-list-headline class="empty-message-headline">No Jobs</h3>
<p class="empty-message-body">
The cluster is currently empty.
</p>
{{else if (eq filteredJobs.length 0)}}
<h3 data-test-empty-jobs-list-headline class="empty-message-headline">No Matches</h3>
<p class="empty-message-body">
No jobs match your current filter selection.
</p>
{{else if searchTerm}}
<h3 data-test-empty-jobs-list-headline class="empty-message-headline">No Matches</h3>
<p class="empty-message-body">No jobs match the term <strong>{{searchTerm}}</strong></p>
Expand Down