From 0bc690ed11182448e33e910f1a697dfbcace63b4 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Wed, 9 Aug 2017 11:56:08 +0900 Subject: [PATCH] FIX: Staged users are still missing primary email. --- .../fix_primary_emails_for_staged_users.rb | 26 +++++++++++++++++ app/models/user.rb | 16 +++++++++-- app/models/user_email.rb | 8 ++++-- ...330_fix_primary_emails_for_staged_users.rb | 28 ------------------- spec/controllers/users_controller_spec.rb | 7 +++-- spec/models/user_spec.rb | 9 ++++-- 6 files changed, 54 insertions(+), 40 deletions(-) create mode 100644 app/jobs/onceoff/fix_primary_emails_for_staged_users.rb delete mode 100644 db/migrate/20170731030330_fix_primary_emails_for_staged_users.rb diff --git a/app/jobs/onceoff/fix_primary_emails_for_staged_users.rb b/app/jobs/onceoff/fix_primary_emails_for_staged_users.rb new file mode 100644 index 00000000000..a75f9f0e375 --- /dev/null +++ b/app/jobs/onceoff/fix_primary_emails_for_staged_users.rb @@ -0,0 +1,26 @@ +module Jobs + class FixPrimaryEmailsForStagedUsers < Jobs::Onceoff + def execute_onceoff + User.exec_sql <<~SQL + INSERT INTO user_emails ( + user_id, + email, + "primary", + created_at, + updated_at + ) SELECT + users.id, + email_tokens.email, + 'TRUE', + users.created_at, + users.updated_at + FROM users + LEFT JOIN user_emails ON user_emails.user_id = users.id + LEFT JOIN email_tokens ON email_tokens.user_id = users.id + WHERE staged + AND NOT active + AND user_emails.id IS NULL + SQL + end + end +end diff --git a/app/models/user.rb b/app/models/user.rb index 3ae37a3d6d1..51644f7cc5a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -82,10 +82,12 @@ class User < ActiveRecord::Base validates :name, user_full_name: true, if: :name_changed?, length: { maximum: 255 } validates :ip_address, allowed_ip_address: { on: :create, message: :signup_not_allowed } validates :primary_email, presence: true - validates_associated :primary_email, if: :should_validate_primary_email? + validates_associated :primary_email after_initialize :add_trust_level + before_validation :set_should_validate_email + after_create :create_email_token after_create :create_user_stat after_create :create_user_option @@ -268,7 +270,7 @@ class User < ActiveRecord::Base used_invite.try(:invited_by) end - def should_validate_primary_email? + def should_validate_email_address? !skip_email_validation && !staged? end @@ -938,7 +940,7 @@ class User < ActiveRecord::Base def email=(email) if primary_email - self.new_record? ? primary_email.email = email : primary_email.update(email: email) + new_record? ? primary_email.email = email : primary_email.update(email: email) else build_primary_email(email: email) end @@ -1098,6 +1100,14 @@ class User < ActiveRecord::Base true end + def set_should_validate_email + if self.primary_email + self.primary_email.should_validate_email = should_validate_email_address? + end + + true + end + end # == Schema Information diff --git a/app/models/user_email.rb b/app/models/user_email.rb index b67a6cd3ca9..2cca0706a73 100644 --- a/app/models/user_email.rb +++ b/app/models/user_email.rb @@ -3,12 +3,14 @@ require_dependency 'email_validator' class UserEmail < ActiveRecord::Base belongs_to :user + attr_accessor :should_validate_email + before_validation :strip_downcase_email validates :email, presence: true, uniqueness: true validates :email, email: true, format: { with: EmailValidator.email_regex }, - if: :skip_email_validation? + if: :validate_email? validates :primary, uniqueness: { scope: [:user_id] } @@ -21,8 +23,8 @@ class UserEmail < ActiveRecord::Base end end - def skip_email_validation? - return true if user && user.skip_email_validation + def validate_email? + return false unless self.should_validate_email email_changed? end end diff --git a/db/migrate/20170731030330_fix_primary_emails_for_staged_users.rb b/db/migrate/20170731030330_fix_primary_emails_for_staged_users.rb deleted file mode 100644 index d31e7578ef3..00000000000 --- a/db/migrate/20170731030330_fix_primary_emails_for_staged_users.rb +++ /dev/null @@ -1,28 +0,0 @@ -class FixPrimaryEmailsForStagedUsers < ActiveRecord::Migration - def up - execute <<~SQL - INSERT INTO user_emails ( - user_id, - email, - "primary", - created_at, - updated_at - ) SELECT - users.id, - email_tokens.email, - 'TRUE', - users.created_at, - users.updated_at - FROM users - LEFT JOIN user_emails ON user_emails.user_id = users.id - LEFT JOIN email_tokens ON email_tokens.user_id = users.id - WHERE staged - AND NOT active - AND user_emails.id IS NULL - SQL - end - - def down - raise ActiveRecord::IrreversibleMigration - end -end diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 1597f4b7e60..8dd54d5b5e8 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -2031,9 +2031,10 @@ describe UsersController do user = Fabricate(:inactive_user) token = user.email_tokens.first - xhr :put, :update_activation_email, username: user.username, - password: 'qwerqwer123', - email: 'updatedemail@example.com' + xhr :put, :update_activation_email, + username: user.username, + password: 'qwerqwer123', + email: 'updatedemail@example.com' expect(response).to be_success diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 138835a39e9..2f2eb281d22 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -28,9 +28,9 @@ describe User do end describe 'when user is staged' do - it 'should still validate primary_email' do + it 'should still validate presence of primary_email' do user.staged = true - user.primary_email = nil + user.email = nil expect(user).to_not be_valid expect(user.errors.messages).to include(:primary_email) @@ -623,7 +623,10 @@ describe User do it "doesn't validate email address for staged users" do SiteSetting.email_domains_whitelist = "foo.com" SiteSetting.email_domains_blacklist = "bar.com" - expect(Fabricate.build(:user, staged: true, email: "foo@bar.com")).to be_valid + + user = Fabricate.build(:user, staged: true, email: "foo@bar.com") + + expect(user.save).to eq(true) end end