UI secret navigation improvements #5976
Conversation
meirish
force-pushed
the
ui-secret-navigation
branch
from
cddd3e0 to
5711204
Compare
| @@ -163,7 +164,7 @@ export default Component.extend(FocusOnInsertMixin, { | |||
| }), | |||
|
|
|||
| transitionToRoute() { | |||
| this.router.transitionTo(...arguments); | |||
| return this.router.transitionTo(...arguments); | |||
There was a problem hiding this comment.
What does returning here do? Still learning Ember
There was a problem hiding this comment.
The EC task needs to yield the promise in order to function properly and here were weren't returning it. The controller transitionToRoute already does return the Transition, which is a promise so this makes it so they match.
There was a problem hiding this comment.
Hey Matt!
I spotted something I might have in Consul that might help here, so posted that then made a few little comments partly to help me learn, and just partly to sanity check things.
I've not tried running this myself, and I might not get around to doing so today, but you have an approval from Madalyn so don't wait up on me if I don't get chance to come back here. I will try to have a peek at running this today, or soon, as I'd very much like to copy this approach in Consul (right now when we get a KV 404 we just blindly send you straight up to root 😬 )
Ta,
John
| await listPage.create(); | ||
| await editPage.createSecret('1/2', 'foo', 'bar'); | ||
|
|
||
| // lol we have to do this because ember-cli-page-object doesn't like *'s in visitable |
There was a problem hiding this comment.
Not sure if I misunderstood this comment but just incase, I saw this and thought I'd share how I got around what I think you mean here in Consul:
https://github.com/hashicorp/consul/blob/master/ui-v2/tests/lib/page-object/visitable.js#L38-L84
As far as I remember I mainly took this from ember-cli-page-object but tweaked it slightly with an injectable encoder so you can configure it to not encode the /s. I 'think' its as close as possible to the original visitable.
Feel free to steal if it helps, if you want to see usage somewhere I can looksee where I am using it and link you to it.
There was a problem hiding this comment.
Ha yep, that was exactly the issue I was having - for sure going to steal this later, but probably just leave as-is for now.
I see you added the ability to pass an array of paths too - how do you determine which one to visit when you do that?
There was a problem hiding this comment.
Cool, I owe you an aws icon anyway 😂
If you create the visitable with 2 paths, say:
visitable(['/:dc/kv/:kv', '/:dc/kv'], str => str).. and you don't provide all the keys when you try to visit it, something like:
visit(
{
dc: 'dc-1'
}
);then it will use the one that it has complete data for.
So that above visit call will use the second array item for the URL in the above code, and:
visit(
{
dc: 'dc-1',
kv: 'some/kv/deep/down'
}
);...would use the first one. Not sure if I've explained that well enough, ping me if you like if it's not clear.
I think (as far as I remember) I made it backwards compatible with the original implementation, so you can still just pass it a string like the original one.
There was a problem hiding this comment.
Ohhh I see it's the keys you pass in that determine the route you visit, that's the part I was missing. Thanks for explaining!
There was a problem hiding this comment.
Cool np, yeah in this case if I want the index page, I don't pass it a KV, whereas if I want to test an actual KV page I pass it a KV. Actually I'm just remembering now, I think I use it a lot for the 'create or edit' thing. If I pass a 'slug' it means I want an edit page, whereas if I don't I want a 'create'. It's almost like how the router works I suppose.
|
|
||
| // This mixin is currently used in a controller and a component, but we | ||
| // don't see cancellation of the task as the while loop runs in either | ||
|
|
There was a problem hiding this comment.
@gregone had some opinions on this type of mixin is currently used in a controller and a component stuff in hashicorp/consul#5056 (comment) .
In my case we judged it not to be a concern right now, does anything of what's said there apply here? Actually do Components have transitionToRoute methods?
There was a problem hiding this comment.
Ah yeah - in this case transitionToRoute was already a method on the component (we implemented it since we're routing there rather than bubbling out to the controller/route).
This mixin is also very specific to the secrets list route - it wouldn't be used elsewhere.
| errored = false; | ||
| } | ||
| } | ||
| this.transitionToRoute('vault.cluster.secrets.backend.list-root'); |
There was a problem hiding this comment.
@madalynrose also made a comment about returning transitions above, and I noticed you aren't using the result of transitionToRoute('vault.cluster.secrets.backend.list-root') here (and where you use navToNearestAncestor).
I'm still partly confused by Ember and Transitions myself and I've found the documentation on them to be sparse.
I've also been tripped up a few times by whether to return transitions or not, and I 'think' that in some places in Consul I do, and in other places I don't, not sure.
I think when I've been working with them previously I've kind of thought 'do everything I can to get it working by returning a transition/promise' and as far as I can remember one time where I didn't return one was because I couldn't for the life of me get things working in a test environment even though it was working fine in a normal environment.
I suppose the TL;DR of all this is, maybe just have a double think to make sure you don't need to return a transition here? If all your tests are fine etc I would think you don't, but I'd rather check with you than not say.
There was a problem hiding this comment.
Ah, P.S. I've just realised, it's an ember-concurrency task, sorry, does that mean you don't need to return transitions/promises? Sorry! Maybe ignore the above comment! Does that final this.transitionToRoute('vault.cluster.secrets.backend.list-root'); need a yield?
There was a problem hiding this comment.
Ah yeah yielding is a good catch! We're not using any of the derived state of this task at the moment, but it might be nice to do that in the future too.
Regarding returning transitions - they're promises so generally I think you want to return them so that the async route hooks block until the promise resolves.
There was a problem hiding this comment.
Added the yield in the latest! Thanks!
| testContainer: '#ember-testing', | ||
| }), | ||
| confirmDelete: clickable('[data-test-confirm-button]', { | ||
| testContainer: '#ember-testing', |
There was a problem hiding this comment.
Just wondering what the testContainer: '#ember-testing' does? How come you need to specify a context here?
There was a problem hiding this comment.
This has to do with ember-basic-dropdown and where the popup gets inserted into the DOM. The test application gets rendered inside of #ember-testing, so that's the default container and the "wormhole" gets rendered along side it. If you don't tell ember-cli-page-object where to look, it defaults to the application div as the test container, so it can't find things that render to the wormhole.
There was a problem hiding this comment.
Ah gotcha, cool thanks for the info (on all of these ^)
This fixes an issue where a whole branch would seem to disappear after creating a nested secret. In addition, this adds a mixin that can try transitions until it doesn't get a 404 for ancestor keys - this lets us delete a secret, then navigate to its nearest ancestor on success.
Fixes #5960