DEV: Improve User#email= behavior (#11338)

[davidtaylorhq]

DEV: Improve User#email= behavior (#11338)

• Only apply the change after save is called on the record
• Automatically remove matching secondary emails

diff --git a/app/models/user.rb b/app/models/user.rb
index c33b5f9..5722ae6 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -23,7 +23,7 @@ class User < ActiveRecord::Base
   has_many :email_tokens, dependent: :destroy
   has_many :topic_links, dependent: :destroy
   has_many :user_uploads, dependent: :destroy
-  has_many :user_emails, dependent: :destroy
+  has_many :user_emails, dependent: :destroy, autosave: true
   has_many :user_associated_accounts, dependent: :destroy
   has_many :oauth2_user_infos, dependent: :destroy
   has_many :user_second_factors, dependent: :destroy
@@ -40,7 +40,7 @@ class User < ActiveRecord::Base
 
   has_one :user_option, dependent: :destroy
   has_one :user_avatar, dependent: :destroy
-  has_one :primary_email, -> { where(primary: true)  }, class_name: 'UserEmail', dependent: :destroy
+  has_one :primary_email, -> { where(primary: true)  }, class_name: 'UserEmail', dependent: :destroy, autosave: true
   has_one :user_stat, dependent: :destroy
   has_one :user_profile, dependent: :destroy, inverse_of: :user
   has_one :single_sign_on_record, dependent: :destroy
@@ -1263,12 +1263,21 @@ class User < ActiveRecord::Base
     primary_email&.email
   end
 
+  # Shortcut to set the primary email of the user.
+  # Automatically removes any identical secondary emails.
   def email=(new_email)
     if primary_email
-      new_record? ? primary_email.email = new_email : primary_email.update(email: new_email)
+      primary_email.email = new_email
     else
-      self.primary_email = UserEmail.new(email: new_email, user: self, primary: true, skip_validate_email: !should_validate_email_address?)
+      build_primary_email email: new_email, skip_validate_email: !should_validate_email_address?
     end
+
+    if secondary_match = user_emails.detect { |ue| !ue.primary && Email.downcase(ue.email) == Email.downcase(new_email) }
+      secondary_match.mark_for_destruction
+      primary_email.skip_validate_unique_email = true
+    end
+
+    new_email
   end
 
   def emails
diff --git a/app/models/user_email.rb b/app/models/user_email.rb
index be1a060..6ab955c 100644
--- a/app/models/user_email.rb
+++ b/app/models/user_email.rb
@@ -4,6 +4,7 @@ class UserEmail < ActiveRecord::Base
   belongs_to :user
 
   attr_accessor :skip_validate_email
+  attr_accessor :skip_validate_unique_email
 
   before_validation :strip_downcase_email
 
@@ -12,7 +13,7 @@ class UserEmail < ActiveRecord::Base
 
   validates :primary, uniqueness: { scope: [:user_id] }, if: [:user_id, :primary]
   validate :user_id_not_changed, if: :primary
-  validate :unique_email
+  validate :unique_email, if: :validate_unique_email?
 
   scope :secondary, -> { where(primary: false) }
 
@@ -30,8 +31,13 @@ class UserEmail < ActiveRecord::Base
     email_changed?
   end
 
+  def validate_unique_email?
+    return false if self.skip_validate_unique_email
+    will_save_change_to_email?
+  end
+
   def unique_email
-    if self.will_save_change_to_email? && self.class.where("lower(email) = ?", email).exists?
+    if self.class.where("lower(email) = ?", email).exists?
       self.errors.add(:email, :taken)
     end
   end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index c6abcb3..d93ff31 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -2069,6 +2069,35 @@ describe User do
     end
   end
 
+  describe "#email=" do
+    let(:new_email) { "newprimary@example.com" }
+    it 'sets the primary email' do
+      user.update!(email: new_email)
+      expect(User.find(user.id).email).to eq(new_email)
+    end
+
+    it 'only saves when save called' do
+      old_email = user.email
+      user.email = new_email
+      expect(User.find(user.id).email).to eq(old_email)
+      user.save!
+      expect(User.find(user.id).email).to eq(new_email)
+    end
+
+    it 'will automatically remove matching secondary emails' do
+      secondary_email_record = Fabricate(:secondary_email, user: user)
+      user.reload
+      expect(user.secondary_emails.count).to eq(1)
+      user.email = secondary_email_record.email
+      puts "done setting"
+      user.save!
+
+      expect(User.find(user.id).email).to eq(secondary_email_record.email)
+      expect(user.secondary_emails.count).to eq(0)
+    end
+
+  end
+
   describe "set_random_avatar" do
     it "sets a random avatar when selectable avatars is enabled" do
       avatar1 = Fabricate(:upload)

GitHub sha: ef19431e