Removed email default value, now facebook users should use null, vali… #41
Conversation
felipegarcia92
requested review from
MaicolBen,
matiasmansilla1989 and
santiagovidal
app/models/user.rb
Outdated
| @@ -45,6 +46,10 @@ def full_name | |||
| "#{first_name} #{last_name}" | |||
| end | |||
|
|
|||
| def uses_email? | |||
| provider == 'email' || !email.nil? | |||
There was a problem hiding this comment.
email.present? does the same job as !email.nil? and it's clearer.
app/models/user.rb
Outdated
| @@ -45,6 +46,10 @@ def full_name | |||
| "#{first_name} #{last_name}" | |||
| end | |||
|
|
|||
| def uses_email? | |||
spec/models/user_spec.rb
Outdated
| @@ -34,7 +34,16 @@ | |||
| describe User do | |||
| describe 'validations' do | |||
| it { should validate_presence_of(:email) } | |||
| it { should validate_uniqueness_of(:email).case_insensitive } | |||
There was a problem hiding this comment.
Change to it { should validate_uniqueness_of(:uid) }
felipegarcia92
force-pushed
the
fix/fb-email-uniqueness
branch
from
a819f00 to
e66530e
Compare
|
@santiagovidal @MaicolBen fixed, thanks! |
| @@ -36,7 +36,8 @@ class User < ApplicationRecord | |||
| :recoverable, :rememberable, :trackable, :validatable | |||
| include DeviseTokenAuth::Concerns::User | |||
|
|
|||
| validates :email, uniqueness: true | |||
| validates :uid, uniqueness: { scope: :provider } | |||
| validates :email, uniqueness: true, if: :uses_email? | |||
There was a problem hiding this comment.
@MaicolBen @felipegarcia92 Based on the discussion that we had today, when signing up with email (not Facebook). Should not being validated that email is present?
There was a problem hiding this comment.
But in that case uses_email? will be true
There was a problem hiding this comment.
@MaicolBen @felipegarcia92
But, if provider == 'email' is true then uses_email? will return true. Does this assure that the email is present? I think no, but maybe I'm loosing something here.
There was a problem hiding this comment.
If provider == 'email' you validate uniqueness because you need the email to be present.
spec/factories/user.rb
Outdated
| @@ -3,5 +3,12 @@ | |||
| email { Faker::Internet.unique.email } | |||
| password { Faker::Internet.password(8) } | |||
| username { Faker::Internet.unique.user_name } | |||
| uid { Faker::Number.unique.number(10) } | |||
There was a problem hiding this comment.
align to the right, same as username
spec/factories/user.rb
Outdated
|
|
||
| trait :with_fb do | ||
| password { nil } | ||
| email { nil } |
There was a problem hiding this comment.
a block is not necessary when using literals (nil, string, integer)
spec/factories/user.rb
Outdated
| trait :with_fb do | ||
| password { nil } | ||
| email { nil } | ||
| provider { 'facebook' } |
There was a problem hiding this comment.
align to the right too
There was a problem hiding this comment.
This is wrong when they aren't blocks, a ruby method should have one space between the method name and the first parameter, that's why rubocop complains about it in the code analysis.
There was a problem hiding this comment.
Yep, just agreed to fix it like it is now
felipegarcia92
force-pushed
the
fix/fb-email-uniqueness
branch
from
e66530e to
18e0c8c
Compare
|
@matiasmansilla1989 fixed, thanks! |
felipegarcia92
force-pushed
the
fix/fb-email-uniqueness
branch
from
18e0c8c to
90e7aec
Compare
|
|
||
| context 'when was created with facebook login' do | ||
| subject { build :user, :with_fb } | ||
| it { should_not validate_uniqueness_of(:email) } |
There was a problem hiding this comment.
Add it { should_not validate_presence_of(:email) }
|
|
||
| context 'when was created with regular login' do | ||
| subject { build :user } | ||
| it { should validate_uniqueness_of(:email).case_insensitive } |
There was a problem hiding this comment.
@MaicolBen following your last comment, he should validate the presence of the email here, right?
falvariza
force-pushed
the
fix/fb-email-uniqueness
branch
from
1bc1380 to
b178f5e
Compare
|
@falvariza @felipegarcia92 Great Job! |
…dations updated
This PR aims to fix the following situation:
User logs in with Facebook but doesn't share their email, has email set to "" (empty string)
Second user logs in with Facebook and doesn't either share their email, so he gets a
RecordNotUniqueexception when trying to be set the very same default "" as emailNow uniqueness validation runs only for users who do have an e-mail set (this shouldn't mess with users adding their email later, after registering, which should be validated)