Create sections for Secrets sync destination fields for create/edit view #27538
Conversation
github-actions
bot
added
the
hashicorp-contributed-pr
|
CI Results: |
Monkeychip
changed the title
|
Build Results: |
ui/lib/sync/addon/components/secrets/page/destinations/create-and-edit.hbs
Outdated
Show resolved
Hide resolved
| Connection credentials are sensitive information | ||
| {{#if @destination.isNew}} used to authenticate with the destination. {{else}} | ||
| and the value cannot be read. Enable the input to update. | ||
| {{/if}} |
There was a problem hiding this comment.
There is a lot more logic now that relies on whether a model isNew or not. Could we add test coverage for this logic?
There was a problem hiding this comment.
Done and dusted. Note the maskedInput vs wrapped maskedInputs by the enableInput component on create vs edit are covered in the create-and-edit test. They're a little hard to see. I added one for create in this PR. And the edit masked check is done by calling enableInput on the test here. If those values were not wrapped by the correct component, you couldn't find this value.
ui/lib/sync/addon/components/secrets/page/destinations/create-and-edit.hbs
Outdated
Show resolved
Hide resolved
ui/lib/sync/addon/components/secrets/page/destinations/create-and-edit.hbs
Outdated
Show resolved
Hide resolved
| this.model = this.generateModel(); | ||
|
|
||
| await this.renderFormComponent(); | ||
| assert.dom(PAGE.breadcrumbs).hasText('Secrets Sync Destinations Destination Edit Destination'); | ||
| assert.dom('h2').hasText('Credentials', 'renders credentials section on edit'); | ||
| assert |
There was a problem hiding this comment.
moved this into a subtext specific test
| class="has-bottom-margin-m" | ||
| data-test-destination-subText={{group}} | ||
| > | ||
| {{(this.groupSubtext group @destination.isNew)}} |
There was a problem hiding this comment.
| {{(this.groupSubtext group @destination.isNew)}} | |
| {{this.groupSubtext group @destination.isNew}} |
| class="has-bottom-margin-m" | ||
| data-test-destination-subText={{group}} | ||
| > | ||
| {{(this.groupSubtext group @destination.isNew)}} |
There was a problem hiding this comment.
instead of passing in isNew here can we just access this.args.destination.isNew in the component helper?
There was a problem hiding this comment.
I tried that and then realized I was hitting issues because this is within an each iteration, so it's easier to access the destination by passing it in like this.
| groupSubtext(group: string, isNew: boolean) { | ||
| const dynamicText = isNew | ||
| ? 'used to authenticate with the destination' | ||
| : 'and the value cannot be read. Enable the input to update'; |
There was a problem hiding this comment.
| groupSubtext(group: string, isNew: boolean) { | |
| const dynamicText = isNew | |
| ? 'used to authenticate with the destination' | |
| : 'and the value cannot be read. Enable the input to update'; | |
| groupSubtext(group: string) { | |
| const dynamicText = this.args.destination.isNew | |
| ? 'used to authenticate with the destination' | |
| : 'and the value cannot be read. Enable the input to update'; |
There was a problem hiding this comment.
I went this exact same route, but it fails to read the args. It might be because I'm iterating over via a each and then each-in and then an each again, but passing in via the template solved the problem
There was a problem hiding this comment.
strange! it must be how the template helper functions compile 🤔
ui/tests/integration/components/sync/secrets/page/destinations/create-and-edit-test.js
Outdated
Show resolved
Hide resolved
| const { type } = SYNC_DESTINATIONS[0]; | ||
| this.model = this.store.createRecord(`sync/destinations/${type}`, { type }); |
There was a problem hiding this comment.
If you look at the test above, this needs to be updated to the following to accurately render the edit view
| const { type } = SYNC_DESTINATIONS[0]; | |
| this.model = this.store.createRecord(`sync/destinations/${type}`, { type }); | |
| const { type } = SYNC_DESTINATIONS[0]; | |
| this.model = this.generateModel() |
There was a problem hiding this comment.
Also since these are "it renders" related tests, the assertions below could also just move into the test above. I don't think separate subtext tests are needed since the existing "edit: it renders" and "create: it renders" are so small already
There was a problem hiding this comment.
Good catch re: 1st comment.
Re 2nd comment: I broke these up because I wanted to isolate the isNew functionality from the transition and breadcrumbs. However, let me clear up the tests a bit to make that more evident.
| @@ -56,13 +56,18 @@ module('Integration | Component | sync | Secrets::Page::Destinations::CreateAndE | |||
| assert.true(transition, 'transitions to vault.cluster.sync.secrets.destinations.create on cancel'); | |||
| }); | |||
|
|
|||
| test('create: it renders fieldGroups subtext', async function (assert) { | |||
| assert.expect(2); | |||
| test('create: it renders headers and fieldGroups subtext', async function (assert) { | |||
Description
Creates a separate section for updating sensitive creds on Secrets sync create/edit form.
Designs:
Implementation create:
Implementation edit:
