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

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 1 commit
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Simplify options and selection names
  • Loading branch information
@DingoEatingFuzz
DingoEatingFuzz committed Feb 1, 2019
commit a207fdd368b4ad9432202247f992c045212ff269
46 changes: 23 additions & 23 deletions ui/app/controllers/jobs/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ const qpDeserialize = str => {
}
};

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 selectionFromQP = qpKey =>
const qpSelection = qpKey =>
computed(qpKey, function() {
return qpDeserialize(this.get(qpKey));
});
Expand Down Expand Up @@ -51,21 +51,21 @@ export default Controller.extend(Sortable, Searchable, {
fuzzySearchProps: computed(() => ['name']),
fuzzySearchEnabled: true,

facetOptionsType: computed(() => [
optionsType: computed(() => [
{ key: 'batch', label: 'Batch' },
{ key: 'parameterized', label: 'Parameterized' },
{ key: 'periodic', label: 'Periodic' },
{ key: 'service', label: 'Service' },
{ key: 'system', label: 'System' },
]),

facetOptionsStatus: computed(() => [
optionsStatus: computed(() => [
{ key: 'pending', label: 'Pending' },
{ key: 'running', label: 'Running' },
{ key: 'dead', label: 'Dead' },
]),

facetOptionsDatacenter: computed('visibleJobs.[]', function() {
optionsDatacenter: computed('visibleJobs.[]', function() {
const flatten = (acc, val) => acc.concat(val);
const allDatacenters = new Set(
this.get('visibleJobs')
Expand All @@ -78,14 +78,14 @@ export default Controller.extend(Sortable, Searchable, {
scheduleOnce('actions', () => {
this.set(
'qpDatacenter',
qpSerialize(intersection(availableDatacenters, this.get('facetSelectionDatacenter')))
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 }));
}),

facetOptionsPrefix: computed('visibleJobs.[]', function() {
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 = /.[-._]/;
Expand Down Expand Up @@ -114,7 +114,7 @@ export default Controller.extend(Sortable, Searchable, {
scheduleOnce('actions', () => {
this.set(
'qpPrefix',
qpSerialize(intersection(availablePrefixes, this.get('facetSelectionPrefix')))
qpSerialize(intersection(availablePrefixes, this.get('selectionPrefix')))
);
});

Expand All @@ -130,10 +130,10 @@ export default Controller.extend(Sortable, Searchable, {
qpDatacenter: '',
qpPrefix: '',

facetSelectionType: selectionFromQP('qpType'),
facetSelectionStatus: selectionFromQP('qpStatus'),
facetSelectionDatacenter: selectionFromQP('qpDatacenter'),
facetSelectionPrefix: selectionFromQP('qpPrefix'),
selectionType: qpSelection('qpType'),
selectionStatus: qpSelection('qpStatus'),
selectionDatacenter: qpSelection('qpDatacenter'),
selectionPrefix: qpSelection('qpPrefix'),

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Expand All @@ -153,21 +153,21 @@ export default Controller.extend(Sortable, Searchable, {

filteredJobs: computed(
'visibleJobs.[]',
'facetSelectionType',
'facetSelectionStatus',
'facetSelectionDatacenter',
'facetSelectionPrefix',
'selectionType',
'selectionStatus',
'selectionDatacenter',
'selectionPrefix',
function() {
const {
facetSelectionType: types,
facetSelectionStatus: statuses,
facetSelectionDatacenter: datacenters,
facetSelectionPrefix: prefixes,
selectionType: types,
selectionStatus: statuses,
selectionDatacenter: datacenters,
selectionPrefix: prefixes,
} = this.getProperties(
'facetSelectionType',
'facetSelectionStatus',
'facetSelectionDatacenter',
'facetSelectionPrefix'
'selectionType',
'selectionStatus',
'selectionDatacenter',
'selectionPrefix'
);

// A job must match ALL filter facets, but it can match ANY selection within a facet
Expand Down
16 changes: 8 additions & 8 deletions ui/app/templates/jobs/index.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,26 +17,26 @@
{{multi-select-dropdown
data-test-type-facet
label="Type"
options=facetOptionsType
selection=facetSelectionType
options=optionsType
selection=selectionType
onSelect=(action setFacetQueryParam "qpType")}}
{{multi-select-dropdown
data-test-status-facet
label="Status"
options=facetOptionsStatus
selection=facetSelectionStatus
options=optionsStatus
selection=selectionStatus
onSelect=(action setFacetQueryParam "qpStatus")}}
{{multi-select-dropdown
data-test-datacenter-facet
label="Datacenter"
options=facetOptionsDatacenter
selection=facetSelectionDatacenter
options=optionsDatacenter
selection=selectionDatacenter
onSelect=(action setFacetQueryParam "qpDatacenter")}}
{{multi-select-dropdown
data-test-prefix-facet
label="Prefix"
options=facetOptionsPrefix
selection=facetSelectionPrefix
options=optionsPrefix
selection=selectionPrefix
onSelect=(action setFacetQueryParam "qpPrefix")}}
</div>
</div>
Expand Down