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

fix nav-to-nearest mixin when there are no ancestors #6198

Merged
merged 1 commit into from
Feb 9, 2019
Merged

fix nav-to-nearest mixin when there are no ancestors #6198

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion ui/app/mixins/with-nav-to-nearest-ancestor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export default Mixin.create({
navToNearestAncestor: task(function*(key) {
let ancestors = utils.ancestorKeysForKey(key);
let errored = false;
let nearest = ancestors.pop();
let nearest = ancestors && ancestors.pop();
Copy link

@johncowen johncowen Feb 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Hey 👋

Just a little observation/suggestion here. I'm not even sure this is possible as I don't know how else you are using utils.ancestorKeysForKey.

Theoretically you could have this bug happening anywhere you are using utils.ancestorKeysForKey just it's not been noticed it yet, but you've only added the ancestors truthy check here.

Ideally, (although I'm not sure if its possible, and would be a bigger/riskier change). If utils.ancestorKeysForKey returned an empty array instead of null where there are no 'ancestorKeys', then your while here would still not run ([].pop() === undefined)

I just had a quick look to see where else you are using this method:

screenshot 2019-02-11 at 09 52 16

In namespace-picker you switch a null result back to an empty array anyway, and in key-value-header you do another truthy check on the return value which you could switch to a length check (Q: How many ancestors are there for this key? A: Zero)

So this is possible I 'think' (as long as my search picked up all places you are using this) and it's probably not that risky to do either and doing this would prevent any further bugs like this slipping in in the future.

TLDR; is You'd be changing it so the method would always return an array (no alarms and no surprises), not 'mostly an array, but sometimes null'.

Anyway, I just had a peek here whilst drinking my morning tea, thought you might like that ^ - no matter if not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out - been waayyy too long since I've looked at the util code and took a wider view of its usage. I made the necessary changes / cleanup in #6205

while (nearest) {
try {
let transition = this.transitionToRoute('vault.cluster.secrets.backend.list', nearest);
Expand Down
23 changes: 23 additions & 0 deletions ui/tests/acceptance/secrets/backend/kv/secret-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,29 @@ module('Acceptance | secrets/secret/create', function(hooks) {
'navigates to the ancestor created earlier'
);
});
test('first level secrets redirect properly upon deletion', async function(assert) {
let enginePath = `kv-${new Date().getTime()}`;
let secretPath = 'test';
// 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');
await showPage.deleteSecret();
assert.equal(
currentRouteName(),
'vault.cluster.secrets.backend.list-root',
'redirected to the list page on delete'
);
});

// https://github.com/hashicorp/vault/issues/5994
test('version 1: key named keys', async function(assert) {
Expand Down
5 changes: 5 additions & 0 deletions ui/tests/pages/secrets/backend/kv/show.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import { code } from 'vault/tests/pages/helpers/codemirror';

export default create({
...Base,
deleteBtn: clickable('[data-test-secret-delete] button'),
confirmBtn: clickable('[data-test-confirm-button]'),
rows: collection('data-test-row-label'),
toggleJSON: clickable('[data-test-secret-json-toggle]'),
toggleIsPresent: isPresent('[data-test-secret-json-toggle]'),
Expand All @@ -12,4 +14,7 @@ export default create({
editor: {
content: code('[data-test-component="json-editor"]'),
},
deleteSecret() {
return this.deleteBtn().confirmBtn();
},
});