UI: Add Skip password rotation field on DB connection creation #29820
UI: Add Skip password rotation field on DB connection creation #29820drivera258 merged 26 commits intomainfrom
Conversation
github-actions
bot
added
the
hashicorp-contributed-pr
drivera258
added
ui
and removed
hashicorp-contributed-pr
|
CI Results: |
github-actions
bot
added
the
hashicorp-contributed-pr
|
Build Results: |
hellobontempo
left a comment
hellobontempo
left a comment
There was a problem hiding this comment.
Nice work! I know tackling database things can be 🫠
Left some comments, mostly just cleanup suggestions or nits. The main items to address are: cleaning up where this.setSkipImport() is called, only making one network request in database-role-edit and the empty state UX to prompt users to select a database first.
I see that tests updates happened, but I don't see any checking this new functionality works as expected. It'd be great to get test coverage that the different settings on the parent database config render what we expect in the child component. Plus, adding a test to the FormField component tests for the new editType
hellobontempo
left a comment
hellobontempo
left a comment
There was a problem hiding this comment.
Nice work! Lots of great improvements here 🎉
Mostly nonblocking comments, I am wondering about moving the resetErrors() function as that's anti-pattern. I'll go ahead and approve, but would love to see some more tests. (Perhaps in a follow on if not in this one). Specifically for 1.) confirming the warning banner in the role form hide/shows as expected and 2) that the roll setting form works as expected both when the parent value is true and false 3) that both empty states (for selecting a db and a role type) show separately
| return warnings; | ||
| } | ||
| get databaseType() { | ||
|
|
There was a problem hiding this comment.
It's the line break for me 😍
| handleCreateEditRole = task( | ||
| waitFor(async (evt) => { | ||
| evt.preventDefault(); | ||
| this.resetErrors(); |
There was a problem hiding this comment.
Usually we reset errors at the beginning of the submit method so any new ones are relevant to the most recent submit attempt. Why did this move?
There was a problem hiding this comment.
missed an update here, there was an issue with invalid form text showing on valid form so i was messing with placement - fixed the logic and guess the change wasnt included
| * @param {string} [roleType] - role type controls which attributes are shown | ||
| * @param {string} [mode=create] - mode of the form (eg. create or edit) | ||
| * @param {string} [dbType=default] - type of database, eg 'mongodb-database-plugin' | ||
| * @param {object} dbParams - holds database config values, (plugin_name [eg 'mongodb-database-plugin'], skip_static_role_rotation_import) |
There was a problem hiding this comment.
since this file isn't typescript, it might be useful to include that here
| * @param {object} dbParams - holds database config values, (plugin_name [eg 'mongodb-database-plugin'], skip_static_role_rotation_import) | |
| * @param {object} dbParams - holds database config values, { plugin_name: string [eg 'mongodb-database-plugin'], skip_static_role_rotation_import: boolean } ) |
| get dbConfig() { | ||
| return this.args.dbParams; | ||
| } |
There was a problem hiding this comment.
No need to change this, but usually it's best to reserve getters for computing values. That way wherever this.dbConfig is we know it comes from an arg, instead of taking another step to find the getter where it's defined (which would be useful if there was some sort of manipulation happening)
| const plugin = this.args.dbType; | ||
| if (!type) return null; | ||
| const dbValidFields = getStatementFields(type, plugin); | ||
| const dbValidFields = getStatementFields(type, this.dbConfig ? this.dbConfig.plugin_name : null); |
There was a problem hiding this comment.
will plugin_name always exist or should we explicitly check that it does here?
this.dbConfig?.plugin_name || null
There was a problem hiding this comment.
plugin_name would never be null, but dbconfig could be null
| <EmptyState | ||
| @title="No database connection selected" | ||
| @message="Choose a connection to be able to configure a role type." | ||
| /> |
| <div class="description has-text-grey" data-test-toggle-subtext> | ||
| <span> | ||
| {{#if this.toggleInputEnabled}} | ||
| {{@attr.options.enabledSubText}} |
There was a problem hiding this comment.
I noticed we already havehelperTextEnabled/helperTextDisabled as an options subkey - do you think it's worth naming these the same to minimize supported attr options (which are already, admittedly, are a mess 🍝 🤯 )
| await rolePage.name('bar'); | ||
| assert | ||
| .dom('[data-test-component="empty-state"]') | ||
| .exists({ count: 2 }, 'Two empty states exist before selections made'); |
There was a problem hiding this comment.
Can we keep this assertion and move it down until after a database is selected? The message could be updated to something like "two empty states show after a database is selected"
| assert | ||
| .dom('[data-test-component="empty-state"]') | ||
| .exists({ count: 2 }, 'Two empty states exist before selections made'); | ||
| .exists({ count: 1 }, 'One empty state exists before selections made'); |
There was a problem hiding this comment.
| .exists({ count: 1 }, 'One empty state exists before selections made'); | |
| .exists({ count: 1 }, 'One empty state exists before database selection is made'); |
| @@ -114,20 +101,19 @@ module('Integration | Component | database-role-setting-form', function (hooks) | |||
|
|
|||
| test('it shows appropriate fields based on roleType and db plugin', async function (assert) { | |||
There was a problem hiding this comment.
it seems like we could add a test for when skip_static_role_rotation_import: true (oof that's a long variable name) to assert the value is also set correctly when true
* adding skip flag to db creation * update field name & add default val change to static role * transfer both fields to be toggle buttons * add changelog * test updates * leftover * test fixes * fix tests pt2 * test pt3 * adding conditional to disable role type selection * adding alert when overriding db default * cleanup * pr comments pt1 - updates to logic, adding empty state & form field test * moving empty state placement * updating form field logic for subtext, test fixes * restructuring a bit to use a getter / eliminate separate function * update * fix typo, bring back tests * fixes and cleanup
2 participants
Description
What does this PR do?
Adding toggle button to form-field.hbs
Update skip_import_rotation field on static role creation to be a toggle button instead of boolean
Adding field for setting skip import rotation on static roles on connection creation
New role creation flow: A user must select a database connection name to be able to select a role type
Added alert to notify user when changing skip import against the DB connection default
before:
connections page = nonexistent
role creation page:
After:
connections page:
role creation page:
Creating a static user with opposite value set from db connection (overriding)
Screen.Recording.2025-03-10.at.12.06.52.PM.mov
TODO only if you're a HashiCorp employee
backport/label that matches the desired release branch. Note that in the CE repo, the latest release branch will look likebackport/x.x.x, but older release branches will bebackport/ent/x.x.x+ent.of a public function, even if that change is in a CE file, double check that
applying the patch for this PR to the ENT repo and running tests doesn't
break any tests. Sometimes ENT only tests rely on public functions in CE
files.
in the PR description, commit message, or branch name.
description. Also, make sure the changelog is in this PR, not in your ENT PR.