diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index d14df4600e1ef70bcf568f7b4c8c91464ed45677..eea4f779447e77dce4771df153bd1eb1e95d95c7 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -227,7 +227,7 @@ def export_csv 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 diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 1129f3ee7e1c650d0d038da9cc853d5eeeda7256..cb68aaf45830cf741098d07be9ac2074c0fe7a08 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -378,7 +378,7 @@ def export_csv 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 diff --git a/app/helpers/issues_helper.rb b/app/helpers/issues_helper.rb index 18dc882679d82c454410da10be3b6e02e3c144c3..9c0db98b88da1ef5092079b22a5f4f9f7ac734e2 100644 --- a/app/helpers/issues_helper.rb +++ b/app/helpers/issues_helper.rb @@ -210,7 +210,7 @@ def issues_list_data(project, current_user, finder) 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'), empty_state_svg_path: image_path('illustrations/issues.svg'), export_csv_path: export_csv_project_issues_path(project), diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index f30223f6f1e0b6fbe4ba436be9a405f3e0f94ed8..d7f1cd505e92bbe79d9b312fb409600fb9afa380 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -435,7 +435,7 @@ def git_user_name def git_user_email if current_user - current_user.commit_email + current_user.commit_email_or_default else "your@email.com" end diff --git a/app/mailers/emails/admin_notification.rb b/app/mailers/emails/admin_notification.rb index f4540ef81a574c3f0c4636ac6f97a27ad44bee2b..e11f06d8fc94d66ae325839688b85267108808dd 100644 --- a/app/mailers/emails/admin_notification.rb +++ b/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 @@ def send_admin_notification(user_id, subject, body) 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 diff --git a/app/mailers/emails/profile.rb b/app/mailers/emails/profile.rb index a8affb34f629f2e8351d4842f4aa36d744c30f15..592c394bb484160258a2ca53853835484eebd962 100644 --- a/app/mailers/emails/profile.rb +++ b/app/mailers/emails/profile.rb @@ -6,7 +6,7 @@ def new_user_email(user_id, token = nil) @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 @@ def instance_access_request_email(user, recipient) @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 @@ def new_ssh_key_email(key_id) @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 @@ def new_gpg_key_email(gpg_key_id) @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 @@ def access_token_about_to_expire_email(user, token_names) @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 @@ def access_token_expired_email(user) @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 @@ def ssh_key_expired_email(user, fingerprints) @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 @@ def ssh_key_expiring_soon_email(user, fingerprints) @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 @@ def unknown_sign_in_email(user, ip, time) 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 @@ def disabled_two_factor_email(user) @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 diff --git a/app/models/user.rb b/app/models/user.rb index c1ef8229898b5c2dc1b0f45a54e92c577ea5eb00..9a941daf34a2e56d867c19f58f58f38f3ae99947 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -229,10 +229,9 @@ def update_tracked_fields!(request) 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 @@ def blocked? 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 @@ def unique_email 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 @@ def owns_runner?(runner) 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 @@ def consume_otp! 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 @@ def public_email_verified 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 diff --git a/app/views/profiles/_email_settings.html.haml b/app/views/profiles/_email_settings.html.haml index 95306633556ad05ef42a6fbd53f0042e9d03a4b8..bc678c2c42991ed42cc3aa60ff7086e0548c4389 100644 --- a/app/views/profiles/_email_settings.html.haml +++ b/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 diff --git a/app/views/profiles/emails/index.html.haml b/app/views/profiles/emails/index.html.haml index c14efa99555478de1f92643429cf06ada34e4f36..35bdfbb1c29415445fdf062f1a6676af9c7a084e 100644 --- a/app/views/profiles/emails/index.html.haml +++ b/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')}" diff --git a/app/views/profiles/notifications/_email_settings.html.haml b/app/views/profiles/notifications/_email_settings.html.haml index e1e4b2bf2d6b775b94b0534f1091da51b1abc8d7..f2121199412f49d5f0526db7dd7a86305543c5b0 100644 --- a/app/views/profiles/notifications/_email_settings.html.haml +++ b/app/views/profiles/notifications/_email_settings.html.haml @@ -1,7 +1,7 @@ - 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 diff --git a/app/views/projects/issues/_nav_btns.html.haml b/app/views/projects/issues/_nav_btns.html.haml index 1289f7aa0c400dc54e1579359f04547bd4d7e7ed..0d69f6f69aab0e49c2f96c33a04c00c7f9415f27 100644 --- a/app/views/projects/issues/_nav_btns.html.haml +++ b/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 diff --git a/app/views/projects/merge_requests/_nav_btns.html.haml b/app/views/projects/merge_requests/_nav_btns.html.haml index 84edf62b300857fe80b80f2059be9e385e772fa3..b34cf23634c348aadcdb1e719fb4c1b0870cb050 100644 --- a/app/views/projects/merge_requests/_nav_btns.html.haml +++ b/app/views/projects/merge_requests/_nav_btns.html.haml @@ -1,5 +1,5 @@ - 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' } } diff --git a/ee/app/controllers/concerns/credentials_inventory_actions.rb b/ee/app/controllers/concerns/credentials_inventory_actions.rb index 7f1007b8bb91af7118d4317f0b8eee0da04fa09d..89223f50a2f994131c1e1e726f2c61a652848745 100644 --- a/ee/app/controllers/concerns/credentials_inventory_actions.rb +++ b/ee/app/controllers/concerns/credentials_inventory_actions.rb @@ -59,7 +59,7 @@ def notify_deleted_or_revoked_credential(credential) 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 diff --git a/ee/app/mailers/credentials_inventory_mailer.rb b/ee/app/mailers/credentials_inventory_mailer.rb index 046fdac23208a9876869a503a4ff91f83e9fc16e..3a1414a64d3952ed5ae3fca78142e50f7358fbe9 100644 --- a/ee/app/mailers/credentials_inventory_mailer.rb +++ b/ee/app/mailers/credentials_inventory_mailer.rb @@ -10,7 +10,7 @@ def personal_access_token_revoked_email(token:, revoked_by:) @token = token mail( - to: token.user.notification_email, + to: token.user.notification_email_or_default, subject: _('Your Personal Access Token was revoked') ) end diff --git a/ee/app/mailers/ee/emails/profile.rb b/ee/app/mailers/ee/emails/profile.rb index 1354a4cfd339b01832ee6ba360c951550f07c86c..b37000ba5cbd7c06c03774b1295112759eb3c6f8 100644 --- a/ee/app/mailers/ee/emails/profile.rb +++ b/ee/app/mailers/ee/emails/profile.rb @@ -11,7 +11,7 @@ def policy_revoked_personal_access_tokens_email(user, revoked_token_names) @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 diff --git a/ee/app/mailers/emails/user_cap.rb b/ee/app/mailers/emails/user_cap.rb index b78f593a4a1c798a20e72943daacb8937cd86524..e9afd1bca6be9fcfc49be665fa01121081879e14 100644 --- a/ee/app/mailers/emails/user_cap.rb +++ b/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' diff --git a/ee/app/services/ee/merge_requests/merge_base_service.rb b/ee/app/services/ee/merge_requests/merge_base_service.rb index 5a106bab41c2603840b4a1f8e429273e78fc4127..10cc4402d1a9ac6c7b99e8863910ff1af9e871cc 100644 --- a/ee/app/services/ee/merge_requests/merge_base_service.rb +++ b/ee/app/services/ee/merge_requests/merge_base_service.rb @@ -42,8 +42,8 @@ def hooks_validation_error(merge_request) "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 diff --git a/ee/spec/features/merge_request/user_merges_with_push_rules_spec.rb b/ee/spec/features/merge_request/user_merges_with_push_rules_spec.rb index ba287a15be7f74d00dbd2fb5a9fa9c0162276a9a..e3dad184b0dd2e6be3dacfb0f45512f3980f5034 100644 --- a/ee/spec/features/merge_request/user_merges_with_push_rules_spec.rb +++ b/ee/spec/features/merge_request/user_merges_with_push_rules_spec.rb @@ -37,7 +37,7 @@ 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 diff --git a/ee/spec/mailers/credentials_inventory_mailer_spec.rb b/ee/spec/mailers/credentials_inventory_mailer_spec.rb index e5868149538ee6576e524004db3a70dbdde81751..3a692ac93e6700b7c4ab7e98aa4d84af7112ae0a 100644 --- a/ee/spec/mailers/credentials_inventory_mailer_spec.rb +++ b/ee/spec/mailers/credentials_inventory_mailer_spec.rb @@ -17,7 +17,7 @@ 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 @@ 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 @@ 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' } diff --git a/ee/spec/mailers/emails/user_cap_spec.rb b/ee/spec/mailers/emails/user_cap_spec.rb index a210e906f109bf4cee3b8035a5fa5a9f6e2b36e7..45adfc7f9f56f34556efc3290f7676e8a7599036 100644 --- a/ee/spec/mailers/emails/user_cap_spec.rb +++ b/ee/spec/mailers/emails/user_cap_spec.rb @@ -11,7 +11,7 @@ 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 diff --git a/ee/spec/services/ee/users/update_service_spec.rb b/ee/spec/services/ee/users/update_service_spec.rb index e1079774ead11fb2cd70a64504be746620f58623..83a54739b07d260cd113676183f36c2df3fa43d8 100644 --- a/ee/spec/services/ee/users/update_service_spec.rb +++ b/ee/spec/services/ee/users/update_service_spec.rb @@ -153,7 +153,7 @@ 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 @@ 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 diff --git a/lib/api/entities/global_notification_setting.rb b/lib/api/entities/global_notification_setting.rb index f3ca64347f08b45c6d3db1d350072886fb91d10b..f35efad5d01e17f08fefbd745248f2b2e56e8137 100644 --- a/lib/api/entities/global_notification_setting.rb +++ b/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 diff --git a/lib/api/entities/user_public.rb b/lib/api/entities/user_public.rb index 78f088d3c1a2fc6708b24475418bf096138504b4..5d0e464abe18c67715e8f20735e787a9aa5ec284 100644 --- a/lib/api/entities/user_public.rb +++ b/lib/api/entities/user_public.rb @@ -14,7 +14,7 @@ class UserPublic < Entities::User 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 diff --git a/lib/gitlab/background_migration/mailers/unconfirm_mailer.rb b/lib/gitlab/background_migration/mailers/unconfirm_mailer.rb index c096dae0631a5d6af9b144da77192256122e24a9..3605b157f4f174e360526e6aebb39eb853e4e532 100644 --- a/lib/gitlab/background_migration/mailers/unconfirm_mailer.rb +++ b/lib/gitlab/background_migration/mailers/unconfirm_mailer.rb @@ -14,7 +14,7 @@ def unconfirm_notification_email(user) 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 diff --git a/lib/gitlab/git/user.rb b/lib/gitlab/git/user.rb index 05ae3391040c2401f47f6d17d64e5bf1616fe39f..0798cc5105511c3147e3f912478d11defe27c05d 100644 --- a/lib/gitlab/git/user.rb +++ b/lib/gitlab/git/user.rb @@ -6,7 +6,7 @@ class User 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) diff --git a/spec/controllers/admin/users_controller_spec.rb b/spec/controllers/admin/users_controller_spec.rb index 2d5125c9d5e15ea5919dd9b9f6c9c2ca223d159a..015c36c93358de99dba4a68c87aef51afa0a7441 100644 --- a/spec/controllers/admin/users_controller_spec.rb +++ b/spec/controllers/admin/users_controller_spec.rb @@ -146,7 +146,7 @@ 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 diff --git a/spec/features/groups/members/request_access_spec.rb b/spec/features/groups/members/request_access_spec.rb index 827962fee6133433a49111ec2512513a3513792a..f806c7d37044a1ba3e66295aaab8d7060b84746a 100644 --- a/spec/features/groups/members/request_access_spec.rb +++ b/spec/features/groups/members/request_access_spec.rb @@ -24,7 +24,7 @@ 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 diff --git a/spec/features/issues/csv_spec.rb b/spec/features/issues/csv_spec.rb index 51e0d54ca5e142588f5ede1889f27aee43c59e71..b4c737495b4792b1a2596fd9e342c69aaa588606 100644 --- a/spec/features/issues/csv_spec.rb +++ b/spec/features/issues/csv_spec.rb @@ -44,7 +44,7 @@ def csv 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 diff --git a/spec/features/projects/members/user_requests_access_spec.rb b/spec/features/projects/members/user_requests_access_spec.rb index 94543290050463e253ea5a1d71e806e25e595a77..113ba69249788e026bbb315b9cf858319a00457c 100644 --- a/spec/features/projects/members/user_requests_access_spec.rb +++ b/spec/features/projects/members/user_requests_access_spec.rb @@ -23,7 +23,7 @@ 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 diff --git a/spec/helpers/issues_helper_spec.rb b/spec/helpers/issues_helper_spec.rb index 898e5d15549ba0352372087940954807d611d195..af6e7f7774dffeaabd31d16080d34a9d95946e55 100644 --- a/spec/helpers/issues_helper_spec.rb +++ b/spec/helpers/issues_helper_spec.rb @@ -310,7 +310,7 @@ 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), diff --git a/spec/mailers/emails/in_product_marketing_spec.rb b/spec/mailers/emails/in_product_marketing_spec.rb index 74354630ade206db39f73435cb2738f2f9788bbe..99beef92deac41e7f917c7e9ef366f60b700254c 100644 --- a/spec/mailers/emails/in_product_marketing_spec.rb +++ b/spec/mailers/emails/in_product_marketing_spec.rb @@ -23,7 +23,7 @@ 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 diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index dd1b08b506fbc149aabca3a418d87a187ca6cc49..f39037cf744aa3dc3ce990a65317ca82ebd4d9c0 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -71,7 +71,7 @@ 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 @@ def id 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 @@ def invite_to_project(project, inviter:, user: nil) 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 @@ def create_note 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 @@ def create_note 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 diff --git a/spec/models/integrations/pipelines_email_spec.rb b/spec/models/integrations/pipelines_email_spec.rb index 761049f25fe6ffd2180191dd41e5794f09d54a24..afd9d71ebc425d376c1b34cdde17e4039bf96b6c 100644 --- a/spec/models/integrations/pipelines_email_spec.rb +++ b/spec/models/integrations/pipelines_email_spec.rb @@ -48,7 +48,7 @@ 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 diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 047e1ac93e2bd2c9fab97202d0810ce2f752e7da..0386ad1329b6b064054ce1372f7e4c831a3ace01 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1826,7 +1826,7 @@ def merge(repository, user, merge_request, message) 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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 306a5534737e84e28154bda43abe088b51b0815e..b0b031c2fae1cbb3181f6016bd44c5e871b5a24e 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -435,12 +435,12 @@ 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 @@ 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,53 +1250,57 @@ end end - describe '#update_notification_email' do - # Regression: https://gitlab.com/gitlab-org/gitlab-foss/issues/22846 - context 'when changing :email' do - let(:user) { create(:user) } - let(:new_email) { 'new-email@example.com' } + 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 - let!(:email_attrs) { attributes_for(:email, :confirmed, user: user) } - let(:secondary) { create(:email, :confirmed, email: 'secondary@example.com', user: user) } + 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) } - before do - user.emails.create!(email_attrs) - user.tap { |u| u.update!(notification_email: email_attrs[:email]) }.reload - end + before do + user.emails.create!(email_attrs) + user.tap { |u| u.update!(notification_email: email_attrs[:email]) }.reload + end - it 'does not change :notification_email to :email' 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 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 - user.update!(email: new_email) + it 'does not change notification_email to email once confirmed' do + user.update!(email: new_email) - expect do - user.tap(&:confirm).reload - end.not_to change(user, :notification_email) - end + expect do + user.tap(&:confirm).reload + end.not_to change(user, :notification_email) end end end @@ -1878,6 +1886,7 @@ end it 'does not send deactivated user an email' do expect(NotificationService).not_to receive(:new) + user.deactivate end end @@ -1885,7 +1894,7 @@ 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 @@ 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 @@ def access_levels(groups) 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 @@ def access_levels(groups) 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 @@ def access_levels(groups) 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 @@ def access_levels(groups) 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 diff --git a/spec/requests/api/notification_settings_spec.rb b/spec/requests/api/notification_settings_spec.rb index 7b4a58e63da87ff44fbf5d0ae80474bcd0963f44..b5551c21738568b16b7189351c05ac0d0e2e0906 100644 --- a/spec/requests/api/notification_settings_spec.rb +++ b/spec/requests/api/notification_settings_spec.rb @@ -13,7 +13,7 @@ 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 diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index 8b864346c5d38bdcf06770f71aee13e88a4b71c4..ac86c922813de996a21c5cc9a95dd58942c1f9b5 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -1202,7 +1202,7 @@ def update_password(user, admin, password = User.random_password) 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 @@ def update_password(user, admin, password = User.random_password) 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 diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 3c4d7d50002ffb7516125e4b4e835b2f44daaccc..a03f1f17b39aace019701a9d1725a9fd63495d7b 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -2623,7 +2623,7 @@ 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 diff --git a/spec/services/suggestions/apply_service_spec.rb b/spec/services/suggestions/apply_service_spec.rb index d3dcbf0b66827012d18a52218753d3bc27563da4..dc330a5546f290b4f18e198346ec5f7c1974a316 100644 --- a/spec/services/suggestions/apply_service_spec.rb +++ b/spec/services/suggestions/apply_service_spec.rb @@ -70,7 +70,7 @@ def apply(suggestions, custom_message = nil) 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 @@ def default_regex 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 @@ def default_regex 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 @@ def default_regex 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 @@ def default_regex 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 diff --git a/spec/services/users/reject_service_spec.rb b/spec/services/users/reject_service_spec.rb index 0e34f0e67ba97023887b223c21831c19edb4c9fb..5a243e876acb00a1c98540c4b4a33664fd6d485e 100644 --- a/spec/services/users/reject_service_spec.rb +++ b/spec/services/users/reject_service_spec.rb @@ -44,7 +44,7 @@ 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 diff --git a/spec/support/helpers/email_helpers.rb b/spec/support/helpers/email_helpers.rb index 6df33e6862942ad18afa07470061e6db78351c40..d0f6fd466d0af715c6d229d3b3745c5660111df9 100644 --- a/spec/support/helpers/email_helpers.rb +++ b/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 @@ def email_recipients(kind: :to) 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) diff --git a/spec/support/shared_examples/mailers/notify_shared_examples.rb b/spec/support/shared_examples/mailers/notify_shared_examples.rb index b10ebb4d2a39eb7d27a829472b42fc7487ae0a4d..e1f7a9030e20f399c8adab5cadd91724136a22fa 100644 --- a/spec/support/shared_examples/mailers/notify_shared_examples.rb +++ b/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 @@ 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 @@ 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