From 39a4479fa3a8ce82dea426456a0536f757950fe9 Mon Sep 17 00:00:00 2001 From: Alexander Machehin Date: Sun, 29 Jan 2012 22:27:46 +0600 Subject: [PATCH] [refs #114] fix emails uniqueness --- app/models/comment.rb | 2 +- app/models/subscribe.rb | 2 +- app/models/user.rb | 4 +- app/models/user_email.rb | 9 +++- ...29120025_add_email_lower_to_user_emails.rb | 18 ++++++++ db/schema.rb | 46 ++++--------------- spec/models/user_email_spec.rb | 11 +++++ 7 files changed, 50 insertions(+), 42 deletions(-) create mode 100644 db/migrate/20120129120025_add_email_lower_to_user_emails.rb diff --git a/app/models/comment.rb b/app/models/comment.rb index 48db60efc..a8b74d3d6 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -28,7 +28,7 @@ class Comment < ActiveRecord::Base self.commentable.subscribes.create(:user_id => self.user_id) if !self.commentable.subscribes.exists?(:user_id => self.user_id) elsif self.commentable.class == Grit::Commit recipients = self.project.relations.by_role('admin').where(:object_type => 'User').map &:object # admins - recipients << self.user << UserEmail.where(:email => self.commentable.committer.email).first.try(:user) # commentor and committer + recipients << self.user << UserEmail.where(:email_lower => self.commentable.committer.email.downcase).first.try(:user) # commentor and committer recipients << self.project.owner if self.project.owner_type == 'User' # project owner recipients.compact.uniq.each {|user| Subscribe.subscribe_user_to_commit(self, user.id)} end diff --git a/app/models/subscribe.rb b/app/models/subscribe.rb index 089c6c97e..cd08c4f1d 100644 --- a/app/models/subscribe.rb +++ b/app/models/subscribe.rb @@ -42,7 +42,7 @@ class Subscribe < ActiveRecord::Base def self.subscribed_to_commit?(project, user, commentable) is_commentor = (Comment.where(:commentable_type => commentable.class.name, :commentable_id => commentable.id).exists?(:user_id => user.id)) - is_committer = (user.emails.exists? :email => commentable.committer.email) + is_committer = (user.emails.exists? :email_lower => commentable.committer.email.downcase) return false if user.subscribes.where(:subscribeable_id => commentable.id, :subscribeable_type => commentable.class.name, :project_id => project.id, :status => Subscribe::OFF).first.present? (project.owner?(user) && user.notifier.new_comment_commit_repo_owner) or diff --git a/app/models/user.rb b/app/models/user.rb index b370c197b..2fe798ebe 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -59,8 +59,8 @@ class User < ActiveRecord::Base def find_for_database_authentication(warden_conditions) conditions = warden_conditions.dup login = conditions.delete(:login) - where(conditions).where("lower(uname) = :value OR " + - "exists (select null from user_emails m where m.user_id = m.user_id and lower(m.email) = :value)", + where(conditions).where("(lower(uname) = :value OR \ + exists (select null from user_emails m where m.user_id = m.user_id and m.email_lower = :value))", {:value => login.downcase}).first end diff --git a/app/models/user_email.rb b/app/models/user_email.rb index 2cec06022..485b9a6f7 100644 --- a/app/models/user_email.rb +++ b/app/models/user_email.rb @@ -3,7 +3,14 @@ class UserEmail < ActiveRecord::Base belongs_to :user - validates :email, :uniqueness => true + validates :email_lower, :uniqueness => true validates :email, :presence => true + before_validation :set_lower + + private + + def set_lower + self.email_lower = self.email.downcase + end end diff --git a/db/migrate/20120129120025_add_email_lower_to_user_emails.rb b/db/migrate/20120129120025_add_email_lower_to_user_emails.rb new file mode 100644 index 000000000..8983c0091 --- /dev/null +++ b/db/migrate/20120129120025_add_email_lower_to_user_emails.rb @@ -0,0 +1,18 @@ +class AddEmailLowerToUserEmails < ActiveRecord::Migration + def self.up + add_column :user_emails, :email_lower, :string + remove_index :user_emails, :email + + UserEmail.reset_column_information + UserEmail.update_all("email_lower = lower(email)") + + change_column :user_emails, :email_lower, :string, :null => false + add_index :user_emails, :email_lower + end + + def self.down + remove_column :user_emails, :email_lower + add_index :user_emails, :email + remove_index :user_emails, :email_lower + end +end diff --git a/db/schema.rb b/db/schema.rb index a4692cd75..1bdee6ff5 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended to check this file into your version control system. -ActiveRecord::Schema.define(:version => 20120123161250) do +ActiveRecord::Schema.define(:version => 20120129120025) do create_table "arches", :force => true do |t| t.string "name", :null => false @@ -167,13 +167,6 @@ ActiveRecord::Schema.define(:version => 20120123161250) do add_index "issues", ["project_id", "serial_id"], :name => "index_issues_on_project_id_and_serial_id", :unique => true - create_table "permissions", :force => true do |t| - t.integer "right_id" - t.integer "role_id" - t.datetime "created_at" - t.datetime "updated_at" - end - create_table "platforms", :force => true do |t| t.string "description" t.string "name" @@ -270,27 +263,6 @@ ActiveRecord::Schema.define(:version => 20120123161250) do t.string "owner_type" end - create_table "rights", :force => true do |t| - t.string "name", :null => false - t.string "controller", :null => false - t.string "action", :null => false - t.datetime "created_at" - t.datetime "updated_at" - end - - create_table "role_lines", :force => true do |t| - t.integer "role_id" - t.integer "relation_id" - t.datetime "created_at" - t.datetime "updated_at" - end - - create_table "roles", :force => true do |t| - t.string "name" - t.datetime "created_at" - t.datetime "updated_at" - end - create_table "rpms", :force => true do |t| t.string "name", :null => false t.integer "arch_id", :null => false @@ -327,29 +299,29 @@ ActiveRecord::Schema.define(:version => 20120123161250) do end create_table "user_emails", :force => true do |t| - t.integer "user_id", :null => false - t.string "email", :null => false + t.integer "user_id", :null => false + t.string "email", :null => false t.datetime "created_at" t.datetime "updated_at" + t.string "email_lower", :null => false end - add_index "user_emails", ["email"], :name => "index_user_emails_on_email" + add_index "user_emails", ["email_lower"], :name => "index_user_emails_on_email_lower" add_index "user_emails", ["user_id"], :name => "index_user_emails_on_user_id" create_table "users", :force => true do |t| t.string "name" - t.string "email", :default => "", :null => false - t.string "encrypted_password", :limit => 128, :default => "", :null => false - t.string "password_salt", :default => "", :null => false + t.string "email", :default => "", :null => false + t.string "encrypted_password", :limit => 128, :default => "", :null => false t.string "reset_password_token" - t.string "remember_token" + t.datetime "reset_password_sent_at" t.datetime "remember_created_at" t.datetime "created_at" t.datetime "updated_at" t.text "ssh_key" t.string "uname" t.string "role" - t.string "language", :default => "en" + t.string "language", :default => "en" end add_index "users", ["email"], :name => "index_users_on_email", :unique => true diff --git a/spec/models/user_email_spec.rb b/spec/models/user_email_spec.rb index 3d663cd6b..ecd6e82e2 100644 --- a/spec/models/user_email_spec.rb +++ b/spec/models/user_email_spec.rb @@ -13,5 +13,16 @@ describe UserEmail do @stranger.emails.create(:email => @user.emails.first.email) @stranger.emails.exists?(:email => @user.emails.first.email).should be_false end + + it 'should not create duplicate lowercase emails' do + @stranger = Factory(:user) + @stranger.emails.create(:email => @user.email.upcase) + @stranger.emails.exists?(:email => @user.email.upcase).should be_false + end + + it 'should not create too many emails' do + 15.times {|i| @user.emails.create(:email => Factory.next(:email))} + @user.emails.count.should be_equal(UserEmail::MAX_EMAILS) + end end end