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

Adds username support #154

Merged
merged 1 commit into from
Jul 10, 2020
Merged

Conversation

ashutosh97
Copy link
Contributor

Closes #152

@ashutosh97
Copy link
Contributor Author

@divyanshu-rawat Please review.

Copy link
Member

@divyanshu-rawat divyanshu-rawat left a comment

Choose a reason for hiding this comment

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

Please address the left comment & resolve codacy errors.

} = req;
try {
const userObject: InterfaceUserModel | null = await User.findOne({ email });
const userObject: InterfaceUserModel | null = await User.findOne({ $or: [ { email }, { userName } ] });
Copy link
Member

Choose a reason for hiding this comment

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

do you really think we need or operator here, as described in schema username is unique so DB will throw an error if username/ email is not unique? and I think in your case it won't even go to userName, as email will never be null. Let me know your thought on it, or correct me if I am wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok so I have put an or condition for a case like this. Let's assume we already have a user { ashu, [email protected]} . This particular condition would prevent a user { ashu, [email protected]} , {ashuK, [email protected]} and { ashu, [email protected]} from registering. This condition ensures uniqueness of both email and username for a new user. In login controller, this same condition ensures the user can either login with user name or email.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just modified the query. I think we need the userObject for socialLoginCheck function.

Copy link
Member

@divyanshu-rawat divyanshu-rawat Jun 5, 2020

Choose a reason for hiding this comment

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

Yeah looks good, didn't know about $or operator, though my intitution was defining unique in schema ensures uniqueness of both email and username for a new user.

Copy link
Member

@divyanshu-rawat divyanshu-rawat left a comment

Choose a reason for hiding this comment

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

LGTM, but this can only be merged once we have functionality on client side to send the username as it is required property. I can add that if you want, else you can also pick up the task, let me know your thoughts on it :)

} = req;
try {
const userObject: InterfaceUserModel | null = await User.findOne({ email });
const userObject: InterfaceUserModel | null = await User.findOne({ $or: [ { email }, { userName } ] });
Copy link
Member

@divyanshu-rawat divyanshu-rawat Jun 5, 2020

Choose a reason for hiding this comment

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

Yeah looks good, didn't know about $or operator, though my intitution was defining unique in schema ensures uniqueness of both email and username for a new user.

@divyanshu-rawat divyanshu-rawat temporarily deployed to ignitus-rest-develop-gzbzoztw2 June 28, 2020 15:42 Inactive
@divyanshu-rawat divyanshu-rawat merged commit d56443c into Ignitus:develop Jul 10, 2020
@divyanshu-rawat divyanshu-rawat mentioned this pull request Jul 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants