Add user friendly validation errors for `required` and `pattern` fields on integration modal #5212

mnholtz · 2023-02-18T00:23:52Z

What does this PR do?

Closes Automation Anywhere Control Room Integration User Friendly Alerts #5119
Follow-up ticket in the app TBD

Reviewer Tips

buildYup has an errMessages options you can pass to config to define custom error messages per field. The approach I've taken here is to walk the schema & add generic validation messages for properties with required and pattern fields

Discussion

Alternative implementation is to add an errMessages field to the schema itself which get collected via getValidationErrMessages (which would allow for more specific validation messages). I originally shied away from doing this, because it wouldn't match the JSONSchema7 type. However, it looks like we're mutating the schema anyways by adding config?

Demo/Discussion

https://www.loom.com/share/2affe022ae9848cebdbf336a2276f997

Checklist

Add tests
Designate a primary reviewer @BLoe

mnholtz · 2023-02-18T00:25:08Z

src/components/fields/fieldUtils.ts

+        };
+      }
+
+      if (schema.properties[key].pattern) {


I'm getting a type error for pattern on this type which is apparently JSONSchema7Definition. Does anyone happen to know what type this object actually is?

JSONSchema has some types where boolean is an allowed variant. So you likely need to handle that case

mnholtz · 2023-02-18T00:25:39Z

src/options/pages/services/ServiceEditorModal.test.tsx

+      name: "controlRoomUrl",
+    });
+    await userEvent.type(controlRoomUrlInput, "https://invalid.control.room/");
+    expect(controlRoomUrlInput).toHaveValue("https://invalid.control.room/");


This test is failing here - no value is actually getting modified. Any ideas what's going on?

I suspect you need to add an await tick() or await waitForEffect(). For controlled components, I think there's some React involved in changing the value of the input in the DOM

twschiller · 2023-02-19T14:01:04Z

However, it looks like we're mutating the schema anyways by adding config?

Could you add a code reference? Where/what is mutating the schema?

Alternative implementation is to add an errMessages field to the schema itself which get collected via

There's some prior art for how people declaratively define custom error messages. You're correct though that the official JSON Schema spec doesn't have them

The AJV project, for example supports an errorMessage prop with handling for the different kinds of errors (pattern, required, etc.): https://github.com/ajv-validator/ajv-errors

We use a different JSON Schema validator that doesn't rely on Function constructor: @cfworker/json-schema

Discussion on the JSON Schema project here: json-schema-org/json-schema-spec#1075

I'm find with the JS approach you're taking for now to solve the immediate UI/UX pain. We'll likely want to move to supporting custom messages in the JSON Schema in the future, but that will require additional work to determine approach

codecov · 2023-02-21T23:54:56Z

Codecov Report

Merging #5212 (f84355a) into main (cb4cfc6) will increase coverage by 0.01%.
The diff coverage is 84.21%.

❗ Current head f84355a differs from pull request most recent head e937bf8. Consider uploading reports for the commit e937bf8 to get more accurate results

@@            Coverage Diff             @@
##             main    #5212      +/-   ##
==========================================
+ Coverage   61.27%   61.28%   +0.01%     
==========================================
  Files         989      989              
  Lines       30746    30764      +18     
  Branches     5922     5928       +6     
==========================================
+ Hits        18840    18855      +15     
- Misses      11906    11909       +3

