Include U.S. territories in allowed countries for IdV phone by aduth · Pull Request #7016 · 18F/identity-idp

aduth · 2022-09-22T17:53:20Z

Why: Since the restriction for phone step is based on the assumption that address verification is only supported for U.S. numbers, and U.S. numbers should include U.S. territories.

Screenshots:

Before	After

Testing Instructions:

Confirm that you can use a U.S. mainland phone number or a U.S. territory phone number at the "Phone" step of the identity verification flow, and not another international phone number.

Sample phone numbers:

U.S. mainland: 5135551234
Puerto Rico: 9395550137
Canada: 3065551234

Steps:

Navigate to http://localhost:3000
Sign in or create an account
Navigate to http://localhost:3000/verify
Complete proofing process up to "Enter a phone number with your name on the plan"
Enter a phone number and click Continue
Observe there is no validation preventing you from continuing to delivery method selection, unless using a non-U.S. number
Observe that only applicable delivery methods are shown for delivery method selection (i.e. only SMS for Puerto Rico)

Context:

changelog: Bug Fixes, Identity Verification, Allow U.S. territory numbers for IdV phone step

app/controllers/idv/otp_delivery_method_controller.rb

Conflicting variable name in "#new", not necessary anyways since we use it once in assignment, and helps collapse lines

app/javascript/packages/phone-input/index.js

We now support multiple. "valid country" should apply based on the selected country code regardless of how many countries are supported

zachmargolis

LGTM

zachmargolis · 2022-09-22T21:10:11Z

app/services/phone_country_codes.rb

+class PhoneCountryCodes
+  US_AND_US_TERRITORY_CODES = ['AS', 'GU', 'MP', 'PR', 'US', 'VI']
+end


I wish we had a better home for this constant, but I can't think of one... PhoneNumberCapabilities maybe since it has INTERNATIONAL_CODES?

I think I'm going to convert this into a config item? PhoneNumberCapabilities makes sense to me too

Config makes sense to me too, maybe even generalized to “supported IDV address verification country codes”.

renamed and made configurable in 37b286e

zachmargolis · 2022-09-22T22:18:01Z

app/controllers/idv/otp_delivery_method_controller.rb

+    end
+
+    def user_phone?
+      MfaContext.new(current_user).phone_configurations.any? { |config| config.phone == idv_phone }


do we want to make sure idv_phone and config.phone are both formatted/normalized when we compare?

Suggested change

MfaContext.new(current_user).phone_configurations.any? { |config| config.phone == idv_phone }

MfaContext.new(current_user).phone_configurations.any? { |config| config.formatted_phone == Phonelib.parse(idv_phone).international }

This method was pulled (mostly) verbatim from another class, FWIW.

eric-gade

You might want to be careful doing live child dom queries in the connectedCallback without first checking if this.isConnected is true. It used to be the case -- and still might be -- that the child content is not guaranteed to be parsed at connect time, and also, hilariously, that connectedCallback can be called when the element is already disconnected (!!)

(Some more info here)

* Include U.S. territories in allowed countries for IdV phone changelog: Bug Fixes, Identity Verification, Allow U.S. territory numbers for IdV phone step * Use form instance for allowed countries * Only validate unsupported delivery methods if all unsupported * limit delivery options to phone number capabilities * Add regression coverage for "valid_phone_for_allowed_countries?" fix * Add test coverage for updated all-method-unsupported logic * Add test coverage for conditional field visibility * Check confirmed phones for PhoneNumberCapabilities initializer * Remove unnecessary memoization Conflicting variable name in "#new", not necessary anyways since we use it once in assignment, and helps collapse lines * Update spec expected error message for unsupported country * Fix OTP delivery method re-render on invalid create * Validate region regardless of number of supported countries We now support multiple. "valid country" should apply based on the selected country code regardless of how many countries are supported * Update server-side logic for U.S. number constraint error message * Fix empty errors value in spec assertion * Add phone analytics to phone finder * rename address proofing country codes constant and make it configurable * fix specs * JavaScript this.isConnected fix Co-authored-by: Mitchell Henke <mitchell.henke@gsa.gov>

aduth · 2022-09-26T14:11:15Z

You might want to be careful doing live child dom queries in the connectedCallback without first checking if this.isConnected is true. It used to be the case -- and still might be -- that the child content is not guaranteed to be parsed at connect time, and also, hilariously, that connectedCallback can be called when the element is already disconnected (!!)

(Some more info here)

That's good to know. This component has been deployed for quite some time now and I haven't seen any errors surface in NewRelic related to this. I see that MDN mentions a warning about this, but it's not clear when that would actually happen. Do you have any more specific details?

aduth · 2022-09-26T14:12:25Z

app/javascript/packages/phone-input/index.js

