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 secret navigation improvements #5976

Merged
merged 7 commits into from
Dec 20, 2018
Merged

UI secret navigation improvements #5976

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0e2ff96
don't pass id when using createRecord
meirish Dec 19, 2018
fedc17e
add find nearest ancestor mixin
meirish Dec 19, 2018
9eb0cf9
re-throw the error if we've deleted something and encounter a 404
meirish Dec 19, 2018
173dd03
use the with-nav-to-nearest-ancestor mixin
meirish Dec 19, 2018
2ea4f10
add some comments
meirish Dec 19, 2018
5711204
add acceptance test to verify new behavior
meirish Dec 20, 2018
832a3f1
yield final transition in ec task
meirish Dec 20, 2018
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
8 changes: 5 additions & 3 deletions ui/app/components/secret-edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { computed, set } from '@ember/object';
import { alias, or } from '@ember/object/computed';
import { task, waitForEvent } from 'ember-concurrency';
import FocusOnInsertMixin from 'vault/mixins/focus-on-insert';
import WithNavToNearestAncestor from 'vault/mixins/with-nav-to-nearest-ancestor';
import keys from 'vault/lib/keycodes';
import KVObject from 'vault/lib/kv-object';
import { maybeQueryRecord } from 'vault/macros/maybe-query-record';
Expand All @@ -13,7 +14,7 @@ const LIST_ROUTE = 'vault.cluster.secrets.backend.list';
const LIST_ROOT_ROUTE = 'vault.cluster.secrets.backend.list-root';
const SHOW_ROUTE = 'vault.cluster.secrets.backend.show';

