Skip to content

LG-5193: Allow supported international phone numbers for IAL2 hybrid flow#5490

Merged
aduth merged 6 commits intomainfrom
aduth-lg-5193-non-us
Oct 14, 2021
Merged

LG-5193: Allow supported international phone numbers for IAL2 hybrid flow#5490
aduth merged 6 commits intomainfrom
aduth-lg-5193-non-us

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Oct 8, 2021

Why: So that a user with an international phone number can complete the proofing process, since address verification can be completed via mailed code.

@zachmargolis
Copy link
Contributor

Definitely out of scope for this PR, but since we're in this area, I'd love for us to see if we can pick back up using an option to use a QR code for this handoff, old JIRA https://cm-jira.usa.gov/browse/LG-2854

@aduth
Copy link
Contributor Author

aduth commented Oct 12, 2021

Definitely out of scope for this PR, but since we're in this area, I'd love for us to see if we can pick back up using an option to use a QR code for this handoff, old JIRA https://cm-jira.usa.gov/browse/LG-2854

Yeah, I can't see me getting to it as part of this work, but I'll surface with the team to see if we could prioritize this, since I agree it could make the flow simpler, and also give some users an option to proof with their phone who might not otherwise have the ability (no cell service, country which can't receive SMS, etc).

@aduth aduth marked this pull request as ready for review October 12, 2021 14:51
@aduth
Copy link
Contributor Author

aduth commented Oct 12, 2021

I noticed we don't show error messages for form validation issues on the IAL2 phone step, which I tracked as a bug at LG-5225. I did verify with binding.pry that the failure is a result of "Invalid phone number. Please make sure you enter a phone number with a U.S. country code.".

@aduth aduth changed the title Allow supported international phone numbers for IAL2 hybrid flow LG-5193: Allow supported international phone numbers for IAL2 hybrid flow Oct 12, 2021
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 15 to 19
Copy link
Contributor

Choose a reason for hiding this comment

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

I would consider moving the default value to the args:

Suggested change
def initialize(user:, previous_params:, allowed_countries: nil, delivery_methods: nil)
previous_params ||= {}
@user = user
@allowed_countries = allowed_countries
@delivery_methods = delivery_methods || ALL_DELIVERY_METHODS
def initialize(user:, previous_params:, allowed_countries: ALL_DELIVERY_METHODS, delivery_methods: nil)
previous_params ||= {}
@user = user
@allowed_countries = allowed_countries
@delivery_methods = delivery_methods

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, makes sense. Admittedly I'd considered this, then out of convenience (read: laziness) for test stubbing opted to move the defaulting to the body. In retrospect, I agree it's better as a default argument.

aduth and others added 6 commits October 13, 2021 08:49
**Why**: So that a user with an international phone number can complete the proofing process, since address verification can be completed via mailed code.
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
See: #5490 (comment)
Co-Authored-By: Zach Margolis <zbmargolis@gmail.com>
See: #5490 (comment)
Co-Authored-By: Zach Margolis <zbmargolis@gmail.com>
@aduth aduth force-pushed the aduth-lg-5193-non-us branch from 827a861 to 78d5530 Compare October 13, 2021 12:49
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