diff options
20 files changed, 182 insertions, 44 deletions
diff --git a/app/assets/javascripts/profile/profile.js b/app/assets/javascripts/profile/profile.js index 6e3800021b4..8dd37aee7e1 100644 --- a/app/assets/javascripts/profile/profile.js +++ b/app/assets/javascripts/profile/profile.js @@ -39,6 +39,7 @@ export default class Profile { bindEvents() { $('.js-preferences-form').on('change.preference', 'input[type=radio]', this.submitForm); + $('.js-group-notification-email').on('change', this.submitForm); $('#user_notification_email').on('change', this.submitForm); $('#user_notified_of_own_activity').on('change', this.submitForm); this.form.on('submit', this.onSubmitForm); diff --git a/app/assets/stylesheets/pages/notifications.scss b/app/assets/stylesheets/pages/notifications.scss index e98cb444f0a..e1cbf0e6654 100644 --- a/app/assets/stylesheets/pages/notifications.scss +++ b/app/assets/stylesheets/pages/notifications.scss @@ -4,6 +4,34 @@ .dropdown-menu { @extend .dropdown-menu-right; } + + @include media-breakpoint-down(sm) { + .notification-dropdown { + width: 100%; + } + + .notification-form { + display: block; + } + + .notifications-btn, + .btn-group { + width: 100%; + } + + .table-section { + border-top: 0; + min-height: unset; + + &:not(:first-child) { + padding-top: 0; + } + } + + .update-notifications { + width: 100%; + } + } } .notification { diff --git a/app/controllers/profiles/groups_controller.rb b/app/controllers/profiles/groups_controller.rb new file mode 100644 index 00000000000..c755bcb718a --- /dev/null +++ b/app/controllers/profiles/groups_controller.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +class Profiles::GroupsController < Profiles::ApplicationController + include RoutableActions + + def update + group = find_routable!(Group, params[:id]) + notification_setting = current_user.notification_settings.find_by(source: group) # rubocop: disable CodeReuse/ActiveRecord + + if notification_setting.update(update_params) + flash[:notice] = "Notification settings for #{group.name} saved" + else + flash[:alert] = "Failed to save new settings for #{group.name}" + end + + redirect_back_or_default(default: profile_notifications_path) + end + + private + + def update_params + params.require(:notification_setting).permit(:notification_email) + end +end diff --git a/app/mailers/emails/issues.rb b/app/mailers/emails/issues.rb index 2b046d17122..f3a3203f7ad 100644 --- a/app/mailers/emails/issues.rb +++ b/app/mailers/emails/issues.rb @@ -83,7 +83,7 @@ module Emails @project = Project.find(project_id) @results = results - mail(to: @user.notification_email, subject: subject('Imported issues')) do |format| + mail(to: recipient(@user.id, @project.group), subject: subject('Imported issues')) do |format| format.html { render layout: 'mailer' } format.text { render layout: 'mailer' } end @@ -103,7 +103,7 @@ module Emails def issue_thread_options(sender_id, recipient_id, reason) { from: sender(sender_id), - to: recipient(recipient_id), + to: recipient(recipient_id, @project.group), subject: subject("#{@issue.title} (##{@issue.iid})"), 'X-GitLab-NotificationReason' => reason } diff --git a/app/mailers/emails/members.rb b/app/mailers/emails/members.rb index 91dfdf58982..8225215ae4d 100644 --- a/app/mailers/emails/members.rb +++ b/app/mailers/emails/members.rb @@ -58,9 +58,8 @@ module Emails @member_source_type = member_source_type @member_source = member_source_class.find(source_id) @invite_email = invite_email - inviter = User.find(created_by_id) - mail(to: inviter.notification_email, + mail(to: recipient(created_by_id, member_source_type == 'project' ? @member_source.group : @member_source), subject: subject('Invitation declined')) end diff --git a/app/mailers/emails/merge_requests.rb b/app/mailers/emails/merge_requests.rb index fc6ed695675..864f9e2975a 100644 --- a/app/mailers/emails/merge_requests.rb +++ b/app/mailers/emails/merge_requests.rb @@ -110,7 +110,7 @@ module Emails def merge_request_thread_options(sender_id, recipient_id, reason = nil) { from: sender(sender_id), - to: recipient(recipient_id), + to: recipient(recipient_id, @project.group), subject: subject("#{@merge_request.title} (#{@merge_request.to_reference})"), 'X-GitLab-NotificationReason' => reason } diff --git a/app/mailers/emails/notes.rb b/app/mailers/emails/notes.rb index 1b3c1f9a8a9..70d296fe3b8 100644 --- a/app/mailers/emails/notes.rb +++ b/app/mailers/emails/notes.rb @@ -51,7 +51,7 @@ module Emails def note_thread_options(recipient_id) { from: sender(@note.author_id), - to: recipient(recipient_id), + to: recipient(recipient_id, @group), subject: subject("#{@note.noteable.title} (#{@note.noteable.reference_link_text})") } end diff --git a/app/mailers/emails/pages_domains.rb b/app/mailers/emails/pages_domains.rb index ce449237ef6..2d390666f65 100644 --- a/app/mailers/emails/pages_domains.rb +++ b/app/mailers/emails/pages_domains.rb @@ -7,7 +7,7 @@ module Emails @project = domain.project mail( - to: recipient.notification_email, + to: recipient(recipient.id, @project.group), subject: subject("GitLab Pages domain '#{domain.domain}' has been enabled") ) end @@ -17,7 +17,7 @@ module Emails @project = domain.project mail( - to: recipient.notification_email, + to: recipient(recipient.id, @project.group), subject: subject("GitLab Pages domain '#{domain.domain}' has been disabled") ) end @@ -27,7 +27,7 @@ module Emails @project = domain.project mail( - to: recipient.notification_email, + to: recipient(recipient.id, @project.group), subject: subject("Verification succeeded for GitLab Pages domain '#{domain.domain}'") ) end @@ -37,7 +37,7 @@ module Emails @project = domain.project mail( - to: recipient.notification_email, + to: recipient(recipient.id, @project.group), subject: subject("ACTION REQUIRED: Verification failed for GitLab Pages domain '#{domain.domain}'") ) end diff --git a/app/mailers/emails/projects.rb b/app/mailers/emails/projects.rb index 2500622caa7..f81f76f67f7 100644 --- a/app/mailers/emails/projects.rb +++ b/app/mailers/emails/projects.rb @@ -7,20 +7,20 @@ module Emails @project = Project.find project_id @target_url = project_url(@project) @old_path_with_namespace = old_path_with_namespace - mail(to: @user.notification_email, + mail(to: recipient(user_id, @project.group), subject: subject("Project was moved")) end def project_was_exported_email(current_user, project) @project = project - mail(to: current_user.notification_email, + mail(to: recipient(current_user.id, project.group), subject: subject("Project was exported")) end def project_was_not_exported_email(current_user, project, errors) @project = project @errors = errors - mail(to: current_user.notification_email, + mail(to: recipient(current_user.id, @project.group), subject: subject("Project export error")) end @@ -28,7 +28,7 @@ module Emails @project = project @user = user - mail(to: user.notification_email, subject: subject("Project cleanup has completed")) + mail(to: recipient(user.id, project.group), subject: subject("Project cleanup has completed")) end def repository_cleanup_failure_email(project, user, error) @@ -36,7 +36,7 @@ module Emails @user = user @error = error - mail(to: user.notification_email, subject: subject("Project cleanup failure")) + mail(to: recipient(user.id, project.group), subject: subject("Project cleanup failure")) end def repository_push_email(project_id, opts = {}) diff --git a/app/mailers/emails/remote_mirrors.rb b/app/mailers/emails/remote_mirrors.rb index 2018eb7260b..2d8137843ec 100644 --- a/app/mailers/emails/remote_mirrors.rb +++ b/app/mailers/emails/remote_mirrors.rb @@ -6,7 +6,7 @@ module Emails @remote_mirror = RemoteMirrorFinder.new(id: remote_mirror_id).execute @project = @remote_mirror.project - mail(to: recipient(recipient_id), subject: subject('Remote mirror update failed')) + mail(to: recipient(recipient_id, @project.group), subject: subject('Remote mirror update failed')) end end end diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb index 0b740809f30..4854ae5f0a7 100644 --- a/app/mailers/notify.rb +++ b/app/mailers/notify.rb @@ -73,12 +73,31 @@ class Notify < BaseMailer # Look up a User by their ID and return their email address # - # recipient_id - User ID + # recipient_id - User ID + # notification_group - The parent group of the notification # # Returns a String containing the User's email address. - def recipient(recipient_id) + def recipient(recipient_id, notification_group = nil) @current_user = User.find(recipient_id) - @current_user.notification_email + + if notification_group + group_notification_email = nil + + # Get notification group's and ancestors' notification settings + group_ids = notification_group.self_and_ancestors_ids + notification_settings = notification_group.notification_settings.where(user: @current_user) # rubocop: disable CodeReuse/ActiveRecord + + # Exploit notification_group.self_and_ancestors_ids being ordered from + # most nested to least nested to iterate through group ancestors + group_ids.each do |group_id| + group_notification_email = notification_settings.find { |ns| ns.source_id == group_id }&.notification_email + break if group_notification_email.present? + end + end + + # Return group-specific email address if present, otherwise return global + # email address + group_notification_email.presence || @current_user.notification_email end # Formats arguments into a String suitable for use as an email subject diff --git a/app/views/profiles/emails/index.html.haml b/app/views/profiles/emails/index.html.haml index 1823f191fb3..c90a0b3e329 100644 --- a/app/views/profiles/emails/index.html.haml +++ b/app/views/profiles/emails/index.html.haml @@ -26,7 +26,9 @@ %li Your Commit Email will be used for web based operations, such as edits and merges. %li - Your Notification Email will be used for account notifications. + Your Default Notification Email will be used for account notifications if a + = link_to 'group-specific email address', profile_notifications_path + is not set. %li Your Public Email will be displayed on your public profile. %li @@ -41,7 +43,7 @@ - if @primary_email === current_user.public_email %span.badge.badge-info Public email - if @primary_email === current_user.notification_email - %span.badge.badge-info Notification email + %span.badge.badge-info Default notification email - @emails.each do |email| %li = render partial: 'shared/email_with_badge', locals: { email: email.email, verified: email.confirmed? } diff --git a/app/views/profiles/notifications/_group_settings.html.haml b/app/views/profiles/notifications/_group_settings.html.haml index a12246bcdcc..cf17ee44145 100644 --- a/app/views/profiles/notifications/_group_settings.html.haml +++ b/app/views/profiles/notifications/_group_settings.html.haml @@ -1,12 +1,17 @@ -%li.notification-list-item - %span.notification.fa.fa-holder.append-right-5 - - if setting.global? - = notification_icon(current_user.global_notification_setting.level) - - else - = notification_icon(setting.level) +.gl-responsive-table-row.notification-list-item + .table-section.section-40 + %span.notification.fa.fa-holder.append-right-5 + - if setting.global? + = notification_icon(current_user.global_notification_setting.level) + - else + = notification_icon(setting.level) - %span.str-truncated - = link_to group.name, group_path(group) + %span.str-truncated + = link_to group.name, group_path(group) - .float-right + .table-section.section-30.text-right = render 'shared/notifications/button', notification_setting: setting + + .table-section.section-30 + = form_for @user.notification_settings.find { |ns| ns.source == group }, url: profile_notifications_group_path(group), method: :put, html: { class: 'update-notifications' } do |f| + = f.select :notification_email, @user.all_emails, { include_blank: 'Global notification email' }, class: 'select2 js-group-notification-email' diff --git a/app/views/profiles/notifications/show.html.haml b/app/views/profiles/notifications/show.html.haml index fa35fbd3961..1f311e9a4a4 100644 --- a/app/views/profiles/notifications/show.html.haml +++ b/app/views/profiles/notifications/show.html.haml @@ -41,9 +41,8 @@ %h5 = _('Groups (%{count})') % { count: @group_notifications.count } %div - %ul.bordered-list - - @group_notifications.each do |setting| - = render 'group_settings', setting: setting, group: setting.source + - @group_notifications.each do |setting| + = render 'group_settings', setting: setting, group: setting.source %h5 = _('Projects (%{count})') % { count: @project_notifications.count } %p.account-well diff --git a/app/views/shared/notifications/_button.html.haml b/app/views/shared/notifications/_button.html.haml index 31cc0c091dd..749aa258af6 100644 --- a/app/views/shared/notifications/_button.html.haml +++ b/app/views/shared/notifications/_button.html.haml @@ -1,14 +1,14 @@ - btn_class = local_assigns.fetch(:btn_class, nil) - if notification_setting - .js-notification-dropdown.notification-dropdown.home-panel-action-button.dropdown.inline + .js-notification-dropdown.notification-dropdown.mr-md-2.home-panel-action-button.dropdown.inline = form_for notification_setting, remote: true, html: { class: "inline notification-form" } do |f| = hidden_setting_source_input(notification_setting) = f.hidden_field :level, class: "notification_setting_level" .js-notification-toggle-btns %div{ class: ("btn-group" if notification_setting.custom?) } - if notification_setting.custom? - %button.dropdown-new.btn.btn-default.has-tooltip.notifications-btn#notifications-button{ type: "button", title: _("Notification setting"), class: "#{btn_class}", "aria-label" => _("Notification setting - %{notification_title}") % { notification_title: notification_title(notification_setting.level) }, data: { container: "body", toggle: "modal", target: "#" + notifications_menu_identifier("modal", notification_setting), display: 'static' } } + %button.dropdown-new.btn.btn-default.has-tooltip.notifications-btn.text-left#notifications-button{ type: "button", title: _("Notification setting"), class: "#{btn_class}", "aria-label" => _("Notification setting - %{notification_title}") % { notification_title: notification_title(notification_setting.level) }, data: { container: "body", toggle: "modal", target: "#" + notifications_menu_identifier("modal", notification_setting), display: 'static' } } = icon("bell", class: "js-notification-loading") = notification_title(notification_setting.level) %button.btn.dropdown-toggle{ data: { toggle: "dropdown", target: notifications_menu_identifier("dropdown", notification_setting), flip: "false" } } @@ -16,9 +16,11 @@ .sr-only Toggle dropdown - else %button.dropdown-new.btn.btn-default.has-tooltip.notifications-btn#notifications-button{ type: "button", title: _("Notification setting"), class: "#{btn_class}", "aria-label" => _("Notification setting - %{notification_title}") % { notification_title: notification_title(notification_setting.level) }, data: { container: "body", toggle: "dropdown", target: notifications_menu_identifier("dropdown", notification_setting), flip: "false" } } - = icon("bell", class: "js-notification-loading") - = notification_title(notification_setting.level) - = icon("caret-down") + .float-left + = icon("bell", class: "js-notification-loading") + = notification_title(notification_setting.level) + .float-right + = icon("caret-down") = render "shared/notifications/notification_dropdown", notification_setting: notification_setting diff --git a/changelogs/unreleased/weimeng-email-routing.yml b/changelogs/unreleased/weimeng-email-routing.yml new file mode 100644 index 00000000000..6536433bd03 --- /dev/null +++ b/changelogs/unreleased/weimeng-email-routing.yml @@ -0,0 +1,5 @@ +--- +title: Add ability to define notification email addresses for groups you belong to. +merge_request: 25299 +author: +type: added diff --git a/config/routes/profile.rb b/config/routes/profile.rb index c1cac3905f1..0e213b0b989 100644 --- a/config/routes/profile.rb +++ b/config/routes/profile.rb @@ -17,7 +17,11 @@ resource :profile, only: [:show, :update] do delete :unlink end end - resource :notifications, only: [:show, :update] + + resource :notifications, only: [:show, :update] do + resources :groups, only: :update + end + resource :password, only: [:new, :create, :edit, :update] do member do put :reset diff --git a/spec/mailers/emails/pages_domains_spec.rb b/spec/mailers/emails/pages_domains_spec.rb index 050af587061..2f594dbf9d1 100644 --- a/spec/mailers/emails/pages_domains_spec.rb +++ b/spec/mailers/emails/pages_domains_spec.rb @@ -5,11 +5,13 @@ describe Emails::PagesDomains do include EmailSpec::Matchers include_context 'gitlab email notification' - set(:project) { create(:project) } set(:domain) { create(:pages_domain, project: project) } - set(:user) { project.owner } + set(:user) { project.creator } shared_examples 'a pages domain email' do + let(:test_recipient) { user } + + it_behaves_like 'an email sent to a user' it_behaves_like 'an email sent from GitLab' it_behaves_like 'it should not have Gmail Actions links' it_behaves_like 'a user cannot unsubscribe through footer link' diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 8f348b1b053..67c1aa69db4 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -45,6 +45,10 @@ describe Notify do context 'for a project' do shared_examples 'an assignee email' do + let(:test_recipient) { assignee } + + it_behaves_like 'an email sent to a user' + it 'is sent to the assignee as the author' do sender = subject.header[:from].addrs.first @@ -618,8 +622,10 @@ describe Notify do end describe 'project was moved' do + let(:test_recipient) { user } subject { described_class.project_was_moved_email(project.id, user.id, "gitlab/gitlab") } + it_behaves_like 'an email sent to a user' it_behaves_like 'an email sent from GitLab' it_behaves_like 'it should not have Gmail Actions links' it_behaves_like "a user cannot unsubscribe through footer link" @@ -1083,8 +1089,6 @@ describe Notify do end context 'for a group' do - set(:group) { create(:group) } - describe 'group access requested' do let(:group) { create(:group, :public, :access_requestable) } let(:group_member) do diff --git a/spec/support/shared_examples/notify_shared_examples.rb b/spec/support/shared_examples/notify_shared_examples.rb index 4fff1c4e228..bcb30a86727 100644 --- a/spec/support/shared_examples/notify_shared_examples.rb +++ b/spec/support/shared_examples/notify_shared_examples.rb @@ -1,5 +1,7 @@ shared_context 'gitlab email notification' do - set(:project) { create(:project, :repository, name: 'a-known-name') } + set(:group) { create(:group) } + set(:subgroup) { create(:group, parent: group) } + set(:project) { create(:project, :repository, name: 'a-known-name', group: group) } set(:recipient) { create(:user, email: 'recipient@example.com') } let(:gitlab_sender_display_name) { Gitlab.config.gitlab.email_display_name } @@ -39,6 +41,48 @@ shared_examples 'an email sent from GitLab' do end end +shared_examples 'an email sent to a user' do + let(:group_notification_email) { 'user+group@example.com' } + + it 'is sent to user\'s global notification email address' do + expect(subject).to deliver_to(test_recipient.notification_email) + end + + context 'that is part of a project\'s group' do + it 'is sent to user\'s group notification email address when set' do + create(:notification_setting, user: test_recipient, source: project.group, notification_email: group_notification_email) + expect(subject).to deliver_to(group_notification_email) + end + + it 'is sent to user\'s global notification email address when no group email set' do + create(:notification_setting, user: test_recipient, source: project.group, notification_email: '') + expect(subject).to deliver_to(test_recipient.notification_email) + end + end + + context 'when project is in a sub-group' do + before do + project.group = subgroup + project.save! + end + + it 'is sent to user\'s subgroup notification email address when set' do + # Set top-level group notification email address to make sure it doesn't get selected + create(:notification_setting, user: test_recipient, source: group, notification_email: group_notification_email) + + subgroup_notification_email = 'user+subgroup@example.com' + create(:notification_setting, user: test_recipient, source: subgroup, notification_email: subgroup_notification_email) + + expect(subject).to deliver_to(subgroup_notification_email) + end + + it 'is sent to user\'s group notification email address when set and subgroup email address not set' do + create(:notification_setting, user: test_recipient, source: subgroup, notification_email: '') + expect(subject).to deliver_to(test_recipient.notification_email) + end + end +end + shared_examples 'an email that contains a header with author username' do it 'has X-GitLab-Author header containing author\'s username' do is_expected.to have_header 'X-GitLab-Author', user.username |