-    if (!this.textInput || !this.codeInput) {
-      return;
-    }
+    if (this.isConnected) {


Aside from understanding whether this is truly necessary, we could have avoided indenting the whole function by implementing this as an early return instead.

if (!this.isConnected) { return; } // ...

eric-gade · 2022-09-26T15:40:57Z

You might want to be careful doing live child dom queries in the connectedCallback without first checking if this.isConnected is true. It used to be the case -- and still might be -- that the child content is not guaranteed to be parsed at connect time, and also, hilariously, that connectedCallback can be called when the element is already disconnected (!!)
(Some more info here)

That's good to know. This component has been deployed for quite some time now and I haven't seen any errors surface in NewRelic related to this. I see that MDN mentions a warning about this, but it's not clear when that would actually happen. Do you have any more specific details?

The way it's currently used, we should not see any issues. However, if this component is re-used in a different context where it might be moved around (perhaps as part of a React application, etc) there is a chance we could run into trouble. From what I recall it's a timing issue. I have not seen it in a long time because I have followed the convention, but it can be a hair-tearing bug to deal with if you don't know about it

aduth · 2022-09-26T17:46:12Z

However, if this component is re-used in a different context where it might be moved around (perhaps as part of a React application, etc) there is a chance we could run into trouble.

I would be very interested to see a proof-of-concept demo of the issue. From what I've been able to find in my searches, it seems like it would only ever be an issue in the case that the element removed itself from the DOM as part of its own initialization, which is quite an edge-case and I'd assume would be easy to identify in a component's own logic.

(And just to clarify, I'm not trying to cast doubt, I just really want to understand the circumstances of the issue more clearly)

aduth added 6 commits September 22, 2022 13:32

Include U.S. territories in allowed countries for IdV phone

e98a334

changelog: Bug Fixes, Identity Verification, Allow U.S. territory numbers for IdV phone step

Use form instance for allowed countries

97499c1

Only validate unsupported delivery methods if all unsupported

d1b1907

limit delivery options to phone number capabilities

17d7526

Add regression coverage for "valid_phone_for_allowed_countries?" fix

b3ef64a

Add test coverage for updated all-method-unsupported logic

da8aef7

aduth commented Sep 22, 2022

View reviewed changes

app/controllers/idv/otp_delivery_method_controller.rb Outdated Show resolved Hide resolved

aduth added 2 commits September 22, 2022 14:17

Add test coverage for conditional field visibility

338cf10

Check confirmed phones for PhoneNumberCapabilities initializer

035f1dc

aduth marked this pull request as ready for review September 22, 2022 18:23

aduth requested a review from a team September 22, 2022 18:23

aduth added 3 commits September 22, 2022 14:38

Remove unnecessary memoization

447d1f2

Conflicting variable name in "#new", not necessary anyways since we use it once in assignment, and helps collapse lines

Update spec expected error message for unsupported country

92cc5e2

Fix OTP delivery method re-render on invalid create

6f48c03

aduth commented Sep 22, 2022

View reviewed changes

app/javascript/packages/phone-input/index.js Outdated Show resolved Hide resolved

aduth added 3 commits September 22, 2022 15:18

Validate region regardless of number of supported countries

09cfb93

We now support multiple. "valid country" should apply based on the selected country code regardless of how many countries are supported

Update server-side logic for U.S. number constraint error message

916e7e3

Fix empty errors value in spec assertion

2067168

zachmargolis approved these changes Sep 22, 2022

View reviewed changes

zachmargolis reviewed Sep 22, 2022

View reviewed changes

Mitchell Henke added 2 commits September 23, 2022 08:52

Add phone analytics to phone finder

9465ba4

rename address proofing country codes constant and make it configurable

37b286e

mdiarra3 approved these changes Sep 23, 2022

View reviewed changes

eric-gade reviewed Sep 23, 2022

View reviewed changes

Mitchell Henke added 2 commits September 23, 2022 09:32

fix specs

e7e276f

JavaScript this.isConnected fix

8c6e05c

mitchellhenke merged commit 5d9ec6a into main Sep 23, 2022

mitchellhenke deleted the aduth-pr-phone-step branch September 23, 2022 15:49

mitchellhenke mentioned this pull request Sep 23, 2022

Deploy RC 211.2 to Production #7024

Merged

aduth commented Sep 26, 2022

View reviewed changes

mitchellhenke mentioned this pull request Sep 26, 2022

Remove isConnected check in phone input JavaScript #7026

Merged

solipet mentioned this pull request Sep 26, 2022

Deploy RC 212 to Prod #7031

Merged

	MfaContext.new(current_user).phone_configurations.any? { \|config\| config.phone == idv_phone }
	MfaContext.new(current_user).phone_configurations.any? { \|config\| config.formatted_phone == Phonelib.parse(idv_phone).international }

Conversation

aduth commented Sep 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zachmargolis left a comment

Choose a reason for hiding this comment

Uh oh!

zachmargolis Sep 22, 2022

Choose a reason for hiding this comment

Uh oh!

mitchellhenke Sep 22, 2022

Choose a reason for hiding this comment

Uh oh!

aduth Sep 22, 2022

Choose a reason for hiding this comment

Uh oh!

mitchellhenke Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachmargolis Sep 22, 2022

Choose a reason for hiding this comment

Uh oh!

zachmargolis Sep 22, 2022

Choose a reason for hiding this comment

Uh oh!

aduth Sep 22, 2022

Choose a reason for hiding this comment

Uh oh!

eric-gade left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth commented Sep 26, 2022

Uh oh!

aduth Sep 26, 2022

Choose a reason for hiding this comment

Uh oh!

eric-gade commented Sep 26, 2022

Uh oh!

aduth commented Sep 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aduth commented Sep 22, 2022 •

edited

Loading

mitchellhenke Sep 23, 2022 •

edited

Loading

eric-gade left a comment •

edited

Loading