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

PKI Issuer Edit #18687

Merged
merged 3 commits into from
Jan 12, 2023
Merged

PKI Issuer Edit #18687

merged 3 commits into from
Jan 12, 2023

Conversation

zofskeez
Copy link
Contributor

Adds PKI issuer edit form.
image

Comment on lines +1 to +10
import Route from '@ember/routing/route';
import { inject as service } from '@ember/service';

export default class PkiRoute extends Route {
@service router;

redirect() {
this.router.transitionTo('vault.cluster.secrets.backend.pki.overview');
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I navigated to the root initially and noticed the blank page so I added a redirect to the overview route.

@@ -383,7 +383,7 @@ module('Acceptance | pki workflow', function (hooks) {
.exists({ count: 9 }, 'Renders 9 info table items under default group');
assert
.dom(`${SELECTORS.issuerDetails.urlsGroup} ${SELECTORS.issuerDetails.row}`)
.exists({ count: 4 }, 'Renders 4 info table items under URLs group');
.exists({ count: 3 }, 'Renders 4 info table items under URLs group');
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 is the result of removing delta_crl_urls

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.

Just some comments! Brain needs some food, so holding off approving

ui/app/models/pki/issuer.js Outdated Show resolved Hide resolved
ui/app/serializers/pki/issuer.js Show resolved Hide resolved
ui/lib/pki/addon/components/page/pki-issuer-edit.hbs Outdated Show resolved Hide resolved
ui/app/models/pki/issuer.js Outdated Show resolved Hide resolved
ui/app/models/pki/issuer.js Show resolved Hide resolved

constructor(owner: unknown, args: Args) {
super(owner, args);
this.usageValues = (this.args.model.usage || '').split(',');
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be a potential use-case for making a transform of attr type stringArray? Maybe it doesn't come up enough, but I feel like there are lots cases where we get a string array from the backend and want it to actually be an array of strings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya that's a really good suggestion. It does seem like this comes up frequently enough that a transform would be helpful so I went ahead and implemented it. I was just about to push but I figured I should run the tests and ran into a catch.

The test for rolling back the attributes on cancel failed and it looks like Ember Data doesn't track the items in an array which is unfortunate. I think in order for it to be worthwhile we would need to figure out a way to retain the rollback functionality. We should definitely keep this in mind because I definitely think it would be valuable but will take a bit more effort than expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for investigating!! This is helpful discovery.

}

toDetails() {
this.router.transitionTo('vault.cluster.secrets.backend.pki.issuers.issuer.details');
Copy link
Contributor

Choose a reason for hiding this comment

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

you mixed it up here! usually I've seen you pass the transition action as an @onSave arg to the component. 💭

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hehe. Ya I suppose I did 😁. I guess in the last couple of projects where we have started using this page component pattern I've handled more things internally since the component is only used in the route template.

ui/package.json Show resolved Hide resolved
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! 🚀

@zofskeez zofskeez merged commit 5fd1b88 into main Jan 12, 2023
@zofskeez zofskeez deleted the ui/VAULT-9260/pki-issuer-edit branch January 12, 2023 23:33
@hellobontempo hellobontempo mentioned this pull request Jan 17, 2023
3 tasks
dhuckins pushed a commit that referenced this pull request Jan 19, 2023
* adds pki issuer edit view

* updates pki issuer details test and fixes styling issue in issuer edit form

* addresses feedback
dhuckins pushed a commit that referenced this pull request Jan 19, 2023
* adds pki issuer edit view

* updates pki issuer details test and fixes styling issue in issuer edit form

* addresses feedback
AnPucel pushed a commit that referenced this pull request Jan 25, 2023
* adds pki issuer edit view

* updates pki issuer details test and fixes styling issue in issuer edit form

* addresses feedback
AnPucel pushed a commit that referenced this pull request Feb 3, 2023
* adds pki issuer edit view

* updates pki issuer details test and fixes styling issue in issuer edit form

* addresses feedback
jayant07-yb pushed a commit to jayant07-yb/hashicorp-vault-integrations that referenced this pull request Mar 15, 2023
* adds pki issuer edit view

* updates pki issuer details test and fixes styling issue in issuer edit form

* addresses feedback
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