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

feature: Make email optional #70

Merged
merged 1 commit into from
Jun 17, 2015
Merged

feature: Make email optional #70

merged 1 commit into from
Jun 17, 2015

Conversation

BerkeleyTrue
Copy link
Contributor

Only add email field to user object if available and not optional. This way emails can still be indexed with sparse indexing.

closes #66

@slnode
Copy link

slnode commented Jun 10, 2015

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@raymondfeng
Copy link
Member

@BerkeleyTrue
Copy link
Contributor Author

signed

var userObj = (options.profileToUser || profileToUser)(provider, profile);
if (!userObj.email) {
var userObj = (options.profileToUser || profileToUser)(provider, profile, options);
if (!userObj.email && !options.emailOptional) {
return cb('email is missing from the user profile');
Copy link
Member

Choose a reason for hiding this comment

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

Let's do this shortcut within a process.nextTick block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean. wrap line 117 in a process.nextTick?

@BerkeleyTrue
Copy link
Contributor Author

Had to change the query structure. In certain situations user identities where being assigned to random user documents. I also wrapped the callback in a nextTick

@pongstr
Copy link
Contributor

pongstr commented Jun 15, 2015

👍

@raymondfeng
Copy link
Member

ok to test

@BerkeleyTrue
Copy link
Contributor Author

@raymondfeng Any feedback on this?

raymondfeng added a commit that referenced this pull request Jun 17, 2015
@raymondfeng raymondfeng merged commit 21a11d3 into strongloop:master Jun 17, 2015
@BerkeleyTrue
Copy link
Contributor Author

woot!

return cb('email is missing from the user profile');
var userObj = (options.profileToUser || profileToUser)(provider, profile, options);
if (!userObj.email && !options.emailOptional) {
process.nextTick(function() {
Copy link

Choose a reason for hiding this comment

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

We're integrating loopback-component-passport into our Loopback app and are having some trouble understanding this code. Why is the nextTick required here - it seems like it allows the execution to fall through and to create the member even if the email is not available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexpit You are correct, this is indeed a bug. There is a missing return before the process.nextTick.

Can you file an issue and maybe create a PR?

Copy link

Choose a reason for hiding this comment

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

@BerkeleyTrue will do - thanks very much for your response!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I'm surprised this hasn't been caught earlier.

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.

Cannot set user without email
5 participants