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.

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

Changed phone number normalization to use more robust method. #32

Merged
merged 1 commit into from Mar 7, 2019

Conversation

shaunpersad
Copy link
Contributor

  • We're now using this method: https://www.npmjs.com/package/libphonenumber-js#parsephonenumberfromstringstring-defaultcountry

  • It also checks if the phone number is possible, which will not allow numbers that are too long or too short.

  • One thing to note, is that for requests that don't have a location associated with it, non-internationally formatted numbers will fail. For example, 516-555-5766 is a valid US number, but it's impossible for the library to format this into the international format that Twilio expects ( which prefixes +{country code} to the number), since it could also be a valid number in another country. Therefore having a location is imperative for best results.

  • I also temporarily fixed the failing tests that depended on request handlers being promise-based.

@shaunpersad
Copy link
Contributor Author

Copy link
Contributor

@prashanthsadasivan prashanthsadasivan left a comment

Choose a reason for hiding this comment

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

Thanks for fixing that test failure. LGTM.

Do you know off the top of your head which requests don't have a location? otherwise we can take a look thru the db. my guess is probably routes? not sure though.

@shaunpersad
Copy link
Contributor Author

This comment has a query that shows some instances where there is no location: https://www.pivotaltracker.com/story/show/162386185/comments/200212105

@shaunpersad shaunpersad merged commit 9fd942e into master Mar 7, 2019
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.

None yet

2 participants