From 04de4aadf28e07ffdf9f025a73031ee457597345 Mon Sep 17 00:00:00 2001 From: Jai Bhagat Date: Thu, 14 Oct 2021 21:21:11 -0400 Subject: [PATCH 1/5] add refreshModel to namespace query param There is a bug in the Ember Router service that does not preserve query parameters during a partial transition which is what happens when we forward a query parameter using transitionTo or LinkTo. That initializes a new set of queryParameters and since we're inheriting from a sibling route, the new navigated to controller and route have no knowledge of that previous query parameter. To work around this, we rely on adding refreshModel to the queryParameter because refreshModel calls Router.refresh which maps all query params from the previous state into the refresh. Since we're relying on this bug in the API (Hyrum's Law), we'll follow the Beyonce rule and test the dependency on the API in an acceptance test (if you like it, then you shoulda put a test on it). If we update Ember and this test fails, its because Ember fixed this Router bug. --- ui/app/controllers/jobs/job.js | 9 +++- ui/tests/acceptance/job-allocations-test.js | 57 +++++++++++++++++++++ 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/ui/app/controllers/jobs/job.js b/ui/app/controllers/jobs/job.js index 519d9eae309..db88b347d66 100644 --- a/ui/app/controllers/jobs/job.js +++ b/ui/app/controllers/jobs/job.js @@ -1,10 +1,15 @@ import Controller from '@ember/controller'; +import { tracked } from '@glimmer/tracking'; export default class JobController extends Controller { queryParams = [ { - jobNamespace: 'namespace', + jobNamespace: { + as: 'namespace', + refreshModel: true, + }, }, ]; - jobNamespace = 'default'; + + @tracked jobNamespace = 'default'; } diff --git a/ui/tests/acceptance/job-allocations-test.js b/ui/tests/acceptance/job-allocations-test.js index 7c86653c339..1538b9228af 100644 --- a/ui/tests/acceptance/job-allocations-test.js +++ b/ui/tests/acceptance/job-allocations-test.js @@ -82,6 +82,63 @@ module('Acceptance | job allocations', function(hooks) { }); }); + test('sorting the allocations tables does not clear namespace query parameter', async function(assert) { + /* + We rely on the router bug reported in https://github.com/emberjs/ember.js/issues/18683 + Nomad passes job namespaces to sibling routes using LinkTo and transitions + These only trigger partial transitions which do not map the query parameters of the previous + state, the workaround to trigger a full transition is calling refreshModel. We depend on this + API to preserve namespaces. If this test breaks, its likely associated to an Ember update and + a change to that API. + */ + server.createList('allocation', Allocations.pageSize - 1); + server.create('namespace', { id: 'afc-richmond' }); + job.update({ + namespaceId: 'afc-richmond', + }); + allocations = server.schema.allocations.where({ + jobId: job.id, + }).models; + + await Allocations.visit({ id: job.id, namespace: 'afc-richmond' }); + await Allocations.sortBy('taskGroupName'); + + assert.equal( + currentURL(), + `/jobs/${job.id}/allocations?namespace=afc-richmond&sort=taskGroupName`, + 'the URL persists the namespace parameter' + ); + }); + + test('searching the allocations tables does not clear namespace query parameter', async function(assert) { + /* + We rely on the router bug reported in https://github.com/emberjs/ember.js/issues/18683 + Nomad passes job namespaces to sibling routes using LinkTo and transitions + These only trigger partial transitions which do not map the query parameters of the previous + state, the workaround to trigger a full transition is calling refreshModel. We depend on this + API to preserve namespaces. If this test breaks, its likely associated to an Ember update and + a change to that API. + */ + server.create('namespace', { id: 'afc-richmond' }); + job.update({ + namespace: 'afc-richmond', + namespaceId: 'afc-richmond', + }); + + makeSearchAllocations(server); + + allocations = server.schema.allocations.where({ jobId: job.id }).models; + + await Allocations.visit({ id: job.id, namespace: 'afc-richmond' }); + await Allocations.search('ffffff'); + + assert.equal( + currentURL(), + `/jobs/${job.id}/allocations?namespace=afc-richmond&search=ffffff`, + 'the URL persists the namespace parameter' + ); + }); + test('allocations table is searchable', async function(assert) { makeSearchAllocations(server); From b0395b199da5636612432a2ce201c6be7272e2ef Mon Sep 17 00:00:00 2001 From: Jai Bhagat Date: Tue, 26 Oct 2021 09:20:09 -0400 Subject: [PATCH 2/5] move refreshModel hook to route from controller Ember documentation shows refreshModel hook on the route instead of the controller. Making this change does trigger a full transition. Unsure why at the moment, however. --- ui/app/controllers/jobs/job.js | 5 +---- ui/app/routes/jobs/job.js | 7 +++++++ 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/ui/app/controllers/jobs/job.js b/ui/app/controllers/jobs/job.js index db88b347d66..3ce5fc9f336 100644 --- a/ui/app/controllers/jobs/job.js +++ b/ui/app/controllers/jobs/job.js @@ -4,10 +4,7 @@ import { tracked } from '@glimmer/tracking'; export default class JobController extends Controller { queryParams = [ { - jobNamespace: { - as: 'namespace', - refreshModel: true, - }, + jobNamespace: 'namespace', }, ]; diff --git a/ui/app/routes/jobs/job.js b/ui/app/routes/jobs/job.js index 32709b0be70..54b6f35c451 100644 --- a/ui/app/routes/jobs/job.js +++ b/ui/app/routes/jobs/job.js @@ -13,6 +13,13 @@ export default class JobRoute extends Route { breadcrumbs = jobCrumbs; + queryParams = { + jobNamespace: { + as: 'namespace', + refreshModel: true, + }, + }; + serialize(model) { return { job_name: model.get('plainId') }; } From af6ca36d053c7ec5672927bf6c9eebf9efc261eb Mon Sep 17 00:00:00 2001 From: Jai Bhagat Date: Tue, 26 Oct 2021 10:11:45 -0400 Subject: [PATCH 3/5] remove tests for query param bug The tests don't actually catch the error. The Ember test server does not fully replicate the behavior of triggering partial and full transitions for routes. The page transition is being triggered by the Search Mixin when resetPagination is called on change. We can't test the route in isolation because we're using the Search mixin and that's where the behavior causing namespaces to disappear is coming from. --- ui/app/routes/jobs/job.js | 6 +++ ui/tests/acceptance/job-allocations-test.js | 57 --------------------- 2 files changed, 6 insertions(+), 57 deletions(-) diff --git a/ui/app/routes/jobs/job.js b/ui/app/routes/jobs/job.js index 54b6f35c451..ed4cb473fdf 100644 --- a/ui/app/routes/jobs/job.js +++ b/ui/app/routes/jobs/job.js @@ -13,6 +13,12 @@ export default class JobRoute extends Route { breadcrumbs = jobCrumbs; + /* + We rely on the router bug reported in https://github.com/emberjs/ember.js/issues/18683 + Nomad passes job namespaces to sibling routes using LinkTo and transitions + These only trigger partial transitions which do not map the query parameters of the previous + state, the workaround to trigger a full transition is calling refreshModel. + */ queryParams = { jobNamespace: { as: 'namespace', diff --git a/ui/tests/acceptance/job-allocations-test.js b/ui/tests/acceptance/job-allocations-test.js index 1538b9228af..7c86653c339 100644 --- a/ui/tests/acceptance/job-allocations-test.js +++ b/ui/tests/acceptance/job-allocations-test.js @@ -82,63 +82,6 @@ module('Acceptance | job allocations', function(hooks) { }); }); - test('sorting the allocations tables does not clear namespace query parameter', async function(assert) { - /* - We rely on the router bug reported in https://github.com/emberjs/ember.js/issues/18683 - Nomad passes job namespaces to sibling routes using LinkTo and transitions - These only trigger partial transitions which do not map the query parameters of the previous - state, the workaround to trigger a full transition is calling refreshModel. We depend on this - API to preserve namespaces. If this test breaks, its likely associated to an Ember update and - a change to that API. - */ - server.createList('allocation', Allocations.pageSize - 1); - server.create('namespace', { id: 'afc-richmond' }); - job.update({ - namespaceId: 'afc-richmond', - }); - allocations = server.schema.allocations.where({ - jobId: job.id, - }).models; - - await Allocations.visit({ id: job.id, namespace: 'afc-richmond' }); - await Allocations.sortBy('taskGroupName'); - - assert.equal( - currentURL(), - `/jobs/${job.id}/allocations?namespace=afc-richmond&sort=taskGroupName`, - 'the URL persists the namespace parameter' - ); - }); - - test('searching the allocations tables does not clear namespace query parameter', async function(assert) { - /* - We rely on the router bug reported in https://github.com/emberjs/ember.js/issues/18683 - Nomad passes job namespaces to sibling routes using LinkTo and transitions - These only trigger partial transitions which do not map the query parameters of the previous - state, the workaround to trigger a full transition is calling refreshModel. We depend on this - API to preserve namespaces. If this test breaks, its likely associated to an Ember update and - a change to that API. - */ - server.create('namespace', { id: 'afc-richmond' }); - job.update({ - namespace: 'afc-richmond', - namespaceId: 'afc-richmond', - }); - - makeSearchAllocations(server); - - allocations = server.schema.allocations.where({ jobId: job.id }).models; - - await Allocations.visit({ id: job.id, namespace: 'afc-richmond' }); - await Allocations.search('ffffff'); - - assert.equal( - currentURL(), - `/jobs/${job.id}/allocations?namespace=afc-richmond&search=ffffff`, - 'the URL persists the namespace parameter' - ); - }); - test('allocations table is searchable', async function(assert) { makeSearchAllocations(server); From 8fe603e653a7bf462e1bd37fc7ce58cfaa5fd577 Mon Sep 17 00:00:00 2001 From: Jai Bhagat Date: Sun, 7 Nov 2021 15:26:55 -0500 Subject: [PATCH 4/5] forward qp to linkto in sort-by compoennt The bug is caused by clicking the sort or search button. This commit resolves the disappearing namespace when the sort button is clicked by ensuring that when we click the sort button and thus are clicking the LinkTo property that we're adding namespace to the query hash. --- ui/app/components/list-table/sort-by.js | 10 ++ ui/app/routes/jobs/job.js | 13 --- ui/app/templates/components/list-table.hbs | 15 +-- .../components/list-table/sort-by.hbs | 6 +- ui/app/templates/jobs/job/allocations.hbs | 96 ++++++++++++++----- ui/app/templates/jobs/job/clients.hbs | 53 +++++++--- 6 files changed, 134 insertions(+), 59 deletions(-) diff --git a/ui/app/components/list-table/sort-by.js b/ui/app/components/list-table/sort-by.js index bdc16efa643..509bedddb6d 100644 --- a/ui/app/components/list-table/sort-by.js +++ b/ui/app/components/list-table/sort-by.js @@ -29,4 +29,14 @@ export default class SortBy extends Component { get shouldSortDescending() { return !this.isActive || !this.sortDescending; } + + @computed('addToQuery', 'prop', 'shouldSortDescending') + get query() { + const addToQuery = this.addToQuery || {}; + return { + sortProperty: this.prop, + sortDescending: this.shouldSortDescending, + ...addToQuery, + }; + } } diff --git a/ui/app/routes/jobs/job.js b/ui/app/routes/jobs/job.js index ed4cb473fdf..32709b0be70 100644 --- a/ui/app/routes/jobs/job.js +++ b/ui/app/routes/jobs/job.js @@ -13,19 +13,6 @@ export default class JobRoute extends Route { breadcrumbs = jobCrumbs; - /* - We rely on the router bug reported in https://github.com/emberjs/ember.js/issues/18683 - Nomad passes job namespaces to sibling routes using LinkTo and transitions - These only trigger partial transitions which do not map the query parameters of the previous - state, the workaround to trigger a full transition is calling refreshModel. - */ - queryParams = { - jobNamespace: { - as: 'namespace', - refreshModel: true, - }, - }; - serialize(model) { return { job_name: model.get('plainId') }; } diff --git a/ui/app/templates/components/list-table.hbs b/ui/app/templates/components/list-table.hbs index 399afd31771..d478bbfc09a 100644 --- a/ui/app/templates/components/list-table.hbs +++ b/ui/app/templates/components/list-table.hbs @@ -1,8 +1,9 @@ -{{yield (hash - head=(component "list-table/table-head") - body=(component "list-table/table-body" rows=this.decoratedSource) - sort-by=(component "list-table/sort-by" - currentProp=this.sortProperty - sortDescending=this.sortDescending +{{yield + (hash + head=(component "list-table/table-head") + body=(component "list-table/table-body" rows=this.decoratedSource) + sort-by=(component + "list-table/sort-by" currentProp=this.sortProperty sortDescending=this.sortDescending + ) ) -)}} +}} \ No newline at end of file diff --git a/ui/app/templates/components/list-table/sort-by.hbs b/ui/app/templates/components/list-table/sort-by.hbs index bf8dd35a6b7..995730cfe4e 100644 --- a/ui/app/templates/components/list-table/sort-by.hbs +++ b/ui/app/templates/components/list-table/sort-by.hbs @@ -1,5 +1,3 @@ - + {{yield}} - + \ No newline at end of file diff --git a/ui/app/templates/jobs/job/allocations.hbs b/ui/app/templates/jobs/job/allocations.hbs index e14452e483d..8ed2e0d43ad 100644 --- a/ui/app/templates/jobs/job/allocations.hbs +++ b/ui/app/templates/jobs/job/allocations.hbs @@ -8,7 +8,8 @@ data-test-allocations-search @searchTerm={{mut this.searchTerm}} @onChange={{action this.resetPagination}} - @placeholder="Search allocations..." /> + @placeholder="Search allocations..." + /> {{#if this.sortedAllocations}} @@ -16,40 +17,80 @@ @source={{this.sortedAllocations}} @size={{this.pageSize}} @page={{this.currentPage}} - @class="allocations" as |p|> + @class="allocations" as |p| + > + @class="with-foot" as |t| + > - ID - Task Group - Created - Modified - Status - Version - Client - Volume - CPU - Memory + + ID + + + Task Group + + + Created + + + Modified + + + Status + + + Version + + + Client + + + Volume + + + CPU + + + Memory + + @onClick={{action "gotoAllocation" row.model}} + />
@@ -57,17 +98,28 @@ {{else}}
-

No Matches

-

No allocations match the term {{this.searchTerm}}

+

+ No Matches +

+

+ No allocations match the term + + {{this.searchTerm}} + +

{{/if}} {{else}}
-

No Allocations

-

No allocations have been placed.

+

+ No Allocations +

+

+ No allocations have been placed. +

{{/if}} - + \ No newline at end of file diff --git a/ui/app/templates/jobs/job/clients.hbs b/ui/app/templates/jobs/job/clients.hbs index b61d6dacfe8..8548122bbce 100644 --- a/ui/app/templates/jobs/job/clients.hbs +++ b/ui/app/templates/jobs/job/clients.hbs @@ -51,27 +51,54 @@ @class="with-foot" as |t| > - Client ID - Client Name - Created - Modified - Job Status - Allocation Summary + + Client ID + + + Client Name + + + Created + + + Modified + + + Job Status + + + Allocation Summary + - +
From 4cc749e7f74983df593211e2f9885613ee334e39 Mon Sep 17 00:00:00 2001 From: Jai Bhagat Date: Sun, 7 Nov 2021 15:51:37 -0500 Subject: [PATCH 5/5] preserve namesapace in setter for search-box --- ui/app/components/search-box.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ui/app/components/search-box.js b/ui/app/components/search-box.js index be9cbcf2697..4190bed3ab0 100644 --- a/ui/app/components/search-box.js +++ b/ui/app/components/search-box.js @@ -36,5 +36,5 @@ export default class SearchBox extends Component { function updateSearch() { const newTerm = this._searchTerm; this.onChange(newTerm); - this.set('searchTerm', newTerm); + this.setProperties({ searchTerm: newTerm, namespace: this._target.job.namespace.get('id') }); }