Merge branch 'weimeng-email-routing' into 'master'

Add group-level email routing

See merge request gitlab-org/gitlab-ce!25299

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);

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 {

app/controllers/profiles/groups_controller.rb

+# 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

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
       }

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
 

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
       }

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

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

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 = {})

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

app/mailers/notify.rb

@@ -73,12 +73,22 @@ 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
+    group_notification_email = nil
+
+    if notification_group
+      notification_settings = notification_group.notification_settings_for(@current_user, hierarchy_order: :asc)
+      group_notification_email = notification_settings.find { |n| n.notification_email.present? }&.notification_email
+    end
+
+    # Return group-specific email address if present, otherwise return global
+    # email address
+    group_notification_email || @current_user.notification_email
   end
 
   # Formats arguments into a String suitable for use as an email subject

app/models/group.rb

@@ -126,10 +126,20 @@ class Group < Namespace
   # Overrides notification_settings has_many association
   # This allows to apply notification settings from parent groups
   # to child groups and projects.
-  def notification_settings
+  def notification_settings(hierarchy_order: nil)
     source_type = self.class.base_class.name
+    settings = NotificationSetting.where(source_type: source_type, source_id: self_and_ancestors_ids)
 
-    NotificationSetting.where(source_type: source_type, source_id: self_and_ancestors_ids)
+    return settings unless hierarchy_order && self_and_ancestors_ids.length > 1
+
+    settings
+      .joins("LEFT JOIN (#{self_and_ancestors(hierarchy_order: hierarchy_order).to_sql}) AS ordered_groups ON notification_settings.source_id = ordered_groups.id")
+      .select('notification_settings.*, ordered_groups.depth AS depth')
+      .order("ordered_groups.depth #{hierarchy_order}")
+  end
+
+  def notification_settings_for(user, hierarchy_order: nil)
+    notification_settings(hierarchy_order: hierarchy_order).where(user: user)
   end
 
   def to_reference(_from = nil, full: nil)

app/models/namespace.rb

@@ -206,12 +206,12 @@ class Namespace < ApplicationRecord
       .ancestors(upto: top, hierarchy_order: hierarchy_order)
   end
 
-  def self_and_ancestors
+  def self_and_ancestors(hierarchy_order: nil)
     return self.class.where(id: id) unless parent_id
 
     Gitlab::ObjectHierarchy
       .new(self.class.where(id: id))
-      .base_and_ancestors
+      .base_and_ancestors(hierarchy_order: hierarchy_order)
   end
 
   # Returns all the descendants of the current namespace.

app/models/notification_recipient.rb

@@ -156,23 +156,11 @@ class NotificationRecipient
   # Returns the notification_setting of the lowest group in hierarchy with non global level
   def closest_non_global_group_notification_settting
     return unless @group
-    return if indexed_group_notification_settings.empty?
 
-    notification_setting = nil
-
-    @group.self_and_ancestors_ids.each do |id|
-      notification_setting = indexed_group_notification_settings[id]
-      break if notification_setting
-    end
-
-    notification_setting
-  end
-
-  def indexed_group_notification_settings
-    strong_memoize(:indexed_group_notification_settings) do
-      @group.notification_settings.where(user_id: user.id)
-        .where.not(level: NotificationSetting.levels[:global])
-        .index_by(&:source_id)
-    end
+    @group
+      .notification_settings(hierarchy_order: :asc)
+      .where(user: user)
+      .where.not(level: NotificationSetting.levels[:global])
+      .first
   end
 end

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? }

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

-%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'

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

app/views/shared/notifications/_button.html.haml

 - 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
 

changelogs/unreleased/weimeng-email-routing.yml

+---
+title: Add ability to define notification email addresses for groups you belong to.
+merge_request: 25299
+author:
+type: added

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

db/migrate/20190327163904_add_notification_email_to_notification_settings.rb

+# frozen_string_literal: true
+
+class AddNotificationEmailToNotificationSettings < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :notification_settings, :notification_email, :string
+  end
+end

db/schema.rb

@@ -1516,6 +1516,7 @@ ActiveRecord::Schema.define(version: 20190527194900) do
     t.boolean "success_pipeline"
     t.boolean "push_to_merge_request"
     t.boolean "issue_due"
+    t.string "notification_email"
     t.index ["source_id", "source_type"], name: "index_notification_settings_on_source_id_and_source_type", using: :btree
     t.index ["user_id", "source_id", "source_type"], name: "index_notifications_on_user_id_and_source_id_and_source_type", unique: true, using: :btree
     t.index ["user_id"], name: "index_notification_settings_on_user_id", using: :btree

doc/workflow/notifications.md

@@ -16,12 +16,16 @@ Notification settings are divided into three groups:
 
 Each of these settings have levels of notification:
 
+- Global: For groups and projects, notifications as per global settings.
 - Watch: Receive notifications for any activity.
-- On Mention: Receive notifications when `@mentioned` in comments.
 - Participate: Receive notifications for threads you have participated in.
+- On Mention: Receive notifications when `@mentioned` in comments.
 - Disabled: Turns off notifications.
 - Custom: Receive notifications for custom selected events.
-- Global: For groups and projects, notifications as per global settings.
+
+> Introduced in GitLab 12.0
+
+You can also select an email address to receive notifications for each group you belong to.
 
 ### Global Settings
 

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'

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"
@@ -773,7 +779,7 @@ describe Notify do
         invitee
       end
 
-      subject { described_class.member_invite_declined_email('project', project.id, project_member.invite_email, maintainer.id) }
+      subject { described_class.member_invite_declined_email('Project', project.id, project_member.invite_email, maintainer.id) }
 
       it_behaves_like 'an email sent from GitLab'
       it_behaves_like 'it should not have Gmail Actions links'
@@ -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

spec/support/shared_examples/notify_shared_examples.rb

 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,47 @@ 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', :nested_groups do
+    before do
+      project.update!(group: subgroup)
+    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