Skip to content

Move phone validation scripts to intl-tel-input pack#4622

Merged
aduth merged 1 commit intomasterfrom
aduth-mv-phone-validation
Jan 28, 2021
Merged

Move phone validation scripts to intl-tel-input pack#4622
aduth merged 1 commit intomasterfrom
aduth-mv-phone-validation

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Jan 27, 2021

Why: Reduce size of main application bundle, which is loaded in all paths. Phone validation dependencies (notably libphonenumber-js) are quite large, and are only used in the context of the intl-tel-input pack (screens with phone input).

Impact:

Reduction of about 57% in application bundle (55.8kb to 24.1kb gzipped).

(Note: Locally disabled chunk splitting to generate these results to be more readable)

Before:

   js/application-8b2a6c3ef1bc883aa56f.js    196 KiB    0, 1  [emitted] [immutable]         application
js/application-8b2a6c3ef1bc883aa56f.js.br   47.9 KiB          [emitted]
js/application-8b2a6c3ef1bc883aa56f.js.gz   55.8 KiB          [emitted]

After:

   js/application-d3084ec0f1f8843f2518.js   75.9 KiB    0, 1  [emitted] [immutable]         application
js/application-d3084ec0f1f8843f2518.js.br   21.3 KiB          [emitted]
js/application-d3084ec0f1f8843f2518.js.gz   24.1 KiB          [emitted]

**Why**: Reduce size of main application bundle, which is loaded in all paths. Phone validation dependencies are quite large, and are only used in the context of the intl-tel-input pack (screens with phone input).
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

Copy link
Contributor

@mitchellhenke mitchellhenke left a comment

Choose a reason for hiding this comment

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

😮 😮 😮

This is great!!

Comment on lines -61 to +64
enablePhoneState(phoneRadio, phoneLabel, smsRadio, deliveryMethodHint);
enablePhoneState(phoneRadio, phoneLabel, deliveryMethodHint);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI this appears to have been previously incorrect. TypeScript caught it. enablePhoneState takes three arguments.

Copy link
Contributor

Choose a reason for hiding this comment

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

would it have been throwing JS errors when used?

Copy link
Contributor Author

@aduth aduth Jan 28, 2021

Choose a reason for hiding this comment

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

would it have been throwing JS errors when used?

Just by looking: It probably wouldn't throw, since the smsRadio we were passing as third argument is an HTMLInputElement, for which the innerText assignment is valid. It probably doesn't behave as we'd expect. I'll do some real-world testing to see that this works correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I think this code is never being reached, due to a bug with how we're populating the country code data.

The warning message is generated if there's a data-sms-only attribute:

const unsupportedInternationalPhoneOTPDeliveryWarningMessage = () => {
const selectedOption = selectedInternationCodeOption();
if (selectedOption.dataset.smsOnly === 'true') {
const messageTemplate = I18n.t(
'two_factor_authentication.otp_delivery_preference.phone_unsupported',
);
return messageTemplate.replace('%{location}', selectedOption.dataset.countryName);
}
return null;
};

We pass the data attributes via:

def international_phone_codes_data(code_data)
{
sms_only: code_data['sms_only'],
country_code: code_data['country_code'],
country_name: code_data['name'],
}
end

Which pulls from this YAML data:

AD:
country_code: '376'
name: Andorra
supports_sms: true
supports_voice: false

sms_only isn't a property. Looks like this was changed in #4331.

We probably want to update this logic, optionally pulling in PhoneNumberCapabilities#sms_only? if it makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh yikes... good catch re sms_only ..... my bad

Copy link
Contributor Author

@aduth aduth Jan 28, 2021

Choose a reason for hiding this comment

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

Re: Wrong arguments being passed: Yes, we're assigning an innerText to the radio input field itself, which AFAICT is a noop. The problem would manifest itself if the user would select an option which cannot receive phone calls, then switch back to one which can. Normally, this should reset the hint text. Instead, it sticks with the error text:

image

