Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update mobile phone validation module (used for 2fa) #29740

Merged
merged 4 commits into from
Jan 3, 2019
Merged

Update mobile phone validation module (used for 2fa) #29740

merged 4 commits into from
Jan 3, 2019

Conversation

yoavf
Copy link
Contributor

@yoavf yoavf commented Dec 24, 2018 •
edited
Loading

Changes proposed in this Pull Request

This updates our version of the phone module following an update to our fork. The update includes:

I've also updated our own tests, and clarified the phone-validation module intention, which is only to validate mobile numbers.

This supercedes #27902

Testing instructions

  • Test adding 2fa with a phone number. Try using a newer phone number, like Vietnam +84 numbers that begin with prefixes found in Vietnam phone numbers not validating for 2FA #25163, or Thailand numbers starting with 6.
  • Another example is the US +1 phone numbers that begin with 986 (ex. +1-986-555-1212)

Fixes #25163, #27110, #23276, #26882, #21862

@yoavf yoavf added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Security labels Dec 24, 2018
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=update/phone-validation-still-fork

@yoavf yoavf self-assigned this Dec 24, 2018
@yoavf yoavf requested review from blowery, kraftbj, akirk and a team December 24, 2018 13:20
@yoavf yoavf mentioned this pull request Dec 27, 2018
Merged
@yoavf yoavf force-pushed the update/phone-validation-still-fork branch from fe26eb5 to 7879164 Compare January 2, 2019 09:51
akirk
akirk approved these changes Jan 2, 2019
View reviewed changes
Copy link
Member

@akirk akirk left a comment

Choose a reason for hiding this comment

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

Tests work fine, let's go!

@akirk akirk added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 2, 2019
@yoavf yoavf force-pushed the update/phone-validation-still-fork branch from 7879164 to fae61c4 Compare January 3, 2019 07:25
@yoavf
Copy link
Contributor Author

yoavf commented Jan 3, 2019 •
edited
Loading

Forced push after a rebase, tagged needs review for the e2e tests

@yoavf yoavf added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Ready to Merge and removed [Status] Ready to Merge [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 3, 2019
@yoavf yoavf merged commit 36e10d5 into master Jan 3, 2019
@yoavf yoavf deleted the update/phone-validation-still-fork branch January 3, 2019 08:30
blowery added a commit that referenced this pull request Jan 3, 2019
@blowery
Merge remote-tracking branch 'origin/master' into feature/color-refre…
ebe595c 
…sh-2019

* origin/master:
  Disable select when only one domain exiists (#29873)
  Tiled galleries: Disable captions (#29776)
  Prevent scrolling up when opening a dialog (#29832)
  Gutenberg: Move Shortlinks to production (#29883)
  Gutenlypso: add convert to blocks dialog (#29790)
  Gutenberg: Move Related Posts to production blocks (#29650)
  Analytics: fix ad-tracking issue on signup for non-gdpr countries (#29741)
  Update mobile phone validation module (used for 2fa) (#29740)
  Gutenlypso: make sure titles load on second edit (#29877)
  Site Picker: Change wording of /page and /block-editor to match /post (#29859)
  Gutenberg: update copy link in page list to be editor aware
  Gutenberg: use core approach of initialEdits over overridePost
  Gutenberg: when duplicating a post, override post content
  Gutenberg: update duplicate url when Gutenlypso is enabled
  Refactor: Replace use of key-mirror with inline code (#29857)
  Fixes wrong selected domain name (#29824)
  Turn off prettier for SASS, use stylelint instead (#29697)
  Gutenberg: Add copy button to Shortlinks (#29556)
  add a section name to the body class (#29563)
@kosiew
Copy link

kosiew commented Jan 25, 2019

@lutuyen's request in #25163 seems to have been missed.
Another customer in #1566536-zen complained of WordPress not accepting +(84) 89 xxxxxx

@yoavf
Copy link
Contributor Author

yoavf commented Jan 30, 2019

@kosiew - I've double checked - Vietnam numbers of 9 digits, starting with 89 are working.
You can try it yourself with 891234567

The customer in that ticket also mentioned +(84) 89 xxx xxxx (7 digits after 89)

Can you please ask them to provide a screenshot of the exact number they add, and the error message? If they do, please don't post that number here, but share it privately. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akirk akirk akirk approved these changes

@blowery blowery Awaiting requested review from blowery

@kraftbj kraftbj Awaiting requested review from kraftbj

Assignees

@yoavf yoavf

Labels
Security
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Vietnam phone numbers not validating for 2FA
4 participants
@yoavf @matticbot @kosiew @akirk