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

Username register login #128

Merged
merged 6 commits into from
Feb 9, 2015
Merged

Username register login #128

merged 6 commits into from
Feb 9, 2015

Conversation

nicolas-besnard
Copy link
Contributor

WIP for #127

For the test, I just C/C the test for 'normal' user. I think it can be cleaned a little more. I know how to do it in Rspec. I'll dig into this later.


if ActiveRecord::Base.connection.adapter_name.downcase.starts_with? 'mysql'
q = "BINARY uid = ? AND provider='email'"
elsif resource_params.include?(:username)
Copy link
Owner

Choose a reason for hiding this comment

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

@nicolas-besnard
Copy link
Contributor Author

I just made a commit based on your comment. What do you think ?

Has mentionned in the doc, we should always make a request in the database with a lowered string (for email or others), instead of making the attempt to login failed even if case_insensitive_keys is not configured.

email = resource_params[:email]
end
# Check
field = (resource_params.keys.map(&:to_sym) & resource_class.authentication_keys).first
Copy link
Owner

Choose a reason for hiding this comment

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

Nicely done 👍

@lynndylanhurley lynndylanhurley merged commit 65f4077 into lynndylanhurley:master Feb 9, 2015
@lynndylanhurley
Copy link
Owner

@nicolas-besnard - as a side-note, I just want to say thanks for your contributions here (fixing bugs, proposing features, fielding issues, etc). Your time and effort are much appreciated 😃

@nicolas-besnard
Copy link
Contributor Author

My pleasure :) I like this gem, I use it on 2 professional project and 1 side projet. So I better be working on it !

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.

2 participants