export default Component.extend(FocusOnInsertMixin, {
export default Component.extend(FocusOnInsertMixin, WithNavToNearestAncestor, {
wizard: service(),
router: service(),
store: service(),
Expand Down Expand Up @@ -163,7 +164,7 @@ export default Component.extend(FocusOnInsertMixin, {
}),

transitionToRoute() {
this.router.transitionTo(...arguments);
return this.router.transitionTo(...arguments);
Copy link
Contributor

Choose a reason for hiding this comment

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

What does returning here do? Still learning Ember

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

},

onEscape(e) {
Expand Down Expand Up @@ -304,8 +305,9 @@ export default Component.extend(FocusOnInsertMixin, {
},

deleteKey() {
let { id } = this.model;
this.model.destroyRecord().then(() => {
this.transitionToRoute(LIST_ROOT_ROUTE);
this.navToNearestAncestor.perform(id);
});
},

Expand Down
5 changes: 3 additions & 2 deletions ui/app/controllers/vault/cluster/secrets/backend/list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@ import { inject as service } from '@ember/service';
import Controller from '@ember/controller';
import utils from 'vault/lib/key-utils';
import BackendCrumbMixin from 'vault/mixins/backend-crumb';
import WithNavToNearestAncestor from 'vault/mixins/with-nav-to-nearest-ancestor';

export default Controller.extend(BackendCrumbMixin, {
export default Controller.extend(BackendCrumbMixin, WithNavToNearestAncestor, {
flashMessages: service(),
queryParams: ['page', 'pageFilter', 'tab'],

Expand Down Expand Up @@ -71,7 +72,7 @@ export default Controller.extend(BackendCrumbMixin, {
this.get('flashMessages').success(`${name} was successfully deleted.`);
this.send('reload');
if (type === 'secret') {
return this.transitionToRoute('vault.cluster.secrets.backend.list-root');
this.navToNearestAncestor.perform(name);
}
});
},
Expand Down
2 changes: 1 addition & 1 deletion ui/app/mixins/key-mixin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { computed } from '@ember/object';
import Mixin from '@ember/object/mixin';
import utils from '../lib/key-utils';
import utils from 'vault/lib/key-utils';

export default Mixin.create({
// what attribute has the path for the key
Expand Down
40 changes: 40 additions & 0 deletions ui/app/mixins/with-nav-to-nearest-ancestor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import Mixin from '@ember/object/mixin';
import utils from 'vault/lib/key-utils';
import { task } from 'ember-concurrency';

// 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

Copy link

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

// Controller in Ember are singletons so there's no cancellation there
// during the loop. For components, it might be expected that the task would
// be cancelled when we transitioned to a new route and a rerender occured, but this is not
// the case since we are catching the error. Since Ember's route transitions are lazy
// and we're catching any 404s, the loop continues until the transtion succeeds, or exhausts
// the ancestors array and transitions to the root
export default Mixin.create({
navToNearestAncestor: task(function*(key) {
let ancestors = utils.ancestorKeysForKey(key);
let errored = false;
let nearest = ancestors.pop();
while (nearest) {
try {
let transition = this.transitionToRoute('vault.cluster.secrets.backend.list', nearest);
transition.data.isDeletion = true;
yield transition.promise;
} catch (e) {
// in the route error event handler, we're only throwing when it's a 404,
// other errors will be in the route and will not be caught, so the task will complete
errored = true;
nearest = ancestors.pop();
} finally {
if (!errored) {
nearest = null;
// eslint-disable-next-line
return;
}
errored = false;
}
}
yield this.transitionToRoute('vault.cluster.secrets.backend.list-root');
}),
});
6 changes: 3 additions & 3 deletions ui/app/routes/vault/cluster/secrets/backend/create-root.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ let secretModel = (store, backend, key) => {
let backendModel = store.peekRecord('secret-engine', backend);
let modelType = backendModel.get('modelTypeForKV');
if (modelType !== 'secret-v2') {
return store.createRecord(modelType, {
id: key,
});
let model = store.createRecord(modelType);
model.set('id', key);
return model;
}
let secret = store.createRecord(modelType);
secret.set('engine', backendModel);
Expand Down
15 changes: 11 additions & 4 deletions ui/app/routes/vault/cluster/secrets/backend/list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,15 +148,22 @@ export default Route.extend({

actions: {
error(error, transition) {
const { secret } = this.paramsFor(this.routeName);
const { backend } = this.paramsFor('vault.cluster.secrets.backend');
let { secret } = this.paramsFor(this.routeName);
let { backend } = this.paramsFor('vault.cluster.secrets.backend');
let is404 = error.httpStatus === 404;
let hasModel = this.controllerFor(this.routeName).get('hasModel');

// this will occur if we've deleted something,
// and navigate to its parent and the parent doesn't exist -
// this if often the case with nested keys in kv-like engines
if (transition.data.isDeletion && is404) {
throw error;
}
set(error, 'secret', secret);
set(error, 'isRoot', true);
set(error, 'backend', backend);
const hasModel = this.controllerFor(this.routeName).get('hasModel');
// only swallow the error if we have a previous model
if (hasModel && error.httpStatus === 404) {
if (hasModel && is404) {
this.set('has404', true);
transition.abort();
return false;
Expand Down
61 changes: 60 additions & 1 deletion ui/tests/acceptance/secrets/backend/kv/secret-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { currentURL, currentRouteName } from '@ember/test-helpers';
import { settled, currentURL, currentRouteName } from '@ember/test-helpers';
import { create } from 'ember-cli-page-object';
import { module, test } from 'qunit';
import { setupApplicationTest } from 'ember-qunit';
Expand Down Expand Up @@ -79,6 +79,65 @@ module('Acceptance | secrets/secret/create', function(hooks) {
assert.ok(showPage.editIsPresent, 'shows the edit button');
});

// https://github.com/hashicorp/vault/issues/5960
test('version 1: nested paths creation maintains ability to navigate the tree', async function(assert) {
let enginePath = `kv-${new Date().getTime()}`;
let secretPath = '1/2/3/4';
// mount version 1 engine
await mountSecrets.visit();
await mountSecrets.selectType('kv');
await withFlash(
mountSecrets
.next()
.path(enginePath)
.version(1)
.submit()
);

await listPage.create();
await editPage.createSecret(secretPath, 'foo', 'bar');

// setup an ancestor for when we delete
await listPage.visitRoot({ backend: enginePath });
await listPage.secrets.filterBy('text', '1/')[0].click();
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
Copy link

@johncowen johncowen Dec 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link

@johncowen johncowen Dec 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

Copy link

Choose a reason for hiding this comment

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

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.

await listPage.visitRoot({ backend: enginePath });
await listPage.secrets.filterBy('text', '1/')[0].click();
await listPage.secrets.filterBy('text', '2/')[0].click();
await listPage.secrets.filterBy('text', '3/')[0].click();
await listPage.create();

await editPage.createSecret(secretPath + 'a', 'foo', 'bar');
await listPage.visitRoot({ backend: enginePath });
await listPage.secrets.filterBy('text', '1/')[0].click();
await listPage.secrets.filterBy('text', '2/')[0].click();
let secretLink = listPage.secrets.filterBy('text', '3/')[0];
assert.ok(secretLink, 'link to the 3/ branch displays properly');

await listPage.secrets.filterBy('text', '3/')[0].click();
await listPage.secrets[0].menuToggle();
await settled();
await listPage.delete();
await listPage.confirmDelete();
await settled();
assert.equal(currentRouteName(), 'vault.cluster.secrets.backend.list');
assert.equal(currentURL(), `/vault/secrets/${enginePath}/list/1/2/3/`, 'remains on the page');

await listPage.secrets[0].menuToggle();
await listPage.delete();
await listPage.confirmDelete();
await settled();
assert.equal(currentRouteName(), 'vault.cluster.secrets.backend.list');
assert.equal(
currentURL(),
`/vault/secrets/${enginePath}/list/1/`,
'navigates to the ancestor created earlier'
);
});

test('it redirects to the path ending in / for list pages', async function(assert) {
const path = `foo/bar/kv-path-${new Date().getTime()}`;
await listPage.visitRoot({ backend: 'secret' });
Expand Down
12 changes: 9 additions & 3 deletions ui/tests/pages/secrets/backend/list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { create, collection, visitable, clickable, isPresent } from 'ember-cli-page-object';
import { create, collection, text, visitable, clickable, isPresent } from 'ember-cli-page-object';
import { getter } from 'ember-cli-page-object/macros';

export default create({
Expand All @@ -8,15 +8,21 @@ export default create({
createIsPresent: isPresent('[data-test-secret-create]'),
configure: clickable('[data-test-secret-backend-configure]'),
configureIsPresent: isPresent('[data-test-secret-backend-configure]'),

tabs: collection('[data-test-tab]'),
secrets: collection('[data-test-secret-link]', {
menuToggle: clickable('[data-test-popup-menu-trigger]'),
id: text(),
click: clickable(),
}),
menuItems: collection('.ember-basic-dropdown-content li', {
testContainer: '#ember-testing',
}),

delete: clickable('[data-test-confirm-action-trigger]', {
testContainer: '#ember-testing',
}),
confirmDelete: clickable('[data-test-confirm-button]', {
testContainer: '#ember-testing',
Copy link

Choose a reason for hiding this comment

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

Just wondering what the testContainer: '#ember-testing' does? How come you need to specify a context here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

Ah gotcha, cool thanks for the info (on all of these ^)

}),
backendIsEmpty: getter(function() {
return this.secrets.length === 0;
}),
Expand Down