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.

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] Searching and filtering options #20459

Conversation

philrenaud
Copy link
Contributor

@philrenaud philrenaud commented Apr 18, 2024

Introduces server-side search and filter to the Jobs index page

image

Uses Filter Expressions with our /statuses endpoint to achieve job-property-level filtering.

Translates dropdown toggles and search queries into filter expression syntax to help teach the syntax for future advanced usage

Re-implements quite a lot of test logic that was performed against an "all jobs are always loaded" version of this page to use the "what we ask for is what we get" version.

@philrenaud philrenaud self-assigned this Apr 18, 2024
Copy link

github-actions bot commented Apr 18, 2024

Ember Test Audit comparison

bff-job-summary-fe-redux-untangle 3e4d579 change
passes 1529 1557 +28
failures 0 0 0
flaky 0 0 0
duration 10m 57s 018ms 11m 17s 593ms +20s 575ms

@philrenaud philrenaud changed the base branch from bff-job-summary-fe-redux to bff-job-summary-fe-redux-untangle April 19, 2024 14:43
}
if (method === 'POST' && query.jobs) {
baseUrl += baseUrl.includes('?') ? '&' : '?';
baseUrl += 'namespace=*';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implementation detail: even though I'm posting jobs with {id:..., namespace:...} in the body, if I don't include the namespaces I'm looking for in my queryParams, I won't get the requested jobs back.

This had the effect of them appearing to have been GC'd to the web user (they showed up on my GET, but not on my subsequent POST).

@type="search"
@value={{@searchText}}
aria-label="Job Search"
placeholder="Name contains myJob"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hinting that this handles filter expressions and not just strings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noting this as the moment where I learned about https://github.com/hashicorp/js-bexpr

@@ -101,6 +193,8 @@ export default class IndexRoute extends Route.extend(
Ember.testing ? 0 : DEFAULT_THROTTLE
);

controller.parseFilter();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Calls the "Change the ?filter=THING and THING and THING into dropdowns with pre-checked boxes" script on initial page load

.filter((job) => job)
.map((j) => this.store.query('allocation', { job_id: j.id }))
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Way out of left-field side effect alert!

The optimize page/tests depended on this thing called isPartial to determine if they should reload a job when you hit a recommendations page. isPartial is signalled by the lack of taskGroups' allocations on a job. The way this worked was you'd go to the jobs index page, some partial jobs would enter the Ember data store (because the /jobs endpoint lacked alloc information) and the now-visited /optimize page would see those in the store and try to fetch their allocations automagically.

WELLLLLLL now we have allocations on our jobs on the jobs index page! And in our jobs serializer, we explicitly give it the level of detail it needs to:

  1. not cause the jobs index page to try to make individual /job/$id/allocations calls
  2. cause this /optimize mechanism to fail

and thus now we just .query for a job's allocations for any jobs we know about (and for which we have recommendations) when you hit the optimize page.

@@ -118,7 +118,69 @@ export default function () {
let nextToken = req.queryParams.next_token || 0;
let reverse = req.queryParams.reverse === 'true';
const json = this.serialize(jobs.all());
let sortedJson = json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just enough filter logic to let us test things reasonably. But no more than that.

@@ -62,13 +62,14 @@ function jobsIndexTestCluster(server) {
faker.seed(1);
server.createList('agent', 1, 'withConsulLink', 'withVaultLink');
server.createList('node', 1);
server.create('node-pool');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our mirage environment for testing purposes should show me a node-pool column on job rows.

resourceSpec: Array(1).fill('M: 256, C: 500'),
groupAllocCount: 1,
resourceSpec: Array(groupCount).fill('M: 256, C: 500'),
groupAllocCount: Math.floor(Math.random() * 3) + 1,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Things weren't very interesting in our test cluster when every job just had 1 alloc.

key: attribute('data-test-hds-facet-option'),
toggle: clickable('.hds-dropdown-list-item__label'),
}),
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A good chunk of work here is figuring out the nuances between the PowerSelect components we were using below for dropdowns, and the new Helios ones. You'll see key change to label throughout the test files generally, among other things.

namespace: hdsFacet('[data-test-facet="Namespace"]'),
type: hdsFacet('[data-test-facet="Type"]'),
status: hdsFacet('[data-test-facet="Status"]'),
nodePool: hdsFacet('[data-test-facet="NodePool"]'),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have datacenter or prefix dropdowns anymore, nor do we have priority or task group columns in the table itself.

@@ -4,7 +4,12 @@
*/

/* eslint-disable qunit/require-expect */
import { currentURL, click, triggerKeyEvent } from '@ember/test-helpers';
import {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generally: added a bunch of tests, and un-skipped the facet and other search/filter tests.

Notable omissions:

  1. things that tested manual sorting (we no longer allow users to sort) although we have a new "are they still sorted by ModifyIndex desc" test
  2. things to do with Prefix and DataCenter dropdown filtering

Almost every test had something like "the currentURL has a literal representation of what you filtered in it", and that is now "the currentURL has a filter expression that corresponds to what you filtered in it"

});

