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

Fix login failure #43

Merged
merged 1 commit into from
Apr 3, 2017
Merged

Fix login failure #43

merged 1 commit into from
Apr 3, 2017

Conversation

eisuke
Copy link
Member

@eisuke eisuke commented Mar 10, 2017

With a user who email address already exists, but different uid.

@cookpad/dev-infra 👓

@takai
Copy link
Contributor

takai commented Mar 13, 2017

It's obviously another account. I do not prefer this changes because it can cause unintended consequence.
You may want to create tiny script to deal with this kind of thing.

@eisuke eisuke force-pushed the fix_login_failure branch from 01cf9a0 to ae49794 Compare March 21, 2017 03:05
@eisuke
Copy link
Member Author

eisuke commented Mar 21, 2017

It's obviously another account.

Yes, I know it as ideal. The reason for taking this approach is

  • This case occurred more than I think. (ie. part-time employee to regular employee)
  • I trust that accounts are under control in G Suite hosted domain.

But I agree this approach is not good. So I redoed it ae49794 (Removing table constaints)
What do you think? @takai @cookpad/dev-infra

@takai
Copy link
Contributor

takai commented Mar 21, 2017

It doesn't look bad 👍

@eisuke eisuke force-pushed the fix_login_failure branch from ae49794 to 0e362cb Compare March 21, 2017 10:13
@@ -0,0 +1,6 @@
class DropUniqConstraintUserEmail < ActiveRecord::Migration
def change
Copy link
Member

Choose a reason for hiding this comment

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

This should be def up because this migration is irreversible.

Copy link
Member Author

Choose a reason for hiding this comment

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

nice catch! fixed e803981

@eisuke eisuke force-pushed the fix_login_failure branch from 0e362cb to e803981 Compare March 31, 2017 05:17
@@ -17,8 +17,8 @@ class Kuroko2::User < Kuroko2::ApplicationRecord
has_many :admin_assignments, dependent: :restrict_with_error
has_many :assigned_job_definitions, through: :admin_assignments, source: :job_definition

validates :name, uniqueness: { case_sensitive: false} , presence: true
validates :email, uniqueness: { case_sensitive: false}, presence: true
validates :name, uniqueness: { case_sensitive: false }, presence: true, unless: :google_account?
Copy link
Member

Choose a reason for hiding this comment

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

The presence should be validated even if google_account?

Copy link
Member Author

Choose a reason for hiding this comment

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

🙈 149ea35

With a user who email address already exists, But defferrent `uid`
@eisuke eisuke force-pushed the fix_login_failure branch from e803981 to 149ea35 Compare March 31, 2017 10:26
@eisuke eisuke merged commit a028cbc into cookpad:master Apr 3, 2017
@eisuke eisuke deleted the fix_login_failure branch April 3, 2017 06:40
@eisuke eisuke mentioned this pull request May 9, 2017
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