PKI role create view

[Monkeychip]

This PR handles the PKI role creation for the default values only. The values hidden behind the Toggle options are more complicated and will have some design implications I wanted to keep in a separate PR (stay tuned).

Below is a video. The form correctly sends the default params on the network request, including the custom yield param (ttl and not_after).

roles_create.mov

Notes:

• Moving the secretListHeader to the respective index routes will be the new pattern. The struggle with this, and why I originally had it on the parent "roles" route, is that the models for the roles vs the roles/index are different. The roles route is the secret-engine mount model and the roles/index route is the list of roles. To solve for this, I added a modelFor in the roles/index route. Please comment if you see any issues with this.
• We can now have custom params on models that yield out whatever components you want. All you need to do is add editType: yield to the model and then yield out whatever you want between formFields.

ex:

// model
  @attr({
    label: 'Some label',
    editType: 'yield',
  })
  customComponent;

// some component
<FormField>
  <YieldedComponent or code />
</FormField>

TODO:

• in the next PR handle the line-border styling at the bottom of the form.

[Monkeychip]

clean up (thank you Claire for pointing this out).

[Monkeychip]

New component that I will use several times in PKI but unsure if it will be used in future designs.

newcomponent.mov

[Monkeychip]

For the new component, needed them to be closer to the label so they seemed "grouped" together.

[Monkeychip]

I suspect there's a better way to do this, but I couldn't figure out how shape the model without giving it a label. I tried spread attrs, and that didn't work.

[zofskeez]

You can change the key specified in RSVP.hash to something other than model

[Monkeychip]

Jordan, you might have some comments on this. I tried not using hash, but had issues with it returning a promise and not a class. I tried a then, and that didn't work.

[zofskeez]

Since modelFor does not return a promise you could return an object in the then of query:

return this.store
  .query('pki/pki-role-engine', { backend: this.secretMountPath.currentPath })
  .then((model) => {
    return { model, parentModel: this.modelFor('roles') };
  })
  .catch((err) => {
    if (err.httpStatus === 404) {
      return [];
    } else {
      throw err;
    }
  });

I don't mind using RSVP.hash though too. It just might not be immediately clear to someone who is unfamiliar with the method.

I would avoid using let to assign those variables since they are not being reassigned if you decide to leave as is.

[Monkeychip]

Awesome, thank you. I see now how to structure the then. I prefer this method only because it doesn't rely on an import, it's pure js.

[Monkeychip]

Some issues on the formField component with the attribute showHelpText not being added to all form field types.

[Monkeychip]

this was my bad 🤦🏻‍♀️

[Monkeychip]

And yeah, ideally we wouldn't return a true by default on a param, but this is an old pattern and if we choose to modify should be done in a cleanup ticket.

[hellobontempo]

Made some comments! Nice work on this. PKI is 🤯

Great job on the new component! It'd be nice to have some tests written for it, maybe as a follow on after this PR? It'd be nice to have early on in PKI development to confirm that the values sent are as expected.

[hellobontempo]

I'm not sure this comment is necessary as we've used this pattern of iterating over FormFields outside of the loop component

[Monkeychip]

I guess I just wanted to make a comment in case a future developer came along and noticed the code was nearly identical to the FormFieldGroupsLoop component and wondered why I didn't just use that component instead.

[zofskeez]

I think I recall discussing this and maybe I didn't have the full context at the time but slightly modifying FormFieldGroupsLoop will prevent having to copy the code out.

Since you added the yield to FormField you can yield that up from the FormFieldGroupsLoop as well. Update both instances to something like:

<FormField data-test-field={{true}} @attr={{attr}} @model={{@model}}>
  {{yield attr}}
</FormField

Then you can use the RadioSelectTtlOrString component which will render in the yield block of FormField:

<FormFieldGroupsLoop @model={{this.model}} @mode={{this.mode}} as |attr|>
  <RadioSelectTtlOrString @model={{@model}} @attr={{attr}} />
</FormFieldGroupsLoop>

It looks like you also need modelValidations and showHelpText passed in so you could add those as args to the groups loop as well.

In a situation where you might have multiple different attr types that yield you can just do a check in the block and display the proper component.

<FormFieldGroupsLoop @model={{this.model}} @mode={{this.mode}} as |attr|>
  {{#if attr.type "someType"}}
    <RadioSelectTtlOrString @model={{@model}} @attr={{attr}} />
  {{else if attr.type "someOtherType"}}
    <SomeOtherComponent />
  {{/if}}
</FormFieldGroupsLoop>

[Monkeychip]

Thank you! (reviewed over zoom), but passing through the attr in the yield the way you showed was awesome. A lot cleaner now.

[zofskeez]

Comparing this to the base method and wondering if you can use it and just update the response? Something like:

createRecord(store, type, snapshot){
  return super.createRecord(...arguments).then(() => {
    return { id: name, name, backend: snapshot.record.backend };
  });
}

[zofskeez]

🎉

[zofskeez]

Ternary might be cleaner here:

return this.args.showHelpText === false ? false : true;

[Monkeychip]

Yes! Thank you. I knew there was a cleaner way.

[zofskeez]

I'm wondering if these props need to be initialized to empty strings? Is there somewhere that expects the value to always be a string, as in maybe does a charAt or calls some other method on the prototype without first checking for existence?

[Monkeychip]

Good question. They are supposed to be strings, and ttl specifically says empty, though not_after doesn't seem to care. But the api seems to handle this. The only thing I think calling them out as strings does here, it let the next developer know these are not numbers but strings. But it's a toss up. I'll ahead and remove the default.

[zofskeez]

I don't feel strongly about it but was curious. Since you are defining them as tracked properties it doesn't hurt to give them an initial value.

[zofskeez]

I think I recall discussing this and maybe I didn't have the full context at the time but slightly modifying FormFieldGroupsLoop will prevent having to copy the code out.

Since you added the yield to FormField you can yield that up from the FormFieldGroupsLoop as well. Update both instances to something like:

<FormField data-test-field={{true}} @attr={{attr}} @model={{@model}}>
  {{yield attr}}
</FormField

Then you can use the RadioSelectTtlOrString component which will render in the yield block of FormField:

<FormFieldGroupsLoop @model={{this.model}} @mode={{this.mode}} as |attr|>
  <RadioSelectTtlOrString @model={{@model}} @attr={{attr}} />
</FormFieldGroupsLoop>

It looks like you also need modelValidations and showHelpText passed in so you could add those as args to the groups loop as well.

In a situation where you might have multiple different attr types that yield you can just do a check in the block and display the proper component.

<FormFieldGroupsLoop @model={{this.model}} @mode={{this.mode}} as |attr|>
  {{#if attr.type "someType"}}
    <RadioSelectTtlOrString @model={{@model}} @attr={{attr}} />
  {{else if attr.type "someOtherType"}}
    <SomeOtherComponent />
  {{/if}}
</FormFieldGroupsLoop>

[zofskeez]

Since modelFor does not return a promise you could return an object in the then of query:

return this.store
  .query('pki/pki-role-engine', { backend: this.secretMountPath.currentPath })
  .then((model) => {
    return { model, parentModel: this.modelFor('roles') };
  })
  .catch((err) => {
    if (err.httpStatus === 404) {
      return [];
    } else {
      throw err;
    }
  });

I don't mind using RSVP.hash though too. It just might not be immediately clear to someone who is unfamiliar with the method.

I would avoid using let to assign those variables since they are not being reassigned if you decide to leave as is.

[zofskeez]

You can change the key specified in RSVP.hash to something other than model