Remove redundant callbacks, rely instead on validations

Remove redundant and confusing checks on secondary emails,
rely on validations. On email update, reset secondary emails
if they were set to primary one

Changelog: fixed

app/models/user.rb

@@ -254,8 +254,7 @@ class User < ApplicationRecord
     message: _("%{placeholder} is not a valid color scheme") % { placeholder: '%{value}' } }
 
   before_validation :sanitize_attrs
-  before_validation :set_public_email, if: :public_email_changed?
-  before_validation :set_commit_email, if: :commit_email_changed?
+  before_validation :reset_secondary_emails, if: :email_changed?
   before_save :default_private_profile_to_false
   before_save :set_public_email, if: :public_email_changed? # in case validation is skipped
   before_save :set_commit_email, if: :commit_email_changed? # in case validation is skipped
@@ -928,24 +927,6 @@ class User < ApplicationRecord
     end
   end
 
-  def notification_email_verified
-    return if read_attribute(:notification_email).blank? || temp_oauth_email?
-
-    errors.add(:notification_email, _("must be an email you have verified")) unless verified_emails.include?(notification_email)
-  end
-
-  def public_email_verified
-    return if public_email.blank?
-
-    errors.add(:public_email, _("must be an email you have verified")) unless verified_emails.include?(public_email)
-  end
-
-  def commit_email_verified
-    return if read_attribute(:commit_email).blank?
-
-    errors.add(:commit_email, _("must be an email you have verified")) unless verified_emails.include?(commit_email)
-  end
-
   # Define commit_email-related attribute methods explicitly instead of relying
   # on ActiveRecord to provide them. Some of the specs use the current state of
   # the model code but an older database schema, so we need to guard against the
@@ -1298,24 +1279,6 @@ class User < ApplicationRecord
     self.name = self.name.gsub(%r{</?[^>]*>}, '')
   end
 
-  def set_notification_email
-    if notification_email.blank? || all_emails.exclude?(notification_email)
-      self.notification_email = email
-    end
-  end
-
-  def set_public_email
-    if public_email.blank? || all_emails.exclude?(public_email)
-      self.public_email = ''
-    end
-  end
-
-  def set_commit_email
-    if commit_email.blank? || verified_emails.exclude?(commit_email)
-      self.commit_email = nil
-    end
-  end
-
   def unset_secondary_emails_matching_deleted_email!(deleted_email)
     secondary_email_attribute_changed = false
     SECONDARY_EMAIL_ATTRIBUTES.each do |attribute|
@@ -2025,6 +1988,48 @@ class User < ApplicationRecord
 
   private
 
+  def notification_email_verified
+    return if read_attribute(:notification_email).blank? || temp_oauth_email?
+
+    errors.add(:notification_email, _("must be an email you have verified")) unless verified_emails.include?(notification_email)
+  end
+
+  def public_email_verified
+    return if public_email.blank?
+
+    errors.add(:public_email, _("must be an email you have verified")) unless verified_emails.include?(public_email)
+  end
+
+  def commit_email_verified
+    return if read_attribute(:commit_email).blank?
+
+    errors.add(:commit_email, _("must be an email you have verified")) unless verified_emails.include?(commit_email)
+  end
+
+  def set_notification_email
+    if notification_email.blank? || verified_emails.exclude?(notification_email)
+      self.notification_email = nil
+    end
+  end
+
+  def set_public_email
+    if public_email.blank? || verified_emails.exclude?(public_email)
+      self.public_email = ''
+    end
+  end
+
+  def set_commit_email
+    if commit_email.blank? || verified_emails.exclude?(commit_email)
+      self.commit_email = nil
+    end
+  end
+
+  def reset_secondary_emails
+    set_public_email
+    set_commit_email
+    set_notification_email
+  end
+
   def callouts_by_feature_name
     @callouts_by_feature_name ||= callouts.index_by(&:feature_name)
   end

spec/factories/users.rb

@@ -11,10 +11,6 @@ FactoryBot.define do
     confirmation_token { nil }
     can_create_group { true }
 
-    after(:stub) do |user|
-      user.notification_email = user.email
-    end
-
     trait :admin do
       admin { true }
     end

spec/features/atom/dashboard_issues_spec.rb

@@ -4,8 +4,20 @@ require 'spec_helper'
 
 RSpec.describe "Dashboard Issues Feed" do
   describe "GET /issues" do
-    let!(:user)     { create(:user, email: 'private1@example.com', public_email: 'public1@example.com') }
-    let!(:assignee) { create(:user, email: 'private2@example.com', public_email: 'public2@example.com') }
+    let!(:user) do
+      user = create(:user, email: 'private1@example.com')
+      public_email = create(:email, :confirmed, user: user, email: 'public1@example.com')
+      user.update!(public_email: public_email.email)
+      user
+    end
+
+    let!(:assignee) do
+      user = create(:user, email: 'private2@example.com')
+      public_email = create(:email, :confirmed, user: user, email: 'public2@example.com')
+      user.update!(public_email: public_email.email)
+      user
+    end
+
     let!(:project1) { create(:project) }
     let!(:project2) { create(:project) }
 

spec/features/atom/issues_spec.rb

@@ -4,11 +4,23 @@ require 'spec_helper'
 
 RSpec.describe 'Issues Feed' do
   describe 'GET /issues' do
-    let_it_be_with_reload(:user) { create(:user, email: 'private1@example.com', public_email: 'public1@example.com') }
-    let_it_be(:assignee) { create(:user, email: 'private2@example.com', public_email: 'public2@example.com') }
-    let_it_be(:group) { create(:group) }
-    let_it_be(:project) { create(:project) }
-    let_it_be(:issue) { create(:issue, author: user, assignees: [assignee], project: project, due_date: Date.today) }
+    let_it_be_with_reload(:user) do
+      user = create(:user, email: 'private1@example.com')
+      public_email = create(:email, :confirmed, user: user, email: 'public1@example.com')
+      user.update!(public_email: public_email.email)
+      user
+    end
+
+    let_it_be(:assignee) do
+      user = create(:user, email: 'private2@example.com')
+      public_email = create(:email, :confirmed, user: user, email: 'public2@example.com')
+      user.update!(public_email: public_email.email)
+      user
+    end
+
+    let_it_be(:group)    { create(:group) }
+    let_it_be(:project)  { create(:project) }
+    let_it_be(:issue)    { create(:issue, author: user, assignees: [assignee], project: project, due_date: Date.today) }
     let_it_be(:issuable) { issue } # "alias" for shared examples
 
     before_all do

spec/features/ics/dashboard_issues_spec.rb

@@ -4,8 +4,20 @@ require 'spec_helper'
 
 RSpec.describe 'Dashboard Issues Calendar Feed' do
   describe 'GET /issues' do
-    let!(:user)     { create(:user, email: 'private1@example.com', public_email: 'public1@example.com') }
-    let!(:assignee) { create(:user, email: 'private2@example.com', public_email: 'public2@example.com') }
+    let!(:user) do
+      user = create(:user, email: 'private1@example.com')
+      public_email = create(:email, :confirmed, user: user, email: 'public1@example.com')
+      user.update!(public_email: public_email.email)
+      user
+    end
+
+    let!(:assignee) do
+      user = create(:user, email: 'private2@example.com')
+      public_email = create(:email, :confirmed, user: user, email: 'public2@example.com')
+      user.update!(public_email: public_email.email)
+      user
+    end
+
     let!(:project) { create(:project) }
     let(:milestone) { create(:milestone, project_id: project.id, title: 'v1.0') }
 

spec/features/ics/group_issues_spec.rb

@@ -4,8 +4,20 @@ require 'spec_helper'
 
 RSpec.describe 'Group Issues Calendar Feed' do
   describe 'GET /issues' do
-    let!(:user)     { create(:user, email: 'private1@example.com', public_email: 'public1@example.com') }
-    let!(:assignee) { create(:user, email: 'private2@example.com', public_email: 'public2@example.com') }
+    let!(:user) do
+      user = create(:user, email: 'private1@example.com')
+      public_email = create(:email, :confirmed, user: user, email: 'public1@example.com')
+      user.update!(public_email: public_email.email)
+      user
+    end
+
+    let!(:assignee) do
+      user = create(:user, email: 'private2@example.com')
+      public_email = create(:email, :confirmed, user: user, email: 'public2@example.com')
+      user.update!(public_email: public_email.email)
+      user
+    end
+
     let!(:group)    { create(:group) }
     let!(:project)  { create(:project, group: group) }
 

spec/features/ics/project_issues_spec.rb

@@ -4,8 +4,20 @@ require 'spec_helper'
 
 RSpec.describe 'Project Issues Calendar Feed' do
   describe 'GET /issues' do
