chore(ui-react-native): Add RN Authenticator fields validations #4107

ioanabrooks · 2023-06-14T18:15:37Z

Description of changes

This change adds validations for email and required fields for RN Authenticator only.

Added unit test for email validation util and required fields validation.
Added e2e test for required field and validation check.
Added coverage report to react-native package.

---------------------------------------------------|---------|----------|---------|---------|-------------------
File                                               | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
---------------------------------------------------|---------|----------|---------|---------|-------------------
All files                                          |    99.3 |    96.09 |   98.98 |   99.29 |

Issue #, if available

#3458

Description of how you validated changes

Ran tests locally, e2e run in CI

Simulator.Screen.Recording.-.iPhone.13.-.2023-06-16.at.17.47.27.mp4

Checklist

Have read the Pull Request Guidelines
PR description included
Relevant documentation is changed or added (and PR referenced)
yarn test passes and tests are updated/added
No side effects or sideEffects field updated

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

changeset-bot · 2023-06-14T18:15:40Z

🦋 Changeset detected

Latest commit: 9cf3b87

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages

Name	Type
@aws-amplify/ui-react-native	Patch
@aws-amplify/ui-react-core	Patch
@aws-amplify/ui-react-core-notifications	Patch
@aws-amplify/ui-react-storage	Patch
@aws-amplify/ui-react	Patch
@aws-amplify/ui-react-notifications	Patch
@aws-amplify/ui-react-liveness	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

packages/react-core/src/Authenticator/hooks/types.ts

hbuchel · 2023-06-20T15:54:17Z

(Chatted offline, not a blocker) Question about setting context for assistive tech: How are we associating the field error messages with the input? Do we need to pass it as an accessibilityHint on the TextInput? Not sure if that's the correct approach; I think that's what they're doing here https://formidable.com/open-source/react-native-ama/components/TextInput/#haserror I also see this issue is still marked open on the RN repo :(

calebpollman

Looking good, excited to see this completed. Let me know your thoughts on the feedback

packages/react-native/jest.config.js

calebpollman · 2023-06-28T15:37:41Z