// TODO: Jobs list filter

// testSingleSelectFacet('Namespace', {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the facet testing was retained, but moved to the bottom of the file

@@ -1209,3 +1579,151 @@ function createJobs(server, jobsToCreate) {
});
}
}

async function facetOptions(assert, beforeEach, facet, expectedOptions) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Facet stuff that got moved

{ qpNamespace: 'namespace' },
// 'type',
// 'searchTerm',
'filter',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gave a lot of thought to how this should work, and landed (after much discussion) on "Show the literal filter in the URL and translate it in the controller", instead of "Show simulacra in the URL and parse them into a real filter expression at network request time"

The main benefit here, I hope, is users learn about Filter Expressions.

@@ -226,6 +183,7 @@ export default class JobsIndexController extends Controller {
jobAllocsQuery(params) {
this.watchList.jobsIndexDetailsController.abort();
this.watchList.jobsIndexDetailsController = new AbortController();
params.namespace = '*';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Phil to test: this may not be necessary. hardcoding this at adapter layer.

options: [
{
key: 'pending',
string: 'Status == pending',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Chose to include string equivalents of what you'd select in your dropdown directly here, rather than have some kind of mapping logic elsewhere

Comment on lines +378 to +381
availableNamespaces.unshift({
key: '*',
label: 'All',
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Always ensure there's an "All" option available

this.updateFilter();
}

/**
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'm actually not super thrilled with how searchText feels right now and may reconsider some assumptions later. Right now, if you type a string, it'll prefix that string with Name contains ______ and it happens a little too quickly for my taste (optimal amount of time it should take my very well be infinity)

@philrenaud philrenaud requested a review from gulducat April 26, 2024 15:58
@philrenaud philrenaud marked this pull request as ready for review April 26, 2024 15:58
Copy link
Member

@gulducat gulducat left a comment

Choose a reason for hiding this comment

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

thanks for the walkthrough, lgtm!

return;
}

const filterParts = filterString.split(' and ');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: Phil to caps-check AND and OR etc.

@philrenaud philrenaud merged commit cf94d52 into bff-job-summary-fe-redux-untangle Apr 27, 2024
15 checks passed
@philrenaud philrenaud deleted the 20425-set-up-searching-and-filtering-options branch April 27, 2024 03:34
philrenaud added a commit that referenced this pull request Apr 30, 2024
* Beginnings of a search box for filter expressions

* jobSearchBox integration test

* jobs list updateFilter initial test

* Basic jobs list filtering tests

* First attempt at side-by-side facets and search with a computed filter

* Weirdly close to an iterative approach but checked isnt tracked properly

* Big rework to make filter composition and decomposition work nicely with the url

* Namespace facet dropdown added

* NodePool facet dropdown added

* hdsFacet for future testing and basic namespace filtering test

* Namespace filter existence test

* Status filtering

* Node pool/dynamic facet test

* Test patchups

* Attempt at optimize test fix

* Allocation re-load on optimize page explainer

* The Big Un-Skip

* Post-PR-review cleanup
philrenaud added a commit that referenced this pull request May 2, 2024
* Beginnings of a search box for filter expressions

* jobSearchBox integration test

* jobs list updateFilter initial test

* Basic jobs list filtering tests

* First attempt at side-by-side facets and search with a computed filter

* Weirdly close to an iterative approach but checked isnt tracked properly

* Big rework to make filter composition and decomposition work nicely with the url

* Namespace facet dropdown added

* NodePool facet dropdown added

* hdsFacet for future testing and basic namespace filtering test

* Namespace filter existence test

* Status filtering

* Node pool/dynamic facet test

* Test patchups

* Attempt at optimize test fix

* Allocation re-load on optimize page explainer

* The Big Un-Skip

* Post-PR-review cleanup
philrenaud added a commit that referenced this pull request May 6, 2024
* Beginnings of a search box for filter expressions

* jobSearchBox integration test

* jobs list updateFilter initial test

* Basic jobs list filtering tests

* First attempt at side-by-side facets and search with a computed filter

* Weirdly close to an iterative approach but checked isnt tracked properly

* Big rework to make filter composition and decomposition work nicely with the url

* Namespace facet dropdown added

* NodePool facet dropdown added

* hdsFacet for future testing and basic namespace filtering test

* Namespace filter existence test

* Status filtering

* Node pool/dynamic facet test

* Test patchups

* Attempt at optimize test fix

* Allocation re-load on optimize page explainer

* The Big Un-Skip

* Post-PR-review cleanup
philrenaud added a commit that referenced this pull request May 6, 2024
* Hook and latch on the initial index

* Serialization and restart of controller and table

* de-log

* allocBlocks reimplemented at job model level

* totalAllocs doesnt mean on jobmodel what it did in steady.js

* Hamburgers to sausages

* Hacky way to bring new jobs back around and parent job handling in list view

* Getting closer to hook/latch

* Latch from update on hook from initialize, but fickle

* Note on multiple-watch problem

* Sensible monday morning comment removal

* use of abortController to handle transition and reset events

* Next token will now update when there's an on-page shift

* Very rough anti-jostle technique

* Demoable, now to move things out of route and into controller

* Into the controller, generally

* Smarter cancellations

* Reset abortController on index models run, and system/sysbatch jobs now have an improved groupCountSum computed property

* Prev Page reverse querying

* n+1th jobs existing will trigger nextToken/pagination display

* Start of a GET/POST statuses return

* Namespace fix

* Unblock tests

* Realizing to my small horror that this skipURLModification flag may be too heavy handed

* Lintfix

* Default liveupdates localStorage setting to true

* Pagination and index rethink

* Big uncoupling of watchable and url-append stuff

* Testfixes for region, search, and keyboard

* Job row class for test purposes

* Allocations in test now contain events

* Starting on the jobs list tests in earnest

* Forbidden state de-bubbling cleanup

* Job list page size fixes

* Facet/Search/Filter jobs list tests skipped

* Maybe it's the automatic mirage logging

* Unbreak task unit test

* Pre-sort sort

* styling for jobs list pagination and general PR cleanup

* moving from Job.ActiveDeploymentID to Job.LatestDeployment.ID

* modifyIndex-based pagination (#20350)

* modifyIndex-based pagination

* modifyIndex gets its own column and pagination compacted with icons

* A generic withPagination handler for mirage

* Some live-PR changes

* Pagination and button disabled tests

* Job update handling tests for jobs index

* assertion timeout in case of long setTimeouts

* assert.timeouts down to 500ms

* de-to-do

* Clarifying comment and test descriptions

* Bugfix: resizing your browser on the new jobs index page would make the viz grow forever (#20458)

* [ui] Searching and filtering options (#20459)

* Beginnings of a search box for filter expressions

* jobSearchBox integration test

* jobs list updateFilter initial test

* Basic jobs list filtering tests

* First attempt at side-by-side facets and search with a computed filter

* Weirdly close to an iterative approach but checked isnt tracked properly

* Big rework to make filter composition and decomposition work nicely with the url

* Namespace facet dropdown added

* NodePool facet dropdown added

* hdsFacet for future testing and basic namespace filtering test

* Namespace filter existence test

* Status filtering

* Node pool/dynamic facet test

* Test patchups

* Attempt at optimize test fix

* Allocation re-load on optimize page explainer

* The Big Un-Skip

* Post-PR-review cleanup

* todo-squashing

* [ui] Handle parent/child jobs with the paginated Jobs Index route (#20493)

* First pass at a non-watchQuery version

* Parameterized jobs get child fetching and jobs index status style for parent jobs

* Completed allocs vs Running allocs in a child-job context, and fix an issue where moving from parent to parent would not reset index

* Testfix and better handling empty-child-statuses-list

* Parent/child test case

* Dont show empty allocation-status bars for parent jobs with no children

* Splits Settings into 2 sections, sign-in/profile and user settings (#20535)

* Changelog
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants