Merge branch 'remove-unnecesary-validation-layer-for-user-emails' into 'master'

Remove callbacks that obscure validations and rely instead on validations for user secondary emails

See merge request gitlab-org/gitlab!68048

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