Skip to content

LG-9782: Phone number validity error does not describe how to correct the error#8525

Merged
jmdembe merged 16 commits intomainfrom
LG-9782-phone-number-validity
Jun 8, 2023
Merged

LG-9782: Phone number validity error does not describe how to correct the error#8525
jmdembe merged 16 commits intomainfrom
LG-9782-phone-number-validity

Conversation

@jmdembe
Copy link
Contributor

@jmdembe jmdembe commented Jun 1, 2023

🎫 Ticket

LG-9782

🛠 Summary of changes

This PR enhances the error message when the user enters an invalid phone number

📜 Testing Plan

Creating an account

From the home screen, follow all instructions to create an account. When on the "Authentication method setup" setup screen, select "Text or voice message"

For US phone number error message:

  • Type out an incomplete phone number. This can be 202555 or any combination of numbers with a US area code and is shorter than 10 digits.
  • Click "Send code" to trigger the next step
  • Result: Error message will say "Enter a 10 digit phone number."

For non-US numbers:

To toggle for entering a non-US phone number, you can do the following:

  • Open the drop-down to select a new country
  • Use the + key with the country code. An example would be +51 for Peru
  • Type out an incomplete phone number based on the country. With the previous example, you can enter +51 333
  • Click "Send code" to go to the next step
  • User will see the error "Enter a phone number with the correct number of digits."

👀 Screenshots

If relevant, include a screenshot or screen capture of the changes.

Before: Current state of Get One-Time code screen with error message phone number is not valid
After:
US phone error International phone error
one-time code screen with the new error enter a 10 digit phone number one time code screen with the new error enter a phone number with the correct number of digits

@jmdembe jmdembe force-pushed the LG-9782-phone-number-validity branch from 8333073 to 4ee812b Compare June 1, 2023 17:08
Comment on lines +116 to +119
expect(phoneNumber.validity.valid).to.be.true();
expect(phoneNumber.validity.valid).to.be.false();
Copy link
Contributor

Choose a reason for hiding this comment

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

The intent of this assertion was to verify that it becomes valid again after entering a complete, valid number.

Could we keep the initial behavior where it validates against a single digit, and then the full number? Since I would expect entering just '5' should have the same behavior.


getInvalidFormatMessage(countryCode: CountryCode): string {
return countryCode === 'US'
? this.strings.invalid_phone_us || ''
Copy link
Contributor

Choose a reason for hiding this comment

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

Another approach to writing this function, and simplifying the string specifications, might be the following:

getInvalidFormatMessage(countryCode: CountryCode): string {
    return countryCode === 'US'
      ? t('errors.messages.invalid_phone_number.us')
      : t('errors.messages.invalid_phone_number.international');
}

Along with adding import { t } from '@18f/identity-i18n'; in the same file.

Glancing at the code quickly, I think that would make it simpler. Maybe I'm overlooking a dependency elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is possible, but we're maintaining the strings specifications in phone_input_component.rb

@jmdembe jmdembe marked this pull request as ready for review June 7, 2023 14:13
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

LGTM 👍


await userEvent.type(phoneNumber, '5');
expect(phoneNumber.validationMessage).to.equal('Phone number is not valid');
expect(phoneNumber.validationMessage).to.equal('Enter a 10 digit phone number.');
Copy link
Contributor

@aduth aduth Jun 7, 2023

Choose a reason for hiding this comment

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

It might be good to create a separate test case for international validation, so that we're checking both, similar to what you had before with the revisions "it validates input for US numbers".

We're testing this indirectly with the test case below ("with single option, it validates phone from region"), but could be good to have the standalone test for the non-singular context.

Basically copy this test case and have two versions:

  • it('validates input for U.S. number')
  • it('validates input for international number')

Or:

context('with international number', () => {
  it('validates input', () => {
    // ...
  });
});

@jmdembe jmdembe merged commit ccd2eab into main Jun 8, 2023
@jmdembe jmdembe deleted the LG-9782-phone-number-validity branch June 8, 2023 15:06
@solipet solipet mentioned this pull request Jun 8, 2023
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