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

Phone Verification #224

Merged
merged 58 commits into from
Oct 28, 2020
Merged

Phone Verification #224

merged 58 commits into from
Oct 28, 2020

Conversation

binh-dam-ibigroup
Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup commented Sep 4, 2020

This PR adds the phone number verification for new and existing MOD UI users when they enter a new phone number for trip notifications. The is a little bit different than in original mockups but for now it is a single flow that works for both new and existing users.

UPDATE: commit ibi-group/otp-middleware@5621a13 or equivalent is required.

UPDATE: commit ibi-group/otp-middleware@a9386a6 or equivalent is required.

UPDATE: there is a are new configuration parameters for:

  • the phone number regex.
  • phone region code
  • phone number placeholder.

This PR depends on ibi-group/otp-middleware#33 ibi-group/otp-middleware#83 (remember to add the Twilio SMS service config variable to the middleware) and implements a workaround for this issue.

@binh-dam-ibigroup binh-dam-ibigroup added WIP Work in progress BLOCKED Blocked (waiting on another PR to be merged) labels Sep 4, 2020
@binh-dam-ibigroup binh-dam-ibigroup removed BLOCKED Blocked (waiting on another PR to be merged) WIP Work in progress labels Sep 8, 2020
lib/actions/user.js Outdated Show resolved Hide resolved
lib/actions/user.js Outdated Show resolved Hide resolved
lib/util/middleware.js Outdated Show resolved Hide resolved
Copy link
Contributor

@evansiroky evansiroky left a comment

Choose a reason for hiding this comment

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

See comments.

lib/actions/user.js Outdated Show resolved Hide resolved
lib/actions/user.js Outdated Show resolved Hide resolved
lib/actions/user.js Outdated Show resolved Hide resolved
Copy link
Contributor

@evansiroky evansiroky left a comment

Choose a reason for hiding this comment

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

State management looks much better now. I have a few comments on probably nice-to-have items, but this seems good to go as-is.

Copy link
Member

@landonreed landonreed left a comment

Choose a reason for hiding this comment

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

Let's get this behemoth merged for now because it's becoming too unwieldy and mostly looks good, but I would like to see a mention of the expiration time for a code and an issue to handle verification throttling on the server side.

Copy link
Member

@landonreed landonreed left a comment

Choose a reason for hiding this comment

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

Changes look good. This is ready to merge.

@binh-dam-ibigroup binh-dam-ibigroup merged commit 98118d7 into dev Oct 28, 2020
@binh-dam-ibigroup binh-dam-ibigroup deleted the phone-verification branch October 28, 2020 21:02
@binh-dam-ibigroup binh-dam-ibigroup mentioned this pull request Oct 30, 2020
@landonreed
Copy link
Member

🎉 This PR is included in version 1.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants