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

Optimize callback and change login field on email change #356

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vinay-mittal
Copy link

Issue:
Admin changes email of a user but the login field is not changed for that user. User is able to login only through old email and not able to login through new email of user.

In this callback login field will only be assigned if its nil which means at the time of user creation. On every other case this callback will run and nothing change would happen.
Now, this callback will only run if email is changed and new value of email will be assigned to login.

@krzysiek1507
Copy link
Contributor

I can see another problem. email and login should be unique as a pair. There shouldn't be a way to set the same value for email for user A and login for user B.

@krzysiek1507
Copy link
Contributor

Here this is described.

@vinay-mittal
Copy link
Author

@krzysiek1507 if we are having login same as email then the login value will be unique as email is always unique.

@krzysiek1507
Copy link
Contributor

  1. By default user can login only using email. If you allow a user to login using login field in your app then you should handle it yourself, but this could be weird to search in 2 columns with same values.
  2. I don't think if we should sync login with email.
  3. There is a missing index for login field.

@@ -35,7 +35,7 @@ def password_required?

def set_login
# for now force login to be same as email, eventually we will make this configurable, etc.
self.login ||= self.email if self.email
self.login = self.email

Choose a reason for hiding this comment

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

@vinay-mittal @krzysiek1507 But then what is the meaning of keeping extra parameter as login.

Its realy good option to have login field separate, It useful in mobile to have mobile number for login parameters.

@vishalzambre
Copy link

vishalzambre commented Aug 12, 2016

@damianlegawiec @vinay-mittal @krzysiek1507 We should allow login only using login not by email and as per code self.login ||= email will take care of login should not be null.
Also yes we can add uniq index on both.

@vishalzambre
Copy link

If required I can raise PR for uniq index on both.

@kushniryb
Copy link
Contributor

@vinay-mittal Please rebase against master & add some specs. Thanks in advance 👍

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.

4 participants