packages/react-native/src/Authenticator/hooks/useFieldValues/types.ts

 }

 export interface UseFieldValues<FieldType extends TypedField> {
  fields: FieldType[]; // return either radio or text
+  fieldValidationErrors?: ValidationError;


What do you think of extending the return of fieldValidationErrors with undefined instead of declaring with the optional operator?

calebpollman · 2023-06-28T15:49:12Z

packages/react-native/src/Authenticator/hooks/useFieldValues/useFieldValues.ts

+      setFieldValidationErrors({
+        ...fieldValidationErrors,
+        [name]: runFieldValidation(field, values[name], validationErrors),
+      });


Rather than spreading fieldValidationErrors here, what are your thoughts on passing an updater callback as the parameter of setFieldValidationErrors? Could be applied to all other state setting functions in this file

Will address as follow-up since touches other parts not related to validation.

calebpollman · 2023-06-28T16:04:08Z

packages/react-native/src/Authenticator/hooks/useFieldValues/useFieldValues.ts

+  return {
+    fields: fieldsWithHandlers,
+    disableFormSubmit,
+    fieldValidationErrors: { ...fieldValidationErrors, ...validationErrors },


Definitely may be missing something here, but curious if validationErrors still needs to be merged with fieldValidationErrors since (if I am reading correctly} runValidations has already concatenated the values contained in validationErrors here?

This covers the case where there is no blur or change event but there are state machine errors. We don't want to lose them.

Maybe we should move the logic related to the machine errors out of runValidations and create a util to be used here so we can have one source of truth in how state machine validations are merged with the local state validations?

I was hesitant to make extra changes to this bc we're removing it with the upcoming auth work, what do you think, is it a blocker?

My concern is mostly just that if for some reason the merging related code needs to get updated, it might be difficult to understand what's happening here without prior context. That being said if anything does come up it'll most likely be addressed by you or i so okay with it as is

packages/react-native/src/Authenticator/hooks/useFieldValues/utils.ts

calebpollman · 2023-06-28T16:28:11Z

packages/ui/src/helpers/authenticator/__tests__/utils.test.ts

+    expect(isValidEmail('TEST@EXAMPLE.COM')).toBe(true);
+  });
+
+  it('should return false for an invalid email address', () => {


Nittish: Rather than running multiple tests that assert the condition within a single block the jest idiomatic it.each can be used for descriptive failures as each test runs independently, as well as improved readability of log output:

it.each([ { desc: 'no domain name', email: 'test@' }, { desc: 'an invalid domain', email: 'test@.' }, // ...other test scenarios ])('should return false for an email address with %s', ({ email }) => { expect(email).toBe(false); });

I think the test is simple enough that it doesn't need "descriptive failures", it reads easier for me than it.each but ultimately don't feel super strong about it, could have a follow up to refactor other tests where it.each could be used. One observation I have is that currently this test runs in 1ms with all assertions, having a separate test for each assertion means each run in 1ms (I tested this). While it is negligible, I think we should pay attention to run times as well, as it can add up if this (it.each) is the pattern we want to adopt going forward.

We already use it.each throughout the codebase. Adding 1 ms per test is fine if we are able to locate failing test case without having to comment out each test individually to find the failure (hypothetical situation). That being said this was a nit, so feel free to ignore it

I'll add an asana task! The failing test case points to the line of code so should be easy to locate but makes sense to be more verbose, there are other tests in the ui utils that don't follow it.each pattern, and could be updated as well!

packages/ui/src/helpers/authenticator/utils.ts

calebpollman

Not sure what's going on with CI here but approved 🚢

ioanabrooks added 3 commits June 12, 2023 08:48

chore(ui-react-native): Add Authenticator fields validations

714aca1

Add integ test

5478595

Add required fields validations

a7883f0

ioanabrooks and others added 2 commits June 14, 2023 11:16

Merge branch 'main' into rna-validations

09f93e0

Fix test

6a2cb68

ioanabrooks commented Jun 14, 2023

View reviewed changes

packages/react-core/src/Authenticator/hooks/types.ts Outdated Show resolved Hide resolved

ioanabrooks temporarily deployed to ci June 14, 2023 19:20 — with GitHub Actions Inactive

ioanabrooks had a problem deploying to ci June 14, 2023 19:20 — with GitHub Actions Failure

Add test for required fields

54bcadc

ioanabrooks temporarily deployed to ci June 14, 2023 21:46 — with GitHub Actions Inactive

ioanabrooks marked this pull request as ready for review June 14, 2023 22:40

ioanabrooks requested a review from a team as a code owner June 14, 2023 22:40

ioanabrooks changed the title ~~chore(ui-react-native): Add Authenticator fields validations~~ chore(ui-react-native): Add RN Authenticator fields validations Jun 15, 2023

ioanabrooks and others added 2 commits June 15, 2023 09:02

Merge branch 'main' into rna-validations

1ec8914

fix merge conflicts

27cf301

ioanabrooks temporarily deployed to ci June 15, 2023 16:25 — with GitHub Actions Inactive

ioanabrooks temporarily deployed to ci June 17, 2023 01:10 — with GitHub Actions Inactive

ioanabrooks marked this pull request as ready for review June 19, 2023 16:59

hbuchel previously approved these changes Jun 28, 2023

View reviewed changes

calebpollman reviewed Jun 28, 2023

View reviewed changes

Address feedback

e493397

ioanabrooks dismissed hbuchel’s stale review via e493397 June 28, 2023 17:44

ioanabrooks temporarily deployed to ci June 28, 2023 18:03 — with GitHub Actions Inactive

ioanabrooks had a problem deploying to ci June 28, 2023 18:03 — with GitHub Actions Failure

ioanabrooks had a problem deploying to ci June 28, 2023 18:03 — with GitHub Actions Error

email regex constant

b2719fb

ioanabrooks had a problem deploying to ci June 30, 2023 17:18 — with GitHub Actions Failure

ioanabrooks had a problem deploying to ci June 30, 2023 17:18 — with GitHub Actions Error

ioanabrooks had a problem deploying to ci June 30, 2023 17:18 — with GitHub Actions Failure

ioanabrooks temporarily deployed to ci June 30, 2023 17:18 — with GitHub Actions Inactive

ioanabrooks had a problem deploying to ci June 30, 2023 17:19 — with GitHub Actions Failure

calebpollman approved these changes Jul 1, 2023

View reviewed changes

Merge branch 'main' into rna-validations

9cf3b87

reesscot approved these changes Jul 14, 2023

View reviewed changes

reesscot temporarily deployed to ci July 14, 2023 00:28 — with GitHub Actions Inactive

ioanabrooks merged commit 6d14bf3 into main Jul 17, 2023

ioanabrooks deleted the rna-validations branch July 17, 2023 17:32

github-actions bot mentioned this pull request Jul 17, 2023

Version Packages #4209

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(ui-react-native): Add RN Authenticator fields validations #4107

chore(ui-react-native): Add RN Authenticator fields validations #4107

ioanabrooks commented Jun 14, 2023 •

edited

Loading

changeset-bot bot commented Jun 14, 2023 •

edited

Loading

hbuchel commented Jun 20, 2023 •

edited

Loading

calebpollman left a comment

calebpollman Jun 28, 2023

calebpollman Jun 28, 2023

ioanabrooks Jun 28, 2023

calebpollman Jun 28, 2023

ioanabrooks Jun 28, 2023

calebpollman Jun 28, 2023

ioanabrooks Jun 30, 2023

calebpollman Jul 1, 2023

calebpollman Jun 28, 2023

ioanabrooks Jun 28, 2023

calebpollman Jun 28, 2023

ioanabrooks Jun 30, 2023 •

edited

Loading

calebpollman left a comment

chore(ui-react-native): Add RN Authenticator fields validations #4107

chore(ui-react-native): Add RN Authenticator fields validations #4107

Conversation

ioanabrooks commented Jun 14, 2023 • edited Loading

Description of changes

Issue #, if available

Description of how you validated changes

Checklist

changeset-bot bot commented Jun 14, 2023 • edited Loading

🦋 Changeset detected

hbuchel commented Jun 20, 2023 • edited Loading

calebpollman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioanabrooks Jun 30, 2023 • edited Loading

Choose a reason for hiding this comment

calebpollman left a comment

Choose a reason for hiding this comment

ioanabrooks commented Jun 14, 2023 •

edited

Loading

changeset-bot bot commented Jun 14, 2023 •

edited

Loading

hbuchel commented Jun 20, 2023 •

edited

Loading

ioanabrooks Jun 30, 2023 •

edited

Loading