-
Notifications
You must be signed in to change notification settings - Fork 260
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): make phone number mandatory when "required" option is specified #5794
base: main
Are you sure you want to change the base?
Conversation
…e number against this map
…e number against this map
…m/aws-amplify/amplify-flutter into fix/authenticator/phone-validator
…e number against this map
…e number against this map
…m/aws-amplify/amplify-flutter into fix/authenticator/phone-validator
@@ -164,6 +165,58 @@ mixin AuthenticatorUsernameField<FieldType extends Enum, | |||
} | |||
} | |||
|
|||
String displayPhoneNumber(String? phoneNumber) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this method defined 3 times in this file? I see there are small differences in the implementation, don't know if it is a Dart thing that I am not familiar with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also duplicated with https://github.com/aws-amplify/amplify-flutter/pull/5794/files#diff-3415b04b0444d866a70af8fcfe8cc0a4f4e5d47ee1e90c644c24916a84096c61R51, maybe worth moving to lib/src/utils
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to note that the comments seem copy pasted too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is because i accidentally copy pasted it too many times, it should be two different functions
@@ -183,7 +236,7 @@ mixin AuthenticatorUsernameField<FieldType extends Enum, | |||
isOptional: isOptional, | |||
context: context, | |||
inputResolver: stringResolver.inputs, | |||
)(input?.username); | |||
)(displayPhoneNumber(input?.username)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are passing the username as a param into the phone number method? that doesn't seem right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The username can be a phone number, and at that point in the code its been validated as such. We use AWS Cognito as our base provider for auth, which accepts phone and email as usernames if it has been configured that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL! thank you for explaining :)
Issue #, if available: #4079
Description of changes:
This is done in the exact same way as #4106 from November last year. This fix was correct for the main issue in the original issue however it didn't address being able to re-input a dial code and still have it pass through validation
Truncate a re-inputted dial code if it exceeds the maximum length of a phone number from that given country
Using a map of dial codes to their countries' maximum length of phone number (generated from this open source repo), we will check if the inputted phone number begins with the selected dial code, if it does, then check if it surpasses the length of number in that country (since it could be the case that the number naturally begins with the dial code), if this is also true then ignore the repeat dial code and take the leftover number as the phone number
Credit: @Jordan-Nelson for most of the code
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.