Impacted Files	Coverage Δ
src/components/fields/fieldUtils.ts	`80.70% <82.35%> (+0.70%)`	⬆️
src/options/pages/services/ServiceEditorModal.tsx	`81.13% <100.00%> (+0.36%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

mnholtz · 2023-02-21T23:54:59Z

src/options/pages/services/ServiceEditorModal.test.tsx

@@ -54,4 +56,32 @@ describe("ServiceEditorModal", () => {
    const dialogRoot = screen.getByRole("dialog");
    expect(dialogRoot).toMatchSnapshot();
  });
+
+  // eslint-disable-next-line jest/no-disabled-tests -- FIXME: for some reason, userEvent.type (the alternative approach of using fireEvent) is not modifying the value of the textarea
+  test.skip("displays user-friendly pattern validation message", async () => {


Ticket for this here: #5237

Spent several hours trying to debug this 🤷‍♀️

BLoe

Had some questions and code style suggestions. Depends on the answer to the questions as to whether they are blockers or not. Overall makes sense though! Sorry the test never worked out properly.

BLoe · 2023-02-22T19:46:55Z

src/components/fields/fieldUtils.ts

+): Record<string, Record<string, string>> => {
+  const errMessages: Record<string, Record<string, string>> = {};
+
+  if (schema) {


You could do an early-return here instead for code readability, to avoid nesting the entire function one level:

if (!schema) { return errMessages; }

BLoe · 2023-02-22T19:54:35Z

src/components/fields/fieldUtils.ts

+        errMessages[key] = {
+          // eslint-disable-next-line security/detect-object-injection
+          ...(errMessages[key] ? {} : errMessages[key]),
+          required: `${key} is required`,
+        };


Not a big deal if you like this syntax, but spreading props like this with a ternary inside is hard to parse at a glance for me. Maybe something like this would be more clear?

const messages = errMessages[key] ? {}; if (...) { messages.required = ... } if (...) { messages.pattern = ... } // Lodash _.isEmpty() if (!isEmpty(messages)) { errMessages[key] = messages; }

Wow, this suggestion made the function look miles better. Thanks!

src/options/pages/services/ServiceEditorModal.test.tsx

src/options/pages/services/ServiceEditorModal.tsx

src/components/fields/fieldUtils.ts

github-actions · 2023-02-23T18:30:56Z

When the PR is merged, the first loom link found on this PR will be posted to #sprint-demo on Slack. Do not edit this comment manually.

Misha Holtz and others added 4 commits February 14, 2023 12:31

add getValidaitonErrMessages to fieldUtils

88e155d

modify getErroMessages

7c083fd

pass service schema to getValidationErrMessages

f8df4af

add broken test

2b442b6

mnholtz commented Feb 18, 2023

View reviewed changes

mnholtz requested review from BLoe and twschiller February 18, 2023 00:25

twschiller assigned mnholtz Feb 19, 2023

twschiller added this to the 1.7.20 milestone Feb 19, 2023

twschiller assigned BLoe Feb 21, 2023

Misha Holtz added 2 commits February 21, 2023 14:07

fix type definitions in getValidationMessages

e0bdd4b

fix type errors in ServiceEditorModal

5bb3ea8

vercel bot deployed to Preview February 21, 2023 22:15 View deployment

Misha Holtz added 2 commits February 21, 2023 15:42

add a skip on the test

b8bb3e5

remove unused imports

1ef4bd9

vercel bot deployed to Preview February 21, 2023 23:49 View deployment

mnholtz commented Feb 21, 2023

View reviewed changes

mnholtz marked this pull request as ready for review February 21, 2023 23:55

BLoe reviewed Feb 22, 2023

View reviewed changes

Misha Holtz added 2 commits February 23, 2023 09:36

remove nest dimension in getValidationErrMessages

80892ac

refactor fieldUtils control flow

2bfdd65

vercel bot deployed to Preview February 23, 2023 17:48 View deployment

mnholtz mentioned this pull request Feb 23, 2023

userEvent.type (and fireEvent) not modifying textarea value in ServiceEditorModal tests #5237

Closed

Misha Holtz added 2 commits February 23, 2023 09:49

remove '... is required' to standardize required messages

4dc72cd

make required field message match the default

6a42d58

vercel bot deployed to Preview February 23, 2023 18:02 View deployment

add missing errMessages clause

26b0f44

remove console log

f84355a

mnholtz requested a review from BLoe February 23, 2023 18:06

vercel bot deployed to Preview February 23, 2023 18:08 View deployment

twschiller reviewed Feb 23, 2023

View reviewed changes

src/components/fields/fieldUtils.ts Outdated Show resolved Hide resolved

BLoe approved these changes Feb 23, 2023

View reviewed changes

fix potential undefined error

e937bf8

mnholtz enabled auto-merge (squash) February 23, 2023 19:31

vercel bot deployed to Preview February 23, 2023 19:33 View deployment

mnholtz merged commit 3cb5b5b into main Feb 23, 2023

mnholtz deleted the bugfix/aa_user_friendly_error branch February 23, 2023 19:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add user friendly validation errors for `required` and `pattern` fields on integration modal #5212

Add user friendly validation errors for `required` and `pattern` fields on integration modal #5212

Uh oh!

mnholtz commented Feb 18, 2023 •

edited

Loading

Uh oh!

mnholtz Feb 18, 2023

Uh oh!

twschiller Feb 19, 2023

Uh oh!

mnholtz Feb 18, 2023

Uh oh!

twschiller Feb 19, 2023

Uh oh!

twschiller commented Feb 19, 2023 •

edited

Loading

Uh oh!

codecov bot commented Feb 21, 2023 •

edited

Loading

Uh oh!

mnholtz Feb 21, 2023

Uh oh!

BLoe left a comment

Uh oh!

BLoe Feb 22, 2023 •

edited

Loading

Uh oh!

BLoe Feb 22, 2023

Uh oh!

mnholtz Feb 23, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Feb 23, 2023

Uh oh!

Uh oh!

Add user friendly validation errors for required and pattern fields on integration modal #5212

Add user friendly validation errors for required and pattern fields on integration modal #5212

Uh oh!

Conversation

mnholtz commented Feb 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Reviewer Tips

Discussion

Demo/Discussion

Checklist

Uh oh!

mnholtz Feb 18, 2023

Choose a reason for hiding this comment

Uh oh!

twschiller Feb 19, 2023

Choose a reason for hiding this comment

Uh oh!

mnholtz Feb 18, 2023

Choose a reason for hiding this comment

Uh oh!

twschiller Feb 19, 2023

Choose a reason for hiding this comment

Uh oh!

twschiller commented Feb 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Feb 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mnholtz Feb 21, 2023

Choose a reason for hiding this comment

Uh oh!

BLoe left a comment

Choose a reason for hiding this comment

Uh oh!

BLoe Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BLoe Feb 22, 2023

Choose a reason for hiding this comment

Uh oh!

mnholtz Feb 23, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Feb 23, 2023

Uh oh!

Uh oh!

Add user friendly validation errors for `required` and `pattern` fields on integration modal #5212

Add user friendly validation errors for `required` and `pattern` fields on integration modal #5212

mnholtz commented Feb 18, 2023 •

edited

Loading

twschiller commented Feb 19, 2023 •

edited

Loading

codecov bot commented Feb 21, 2023 •

edited

Loading

BLoe Feb 22, 2023 •

edited

Loading