Merge branch 'remove-custom-getter-methods-for-secondary-emails' into 'master'

Remove custom getter methods for secondary emails

See merge request gitlab-org/gitlab!69648

app/controllers/projects/issues_controller.rb

@@ -227,7 +227,7 @@ class Projects::IssuesController < Projects::ApplicationController
     IssuableExportCsvWorker.perform_async(:issue, current_user.id, project.id, finder_options.to_h) # rubocop:disable CodeReuse/Worker
 
     index_path = project_issues_path(project)
-    message = _('Your CSV export has started. It will be emailed to %{email} when complete.') % { email: current_user.notification_email }
+    message = _('Your CSV export has started. It will be emailed to %{email} when complete.') % { email: current_user.notification_email_or_default }
     redirect_to(index_path, notice: message)
   end
 

app/controllers/projects/merge_requests_controller.rb

@@ -378,7 +378,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     IssuableExportCsvWorker.perform_async(:merge_request, current_user.id, project.id, finder_options.to_h) # rubocop:disable CodeReuse/Worker
 
     index_path = project_merge_requests_path(project)
-    message = _('Your CSV export has started. It will be emailed to %{email} when complete.') % { email: current_user.notification_email }
+    message = _('Your CSV export has started. It will be emailed to %{email} when complete.') % { email: current_user.notification_email_or_default }
     redirect_to(index_path, notice: message)
   end
 

app/helpers/issues_helper.rb

@@ -221,7 +221,7 @@ module IssuesHelper
       can_bulk_update: can?(current_user, :admin_issue, project).to_s,
       can_edit: can?(current_user, :admin_project, project).to_s,
       can_import_issues: can?(current_user, :import_issues, @project).to_s,
-      email: current_user&.notification_email,
+      email: current_user&.notification_email_or_default,
       emails_help_page_path: help_page_path('development/emails', anchor: 'email-namespace'),
       export_csv_path: export_csv_project_issues_path(project),
       has_any_issues: project_issues(project).exists?.to_s,

app/helpers/projects_helper.rb

@@ -435,7 +435,7 @@ module ProjectsHelper
 
   def git_user_email
     if current_user
-      current_user.commit_email
+      current_user.commit_email_or_default
     else
       "your@email.com"
     end

app/mailers/emails/admin_notification.rb

@@ -4,7 +4,7 @@ module Emails
   module AdminNotification
     def send_admin_notification(user_id, subject, body)
       user = User.find(user_id)
-      email = user.notification_email
+      email = user.notification_email_or_default
       @unsubscribe_url = unsubscribe_url(email: Base64.urlsafe_encode64(email))
       @body = body
       mail to: email, subject: subject
@@ -12,7 +12,7 @@ module Emails
 
     def send_unsubscribed_notification(user_id)
       user = User.find(user_id)
-      email = user.notification_email
+      email = user.notification_email_or_default
       mail to: email, subject: "Unsubscribed from GitLab administrator notifications"
     end
   end

app/mailers/emails/profile.rb

@@ -6,7 +6,7 @@ module Emails
       @current_user = @user = User.find(user_id)
       @target_url = user_url(@user)
       @token = token
-      mail(to: @user.notification_email, subject: subject("Account was created for you"))
+      mail(to: @user.notification_email_or_default, subject: subject("Account was created for you"))
     end
 
     def instance_access_request_email(user, recipient)
@@ -14,7 +14,7 @@ module Emails
       @recipient = recipient
 
       profile_email_with_layout(
-        to: recipient.notification_email,
+        to: recipient.notification_email_or_default,
         subject: subject(_("GitLab Account Request")))
     end
 
@@ -42,7 +42,7 @@ module Emails
 
       @current_user = @user = @key.user
       @target_url = user_url(@user)
-      mail(to: @user.notification_email, subject: subject("SSH key was added to your account"))
+      mail(to: @user.notification_email_or_default, subject: subject("SSH key was added to your account"))
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
@@ -54,7 +54,7 @@ module Emails
 
       @current_user = @user = @gpg_key.user
       @target_url = user_url(@user)
-      mail(to: @user.notification_email, subject: subject("GPG key was added to your account"))
+      mail(to: @user.notification_email_or_default, subject: subject("GPG key was added to your account"))
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
@@ -67,7 +67,7 @@ module Emails
       @days_to_expire = PersonalAccessToken::DAYS_TO_EXPIRE
 
       Gitlab::I18n.with_locale(@user.preferred_language) do
-        mail(to: @user.notification_email, subject: subject(_("Your personal access tokens will expire in %{days_to_expire} days or less") % { days_to_expire: @days_to_expire }))
+        mail(to: @user.notification_email_or_default, subject: subject(_("Your personal access tokens will expire in %{days_to_expire} days or less") % { days_to_expire: @days_to_expire }))
       end
     end
 
@@ -78,7 +78,7 @@ module Emails
       @target_url = profile_personal_access_tokens_url
 
       Gitlab::I18n.with_locale(@user.preferred_language) do
-        mail(to: @user.notification_email, subject: subject(_("Your personal access token has expired")))
+        mail(to: @user.notification_email_or_default, subject: subject(_("Your personal access token has expired")))
       end
     end
 
@@ -90,7 +90,7 @@ module Emails
       @target_url = profile_keys_url
 
       Gitlab::I18n.with_locale(@user.preferred_language) do
-        mail(to: @user.notification_email, subject: subject(_("Your SSH key has expired")))
+        mail(to: @user.notification_email_or_default, subject: subject(_("Your SSH key has expired")))
       end
     end
 
@@ -102,7 +102,7 @@ module Emails
       @target_url = profile_keys_url
 
       Gitlab::I18n.with_locale(@user.preferred_language) do
-        mail(to: @user.notification_email, subject: subject(_("Your SSH key is expiring soon.")))
+        mail(to: @user.notification_email_or_default, subject: subject(_("Your SSH key is expiring soon.")))
       end
     end
 
@@ -114,7 +114,7 @@ module Emails
 
       Gitlab::I18n.with_locale(@user.preferred_language) do
         profile_email_with_layout(
-          to: @user.notification_email,
+          to: @user.notification_email_or_default,
           subject: subject(_("%{host} sign-in from new location") % { host: Gitlab.config.gitlab.host }))
       end
     end
@@ -125,7 +125,7 @@ module Emails
       @user = user
 
       Gitlab::I18n.with_locale(@user.preferred_language) do
-        mail(to: @user.notification_email, subject: subject(_("Two-factor authentication disabled")))
+        mail(to: @user.notification_email_or_default, subject: subject(_("Two-factor authentication disabled")))
       end
     end
 

app/models/user.rb

@@ -229,10 +229,9 @@ class User < ApplicationRecord
   validates :first_name, length: { maximum: 127 }
   validates :last_name, length: { maximum: 127 }
   validates :email, confirmation: true
-  validates :notification_email, presence: true
-  validates :notification_email, devise_email: true, if: ->(user) { user.notification_email != user.email }
+  validates :notification_email, devise_email: true, allow_blank: true, if: ->(user) { user.notification_email != user.email }
   validates :public_email, uniqueness: true, devise_email: true, allow_blank: true
-  validates :commit_email, devise_email: true, allow_nil: true, if: ->(user) { user.commit_email != user.email }
+  validates :commit_email, devise_email: true, allow_blank: true, if: ->(user) { user.commit_email != user.email }
   validates :projects_limit,
     presence: true,
     numericality: { greater_than_or_equal_to: 0, less_than_or_equal_to: Gitlab::Database::MAX_INT_VALUE }
@@ -384,7 +383,7 @@ class User < ApplicationRecord
     after_transition any => :deactivated do |user|
       next unless Gitlab::CurrentSettings.user_deactivation_emails_enabled
 
-      NotificationService.new.user_deactivated(user.name, user.notification_email)
+      NotificationService.new.user_deactivated(user.name, user.notification_email_or_default)
     end
     # rubocop: enable CodeReuse/ServiceClass
 
@@ -932,33 +931,18 @@ class User < ApplicationRecord
     end
   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
-  # possibility of the commit_email column not existing.
-
-  def commit_email
-    return self.email unless has_attribute?(:commit_email)
-
-    if super == Gitlab::PrivateCommitEmail::TOKEN
+  def commit_email_or_default
+    if self.commit_email == Gitlab::PrivateCommitEmail::TOKEN
       return private_commit_email
     end
 
     # The commit email is the same as the primary email if undefined
-    super.presence || self.email
-  end
-
-  def commit_email=(email)
-    super if has_attribute?(:commit_email)
-  end
-
-  def commit_email_changed?
-    has_attribute?(:commit_email) && super
+    self.commit_email.presence || self.email
   end
 
-  def notification_email
+  def notification_email_or_default
     # The notification email is the same as the primary email if undefined
-    super.presence || self.email
+    self.notification_email.presence || self.email
   end
 
   def private_commit_email
@@ -1640,7 +1624,7 @@ class User < ApplicationRecord
 
   def notification_email_for(notification_group)
     # Return group-specific email address if present, otherwise return global notification email address
-    notification_group&.notification_email_for(self) || notification_email
+    notification_group&.notification_email_for(self) || notification_email_or_default
   end
 
   def notification_settings_for(source, inherit: false)
@@ -2019,7 +2003,7 @@ class User < ApplicationRecord
   private
 
   def notification_email_verified
-    return if read_attribute(:notification_email).blank? || temp_oauth_email?
+    return if notification_email.blank? || temp_oauth_email?
 
     errors.add(:notification_email, _("must be an email you have verified")) unless verified_emails.include?(notification_email)
   end
@@ -2031,7 +2015,7 @@ class User < ApplicationRecord
   end
 
   def commit_email_verified
-    return if read_attribute(:commit_email).blank?
+    return if commit_email.blank?
 
     errors.add(:commit_email, _("must be an email you have verified")) unless verified_emails.include?(commit_email)
   end

app/views/profiles/_email_settings.html.haml

@@ -11,6 +11,6 @@
 - commit_email_link_url = help_page_path('user/profile/index', anchor: 'change-the-email-displayed-on-your-commits', target: '_blank')
 - commit_email_link_start = '<a href="%{url}">'.html_safe % { url: commit_email_link_url }
 - commit_email_docs_link = s_('Profiles|This email will be used for web based operations, such as edits and merges. %{commit_email_link_start}Learn more%{commit_email_link_end}').html_safe % { commit_email_link_start: commit_email_link_start, commit_email_link_end: '</a>'.html_safe }
-= form.select :commit_email, options_for_select(commit_email_select_options(@user), selected: @user.read_attribute(:commit_email)),
+= form.select :commit_email, options_for_select(commit_email_select_options(@user), selected: @user.commit_email),
   { help: commit_email_docs_link },
   control_class: 'select2 input-lg', disabled: email_change_disabled

app/views/profiles/emails/index.html.haml

@@ -38,21 +38,21 @@
         = render partial: 'shared/email_with_badge', locals: { email: @primary_email, verified: current_user.confirmed? }
         %span.float-right
           %span.badge.badge-muted.badge-pill.gl-badge.badge-success= s_('Profiles|Primary email')
-          - if @primary_email === current_user.commit_email
+          - if @primary_email === current_user.commit_email_or_default
             %span.badge.badge-muted.badge-pill.gl-badge.badge-info= s_('Profiles|Commit email')
           - if @primary_email === current_user.public_email
             %span.badge.badge-muted.badge-pill.gl-badge.badge-info= s_('Profiles|Public email')
-          - if @primary_email === current_user.notification_email
+          - if @primary_email === current_user.notification_email_or_default
             %span.badge.badge-muted.badge-pill.gl-badge.badge-info= s_('Profiles|Default notification email')
       - @emails.each do |email|
         %li{ data: { qa_selector: 'email_row_content' } }
           = render partial: 'shared/email_with_badge', locals: { email: email.email, verified: email.confirmed? }
           %span.float-right
-            - if email.email === current_user.commit_email
+            - if email.email === current_user.commit_email_or_default
               %span.badge.badge-muted.badge-pill.gl-badge.badge-info= s_('Profiles|Commit email')
             - if email.email === current_user.public_email
               %span.badge.badge-muted.badge-pill.gl-badge.badge-info= s_('Profiles|Public email')
-            - if email.email === current_user.notification_email
+            - if email.email === current_user.notification_email_or_default
               %span.badge.badge-muted.badge-pill.gl-badge.badge-info= s_('Profiles|Notification email')
             - unless email.confirmed?
               - confirm_title = "#{email.confirmation_sent_at ? _('Resend confirmation email') : _('Send confirmation email')}"

app/views/profiles/notifications/_email_settings.html.haml

 - form = local_assigns.fetch(:form)
 .form-group
   = form.label :notification_email, class: "label-bold"
-  = form.select :notification_email, @user.public_verified_emails, { include_blank: _('Use primary email (%{email})') % { email: @user.email }, selected: @user.read_attribute(:notification_email) }, class: "select2", disabled: local_assigns.fetch(:email_change_disabled, nil)
+  = form.select :notification_email, @user.public_verified_emails, { include_blank: _('Use primary email (%{email})') % { email: @user.email }, selected: @user.notification_email }, class: "select2", disabled: local_assigns.fetch(:email_change_disabled, nil)
   .help-block
     = local_assigns.fetch(:help_text, nil)
 .form-group

app/views/projects/issues/_nav_btns.html.haml

@@ -3,7 +3,7 @@
 - show_export_button = local_assigns.fetch(:show_export_button, true)
 - issuable_type = 'issues'
 - can_edit = can?(current_user, :admin_project, @project)
-- notification_email = @current_user.present? ? @current_user.notification_email : nil
+- notification_email = @current_user.present? ? @current_user.notification_email_or_default : nil
 
 .nav-controls.issues-nav-controls
   - if show_feed_buttons

app/views/projects/merge_requests/_nav_btns.html.haml

 - issuable_type = 'merge-requests'
-- notification_email = @current_user.present? ? @current_user.notification_email : nil
+- notification_email = @current_user.present? ? @current_user.notification_email_or_default : nil
 
 = render 'shared/issuable/feed_buttons', show_calendar_button: false
 .js-csv-import-export-buttons{ data: { show_export_button: "true", issuable_type: issuable_type, issuable_count: issuables_count_for_state(issuable_type.to_sym, params[:state]), email: notification_email, export_csv_path: export_csv_project_merge_requests_path(@project, request.query_parameters), container_class: 'gl-mr-3' } }

ee/app/controllers/concerns/credentials_inventory_actions.rb