(Again, due to above behavior with sms_only, this won't ever happen currently)

The same, after the changes here:

image

@aduth
Copy link
Contributor Author

aduth commented Jan 28, 2021

I created LG-4144 to track issue with SMS only country options. I expect everything else here should be in good shape.

@aduth aduth merged commit ba71649 into master Jan 28, 2021
@aduth aduth deleted the aduth-mv-phone-validation branch January 28, 2021 21:21
mitchellhenke pushed a commit that referenced this pull request Feb 4, 2021
* Make AgencySeeder ignore the abbreviation attribute (#4617)

Temporarily ignore the abbreviation attribute in agencies.yml to allow
the config repo updates to be merged without breaking the IdP while we
work on adding the Partnerships data model to the IdP schema. We will
ultimately add the attribute to the `agencies` table and at that point
remove this change.

Also updates the localdev template YAML file and adds an explicit
fixture file for testing, which is passed to the service object through
dependency injection.

* Add feature flag to send partial DOB to LexisNexis (LG-3706) (#4613)

* Bump identity-proofer-gem to get Proofer::Result#transaction_id (#4624)

* LG-4129: Add test case for cancelled IAL2 session (#4621)

**Why**: Follow-up task from #4620 (comment)

* LG-4097: Fix "Submit" button always appearing disabled on document capture review step (#4623)

* Omit unknown errors when considering to allow step continue

**Why**: Active errors contains both field-specific and general messages, e.g. received from a failed validation attempt. The latter will never become unresolved by changing field values. Only disable continue if the step is invalid OR if there are field-specific errors yet to be addressed.

* Add ReviewIssuesStep validator

* Fix lint error

* Merge 3rd and 4th screens in mobile IAL2 flow (LG-3962) (#4626)

* placeholder

* remove mobile intro step

* Update app/javascript/packages/document-capture/components/document-capture.jsx

Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>

* update comment and remove unnecessary text

* remove new line

* fix merge

Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>

* Move phone internationalization scripts to intl-tel-input pack (#4622)

**Why**: Reduce size of main application bundle, which is loaded in all paths. Phone validation dependencies are quite large, and are only used in the context of the intl-tel-input pack (screens with phone input).

* Add separate feature flag to disable VOIP checks entirely (#4629)

* Retire the remote settings tooling (#4619)

The remote setting tooling was added at a time when `service_providers.yml` was tracked in the IdP codebase. At that time it was not possible to update the configs for a service provider without changing the IdP code and deploying a new sha. With identity-idp-config, this is no longer true.

This commit removes the code since now we can modify service provider configs by updating identity-idp-config and recycling to apply the changes.

* Drop doc_captures table (#4519)

**Why**: Because after #4508 is merged, we won't be using this table. We will be using document_capture_sessions instead.

* LG-4114 Log Async Timeout events (#4618)

* remove access rejected when capturing video (#4636)

* LG-3769: Optimize image assets (#4635)

* Add image optimization lint task

* Remove "data-name" attribute from SVG

* Optimize images

* Restore keyframes to ID card SVG

**Why**: Bug with SVGO removes keyframes. Comments are ignored by default, which are necessary for allowable exceptions to SVG inline styles. SVGO's `removeComments` will preserve comments prefixed with an exclamation point.

See: svg/svgo#888
See: https://github.com/svg/svgo/blob/master/plugins/removeComments.js

* Compress PNG images

* Update lambda ref to get x-ray tracing functionality (#4637)

* remove aal field from service_provider (LG-4119) (#4612)

* remove aal field from service_provider

* make remove_column reversible

* Fix image upload 500 (LG-4154) (#4640)

* add specs

* do not attempt throttling if document capture session does not exist

* do not raise exception on null byte in OIDC token code (#4641)

* Remove migration that drops the AAL column (#4642)

* Revert "remove aal field from service_provider (LG-4119) (#4612)"

This reverts commit c9ce970.

* Add AAL to the ignored column

**Why**: We will be dropping this column in a future commit

* Update proofer gem (#4646)

**Why**: This version includes support for non-string attributes

Co-authored-by: Oren Kanner <oren.kanner@gsa.gov>
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>
Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov>
Co-authored-by: Steve Urciuoli <steve.urciuoli@gsa.gov>
Co-authored-by: Doug Price <douglas.price@gsa.gov>
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