Commit 1a402d88 authored by Wei-Meng Lee's avatar Wei-Meng Lee

Send notifications to group-specific email address

- Select notification email by walking up group/subgroup path
- Add settings UI to set group email notification address
- Add tests
parent 6d73ce69
...@@ -39,6 +39,7 @@ export default class Profile { ...@@ -39,6 +39,7 @@ export default class Profile {
bindEvents() { bindEvents() {
$('.js-preferences-form').on('change.preference', 'input[type=radio]', this.submitForm); $('.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_notification_email').on('change', this.submitForm);
$('#user_notified_of_own_activity').on('change', this.submitForm); $('#user_notified_of_own_activity').on('change', this.submitForm);
this.form.on('submit', this.onSubmitForm); this.form.on('submit', this.onSubmitForm);
......
...@@ -4,6 +4,34 @@ ...@@ -4,6 +4,34 @@
.dropdown-menu { .dropdown-menu {
@extend .dropdown-menu-right; @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 { .notification {
......
# 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
...@@ -83,7 +83,7 @@ module Emails ...@@ -83,7 +83,7 @@ module Emails
@project = Project.find(project_id) @project = Project.find(project_id)
@results = results @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.html { render layout: 'mailer' }
format.text { render layout: 'mailer' } format.text { render layout: 'mailer' }
end end
...@@ -103,7 +103,7 @@ module Emails ...@@ -103,7 +103,7 @@ module Emails
def issue_thread_options(sender_id, recipient_id, reason) def issue_thread_options(sender_id, recipient_id, reason)
{ {
from: sender(sender_id), from: sender(sender_id),
to: recipient(recipient_id), to: recipient(recipient_id, @project.group),
subject: subject("#{@issue.title} (##{@issue.iid})"), subject: subject("#{@issue.title} (##{@issue.iid})"),
'X-GitLab-NotificationReason' => reason 'X-GitLab-NotificationReason' => reason
} }
......
...@@ -58,9 +58,8 @@ module Emails ...@@ -58,9 +58,8 @@ module Emails
@member_source_type = member_source_type @member_source_type = member_source_type
@member_source = member_source_class.find(source_id) @member_source = member_source_class.find(source_id)
@invite_email = invite_email @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')) subject: subject('Invitation declined'))
end end
......
...@@ -110,7 +110,7 @@ module Emails ...@@ -110,7 +110,7 @@ module Emails
def merge_request_thread_options(sender_id, recipient_id, reason = nil) def merge_request_thread_options(sender_id, recipient_id, reason = nil)
{ {
from: sender(sender_id), from: sender(sender_id),
to: recipient(recipient_id), to: recipient(recipient_id, @project.group),
subject: subject("#{@merge_request.title} (#{@merge_request.to_reference})"), subject: subject("#{@merge_request.title} (#{@merge_request.to_reference})"),
'X-GitLab-NotificationReason' => reason 'X-GitLab-NotificationReason' => reason
} }
......
...@@ -51,7 +51,7 @@ module Emails ...@@ -51,7 +51,7 @@ module Emails
def note_thread_options(recipient_id) def note_thread_options(recipient_id)
{ {
from: sender(@note.author_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})") subject: subject("#{@note.noteable.title} (#{@note.noteable.reference_link_text})")
} }
end end
......
...@@ -7,7 +7,7 @@ module Emails ...@@ -7,7 +7,7 @@ module Emails
@project = domain.project @project = domain.project
mail( mail(
to: recipient.notification_email, to: recipient(recipient.id, @project.group),
subject: subject("GitLab Pages domain '#{domain.domain}' has been enabled") subject: subject("GitLab Pages domain '#{domain.domain}' has been enabled")
) )
end end
...@@ -17,7 +17,7 @@ module Emails ...@@ -17,7 +17,7 @@ module Emails
@project = domain.project @project = domain.project
mail( mail(
to: recipient.notification_email, to: recipient(recipient.id, @project.group),
subject: subject("GitLab Pages domain '#{domain.domain}' has been disabled") subject: subject("GitLab Pages domain '#{domain.domain}' has been disabled")
) )
end end
...@@ -27,7 +27,7 @@ module Emails ...@@ -27,7 +27,7 @@ module Emails
@project = domain.project @project = domain.project
mail( mail(
to: recipient.notification_email, to: recipient(recipient.id, @project.group),
subject: subject("Verification succeeded for GitLab Pages domain '#{domain.domain}'") subject: subject("Verification succeeded for GitLab Pages domain '#{domain.domain}'")
) )
end end
...@@ -37,7 +37,7 @@ module Emails ...@@ -37,7 +37,7 @@ module Emails
@project = domain.project @project = domain.project
mail( mail(
to: recipient.notification_email, to: recipient(recipient.id, @project.group),
subject: subject("ACTION REQUIRED: Verification failed for GitLab Pages domain '#{domain.domain}'") subject: subject("ACTION REQUIRED: Verification failed for GitLab Pages domain '#{domain.domain}'")
) )
end end
......
...@@ -7,20 +7,20 @@ module Emails ...@@ -7,20 +7,20 @@ module Emails
@project = Project.find project_id @project = Project.find project_id
@target_url = project_url(@project) @target_url = project_url(@project)
@old_path_with_namespace = old_path_with_namespace @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")) subject: subject("Project was moved"))
end end
def project_was_exported_email(current_user, project) def project_was_exported_email(current_user, project)
@project = project @project = project
mail(to: current_user.notification_email, mail(to: recipient(current_user.id, project.group),
subject: subject("Project was exported")) subject: subject("Project was exported"))
end end
def project_was_not_exported_email(current_user, project, errors) def project_was_not_exported_email(current_user, project, errors)
@project = project @project = project
@errors = errors @errors = errors
mail(to: current_user.notification_email, mail(to: recipient(current_user.id, @project.group),
subject: subject("Project export error")) subject: subject("Project export error"))
end end
...@@ -28,7 +28,7 @@ module Emails ...@@ -28,7 +28,7 @@ module Emails
@project = project @project = project
@user = user @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 end
def repository_cleanup_failure_email(project, user, error) def repository_cleanup_failure_email(project, user, error)
...@@ -36,7 +36,7 @@ module Emails ...@@ -36,7 +36,7 @@ module Emails
@user = user @user = user
@error = error @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 end
def repository_push_email(project_id, opts = {}) def repository_push_email(project_id, opts = {})
......
...@@ -6,7 +6,7 @@ module Emails ...@@ -6,7 +6,7 @@ module Emails
@remote_mirror = RemoteMirrorFinder.new(id: remote_mirror_id).execute @remote_mirror = RemoteMirrorFinder.new(id: remote_mirror_id).execute
@project = @remote_mirror.project @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 end
end end
...@@ -74,11 +74,30 @@ class Notify < BaseMailer ...@@ -74,11 +74,30 @@ class Notify < BaseMailer
# Look up a User by their ID and return their email address # 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. # 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 = 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 end
# Formats arguments into a String suitable for use as an email subject # Formats arguments into a String suitable for use as an email subject
......
...@@ -26,7 +26,9 @@ ...@@ -26,7 +26,9 @@
%li %li
Your Commit Email will be used for web based operations, such as edits and merges. Your Commit Email will be used for web based operations, such as edits and merges.
%li %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 %li
Your Public Email will be displayed on your public profile. Your Public Email will be displayed on your public profile.
%li %li
...@@ -41,7 +43,7 @@ ...@@ -41,7 +43,7 @@
- if @primary_email === current_user.public_email - if @primary_email === current_user.public_email
%span.badge.badge-info Public email %span.badge.badge-info Public email
- if @primary_email === current_user.notification_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| - @emails.each do |email|
%li %li
= render partial: 'shared/email_with_badge', locals: { email: email.email, verified: email.confirmed? } = render partial: 'shared/email_with_badge', locals: { email: email.email, verified: email.confirmed? }
......
%li.notification-list-item .gl-responsive-table-row.notification-list-item
.table-section.section-40
%span.notification.fa.fa-holder.append-right-5 %span.notification.fa.fa-holder.append-right-5
- if setting.global? - if setting.global?
= notification_icon(current_user.global_notification_setting.level) = notification_icon(current_user.global_notification_setting.level)
...@@ -8,5 +9,9 @@ ...@@ -8,5 +9,9 @@
%span.str-truncated %span.str-truncated
= link_to group.name, group_path(group) = link_to group.name, group_path(group)
.float-right .table-section.section-30.text-right
= render 'shared/notifications/button', notification_setting: setting = 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'
...@@ -41,7 +41,6 @@ ...@@ -41,7 +41,6 @@
%h5 %h5
= _('Groups (%{count})') % { count: @group_notifications.count } = _('Groups (%{count})') % { count: @group_notifications.count }
%div %div
%ul.bordered-list
- @group_notifications.each do |setting| - @group_notifications.each do |setting|
= render 'group_settings', setting: setting, group: setting.source = render 'group_settings', setting: setting, group: setting.source
%h5 %h5
......
- btn_class = local_assigns.fetch(:btn_class, nil) - btn_class = local_assigns.fetch(:btn_class, nil)
- if notification_setting - 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| = form_for notification_setting, remote: true, html: { class: "inline notification-form" } do |f|
= hidden_setting_source_input(notification_setting) = hidden_setting_source_input(notification_setting)
= f.hidden_field :level, class: "notification_setting_level" = f.hidden_field :level, class: "notification_setting_level"
.js-notification-toggle-btns .js-notification-toggle-btns
%div{ class: ("btn-group" if notification_setting.custom?) } %div{ class: ("btn-group" if notification_setting.custom?) }
- 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") = icon("bell", class: "js-notification-loading")
= notification_title(notification_setting.level) = notification_title(notification_setting.level)
%button.btn.dropdown-toggle{ data: { toggle: "dropdown", target: notifications_menu_identifier("dropdown", notification_setting), flip: "false" } } %button.btn.dropdown-toggle{ data: { toggle: "dropdown", target: notifications_menu_identifier("dropdown", notification_setting), flip: "false" } }
...@@ -16,8 +16,10 @@ ...@@ -16,8 +16,10 @@
.sr-only Toggle dropdown .sr-only Toggle dropdown
- else - 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" } } %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" } }
.float-left
= icon("bell", class: "js-notification-loading") = icon("bell", class: "js-notification-loading")
= notification_title(notification_setting.level) = notification_title(notification_setting.level)
.float-right
= icon("caret-down") = icon("caret-down")
= render "shared/notifications/notification_dropdown", notification_setting: notification_setting = render "shared/notifications/notification_dropdown", notification_setting: notification_setting
......
---
title: Add ability to define notification email addresses for groups you belong to.
merge_request: 25299
author:
type: added
...@@ -17,7 +17,11 @@ resource :profile, only: [:show, :update] do ...@@ -17,7 +17,11 @@ resource :profile, only: [:show, :update] do
delete :unlink delete :unlink
end end
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 resource :password, only: [:new, :create, :edit, :update] do
member do member do
put :reset put :reset
......
...@@ -5,11 +5,13 @@ describe Emails::PagesDomains do ...@@ -5,11 +5,13 @@ describe Emails::PagesDomains do
include EmailSpec::Matchers include EmailSpec::Matchers
include_context 'gitlab email notification' include_context 'gitlab email notification'
set(:project) { create(:project) }
set(:domain) { create(:pages_domain, project: project) } set(:domain) { create(:pages_domain, project: project) }
set(:user) { project.owner } set(:user) { project.creator }
shared_examples 'a pages domain email' do 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 'an email sent from GitLab'
it_behaves_like 'it should not have Gmail Actions links' it_behaves_like 'it should not have Gmail Actions links'
it_behaves_like 'a user cannot unsubscribe through footer link' it_behaves_like 'a user cannot unsubscribe through footer link'
......
...@@ -45,6 +45,10 @@ describe Notify do ...@@ -45,6 +45,10 @@ describe Notify do
context 'for a project' do context 'for a project' do
shared_examples 'an assignee email' 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 it 'is sent to the assignee as the author' do
sender = subject.header[:from].addrs.first sender = subject.header[:from].addrs.first
...@@ -618,8 +622,10 @@ describe Notify do ...@@ -618,8 +622,10 @@ describe Notify do
end end
describe 'project was moved' do describe 'project was moved' do
let(:test_recipient) { user }
subject { described_class.project_was_moved_email(project.id, user.id, "gitlab/gitlab") } 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 'an email sent from GitLab'
it_behaves_like 'it should not have Gmail Actions links' it_behaves_like 'it should not have Gmail Actions links'
it_behaves_like "a user cannot unsubscribe through footer link" it_behaves_like "a user cannot unsubscribe through footer link"
...@@ -1083,8 +1089,6 @@ describe Notify do ...@@ -1083,8 +1089,6 @@ describe Notify do
end end
context 'for a group' do context 'for a group' do
set(:group) { create(:group) }
describe 'group access requested' do describe 'group access requested' do
let(:group) { create(:group, :public, :access_requestable) } let(:group) { create(:group, :public, :access_requestable) }
let(:group_member) do let(:group_member) do
......
shared_context 'gitlab email notification' do 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') } set(:recipient) { create(:user, email: 'recipient@example.com') }
let(:gitlab_sender_display_name) { Gitlab.config.gitlab.email_display_name } let(:gitlab_sender_display_name) { Gitlab.config.gitlab.email_display_name }
...@@ -39,6 +41,48 @@ shared_examples 'an email sent from GitLab' do ...@@ -39,6 +41,48 @@ shared_examples 'an email sent from GitLab' do
end end
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 shared_examples 'an email that contains a header with author username' do
it 'has X-GitLab-Author header containing author\'s username' do it 'has X-GitLab-Author header containing author\'s username' do
is_expected.to have_header 'X-GitLab-Author', user.username is_expected.to have_header 'X-GitLab-Author', user.username
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment