add refreshModel to namespace query param #11326
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Ember Asset Size actionAs of 4cc749e Files that got Bigger 🚨:
Files that stayed the same size 🤷:
|
Ember Test Audit comparison
|
Ember Test Audit flaky testsEmber Test Audit detected these flaky tests on d9f1e18:
|
ChaiWithJai
commented
Oct 26, 2021
ui/app/routes/jobs/job.js
Outdated
| queryParams = { | ||
| jobNamespace: { | ||
| as: 'namespace', | ||
| refreshModel: true, |
There was a problem hiding this comment.
@lgfa29 The reason this wasn't working earlier is that the refreshModel hook must be on the route and not the controller. There is no explanation why, but reading the test coverage - it appears that having refreshModel set on the route opts us into a full transition causing queryParams from the previous state to map to the new state.
ChaiWithJai
commented
Oct 26, 2021
Comment on lines
85
to
140
| 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' | ||
| ); | ||
| }); |
There was a problem hiding this comment.
We should remove these tests since they didn't catch the error. I'll just make a comment and that's how we'll live for now.
Ember Test Audit flaky testsEmber Test Audit detected these flaky tests on main:
Ember Test Audit detected these flaky tests on 21fdeef:
|
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.
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.
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.
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.
ChaiWithJai
force-pushed
the
b-ui/namespace-qp-fwd
branch
from
21fdeef
to
4cc749e
Compare
Ember Test Audit flaky testsEmber Test Audit detected these flaky tests on main:
|
|
Closing this one since it requires a different approach. |
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.