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.

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: PKI Role toolbar #18229

Merged
merged 13 commits into from
Dec 6, 2022
Merged

UI: PKI Role toolbar #18229

merged 13 commits into from
Dec 6, 2022

Conversation

hashishaw
Copy link
Collaborator

@hashishaw hashishaw commented Dec 5, 2022

This PR adds capability checks on the role toolbar links, adds the ability to delete the role, and cleans up some breadcrumb and route logic (mostly prompted by test coverage)

Role toolbar links go to the right place (not all pages built out yet)
role-toolbar-links

Role view when you only have read/list access
Reader view

Role view when you have all engine permissions
Admin view

@hashishaw hashishaw marked this pull request as ready for review December 5, 2022 20:22
Copy link
Contributor

@hellobontempo hellobontempo left a comment

Choose a reason for hiding this comment

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

Nice job! That workflow module is great! 😍 Mostly comments, just a couple small changes requested with the fieldToAttrs getter and the delete flash message

ui/app/models/pki/role.js Outdated Show resolved Hide resolved
// }
import FlashMessageService from 'vault/services/flash-messages';
import { inject as service } from '@ember/service';
import errorMessage from 'vault/utils/error-message';

// TODO: pull this in from route model once it's TS
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed this comment before - what does it mean? Just personal curiosity 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In cloud-ui, since the route is also TS we can export it from that file and then import it, and it knows the shape based on what's returned in the model hook. Although here, we really just need the ember-data model which I haven't quite figured out how to do yet 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

oh wow! neat - thanks for explaining! I'll also keep an 👀 for how to do this 🤔

ui/lib/pki/addon/components/page/pki-role-details.ts Outdated Show resolved Hide resolved
@@ -8,7 +8,7 @@ export default class PkiCertificatesIndexRoute extends Route {

beforeModel() {
// Must call this promise before the model hook otherwise it doesn't add OpenApi to record.
return this.pathHelp.getNewModel('pki/certificate', 'pki');
return this.pathHelp.getNewModel('pki/certificate', this.secretMountPath.currentPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

nice catch! 🥇

@@ -8,4 +8,16 @@ export default class RolesRoleDetailsRoute extends PkiRolesIndexRoute {
id: role,
});
}

setupController(controller, resolvedModel) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! I was wondering about a way to handle this in the controller 👏

hooks.afterEach(async function () {
await logout.visit();
await authPage.login();
// Cleanup engine
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

await logout.visit();
});

module('roles', function (hooks) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the thought to have a module for each resource?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that was my thought. Then we don't need to make policies etc for the entire thing, we can test each section piecemeal

}
};

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

this is great! 💯

ui/lib/pki/addon/templates/roles/role/edit.hbs Outdated Show resolved Hide resolved
Co-authored-by: claire bontempo <68122737+hellobontempo@users.noreply.github.com>
@vercel
Copy link

vercel bot commented Dec 6, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated
vault ⬜️ Ignored (Inspect) Dec 6, 2022 at 6:46PM (UTC)

Copy link
Contributor

@hellobontempo hellobontempo left a comment

Choose a reason for hiding this comment

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

🚀 nice job! I'm glad the decorator worked! Just a comment/question


const validations = {
name: [{ type: 'presence', message: 'Name is required.' }],
};

const fieldGroups = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! 👏

@@ -1,6 +1,6 @@
<Toolbar>
<ToolbarActions>
{{#if @role.canDelete}}
{{#if (or @role.updatePath.isLoading @role.canDelete)}}
Copy link
Contributor

Choose a reason for hiding this comment

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

So the toolbar action will show if the path is loading or they have permission?

For these capabilities conditionals - I remember a while ago you suggesting I explicitly check for false to avoid them not showing in case for some reason they're undefined. Not sure if you want to apply that same logic here? Although that was specifically for setting the disabled value of a link, so slightly different situation, plus the following is not very pretty 😵

    {{#if (or @role.updatePath.isLoading (not (eq @role.canDelete false)))}}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are other areas of the app where we use this or logic, but I do remember us checking explicitly for false at some point. The way I have it, if the capabilities aren't loaded yet the item will show, and once the value is loaded we can show the value.

however now that the capabilities values are getters instead of aliases, I think we can put the logic within the getter and that way we can rely on just a single check in the template! I'll update and you can let me know what you think

Copy link
Contributor

@hellobontempo hellobontempo left a comment

Choose a reason for hiding this comment

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

🚢 nice job! really like how you handled capabilities

@hashishaw hashishaw enabled auto-merge (squash) December 6, 2022 20:33
@hashishaw hashishaw merged commit 724e3fd into main Dec 6, 2022
@hashishaw hashishaw deleted the ui/VAULT-9349/delete-pki-role branch December 6, 2022 21:40
AnPucel pushed a commit that referenced this pull request Jan 14, 2023
jayant07-yb pushed a commit to jayant07-yb/hashicorp-vault-integrations that referenced this pull request Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants