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

LDAP Roles Create and Edit #21818

Merged

Conversation

zofskeez
Copy link
Contributor

This PR adds a page component for creating and editing LDAP roles.

image image image

@zofskeez zofskeez added this to the 1.15 milestone Jul 13, 2023
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.

Mostly some comments/questions! Just one copy change requested for the flash message text

ui/app/styles/components/ttl-picker.scss Outdated Show resolved Hide resolved
<button
type="button"
class="toolbar-link"
disabled={{not @value}}
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 restore button is disabled if there's no value? I'd think that we'd want to also be able to restore example if the input is empty 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there is no value the example is showing so there is nothing to restore. As soon as the user makes any change to the example the value is set and then the original example can be restored.

I think I see your point though. If the user clears out the input the example will display which may lead them to believe that it will be saved which is not the case. I think I need more clarity from design on how this should function. We could show the editor as empty if there is no value set and allow the example to be restored but I'm also wondering now if restoring the example should also set it to the value or if we wait for explicit input in the text area.

Copy link
Contributor Author

@zofskeez zofskeez Jul 20, 2023

Choose a reason for hiding this comment

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

Quick follow up. I found this in the designs which seems to answer the question:

LDIF STATEMENTS
We will provide some default example here, but those are considered as comments and we won’t submit them during create

I think we are treating the examples as a placeholder when the user has not input a value.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose this is more of a pattern question, but I would have expected the page components to correspond more to the route and then each component render a <Ldap::RoleForm> component or something. This is what we did in PKI, but using page components is a newer pattern for us, so it might be worth clarifying how we want to use them. I may have wrongly assumed they were intended to correspond more directly to the route for testing purposes.

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 might be something that I decided on during the k8s project but since the views are almost identical other than the title, it didn't make sense to me to create multiple components which would require more tests. The pattern docs don't mention anything in regards to requiring a page component per route but I think with any of the patterns they should be treated as guidelines with the flexibility to do what works best for the project.

<label class="is-label">
Role type
</label>
<Hds::Form::RadioCard::Group @name="role type options" class="has-bottom-margin-m" as |RadioGroup|>
Copy link
Contributor

Choose a reason for hiding this comment

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

If there isn't already - could you make a Jira ticket tracking this HDS component has been implemented?

