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: remove references to comma separation for string array edit types #20163

Merged
merged 21 commits into from
Apr 19, 2023

Conversation

hellobontempo
Copy link
Contributor

@hellobontempo hellobontempo commented Apr 13, 2023

Models that build from openApi use the description key as helpText which renders as a tooltip beside the label. These descriptions are written for CLI users and so the tooltips are misleading in the UI for inputs with editType: stringArray. The tooltip will say to input comma separated values, but the UI doesn't actually accept this for that specific input type.

This PR:

  1. Adds a new description block to the DisplayAttributes struct that generates an attrs open api schema
  2. globally adds Add one item per row as subtext to all <StringList> inputs rendered by the <FormField> component (which all models that leverage openAPI use)
  3. adds a warning to the <StringList> input itself if a comma exists and reminding to input separate values individually

Fixes #19451
Fixes #10346

demo of fix and validations


ldap

k8 role

Screenshot 2023-04-17 at 3 16 40 PM


Abandoned idea: sanitizing the input and stripping out the comma

The CLI can accept an array of strings or a comma separated string for these parameters. However, these type values map to type stringArray which are only treated as an array in the UI.

# sample schema from the backend that maps to stringArray in the UI
{
    "type": "array", 
    "description": "A comma-separated list of names. At least one must exist in the Common Name. Supports globbing.",
    "items": {
        "type": "string" 
    }
}

Technically a ["string, with comma"] inside an array is allowed, though probably unlikely. We also don't have an existing pattern of changing user-inputted data under the hood. Instead we display warnings to confirm any unexpected characters/values are intentional (i.e. extra spaces) instead of stripping them out in the serializer and changing the data unbeknownst to the user.

@hellobontempo hellobontempo added this to the 1.14 milestone Apr 13, 2023
@@ -107,7 +107,7 @@ export default class PkiIssuerModel extends Model {

@attr('string', {
subText:
'The URL values for the Issuing Certificate field. These are different URLs for the same resource, and should be added individually, not in a comma-separated list.',
'The URL values for the Issuing Certificate field; these are different URLs for the same resource.',
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure how often and for what cases we would use a semicolon. This comes from my experience making language changes and Ivana correcting them :) . From what I can remember, she avoided them and stuck with a period. (Note: trying to search for text with semicolons is a challenge given how much we use them for all other things...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since they're related independent clauses a semicolon felt appropriate here, the super short sentences seemed really choppy. But I can defer to design!

I also thought about removing the second part all together, since it felt like it was only there so add the part about adding one per row

@@ -20,7 +20,7 @@ export default class PkiUrlsModel extends Model {
@attr({
label: 'Issuing certificates',
subText:
'The URL values for the Issuing Certificate field. These are different URLs for the same resource, and should be added individually, not in a comma-separated list.',
'The URL values for the Issuing Certificate field; these are different URLs for the same resource.',
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment here re: ;

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.

LGTM 🚀

ui/app/utils/openapi-to-attrs.js Outdated Show resolved Hide resolved
@@ -39,7 +39,6 @@
{{else if (eq group "Additional subject fields")}}
These fields provide more information about the client to which the certificate belongs.
{{/if}}
Add one item per row.
Copy link
Contributor Author

@hellobontempo hellobontempo Apr 13, 2023

Choose a reason for hiding this comment

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

Removed here since it now renders above each input.
Screenshot 2023-04-13 at 4 30 54 PM

@hellobontempo hellobontempo marked this pull request as ready for review April 14, 2023 01:02
@hellobontempo hellobontempo requested a review from a team April 14, 2023 19:59
@hellobontempo hellobontempo requested a review from a team April 17, 2023 17:36
Copy link
Contributor

@hashishaw hashishaw left a comment

Choose a reason for hiding this comment

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

Great work! Just one small nit, otherwise LGTM

@@ -22,6 +22,10 @@ export const expandOpenApiProps = function (props) {
type = 'number';
}

if (prop['x-vault-displayAttrs']?.description) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we not add description above on line 19? Would make this a bit clearer. If we needed to rename it to something so a falsey value doesn't override the previous definition of description you could rename it like { ..., description: openApiDescription } = prop[...]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea!

@hellobontempo
Copy link
Contributor Author

Opened #20231 without ui/ prefix to run backend tests

@hellobontempo hellobontempo enabled auto-merge (squash) April 19, 2023 15:50
@hellobontempo hellobontempo merged commit 9afac14 into main Apr 19, 2023
@hellobontempo hellobontempo deleted the ui/VAULT-15523/add-string-array-validations branch April 19, 2023 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants