Skip to content

Hotfix twitter login#802

Closed
ebihara99999 wants to merge 2 commits into
NoamB:masterfrom
ebihara99999:hotfix-twitter-login
Closed

Hotfix twitter login#802
ebihara99999 wants to merge 2 commits into
NoamB:masterfrom
ebihara99999:hotfix-twitter-login

Conversation

@ebihara99999

Copy link
Copy Markdown

About the bug

I found a bug with Twitter login with external module reported at the issue #800

How to fix

I tried to fix it by myself, then I add username property to and remove not null constraint of email from users in lib/generators/sorcery/templates/migration/core.rb.

Username is needed when you login with Twitter, but email isn't needed.

Without username property, it raises error(mentioned in issue #800).

Thinking about email, not null constraint in a database layer raises an error
at user.sorcery_adapter.save(:validate => false) in #create_from_provider in lib/sorcery/model/submodules/external.rb.
The error is:

Mysql2::Error: Field 'email' doesn't have a default value: INSERT INTO `users` (`username`, `created_at`, `updated_at`) VALUES ('blablabla', '2016-09-22 15:56:22', '2016-09-22 15:56:22')

Using #save with validate option, expected that a validation of email is ignored. The validation should be imposed only in model.

I wonder why tests don't fail, looking into specs.
In spec/rails_app/db/migrate/core/20101224223620_create_users.rb, different from the migration template mentioned above, defined are columns username and email without not null constraint.

I think users defined in tests is expected, and I fix in this way.

How to test

I run all tests by bundle exec rpsec.
I also confirmed expected migration template as follows:

class SorceryCore < ActiveRecord::Migration
  def change
    create_table :users do |t|
      t.string :email
      t.string :username
      t.string :crypted_password
      t.string :salt

      t.timestamps
    end

    add_index :users, :email, unique: true
  end
end

I'm looking forward to a comment. Thank you for reading.

@Ch4s3

Ch4s3 commented Sep 26, 2016

Copy link
Copy Markdown
Contributor

@ebihara99999 Can you move this over to https://github.com/Sorcery/sorcery/?

@ebihara99999

Copy link
Copy Markdown
Author

Do you mean you ask me to create the same new pull request to the repo?
Then yes, I will do that.

@Ch4s3

Ch4s3 commented Sep 26, 2016

Copy link
Copy Markdown
Contributor

@ebihara99999 Yeah, this repo is going to be deprecated soon in favor of the Sorcery org.

@joshbuker

Copy link
Copy Markdown
Contributor

Once this is moved over to the Sorcery org repo, I'll take a look at this.

@ebihara99999

Copy link
Copy Markdown
Author

I moved this to Sorcery/sorcery pull requests. I hope you read.

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.

3 participants