Thank you! 🤩

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! I did make tickets and left a comment regarding <Hds::Form::RadioCard::Group> since <Hds::Form:RadioCard> can be used on its own but it seems like we should consider it the same adoption (you wouldn't use the group component with the cards).


{{#each @model.formFields as |field|}}
{{! display section heading ahead of ldif fields }}
{{#if field.options.sectionHeading}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity - why didn't you opt for using form field groups here?

I don't really see a problem with adding model-specific options, especially when we're not adding them to the already intense <FormField> situation. But just curious why you went this route 😄

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 should have responded sooner because now I can't remember exactly but there definitely was a reason because I originally tried using form groups 😁!

If I recall now it's because we need to dynamically generate the form fields based on the role type (static or dyanmic), so I am not passing anything into the withFormFields decorator and relying on the allFields array to filter through when the type is changed.

const { model } = this.args;
const cleanupMethod = model.isNew ? 'unloadRecord' : 'rollbackAttributes';
model[cleanupMethod]();
this.router.transitionTo('vault.cluster.secrets.backend.ldap.roles');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should cancel bring us back to the details view if we're editing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question. I see both the list and details routes navigated back to on cancel. Something to bring up with design. The edit route can be accessed from the list view as well as details so without tracking the previously active route we need to have a default and it should be consistent.

ui/lib/ldap/addon/routes/roles/role/index.ts Show resolved Hide resolved
/>
`);

assert.dom('.CodeMirror-code').hasText(`1${this.example}`, 'Example renders when there is no value');
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 1 something codemirror adds?

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 so I've spent more time then I would like to admit trying to extract only the user inputted text from the code editor for a strict comparison but the textarea is actually hidden and then it renders the input in divs as individual code lines and then adds the line numbers.

@zofskeez zofskeez merged commit 6bea9cc into ui/VAULT-12945/ldap-secrets-engine Jul 20, 2023
@zofskeez zofskeez deleted the ui/VAULT-16548/ldap-role-create branch July 20, 2023 21:48
@zofskeez zofskeez added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Aug 2, 2023
Monkeychip added a commit that referenced this pull request Aug 25, 2023
* adds ldap ember engine (#20786)

* adds ldap as mountable and supported secrets engine (#20793)

* removes active directory as mountable secrets engine (#20798)

* LDAP Config Ember Data Setup (#20863)

* adds secret-engine-path adapter

* adds model, adapater and serializer for ldap config

* adds test for ldap config adapter

* addresses PR feedback

* updates remaining instances of getURL in secrets-engine-path adapter

* adds underscore to getURL method in kubernetes/config adapter

* adds check config vars test for kubernetes/config adapter

* adds comment regarding primaryKey in secrets-engine-path adapter

* adds tab-page-header component for ldap secrets engine (#20941)

* LDAP Config Route (#21059)

* converts secret-mount-path service to ts and moves kubernetes fetch-config decorator to core addon and converts to ts

* adds ldap config route

* fixes withConfig import path in kubernetes roles route

* updates types in ldap config route

* adds unit tests for fetch-secret-config decorator

* updates comments in fetch-secret-config decorator

* renames fetch-secret-config decorator

* LDAP Configure Page Component (#21384)

* adds ldap page configure component

* removes pauseTest and updates radio card selector in ldap config test

* LDAP Configuration (#21430)

* adds ldap configuration route

* adds secrets-engine-mount-config component to core addon

* adds ldap config-cta component

* adds display fields to ldap configuration page and test

* fixes ldap config-cta test

* adds yield to secrets-engine-mount-config component

* fixes tests

* LDAP Overview Route and Page Component (#21579)

* adds ldap overview route and page component

* changes toolbar link action type for create role on overview page

* LDAP Role Model, Adapter and Serializer (#21655)

* adds model, adapter and serializer for ldap roles

* addresses review feedback

* changes ldap role type from tracked prop to attr and sets in adapter for query methods

* adds assertions to verify that frontend only props are returned from query methods in ldap role adapter

* LDAP Library Model, Adapter and Serializer (#21728)

* adds model, adapter and serializer for ldap library

* updates capitalization and punction for ldap role and library form fields

* LDAP Roles Create and Edit (#21818)

* moves stringify and jsonify helpers to core addon

* adds validation error for ttl picker in form field component

* adds ldap roles create and edit routes and page component

* adds ldap mirage handler and factory for roles

* adds example workflow to json editor component

* adds tests for ldap page create and edit component

* addresses feedback

* LDAP Role Details (#22036)

* adds ldap role route to pass down model to child routes

* adds ldap role details route and page component

* updates ldap role model capabilities checks

* adds periods to error messages

* removes modelFor from ldap roles edit and details routes

* adds flash message on ldap role delete success

* LDAP Roles (#22070)

* adds ldap roles route and page component

* update ldap role adapter tests and adds adapter options to query for partialErrorInfo

* updates ldap role adapter based on PR feedback

* adds filter-input component to core addon

* updates ldap roles page to use filter-input component

* updates ldap role adapter tests

* LDAP Role Credentials (#22142)

* adds ldap roles route and page component

* update ldap role adapter tests and adds adapter options to query for partialErrorInfo

* adds credentials actions to ldap roles list menu and fixes rotate action in details view

* adds ldap role credentials route and page component

* adds tests for ldap role credentials

* LDAP Library Create and Edit (#22171)

* adds ldap library create/edit routes and page component

* adds ldap library create-and-edit tests and library mirage factory

* updates form-field component to display validation errors and warnings for all fields

* updates ldap library edit route class name

* updates ldap library model interface name

* adds missing period in flash message

* LDAP Libraries (#22184)

* updates interface and class names in ldap roles route

* adds ldap libraries route and page component

* fixes lint error

* LDAP Library Details (#22200)

* updates interface and class names in ldap roles route

* adds ldap libraries route and page component

* fixes lint error

* adds ldap library details route and page component

* LDAP Library Details Configuration (#22201)

* updates interface and class names in ldap roles route

* adds ldap libraries route and page component

* fixes lint error

* adds ldap library details route and page component

* adds ldap library details configuration route and page component

* updates ldap library check-in enforcement value mapping

* fixes issue in code mirror modifier after merging upgrade

* fixes failing database secrets test

* LDAP Library Account Details (#22287)

* adds route and page component for ldap library accounts

* adds ldap component for checked out accounts

* updates ldap library adapter tests

* LDAP Library Check-out (#22289)

* adds route and page component for ldap library accounts

* adds ldap component for checked out accounts

* adds route and page component for ldap library checkout

* addresses PR feedback

* LDAP Overview Cards (#22325)

* adds overview cards to ldap overview route

* adds create library toolbar action to ldap overview route

* adds acceptance tests for ldap workflows (#22375)

* Fetch Secrets Engine Config Decorator Docs (#22416)

* removes uneccesary asyncs from ldap route model hooks

* updates ldap overview route class name

* adds documentation for fetch-secrets-engine-config decorator

* add changelog

* adding back external links, missed due to merge.

* changelog

* fix test after merging in dashboard work

* Update 20790.txt

---------

Co-authored-by: Angel Garbarino <[email protected]>
Co-authored-by: Angel Garbarino <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed pr/no-changelog ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants