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

fix(authenticator): required phone number validator #4106

Merged
merged 2 commits into from
Nov 10, 2023

Conversation

Jordan-Nelson
Copy link
Member

@Jordan-Nelson Jordan-Nelson commented Nov 10, 2023

Issue #, if available: #4079

Description of changes:

  • Drop the country code prefix before performing validation

The country code is populated from a drop down list of known valid numbers so there is no need to validate it. Including it in validation makes it challenging to check for a blank phone number since the country code is always present.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@Jordan-Nelson Jordan-Nelson marked this pull request as ready for review November 10, 2023 16:57
@Jordan-Nelson Jordan-Nelson requested a review from a team as a code owner November 10, 2023 16:57
@Jordan-Nelson Jordan-Nelson changed the title fix(authenticator):required phone number validator fix(authenticator): required phone number validator Nov 10, 2023
khatruong2009
khatruong2009 previously approved these changes Nov 10, 2023
/// The "+" prefix is excluded since validation is performed against the number
/// without the prefix. That is, if the full number is "+1-123-555-7890", the
/// "+1" is dropped prior to validation.
final phoneNumberRegex = RegExp(r'^d+$');
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this meant to match? Should it not be ^[0-9]+$?

Copy link
Contributor

Choose a reason for hiding this comment

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

or ^\d+$?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the \ should not have been removed. Updated.

@@ -118,6 +118,44 @@ void main() {
},
);

testWidgets(
'displays message when submitted with empty phone number if the field is required',
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Does this test cover the issue that triggered this change? It's unclear how this test was untrue before the changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Line 149 would fail today on main as the phone number is currently never considered empty as there is always a dial code.

@Jordan-Nelson Jordan-Nelson merged commit eb6e68b into main Nov 10, 2023
41 of 42 checks passed
@Jordan-Nelson Jordan-Nelson deleted the fix/authenticator/phone-validator branch November 10, 2023 20:56
Jordan-Nelson added a commit to Jordan-Nelson/amplify-flutter that referenced this pull request Nov 13, 2023
Jordan-Nelson added a commit that referenced this pull request Nov 13, 2023
…4106)" (#4117)

Revert "fix(authenticator): required phone number validator (#4106)"

This reverts commit eb6e68b.
NikaHsn pushed a commit that referenced this pull request Dec 5, 2023
* fix(authenticator):required phone number validator

* fix: typo in validator
NikaHsn pushed a commit that referenced this pull request Dec 5, 2023
…4106)" (#4117)

Revert "fix(authenticator): required phone number validator (#4106)"

This reverts commit eb6e68b.
NikaHsn pushed a commit that referenced this pull request Dec 5, 2023
- fix(api): GraphQL Model Helpers support lowercase model names #4143 (#4144)
- fix(authenticator): required phone number validator ([#4106](#4106))
- fix(core): pub docs ([#4049](#4049))
- fix(datastore): emit observeQuery snapshot when model create mutation results in an updated model ([#4084](#4084))

Updated-Components: amplify_lints, Amplify Flutter, Amplify Dart, Amplify UI, DB Common, Secure Storage, AWS Common, Smithy, Worker Bee
NikaHsn pushed a commit that referenced this pull request Dec 5, 2023
### Fixes
- fix(api): GraphQL Model Helpers support lowercase model names #4143 (#4144)
- fix(authenticator): required phone number validator ([#4106](#4106))
- fix(core): pub docs ([#4049](#4049))
- fix(datastore): emit observeQuery snapshot when model create mutation results in an updated model ([#4084](#4084))

Updated-Components: amplify_lints, Amplify Flutter, Amplify Dart, Amplify UI, DB Common, Secure Storage, AWS Common, Smithy, Worker Bee
NikaHsn pushed a commit that referenced this pull request Dec 6, 2023
* fix(authenticator):required phone number validator

* fix: typo in validator
NikaHsn pushed a commit that referenced this pull request Dec 6, 2023
…4106)" (#4117)

Revert "fix(authenticator): required phone number validator (#4106)"

This reverts commit eb6e68b.
NikaHsn pushed a commit that referenced this pull request Dec 6, 2023
### Fixes
- fix(api): GraphQL Model Helpers support lowercase model names #4143 (#4144)
- fix(authenticator): required phone number validator ([#4106](#4106))
- fix(core): pub docs ([#4049](#4049))
- fix(datastore): emit observeQuery snapshot when model create mutation results in an updated model ([#4084](#4084))

Updated-Components: amplify_lints, Amplify Flutter, Amplify Dart, Amplify UI, DB Common, Secure Storage, AWS Common, Smithy, Worker Bee
NikaHsn pushed a commit that referenced this pull request Dec 6, 2023
### Fixes
- fix(api): GraphQL Model Helpers support lowercase model names #4143 (#4144)
- fix(authenticator): required phone number validator ([#4106](#4106))
- fix(core): pub docs ([#4049](#4049))
- fix(datastore): emit observeQuery snapshot when model create mutation results in an updated model ([#4084](#4084))

Updated-Components: amplify_lints, Amplify Flutter, Amplify Dart, Amplify UI, DB Common, Secure Storage, AWS Common, Smithy, Worker Bee
NikaHsn added a commit that referenced this pull request Dec 6, 2023
### Fixes
- fix(api): GraphQL Model Helpers support lowercase model names #4143 (#4144)
- fix(authenticator): required phone number validator ([#4106](#4106))
- fix(core): pub docs ([#4049](#4049))
- fix(datastore): emit observeQuery snapshot when model create mutation results in an updated model ([#4084](#4084))

Updated-Components: amplify_lints, Amplify Flutter, Amplify Dart, Amplify UI, DB Common, Secure Storage, AWS Common, Smithy, Worker Bee
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants