LG-8976: Allow using Puerto Rico phone numbers for hybrid handoff#7874
Merged
LG-8976: Allow using Puerto Rico phone numbers for hybrid handoff#7874
Conversation
matthinz
force-pushed
the
matthinz/8976-pr-phone-hybrid
branch
2 times, most recently
from
727f848 to
956ea79
Compare
matthinz
commented
Feb 23, 2023
matthinz
commented
Feb 23, 2023
Comment on lines
168
to
172
Contributor
Author
There was a problem hiding this comment.
We were updating the underlying country code <select> on change, but not on initialization.
matthinz
force-pushed
the
matthinz/8976-pr-phone-hybrid
branch
from
3b294ce to
747601c
Compare
Looks like we can have disagreement between what the backend thinks the country is and what the frontend does. Here we're letting the frontend win.
…e blocking some Puerto Rico users from using their phones to take pictures of their identity documents.
Was leading to input being initialized to "+1"
Use Phonelib to parse the country out of the user's phone number and intialize the `international_code` dropdown rendered by `PhoneInputComponent` Update `PhoneFormatter` to use `national` format for the default country (US), `international` for others.
This change probably broke a bunch of tests anyway
Init from previous_params if there, otherwise infer from phone number. (There are some cases where the international_code value is not submitted, e.g. fetch() calls) Also took the opportunity to integrate new tests better with existing test infra
matthinz
force-pushed
the
matthinz/8976-pr-phone-hybrid
branch
from
747601c to
53c314e
Compare
We format PR numbers as just (XXX) XXX-XXXX on the front end
matthinz
commented
Feb 24, 2023
app/forms/idv/phone_form.rb
Outdated
| user_phone = MfaContext.new(user).phone_configurations.take&.phone | ||
| user_phone if valid_phone?(user_phone, phone_confirmed: true) | ||
| # @return [Array<string,string>] The international_code and phone values to use. | ||
| def determine_initial_values(international_code: nil, phone: nil, **_) |
Contributor
Author
There was a problem hiding this comment.
Previously, we were initializing the phone field but not the international_code field. This could lead to cases where non-US phone numbers would end up not passing client-side validation, since the PhoneInputComponent would attempt to parse them as with a US country code.
aduth
approved these changes
Feb 27, 2023
Comment on lines
+168
to
+171
| const country = iti.getSelectedCountryData(); | ||
| if (country.iso2 && this.codeInput) { | ||
| this.codeInput.value = country.iso2.toUpperCase(); | ||
| } |
Contributor
There was a problem hiding this comment.
Is it important that we avoid the change event that would be triggered by calling to syncCountryChangeToCodeInput instead?
Suggested change
| const country = iti.getSelectedCountryData(); | |
| if (country.iso2 && this.codeInput) { | |
| this.codeInput.value = country.iso2.toUpperCase(); | |
| } | |
| this.syncCountryChangeToCodeInput(); |
Contributor
There was a problem hiding this comment.
Also, could we add some JavaScript test coverage for these changes?
Contributor
Author
There was a problem hiding this comment.
Yeah, I originally tried just calling syncCountryChangeToCodeInput, but I believe it would replace a blank value with "+1" initially, which looked strange.
(These are slow tests)
Don't include a spread arg
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎫 Ticket
LG-8976
(Also a dash of LG-8839, since I was in there.)
🛠 Summary of changes
A user reported an issue using their Puerto Rican phone number for IdV's hybrid handoff screen. In this case, they had their phone number prefilled, since they were using it for SMS 2FA. It's likely this came in with #7762.
📜 Testing Plan