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

Login with Email Otps #645

Open
wants to merge 11 commits into
base: master
from

Conversation

@imdhruvgupta
Copy link
Contributor

commented Jun 27, 2019

Developed feature discussed in #609
The template needs to be setup on sendgrid and the id is to be added in config.js file for successfully sending emails.

@championswimmer

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

This pull request introduces 4 alerts when merging f146a1d into 2b558bb - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class
  • 1 for Implicit operand conversion
  • 1 for Missing rate limiting

This project has automated code review enabled, but doesn't use the LGTM GitHub App. Migrate over by installing the app. Read about the benefits of migrating to GitHub Apps in the blog.


Comment posted by LGTM.com

@codecov

This comment has been minimized.

Copy link

commented Jun 28, 2019

Codecov Report

Merging #645 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #645   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      2           
  Lines          15     15           
=====================================
  Hits           15     15

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b558bb...1c08474. Read the comment docs.

@championswimmer

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

This pull request introduces 2 alerts when merging 78c4f41 into 2b558bb - view on LGTM.com

new alerts:

  • 1 for Implicit operand conversion
  • 1 for Missing rate limiting

This project has automated code review enabled, but doesn't use the LGTM GitHub App. Migrate over by installing the app. Read about the benefits of migrating to GitHub Apps in the blog.


Comment posted by LGTM.com

@abhishek97 abhishek97 requested a review from TdevM Jun 30, 2019

@@ -90,6 +90,16 @@ const UserMobileOTP = db.define('usermobileotp', {
paranoid: true
});

const UserEmailOTP = db.define('useremailotp', {

This comment has been minimized.

Copy link
@TdevM

TdevM Jul 4, 2019

Collaborator

Please add a migration for this model at /migrations.

@TdevM
Copy link
Collaborator

left a comment

Also handle username case where an OTP is sent to mobile number, storing OTPs in usermobileotps

@championswimmer

This comment has been minimized.

Copy link
Member

commented Jul 6, 2019

This pull request introduces 2 alerts when merging 1b46fa0 into 2b558bb - view on LGTM.com

new alerts:

  • 1 for Implicit operand conversion
  • 1 for Missing rate limiting

This project has automated code review enabled, but doesn't use the LGTM GitHub App. Migrate over by installing the app. Read about the benefits of migrating to GitHub Apps in the blog.


Comment posted by LGTM.com

@imdhruvgupta

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2019

@TdevM Please review.

return cb(null, false, {message: 'You have entered an incorrect OTP.'});
}

} else {

This comment has been minimized.

Copy link
@TdevM

TdevM Jul 9, 2019

Collaborator

Handle username case here as well.

@TdevM
Copy link
Collaborator

left a comment

handle case for credentials asusername & otp

@imdhruvgupta

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

@TdevM Please review.

@championswimmer

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

This pull request introduces 2 alerts when merging e95990b into 2b558bb - view on LGTM.com

new alerts:

  • 1 for Implicit operand conversion
  • 1 for Missing rate limiting

This project has automated code review enabled, but doesn't use the LGTM GitHub App. Migrate over by installing the app. Read about the benefits of migrating to GitHub Apps in the blog.


Comment posted by LGTM.com

@imdhruvgupta

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2019

@championswimmer Please review the code and bounty for this feature.

@championswimmer

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

This pull request introduces 2 alerts when merging 22e4f06 into ab68638 - view on LGTM.com

new alerts:

  • 1 for Implicit operand conversion
  • 1 for Missing rate limiting

This project has automated code review enabled, but doesn't use the LGTM GitHub App. Migrate over by installing the app. Read about the benefits of migrating to GitHub Apps in the blog.


Comment posted by LGTM.com

@championswimmer

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

This pull request introduces 2 alerts when merging cbaadba into ab68638 - view on LGTM.com

new alerts:

  • 1 for Implicit operand conversion
  • 1 for Missing rate limiting

This project has automated code review enabled, but doesn't use the LGTM GitHub App. Migrate over by installing the app. Read about the benefits of migrating to GitHub Apps in the blog.


Comment posted by LGTM.com

@@ -19,34 +19,74 @@ module.exports = new LocalStrategy({
Raven.setContext({extra: {file: 'otp_login_strategy'}});
try {

let user = await findUserByParams({verifiedmobile: mobile_number});
// Case: Mobile Number
let user = await findUserByParams({verifiedmobile: username});

This comment has been minimized.

Copy link
@TdevM

TdevM Aug 7, 2019

Collaborator

change this to verifiedmobile: +91-${username}

This comment has been minimized.

Copy link
@imdhruvgupta

imdhruvgupta Aug 7, 2019

Author Contributor

@TdevM Done!

@TdevM
TdevM approved these changes Aug 7, 2019
@championswimmer

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

This pull request introduces 2 alerts when merging 399d5d8 into ab68638 - view on LGTM.com

new alerts:

  • 1 for Implicit operand conversion
  • 1 for Missing rate limiting

This project has automated code review enabled, but doesn't use the LGTM GitHub App. Migrate over by installing the app. Read about the benefits of migrating to GitHub Apps in the blog.


Comment posted by LGTM.com

@championswimmer

This comment has been minimized.

Copy link
Member

commented Aug 10, 2019

add the email template id

@imdhruvgupta

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

add the email template id

@TdevM can you please help with that?

@imdhruvgupta

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

@championswimmer Please merge this now.

@championswimmer

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

This pull request introduces 2 alerts when merging 1c08474 into 4be0b03 - view on LGTM.com

new alerts:

  • 1 for Implicit operand conversion
  • 1 for Missing rate limiting

Warning - Automated code review for coding-blocks/oneauth will be disabled on October 1, 2019. You can avoid this by installing the LGTM.com GitHub App. Read about the benefits of migrating to GitHub Apps in the blog.


Comment posted by LGTM.com

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.