@@ -59,7 +59,7 @@ module CredentialsInventoryActions
     if credential.is_a?(Key)
       CredentialsInventoryMailer.ssh_key_deleted_email(
         params: {
-          notification_email: credential.user.notification_email,
+          notification_email: credential.user.notification_email_or_default,
           title: credential.title,
           last_used_at: credential.last_used_at,
           created_at: credential.created_at

ee/app/mailers/credentials_inventory_mailer.rb

@@ -10,7 +10,7 @@ class CredentialsInventoryMailer < ApplicationMailer
     @token = token
 
     mail(
-      to: token.user.notification_email,
+      to: token.user.notification_email_or_default,
       subject: _('Your Personal Access Token was revoked')
     )
   end

ee/app/mailers/ee/emails/profile.rb

@@ -11,7 +11,7 @@ module EE
         @target_url = profile_personal_access_tokens_url
 
         ::Gitlab::I18n.with_locale(@user.preferred_language) do
-          mail(to: user.notification_email, subject: subject(_("One or more of you personal access tokens were revoked")))
+          mail(to: user.notification_email_or_default, subject: subject(_("One or more of you personal access tokens were revoked")))
         end
       end
     end

ee/app/mailers/emails/user_cap.rb

@@ -4,7 +4,7 @@ module Emails
   module UserCap
     def user_cap_reached(user_id)
       user = User.find(user_id)
-      email = user.notification_email
+      email = user.notification_email_or_default
 
       @url_to_user_cap = 'https://docs.gitlab.com/ee/user/admin_area/settings/sign_up_restrictions.html#user-cap'
       @url_to_pending_users = 'https://docs.gitlab.com/ee/user/admin_area/approving_users.html#view-user-sign-ups-pending-approval'

ee/app/services/ee/merge_requests/merge_base_service.rb

@@ -42,8 +42,8 @@ module EE
           "Commit message does not follow the pattern '#{push_rule.commit_message_regex}'"
         elsif push_rule.commit_message_blocked?(params[:commit_message])
           "Commit message contains the forbidden pattern '#{push_rule.commit_message_negative_regex}'"
-        elsif !push_rule.author_email_allowed?(current_user.commit_email)
-          "Commit author's email '#{current_user.commit_email}' does not follow the pattern '#{push_rule.author_email_regex}'"
+        elsif !push_rule.author_email_allowed?(current_user.commit_email_or_default)
+          "Author's commit email '#{current_user.commit_email_or_default}' does not follow the pattern '#{push_rule.author_email_regex}'"
         end
       end
 

ee/spec/features/merge_request/user_merges_with_push_rules_spec.rb

@@ -37,7 +37,7 @@ RSpec.describe 'Merge request > User merges with Push Rules', :js do
     it 'displays error message after merge request is clicked' do
       click_button 'Merge'
 
-      expect(page).to have_content("Commit author's email '#{user.email}' does not follow the pattern '#{push_rule.author_email_regex}'")
+      expect(page).to have_content("Author's commit email '#{user.email}' does not follow the pattern '#{push_rule.author_email_regex}'")
     end
   end
 

ee/spec/mailers/credentials_inventory_mailer_spec.rb

@@ -17,7 +17,7 @@ RSpec.describe CredentialsInventoryMailer do
     it { is_expected.to have_body_text token.name }
     it { is_expected.to have_body_text "Created on #{token.created_at.to_date.to_s(:medium)}" }
     it { is_expected.to have_body_text 'Scopes: api, sudo'}
-    it { is_expected.to be_delivered_to [token.user.notification_email] }
+    it { is_expected.to be_delivered_to [token.user.notification_email_or_default] }
     it { is_expected.to have_body_text 'Last used 21 days ago' }
   end
 
@@ -26,7 +26,7 @@ RSpec.describe CredentialsInventoryMailer do
 
     let(:params) do
       {
-          notification_email: ssh_key.user.notification_email,
+          notification_email: ssh_key.user.notification_email_or_default,
           title: ssh_key.title,
           last_used_at: ssh_key.last_used_at,
           created_at: ssh_key.created_at
@@ -37,7 +37,7 @@ RSpec.describe CredentialsInventoryMailer do
 
     it { is_expected.to have_subject 'Your SSH key was deleted' }
     it { is_expected.to have_body_text 'The following SSH key was deleted by an administrator, Revoker' }
-    it { is_expected.to be_delivered_to [ssh_key.user.notification_email] }
+    it { is_expected.to be_delivered_to [ssh_key.user.notification_email_or_default] }
     it { is_expected.to have_body_text ssh_key.title }
     it { is_expected.to have_body_text "Created on #{ssh_key.created_at.to_date.to_s(:medium)}" }
     it { is_expected.to have_body_text 'Last used 21 days ago' }

ee/spec/mailers/emails/user_cap_spec.rb

@@ -11,7 +11,7 @@ RSpec.describe Emails::UserCap do
     subject { Notify.user_cap_reached(user.id) }
 
     it { is_expected.to have_subject('Important information about usage on your GitLab instance') }
-    it { is_expected.to be_delivered_to([user.notification_email]) }
+    it { is_expected.to be_delivered_to([user.notification_email_or_default]) }
     it { is_expected.to have_body_text('Your GitLab instance has reached the maximum allowed') }
     it { is_expected.to have_body_text('user cap') }
   end

ee/spec/services/ee/users/update_service_spec.rb

@@ -153,7 +153,7 @@ RSpec.describe Users::UpdateService do
       end.not_to change { user.reload.commit_email }
     end
 
-    it 'does not update public if an user has group managed account' do
+    it 'does not update public email if an user has group managed account' do
       allow(user).to receive(:group_managed_account?).and_return(true)
 
       expect do
@@ -161,7 +161,7 @@ RSpec.describe Users::UpdateService do
       end.not_to change { user.reload.public_email }
     end
 
-    it 'does not update public if an user has group managed account' do
+    it 'does not update notification email if an user has group managed account' do
       allow(user).to receive(:group_managed_account?).and_return(true)
 
       expect do

lib/api/entities/global_notification_setting.rb

@@ -4,7 +4,7 @@ module API
   module Entities
     class GlobalNotificationSetting < Entities::NotificationSetting
       expose :notification_email do |notification_setting, options|
-        notification_setting.user.notification_email
+        notification_setting.user.notification_email_or_default
       end
     end
   end

lib/api/entities/user_public.rb

@@ -14,7 +14,7 @@ module API
       expose :two_factor_enabled?, as: :two_factor_enabled
       expose :external
       expose :private_profile
-      expose :commit_email
+      expose :commit_email_or_default, as: :commit_email
     end
   end
 end

lib/gitlab/background_migration/mailers/unconfirm_mailer.rb

@@ -14,7 +14,7 @@ module Gitlab
           mail(
             template_path: 'unconfirm_mailer',
             template_name: 'unconfirm_notification_email',
-            to: @user.notification_email,
+            to: @user.notification_email_or_default,
             subject: subject('GitLab email verification request')
           )
         end

lib/gitlab/git/user.rb

@@ -6,7 +6,7 @@ module Gitlab
       attr_reader :username, :name, :email, :gl_id, :timezone
 
       def self.from_gitlab(gitlab_user)
-        new(gitlab_user.username, gitlab_user.name, gitlab_user.commit_email, Gitlab::GlId.gl_id(gitlab_user), gitlab_user.timezone)
+        new(gitlab_user.username, gitlab_user.name, gitlab_user.commit_email_or_default, Gitlab::GlId.gl_id(gitlab_user), gitlab_user.timezone)
       end
 
       def self.from_gitaly(gitaly_user)

spec/controllers/admin/users_controller_spec.rb

@@ -146,7 +146,7 @@ RSpec.describe Admin::UsersController do
 
       it 'sends the user a rejection email' do
         expect_next_instance_of(NotificationService) do |notification|
-          allow(notification).to receive(:user_admin_rejection).with(user.name, user.notification_email)
+          allow(notification).to receive(:user_admin_rejection).with(user.name, user.notification_email_or_default)
         end
 
         subject

spec/features/groups/members/request_access_spec.rb

@@ -24,7 +24,7 @@ RSpec.describe 'Groups > Members > Request access' do
   it 'user can request access to a group' do
     perform_enqueued_jobs { click_link 'Request Access' }
 
-    expect(ActionMailer::Base.deliveries.last.to).to eq [owner.notification_email]
+    expect(ActionMailer::Base.deliveries.last.to).to eq [owner.notification_email_or_default]
     expect(ActionMailer::Base.deliveries.last.subject).to match "Request to join the #{group.name} group"
 
     expect(group.requesters.exists?(user_id: user)).to be_truthy

spec/features/issues/csv_spec.rb

@@ -44,7 +44,7 @@ RSpec.describe 'Issues csv', :js do
     request_csv
 
     expect(page).to have_content 'CSV export has started'
-    expect(page).to have_content "emailed to #{user.notification_email}"
+    expect(page).to have_content "emailed to #{user.notification_email_or_default}"
   end
 
   it 'includes a csv attachment', :sidekiq_might_not_need_inline do

spec/features/projects/members/user_requests_access_spec.rb

@@ -23,7 +23,7 @@ RSpec.describe 'Projects > Members > User requests access', :js do
   it 'user can request access to a project' do
     perform_enqueued_jobs { click_link 'Request Access' }
 
-    expect(ActionMailer::Base.deliveries.last.to).to eq [maintainer.notification_email]
+    expect(ActionMailer::Base.deliveries.last.to).to eq [maintainer.notification_email_or_default]
     expect(ActionMailer::Base.deliveries.last.subject).to eq "Request to join the #{project.full_name} project"
 
     expect(project.requesters.exists?(user_id: user)).to be_truthy

spec/helpers/issues_helper_spec.rb

@@ -310,7 +310,7 @@ RSpec.describe IssuesHelper do
         can_bulk_update: 'true',
         can_edit: 'true',
         can_import_issues: 'true',
-        email: current_user&.notification_email,
+        email: current_user&.notification_email_or_default,
         emails_help_page_path: help_page_path('development/emails', anchor: 'email-namespace'),
         empty_state_svg_path: '#',
         export_csv_path: export_csv_project_issues_path(project),

spec/mailers/emails/in_product_marketing_spec.rb

@@ -23,7 +23,7 @@ RSpec.describe Emails::InProductMarketing do
 
     it 'sends to the right user with a link to unsubscribe' do
       aggregate_failures do
-        expect(subject).to deliver_to(user.notification_email)
+        expect(subject).to deliver_to(user.notification_email_or_default)
         expect(subject).to have_body_text(profile_notifications_url)
       end
     end

spec/mailers/notify_spec.rb

@@ -71,7 +71,7 @@ RSpec.describe Notify do
       it 'is sent to the assignee as the author' do
         aggregate_failures do
           expect_sender(current_user)
-          expect(subject).to deliver_to(recipient.notification_email)
+          expect(subject).to deliver_to(recipient.notification_email_or_default)
         end
       end
     end
@@ -710,7 +710,7 @@ RSpec.describe Notify do
 
       it 'contains all the useful information' do
         to_emails = subject.header[:to].addrs.map(&:address)
-        expect(to_emails).to eq([recipient.notification_email])
+        expect(to_emails).to eq([recipient.notification_email_or_default])
 
         is_expected.to have_subject "Request to join the #{project.full_name} project"
         is_expected.to have_body_text project.full_name
@@ -1047,7 +1047,7 @@ RSpec.describe Notify do
         it 'is sent to the given recipient as the author' do
           aggregate_failures do
             expect_sender(note_author)
-            expect(subject).to deliver_to(recipient.notification_email)
+            expect(subject).to deliver_to(recipient.notification_email_or_default)
           end
         end
 
@@ -1204,7 +1204,7 @@ RSpec.describe Notify do
         it 'is sent to the given recipient as the author' do
           aggregate_failures do
             expect_sender(note_author)
-            expect(subject).to deliver_to(recipient.notification_email)
+            expect(subject).to deliver_to(recipient.notification_email_or_default)
           end
         end
 
@@ -1341,7 +1341,7 @@ RSpec.describe Notify do
 
       it 'contains all the useful information' do
         to_emails = subject.header[:to].addrs.map(&:address)
-        expect(to_emails).to eq([recipient.notification_email])
+        expect(to_emails).to eq([recipient.notification_email_or_default])
 
         is_expected.to have_subject "Request to join the #{group.name} group"
         is_expected.to have_body_text group.name

spec/models/integrations/pipelines_email_spec.rb

@@ -48,7 +48,7 @@ RSpec.describe Integrations::PipelinesEmail, :mailer do
     end
 
     it 'sends email' do
-      emails = receivers.map { |r| double(notification_email: r) }
+      emails = receivers.map { |r| double(notification_email_or_default: r) }
 
       should_only_email(*emails, kind: :bcc)
     end

spec/models/repository_spec.rb

@@ -1826,7 +1826,7 @@ RSpec.describe Repository do
 
       expect(merge_commit.message).to eq('Custom message')
       expect(merge_commit.author_name).to eq(user.name)
-      expect(merge_commit.author_email).to eq(user.commit_email)
+      expect(merge_commit.author_email).to eq(user.commit_email_or_default)
       expect(repository.blob_at(merge_commit.id, 'files/ruby/feature.rb')).to be_present
     end
   end

spec/models/user_spec.rb

@@ -435,12 +435,12 @@ RSpec.describe User do
       subject { create(:user).tap { |user| user.emails << build(:email, email: email_value, confirmed_at: Time.current) } }
     end
 
-    describe '#commit_email' do
+    describe '#commit_email_or_default' do
       subject(:user) { create(:user) }
 
       it 'defaults to the primary email' do
         expect(user.email).to be_present
-        expect(user.commit_email).to eq(user.email)
+        expect(user.commit_email_or_default).to eq(user.email)
       end
 
       it 'defaults to the primary email when the column in the database is null' do
@@ -448,14 +448,18 @@ RSpec.describe User do
 
         found_user = described_class.find_by(id: user.id)
 
-        expect(found_user.commit_email).to eq(user.email)
+        expect(found_user.commit_email_or_default).to eq(user.email)
       end
 
       it 'returns the private commit email when commit_email has _private' do
         user.update_column(:commit_email, Gitlab::PrivateCommitEmail::TOKEN)
 
-        expect(user.commit_email).to eq(user.private_commit_email)
+        expect(user.commit_email_or_default).to eq(user.private_commit_email)
       end
+    end
+
+    describe '#commit_email=' do
+      subject(:user) { create(:user) }
 
       it 'can be set to a confirmed email' do
         confirmed = create(:email, :confirmed, user: user)
@@ -1246,32 +1250,37 @@ RSpec.describe User do
       end
     end
 
-    describe '#update_notification_email' do
-      # Regression: https://gitlab.com/gitlab-org/gitlab-foss/issues/22846
-      context 'when changing :email' do
+    describe 'when changing email' do
       let(:user) { create(:user) }
       let(:new_email) { 'new-email@example.com' }
 
+      context 'if notification_email was nil' do
         it 'sets :unconfirmed_email' do
           expect do
             user.tap { |u| u.update!(email: new_email) }.reload
           end.to change(user, :unconfirmed_email).to(new_email)
         end
-        it 'does not change :notification_email' do
+
+        it 'does not change notification_email or notification_email_or_default before email is confirmed' do
           expect do
             user.tap { |u| u.update!(email: new_email) }.reload
-          end.not_to change(user, :notification_email)
+          end.not_to change(user, :notification_email_or_default)
+
+          expect(user.notification_email).to be_nil
         end
 
-        it 'updates :notification_email to the new email once confirmed' do
+        it 'updates notification_email_or_default to the new email once confirmed' do
           user.update!(email: new_email)
 
           expect do
             user.tap(&:confirm).reload
-          end.to change(user, :notification_email).to eq(new_email)
+          end.to change(user, :notification_email_or_default).to eq(new_email)
+
+          expect(user.notification_email).to be_nil
+        end
       end
 
-        context 'and :notification_email is set to a secondary email' do
+      context 'when notification_email is set to a secondary email' do
         let!(:email_attrs) { attributes_for(:email, :confirmed, user: user) }
         let(:secondary) { create(:email, :confirmed, email: 'secondary@example.com', user: user) }
 
@@ -1280,13 +1289,13 @@ RSpec.describe User do
           user.tap { |u| u.update!(notification_email: email_attrs[:email]) }.reload
         end
 
-          it 'does not change :notification_email to :email' do
+        it 'does not change notification_email to email before email is confirmed' do
           expect do
             user.tap { |u| u.update!(email: new_email) }.reload
           end.not_to change(user, :notification_email)
         end
 
-          it 'does not change :notification_email to :email once confirmed' do
+        it 'does not change notification_email to email once confirmed' do
           user.update!(email: new_email)
 
           expect do
@@ -1295,7 +1304,6 @@ RSpec.describe User do
         end
       end
     end
-    end
 
     describe '#update_invalid_gpg_signatures' do
       let(:user) do
@@ -1878,6 +1886,7 @@ RSpec.describe User do
         end
         it 'does not send deactivated user an email' do
           expect(NotificationService).not_to receive(:new)
+
           user.deactivate
         end
       end
@@ -1885,7 +1894,7 @@ RSpec.describe User do
       context "when user deactivation emails are enabled" do
         it 'sends deactivated user an email' do
           expect_next_instance_of(NotificationService) do |notification|
-            allow(notification).to receive(:user_deactivated).with(user.name, user.notification_email)
+            allow(notification).to receive(:user_deactivated).with(user.name, user.notification_email_or_default)
           end
 
           user.deactivate
@@ -3084,15 +3093,15 @@ RSpec.describe User do
     end
   end
 
-  describe '#notification_email' do
+  describe '#notification_email_or_default' do
     let(:email) { 'gonzo@muppets.com' }
 
     context 'when the column in the database is null' do
       subject { create(:user, email: email, notification_email: nil) }
 
       it 'defaults to the primary email' do
-        expect(subject.read_attribute(:notification_email)).to be nil
-        expect(subject.notification_email).to eq(email)
+        expect(subject.notification_email).to be nil
+        expect(subject.notification_email_or_default).to eq(email)
       end
     end
   end
@@ -5335,7 +5344,7 @@ RSpec.describe User do
       let(:group) { nil }
 
       it 'returns global notification email' do
-        is_expected.to eq(user.notification_email)
+        is_expected.to eq(user.notification_email_or_default)
       end
     end
 
@@ -5343,7 +5352,7 @@ RSpec.describe User do
       it 'returns global notification email' do
         create(:notification_setting, user: user, source: group, notification_email: '')
 
-        is_expected.to eq(user.notification_email)
+        is_expected.to eq(user.notification_email_or_default)
       end
     end
 
@@ -6132,7 +6141,7 @@ RSpec.describe User do
       it 'does nothing' do
         expect(subject).not_to receive(:save)
         subject.unset_secondary_emails_matching_deleted_email!(deleted_email)
-        expect(subject.read_attribute(:commit_email)).to eq commit_email
+        expect(subject.commit_email).to eq commit_email
       end
     end
 
@@ -6142,7 +6151,7 @@ RSpec.describe User do
       it 'un-sets the secondary email' do
         expect(subject).to receive(:save)
         subject.unset_secondary_emails_matching_deleted_email!(deleted_email)
-        expect(subject.read_attribute(:commit_email)).to be nil
+        expect(subject.commit_email).to be nil
       end
     end
   end

spec/requests/api/notification_settings_spec.rb

@@ -13,7 +13,7 @@ RSpec.describe API::NotificationSettings do
 
       expect(response).to have_gitlab_http_status(:ok)
       expect(json_response).to be_a Hash
-      expect(json_response['notification_email']).to eq(user.notification_email)
+      expect(json_response['notification_email']).to eq(user.notification_email_or_default)
       expect(json_response['level']).to eq(user.global_notification_setting.level)
     end
   end

spec/requests/api/users_spec.rb

@@ -1202,7 +1202,7 @@ RSpec.describe API::Users do
 
     it 'updates user with a new email' do
       old_email = user.email
-      old_notification_email = user.notification_email
+      old_notification_email = user.notification_email_or_default
       put api("/users/#{user.id}", admin), params: { email: 'new@email.com' }
 
       user.reload
@@ -1210,7 +1210,7 @@ RSpec.describe API::Users do
       expect(response).to have_gitlab_http_status(:ok)
       expect(user).to be_confirmed
       expect(user.email).to eq(old_email)
-      expect(user.notification_email).to eq(old_notification_email)
+      expect(user.notification_email_or_default).to eq(old_notification_email)
       expect(user.unconfirmed_email).to eq('new@email.com')
     end
 

spec/services/notification_service_spec.rb

@@ -2623,7 +2623,7 @@ RSpec.describe NotificationService, :mailer do
     let_it_be(:user) { create(:user) }
 
     it 'sends the user an email' do
-      notification.user_deactivated(user.name, user.notification_email)
+      notification.user_deactivated(user.name, user.notification_email_or_default)
 
       should_only_email(user)
     end

spec/services/suggestions/apply_service_spec.rb

@@ -70,7 +70,7 @@ RSpec.describe Suggestions::ApplyService do
       author = suggestions.first.note.author
 
       expect(user.commit_email).not_to eq(user.email)
-      expect(commit.author_email).to eq(author.commit_email)
+      expect(commit.author_email).to eq(author.commit_email_or_default)
       expect(commit.committer_email).to eq(user.commit_email)
       expect(commit.author_name).to eq(author.name)
       expect(commit.committer_name).to eq(user.name)
@@ -333,9 +333,9 @@ RSpec.describe Suggestions::ApplyService do
           end
 
           it 'created commit by same author and committer' do
-            expect(user.commit_email).to eq(author.commit_email)
+            expect(user.commit_email).to eq(author.commit_email_or_default)
             expect(author).to eq(user)
-            expect(commit.author_email).to eq(author.commit_email)
+            expect(commit.author_email).to eq(author.commit_email_or_default)
             expect(commit.committer_email).to eq(user.commit_email)
             expect(commit.author_name).to eq(author.name)
             expect(commit.committer_name).to eq(user.name)
@@ -350,7 +350,7 @@ RSpec.describe Suggestions::ApplyService do
           it 'created commit has authors info and commiters info' do
             expect(user.commit_email).not_to eq(user.email)
             expect(author).not_to eq(user)
-            expect(commit.author_email).to eq(author.commit_email)
+            expect(commit.author_email).to eq(author.commit_email_or_default)
             expect(commit.committer_email).to eq(user.commit_email)
             expect(commit.author_name).to eq(author.name)
             expect(commit.committer_name).to eq(user.name)
@@ -359,7 +359,7 @@ RSpec.describe Suggestions::ApplyService do
       end
 
       context 'multiple suggestions' do
-        let(:author_emails) { suggestions.map {|s| s.note.author.commit_email } }
+        let(:author_emails) { suggestions.map {|s| s.note.author.commit_email_or_default } }
         let(:first_author) { suggestion.note.author }
         let(:commit) { project.repository.commit }
 
@@ -369,8 +369,8 @@ RSpec.describe Suggestions::ApplyService do
           end
 
           it 'uses first authors information' do
-            expect(author_emails).to include(first_author.commit_email).exactly(3)
-            expect(commit.author_email).to eq(first_author.commit_email)
+            expect(author_emails).to include(first_author.commit_email_or_default).exactly(3)
+            expect(commit.author_email).to eq(first_author.commit_email_or_default)
           end
         end
 

spec/services/users/reject_service_spec.rb

@@ -44,7 +44,7 @@ RSpec.describe Users::RejectService do
 
         it 'emails the user on rejection' do
           expect_next_instance_of(NotificationService) do |notification|
-            allow(notification).to receive(:user_admin_rejection).with(user.name, user.notification_email)
+            allow(notification).to receive(:user_admin_rejection).with(user.name, user.notification_email_or_default)
           end
 
           subject

spec/support/helpers/email_helpers.rb

@@ -2,7 +2,7 @@
 
 module EmailHelpers
   def sent_to_user(user, recipients: email_recipients)
-    recipients.count { |to| to == user.notification_email }
+    recipients.count { |to| to == user.notification_email_or_default }
   end
 
   def reset_delivered_emails!
@@ -45,7 +45,7 @@ module EmailHelpers
   end
 
   def find_email_for(user)
-    ActionMailer::Base.deliveries.find { |d| d.to.include?(user.notification_email) }
+    ActionMailer::Base.deliveries.find { |d| d.to.include?(user.notification_email_or_default) }
   end
 
   def have_referable_subject(referable, include_project: true, reply: false)

spec/support/shared_examples/mailers/notify_shared_examples.rb

@@ -2,7 +2,7 @@
 
 RSpec.shared_examples 'a multiple recipients email' do
   it 'is sent to the given recipient' do
-    is_expected.to deliver_to recipient.notification_email
+    is_expected.to deliver_to recipient.notification_email_or_default
   end
 end
 
@@ -21,7 +21,7 @@ end
 
 RSpec.shared_examples 'an email sent to a user' do
   it 'is sent to user\'s global notification email address' do
-    expect(subject).to deliver_to(recipient.notification_email)
+    expect(subject).to deliver_to(recipient.notification_email_or_default)
   end
 
   context 'with group notification email' do
@@ -227,7 +227,7 @@ RSpec.shared_examples 'a note email' do
     aggregate_failures do
       expect(sender.display_name).to eq("#{note_author.name} (@#{note_author.username})")
       expect(sender.address).to eq(gitlab_sender)
-      expect(subject).to deliver_to(recipient.notification_email)
+      expect(subject).to deliver_to(recipient.notification_email_or_default)
     end
   end