-    let!(:user)     { create(:user, email: 'private1@example.com', public_email: 'public1@example.com') }
-    let!(:assignee) { create(:user, email: 'private2@example.com', public_email: 'public2@example.com') }
+    let!(:user) do
+      user = create(:user, email: 'private1@example.com')
+      public_email = create(:email, :confirmed, user: user, email: 'public1@example.com')
+      user.update!(public_email: public_email.email)
+      user
+    end
+
+    let!(:assignee) do
+      user = create(:user, email: 'private2@example.com')
+      public_email = create(:email, :confirmed, user: user, email: 'public2@example.com')
+      user.update!(public_email: public_email.email)
+      user
+    end
+
     let!(:project)  { create(:project) }
     let!(:issue)    { create(:issue, author: user, assignees: [assignee], project: project) }
 

spec/helpers/projects_helper_spec.rb

@@ -498,7 +498,7 @@ RSpec.describe ProjectsHelper do
       context 'user has a configured commit email' do
         before do
           confirmed_email = create(:email, :confirmed, user: user)
-          user.update!(commit_email: confirmed_email)
+          user.update!(commit_email: confirmed_email.email)
         end
 
         it 'returns the commit email' do

spec/models/user_spec.rb

@@ -465,24 +465,19 @@ RSpec.describe User do
         user.commit_email = confirmed.email
 
         expect(user).to be_valid
-        expect(user.commit_email).to eq(confirmed.email)
       end
 
       it 'can not be set to an unconfirmed email' do
         unconfirmed = create(:email, user: user)
         user.commit_email = unconfirmed.email
 
-        # This should set the commit_email attribute to the primary email
-        expect(user).to be_valid
-        expect(user.commit_email).to eq(user.email)
+        expect(user).not_to be_valid
       end
 
       it 'can not be set to a non-existent email' do
         user.commit_email = 'non-existent-email@nonexistent.nonexistent'
 
-        # This should set the commit_email attribute to the primary email
-        expect(user).to be_valid
-        expect(user.commit_email).to eq(user.email)
+        expect(user).not_to be_valid
       end
 
       it 'can not be set to an invalid email, even if confirmed' do
@@ -691,75 +686,6 @@ RSpec.describe User do
           end
         end
       end
-
-      context 'owns_notification_email' do
-        it 'accepts temp_oauth_email emails' do
-          user = build(:user, email: "temp-email-for-oauth@example.com")
-          expect(user).to be_valid
-        end
-
-        it 'does not accept not verified emails' do
-          email = create(:email)
-          user = email.user
-          user.notification_email = email.email
-
-          expect(user).to be_invalid
-          expect(user.errors[:notification_email]).to include(_('must be an email you have verified'))
-        end
-      end
-
-      context 'owns_public_email' do
-        it 'accepts verified emails' do
-          email = create(:email, :confirmed, email: 'test@test.com')
-          user = email.user
-          user.notification_email = email.email
-
-          expect(user).to be_valid
-        end
-
-        it 'does not accept not verified emails' do
-          email = create(:email)
-          user = email.user
-          user.public_email = email.email
-
-          expect(user).to be_invalid
-          expect(user.errors[:public_email]).to include(_('must be an email you have verified'))
-        end
-      end
-
-      context 'set_commit_email' do
-        it 'keeps commit email when private commit email is being used' do
-          user = create(:user, commit_email: Gitlab::PrivateCommitEmail::TOKEN)
-
-          expect(user.read_attribute(:commit_email)).to eq(Gitlab::PrivateCommitEmail::TOKEN)
-        end
-
-        it 'keeps the commit email when nil' do
-          user = create(:user, commit_email: nil)
-
-          expect(user.read_attribute(:commit_email)).to be_nil
-        end
-
-        it 'reverts to nil when email is not verified' do
-          user = create(:user, commit_email: "foo@bar.com")
-
-          expect(user.read_attribute(:commit_email)).to be_nil
-        end
-      end
-
-      context 'owns_commit_email' do
-        it 'accepts private commit email' do
-          user = build(:user, commit_email: Gitlab::PrivateCommitEmail::TOKEN)
-
-          expect(user).to be_valid
-        end
-
-        it 'accepts nil commit email' do
-          user = build(:user, commit_email: nil)
-
-          expect(user).to be_valid
-        end
-      end
     end
   end