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: refactor pki role form to reuse PkiKeyParameters component #18069

Merged
merged 2 commits into from
Nov 21, 2022

Conversation

hellobontempo
Copy link
Contributor

@hellobontempo hellobontempo commented Nov 21, 2022

This PR removes any <ToggleGroup>-ing from the new pki form components to the parent so they can be reused elsewhere

@@ -287,6 +270,11 @@ export default class PkiRoleModel extends Model {
docLink: '/api-docs/secret/pki#allowed_domains',
},
},
'Key parameters': {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed <FormFieldLabel> from the component template below, so parent is responsible for rendering toggle group-specific text

</div>
</div>
{{else}}
<div class="field">
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 didn't see the need for this to be anything other than a general <FormField> so it could render any attr type?

@onClick={{fn (mut (get @model prop))}}
class="is-block"
data-test-toggle-group={{@group}}
<CheckboxGrid
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Figured I'd apply the same pattern of parent handling the toggle business?

@@ -35,10 +36,9 @@ export default class PkiKeyParameters extends Component {

@action onSignatureBitsOrKeyTypeChange(name, selection) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keeping this function as is for now, but I'd like to convert this file to typescript and will refactor these methods accordingly!

{{#if groupInfo.footer}}
{{! FIELDS }}
{{#if (eq group "Key usage")}}
<PkiKeyUsage @model={{@model}} />
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 didn't see a need to pass anything but @model down? Unless I totally missed something

{{! Header }}
{{#if groupInfo.header}}
<div class="box is-tall is-marginless" data-test-toggle-div={{group}}>
{{#let (get @model.fieldGroupsInfo group) as |toggleGroup|}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

toggleGroup came to mind here as a clearer variable name, but open to suggestions! Also happy to keep as groupInfo

@@ -13,6 +13,7 @@ module('Integration | Component | pki-key-parameters', function (hooks) {
this.store = this.owner.lookup('service:store');
this.model = this.store.createRecord('pki/role');
this.model.backend = 'pki';
[this.fields] = Object.values(this.model.fieldGroups.find((g) => g['Key parameters']));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

rather than hardcoding fields - decided to pull from the model so that if there are openapi changes they will be reflected in the test

though I think I'll move checking for these values/defaults to the pki role-form itself and generalize this component test when writing test for generating a key

Copy link
Contributor

@zofskeez zofskeez left a comment

Choose a reason for hiding this comment

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

Nice updates thanks!

ui/app/models/pki/role.js Show resolved Hide resolved
<FormFieldLabel for={{attr.name}} @label={{attr.options.label}} />
<div class="control is-expanded">
<div class="select is-fullwidth">
<select name={{attr.name}} id={{attr.name}} onchange={{this.onKeyBitsChange}} data-test-input={{attr.name}}>
Copy link
Contributor

Choose a reason for hiding this comment

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

All these code scanning warnings seem new?? We have many instances where html attribute values are bound without the wrapping quotes and I have never noticed this before. It also looks like it's complaining for arguments as well.

@hellobontempo hellobontempo merged commit 4690c54 into main Nov 21, 2022
@hellobontempo hellobontempo deleted the ui/VAULT-11643/abstract-key-parameters branch November 21, 2022 22:58
jayant07-yb pushed a commit to jayant07-yb/hashicorp-vault-integrations that referenced this pull request Mar 15, 2023
…corp#18069)

* abstract pki-key-parameters from pki-role-form

* finish refactor, update tests
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