Commit a81f41c0 authored by Grzegorz Bizon's avatar Grzegorz Bizon

Merge branch '5863-customize-notifications-for-new-epic' into 'master'

Resolve "Customize notifications for new epic - Web and API"

Closes #5863

See merge request gitlab-org/gitlab-ee!6626
parents 6e27e28e 20536822
......@@ -48,6 +48,8 @@ Naming/FileName:
- 'qa/bin/*'
- 'config/**/*'
- 'lib/generators/**/*'
- 'locale/unfound_translations.rb'
- 'ee/locale/unfound_translations.rb'
- 'ee/lib/generators/**/*'
IgnoreExecutableScripts: true
AllowedAcronyms:
......
......@@ -5,14 +5,14 @@ class NotificationSettingsController < ApplicationController
return render_404 unless can_read?(resource)
@notification_setting = current_user.notification_settings_for(resource)
@saved = @notification_setting.update(notification_setting_params)
@saved = @notification_setting.update(notification_setting_params_for(resource))
render_response
end
def update
@notification_setting = current_user.notification_settings.find(params[:id])
@saved = @notification_setting.update(notification_setting_params)
@saved = @notification_setting.update(notification_setting_params_for(@notification_setting.source))
render_response
end
......@@ -42,8 +42,8 @@ class NotificationSettingsController < ApplicationController
}
end
def notification_setting_params
allowed_fields = NotificationSetting::EMAIL_EVENTS.dup
def notification_setting_params_for(source)
allowed_fields = NotificationSetting.email_events(source).dup
allowed_fields << :level
params.require(:notification_setting).permit(allowed_fields)
end
......
......@@ -83,21 +83,11 @@ module NotificationsHelper
end
def notification_event_name(event)
# All values from NotificationSetting::EMAIL_EVENTS
# All values from NotificationSetting.email_events
case event
when :success_pipeline
s_('NotificationEvent|Successful pipeline')
else
N_('NotificationEvent|New note')
N_('NotificationEvent|New issue')
N_('NotificationEvent|Reopen issue')
N_('NotificationEvent|Close issue')
N_('NotificationEvent|Reassign issue')
N_('NotificationEvent|New merge request')
N_('NotificationEvent|Close merge request')
N_('NotificationEvent|Reassign merge request')
N_('NotificationEvent|Merge merge request')
N_('NotificationEvent|Failed pipeline')
s_(event.to_s.humanize)
end
end
......
# frozen_string_literal: true
class NotificationSetting < ActiveRecord::Base
prepend EE::NotificationSetting
include IgnorableColumn
ignore_column :events
......@@ -45,6 +46,15 @@ class NotificationSetting < ActiveRecord::Base
:success_pipeline
].freeze
# Update unfound_translations.rb when events are changed
def self.email_events(source = nil)
EMAIL_EVENTS
end
def email_events
self.class.email_events(source)
end
EXCLUDED_PARTICIPATING_EVENTS = [
:success_pipeline
].freeze
......
......@@ -130,7 +130,7 @@ module NotificationRecipientService
end
def add_project_watchers
add_recipients(project_watchers, :watch, nil)
add_recipients(project_watchers, :watch, nil) if project
end
def add_group_watchers
......@@ -220,6 +220,10 @@ module NotificationRecipientService
end
class Default < Base
prepend ::EE::NotificationRecipientBuilders::Default
MENTION_TYPE_ACTIONS = [:new_issue, :new_merge_request].freeze
attr_reader :target
attr_reader :current_user
attr_reader :action
......@@ -236,7 +240,13 @@ module NotificationRecipientService
def build!
add_participants(current_user)
add_project_watchers
if project
add_project_watchers
else # for group level targets
add_group_watchers
end
add_custom_notifications
# Re-assign is considered as a mention of the new assignee
......@@ -252,7 +262,7 @@ module NotificationRecipientService
add_subscribed_users
if [:new_issue, :new_merge_request].include?(custom_action)
if self.class.mention_type_actions.include?(custom_action)
# These will all be participants as well, but adding with the :mention
# type ensures that users with the mention notification level will
# receive them, too.
......@@ -279,10 +289,14 @@ module NotificationRecipientService
end
# Build event key to search on custom notification level
# Check NotificationSetting::EMAIL_EVENTS
# Check NotificationSetting.email_events
def custom_action
@custom_action ||= "#{action}_#{target.class.model_name.name.underscore}".to_sym
end
def self.mention_type_actions
MENTION_TYPE_ACTIONS.dup
end
end
class NewNote < Base
......
......@@ -19,7 +19,7 @@
- paragraph = _('Custom notification levels are the same as participating levels. With custom notification levels you will also receive notifications for select events. To find out more, check out %{notification_link}.') % { notification_link: notification_link.html_safe }
#{ paragraph.html_safe }
.col-lg-8
- NotificationSetting::EMAIL_EVENTS.each_with_index do |event, index|
- notification_setting.email_events.each_with_index do |event, index|
- field_id = "#{notifications_menu_identifier("modal", notification_setting)}_notification_setting[#{event}]"
.form-group
.form-check{ class: ("prepend-top-0" if index == 0) }
......
......@@ -181,6 +181,7 @@
- elastic_indexer
- export_csv
- ldap_group_sync
- new_epic
- project_import_schedule
- project_update_repository_storage
- rebase
......
......@@ -85,6 +85,7 @@
- [chat_notification, 2]
- [geo, 1]
- [repository_update_mirror, 1]
- [new_epic, 2]
- [project_import_schedule, 1]
- [project_update_repository_storage, 1]
- [admin_emails, 1]
......
......@@ -1866,6 +1866,7 @@ ActiveRecord::Schema.define(version: 20180807153545) do
t.boolean "success_pipeline"
t.boolean "push_to_merge_request"
t.boolean "issue_due"
t.boolean "new_epic"
end
add_index "notification_settings", ["source_id", "source_type"], name: "index_notification_settings_on_source_id_and_source_type", using: :btree
......
......@@ -15,7 +15,7 @@ mention
custom
```
If the `custom` level is used, specific email events can be controlled. Notification email events are defined in the `NotificationSetting::EMAIL_EVENTS` model variable. Currently, these events are recognized:
If the `custom` level is used, specific email events can be controlled. Available events are returned by `NotificationSetting.email_events`. Currently, these events are recognized:
```
new_note
......@@ -32,6 +32,7 @@ reassign_merge_request
merge_merge_request
failed_pipeline
success_pipeline
new_epic
```
## Global notification settings
......@@ -85,6 +86,7 @@ curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab
| `merge_merge_request` | boolean | no | Enable/disable this notification |
| `failed_pipeline` | boolean | no | Enable/disable this notification |
| `success_pipeline` | boolean | no | Enable/disable this notification |
| `new_epic` | boolean | no | Enable/disable this notification ([Introduced][ee-6626] in 11.2) |
Example response:
......@@ -153,6 +155,7 @@ curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab
| `merge_merge_request` | boolean | no | Enable/disable this notification |
| `failed_pipeline` | boolean | no | Enable/disable this notification |
| `success_pipeline` | boolean | no | Enable/disable this notification |
| `new_epic` | boolean | no | Enable/disable this notification ([Introduced][ee-6626] in 11.2) |
Example responses:
......@@ -177,9 +180,11 @@ Example responses:
"reassign_merge_request": false,
"merge_merge_request": false,
"failed_pipeline": false,
"success_pipeline": false
"success_pipeline": false,
"new_epic": false
}
}
```
[ce-5632]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5632
[ee-6626]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/6626
......@@ -7,6 +7,7 @@ module EE
include ::Emails::AdminNotification
include ::Emails::CsvExport
include ::Emails::ServiceDesk
include ::Emails::Epics
attr_reader :group
end
......
# frozen_string_literal: true
module Emails
module Epics
def new_epic_email(recipient_id, epic_id, reason = nil)
@epic = Epic.find_by_id(epic_id)
return unless @epic
setup_epic_mail(recipient_id)
mail_new_thread(@epic, epic_thread_options(@epic.author_id, recipient_id, reason))
end
private
def setup_epic_mail(recipient_id)
@group = @epic.group
@target_url = group_epic_url(@epic.group, @epic)
@sent_notification = SentNotification.record(@epic, recipient_id, reply_key)
end
def epic_thread_options(sender_id, recipient_id, reason)
{
from: sender(sender_id),
to: recipient(recipient_id),
subject: subject("#{@epic.title} (#{@epic.to_reference})"),
'X-GitLab-NotificationReason' => reason
}
end
end
end
# frozen_string_literal: true
module EE
module NotificationSetting
extend ActiveSupport::Concern
EMAIL_EVENTS_MAPPING = {
::Group => [:new_epic]
}.freeze
FULL_EMAIL_EVENTS = EMAIL_EVENTS_MAPPING.values.flatten.freeze
class_methods do
extend ::Gitlab::Utils::Override
# Update unfound_translations.rb when events are changed
override :email_events
def email_events(source = nil)
result = super.dup
if source.nil?
# Global setting
result.concat(FULL_EMAIL_EVENTS)
else
source_class = source.is_a?(Class) ? source : source.class
EMAIL_EVENTS_MAPPING[source_class]&.tap do |events|
result.concat(events)
end
end
result
end
end
end
end
# frozen_string_literal: true
module EE
module NotificationRecipientBuilders
module Default
extend ActiveSupport::Concern
class_methods do
extend ::Gitlab::Utils::Override
override :mention_type_actions
def mention_type_actions
super.append(:new_epic)
end
end
end
end
end
......@@ -38,6 +38,10 @@ module EE
end
end
def new_epic(epic)
new_resource_email(epic, :new_epic_email)
end
def project_mirror_user_changed(new_mirror_user, deleted_user_name, project)
mailer.project_mirror_user_changed_email(new_mirror_user.id, deleted_user_name, project.id).deliver_later
end
......
......@@ -7,6 +7,14 @@ module Epics
private
def before_create(epic)
# current_user (defined in BaseService) is not available within run_after_commit block
user = current_user
epic.run_after_commit do
NewEpicWorker.perform_async(epic.id, user.id)
end
end
def whitelisted_epic_params
params.slice(:title, :description, :start_date, :end_date)
end
......
- if Gitlab::CurrentSettings.email_author_in_body
%p.details
#{link_to @epic.author_name, user_url(@epic.author)} created an epic:
- if @epic.assignee
%p
Assignee: #{@epic.assignee.name}
- if @epic.description
%div
= markdown(@epic.description, pipeline: :email, author: @epic.author)
New Epic <%= @epic.to_reference(full: true) %> was created.
<%= url_for(group_epic_url(@epic.group, @epic)) %>
Author: <%= @epic.author_name %>
<% if @epic.assignee %>
Assignee: <%= @epic.assignee.name %>
<% end %>
<%= @epic.description %>
# frozen_string_literal: true
class NewEpicWorker
include ApplicationWorker
include NewIssuable
def perform(epic_id, user_id)
return unless objects_found?(epic_id, user_id)
NotificationService.new.new_epic(issuable)
issuable.create_cross_references!(user)
end
def issuable_class
Epic
end
end
---
title: Allow custom notification for new epic event
merge_request: 5863
author:
type: added
# frozen_string_literal: true
class AddNewEpicToNotificationSettings < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
def change
add_column :notification_settings, :new_epic, :boolean
end
end
# frozen_string_literal: true
# Dynamic translations which needs to be marked by `N_` so they can be found by `rake gettext:find`, see:
# https://github.com/grosser/gettext_i18n_rails#unfound-translations-with-rake-gettextfind
# EE::NotificationSetting.email_events
N_('NotificationEvent|New epic')
......@@ -151,46 +151,77 @@ describe Notify do
end
context 'for a group' do
context 'for epic notes' do
describe 'for epics' do
set(:group) { create(:group) }
set(:epic) { create(:epic, group: group) }
set(:note) { create(:note, project: nil, noteable: epic) }
let(:note_author) { note.author }
let(:epic_note_path) { group_epic_path(group, epic, anchor: "note_#{note.id}") }
subject { described_class.note_epic_email(recipient.id, note.id) }
context 'that are new' do
subject { described_class.new_epic_email(recipient.id, epic.id) }
it_behaves_like 'a note email'
it_behaves_like 'an epic email starting a new thread with reply-by-email enabled' do
let(:model) { epic }
end
it_behaves_like 'it should show Gmail Actions View Epic link'
it_behaves_like 'an unsubscribeable thread'
it 'has the correct subject and body' do
prefix = "#{epic.group.name} | "
suffix = "#{epic.title} (#{epic.to_reference})"
it_behaves_like 'an unsubscribeable thread'
aggregate_failures do
is_expected.to have_subject [prefix, suffix].compact.join
is_expected.to have_body_text(group_epic_path(group, epic))
end
end
it 'has the characteristics of a threaded reply' do
host = Gitlab.config.gitlab.host
route_key = "#{epic.class.model_name.singular_route_key}_#{epic.id}"
context 'got deleted before notification' do
subject { described_class.new_epic_email(recipient.id, 0) }
aggregate_failures do
is_expected.to have_header('Message-ID', /\A<.*@#{host}>\Z/)
is_expected.to have_header('In-Reply-To', "<#{route_key}@#{host}>")
is_expected.to have_header('References', /\A<reply\-.*@#{host}> <#{route_key}@#{host}>\Z/ )
is_expected.to have_subject(/^Re: /)
it 'does not send email' do
expect(subject.message).to be_a_kind_of ActionMailer::Base::NullMail
end
end
end
context 'when reply-by-email is enabled with incoming address with %{key}' do
it 'has a Reply-To header' do
is_expected.to have_header 'Reply-To', /<reply+(.*)@#{Gitlab.config.gitlab.host}>\Z/
context 'for epic notes' do
set(:note) { create(:note, project: nil, noteable: epic) }
let(:note_author) { note.author }
let(:epic_note_path) { group_epic_path(group, epic, anchor: "note_#{note.id}") }
subject { described_class.note_epic_email(recipient.id, note.id) }
it_behaves_like 'a note email'
it_behaves_like 'an unsubscribeable thread'
it 'has the characteristics of a threaded reply' do
host = Gitlab.config.gitlab.host
route_key = "#{epic.class.model_name.singular_route_key}_#{epic.id}"
aggregate_failures do
is_expected.to have_header('Message-ID', /\A<.*@#{host}>\Z/)
is_expected.to have_header('In-Reply-To', "<#{route_key}@#{host}>")
is_expected.to have_header('References', /\A<reply\-.*@#{host}> <#{route_key}@#{host}>\Z/ )
is_expected.to have_subject(/^Re: /)
end
end
end
it { is_expected.to have_body_text('View Epic') }
context 'when reply-by-email is enabled with incoming address with %{key}' do
it 'has a Reply-To header' do
is_expected.to have_header 'Reply-To', /<reply+(.*)@#{Gitlab.config.gitlab.host}>\Z/
end
end
it 'has the correct subject and body' do
prefix = "Re: #{epic.group.name} | "
suffix = "#{epic.title} (#{epic.to_reference})"
it_behaves_like 'it should show Gmail Actions View Epic link'
aggregate_failures do
is_expected.to have_subject [prefix, suffix].compact.join
is_expected.to have_body_text(epic_note_path)
it 'has the correct subject and body' do
prefix = "Re: #{epic.group.name} | "
suffix = "#{epic.title} (#{epic.to_reference})"
aggregate_failures do
is_expected.to have_subject [prefix, suffix].compact.join
is_expected.to have_body_text(epic_note_path)
end
end
end
end
......
# frozen_string_literal: true
require 'spec_helper'
describe NotificationSetting do
describe '.email_events' do
subject { described_class.email_events(target) }
context 'group' do
let(:target) { build_stubbed(:group) }
it 'appends EE specific events' do
expect(subject).to eq(
[
:new_note,
:new_issue,
:reopen_issue,
:close_issue,
:reassign_issue,
:issue_due,
:new_merge_request,
:push_to_merge_request,
:reopen_merge_request,
:close_merge_request,
:reassign_merge_request,
:merge_merge_request,
:failed_pipeline,
:success_pipeline,
:new_epic
]
)
end
end
context 'project' do
let(:target) { build_stubbed(:project) }
it 'returns CE list' do
expect(subject).to eq(
[
:new_note,
:new_issue,
:reopen_issue,
:close_issue,
:reassign_issue,
:issue_due,
:new_merge_request,
:push_to_merge_request,
:reopen_merge_request,
:close_merge_request,
:reassign_merge_request,
:merge_merge_request,
:failed_pipeline,
:success_pipeline
]
)
end
end
context 'global' do
let(:target) { nil }
it 'appends EE specific events' do
expect(subject).to eq(
[
:new_note,
:new_issue,
:reopen_issue,
:close_issue,
:reassign_issue,
:issue_due,
:new_merge_request,
:push_to_merge_request,
:reopen_merge_request,
:close_merge_request,
:reassign_merge_request,
:merge_merge_request,
:failed_pipeline,
:success_pipeline,
:new_epic
]
)
end
end
end
end
......@@ -9,12 +9,15 @@ describe Epics::CreateService do
describe '#execute' do
it 'creates one issue correctly' do
allow(NewEpicWorker).to receive(:perform_async)
expect { subject }.to change { Epic.count }.from(0).to(1)
epic = Epic.last
expect(epic).to be_persisted
expect(epic.title).to eq('new epic')
expect(epic.description).to eq('epic description')
expect(NewEpicWorker).to have_received(:perform_async).with(epic.id, user.id)
end
end
end
# frozen_string_literal: true
shared_examples 'it should show Gmail Actions View Epic link' do
it_behaves_like 'it should have Gmail Actions links'
it { is_expected.to have_body_text('View Epic') }
end
shared_examples 'an epic email starting a new thread with reply-by-email enabled' do
include_examples 'a new thread email with reply-by-email enabled'
context 'when reply-by-email is enabled with incoming address with %{key}' do
it 'has a Reply-To header' do
is_expected.to have_header 'Reply-To', /<reply+(.*)@#{Gitlab.config.gitlab.host}>\Z/
end
end
context 'when reply-by-email is enabled with incoming address without %{key}' do
include_context 'reply-by-email is enabled with incoming address without %{key}'
include_examples 'a new thread email with reply-by-email enabled'
it 'has a Reply-To header' do
is_expected.to have_header 'Reply-To', /<reply@#{Gitlab.config.gitlab.host}>\Z/
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe NewEpicWorker do
describe '#perform' do
let(:worker) { described_class.new }
context 'when an epic not found' do
it 'does not call Services' do
expect(NotificationService).not_to receive(:new)
worker.perform(99, create(:user).id)
end
it 'logs an error' do
expect(Rails.logger).to receive(:error).with('NewEpicWorker: couldn\'t find Epic with ID=99, skipping job')
worker.perform(99, create(:user).id)
end
end
context 'when a user not found' do
it 'does not call Services' do
expect(NotificationService).not_to receive(:new)
worker.perform(create(:epic).id, 99)
end
it 'logs an error' do
expect(Rails.logger).to receive(:error).with('NewEpicWorker: couldn\'t find User with ID=99, skipping job')
worker.perform(create(:epic).id, 99)
end
end
context 'when everything is ok' do
let(:user) { create(:user) }
let(:epic) { create(:epic) }
before do
stub_licensed_features(epics: true)
end
context 'user watches group' do
before do
create(
:notification_setting,
user: user,
source: epic.group,
level: NotificationSetting.levels[:watch]
)
end
it 'creates a notification for watcher' do
expect(Notify).to receive(:new_epic_email).with(user.id, epic.id, nil)
.and_return(double(deliver_later: true))
worker.perform(epic.id, user.id)
end
end
context 'mention' do
let(:epic) { create(:epic, description: "epic for #{user.to_reference}") }
it 'creates a notification for the mentioned user' do
expect(Notify).to receive(:new_epic_email).with(user.id, epic.id, NotificationReason::MENTIONED)
.and_return(double(deliver_later: true))
worker.perform(epic.id, user.id)
end
end
end
end
end
......@@ -884,7 +884,7 @@ module API
class NotificationSetting < Grape::Entity
expose :level
expose :events, if: ->(notification_setting, _) { notification_setting.custom? } do
::NotificationSetting::EMAIL_EVENTS.each do |event|
::NotificationSetting.email_events.each do |event|
expose event
end
end
......
......@@ -23,7 +23,7 @@ module API
params do
optional :level, type: String, desc: 'The global notification level'
optional :notification_email, type: String, desc: 'The email address to send notifications'
NotificationSetting::EMAIL_EVENTS.each do |event|
NotificationSetting.email_events.each do |event|
optional event, type: Boolean, desc: 'Enable/disable this notification'
end
end
......@@ -50,7 +50,9 @@ module API
end
end
%w[group project].each do |source_type|
[Group, Project].each do |source_class|
source_type = source_class.name.underscore
params do
requires :id, type: String, desc: "The #{source_type} ID"
end
......@@ -73,7 +75,7 @@ module API
end
params do
optional :level, type: String, desc: "The #{source_type} notification level"
NotificationSetting::EMAIL_EVENTS.each do |event|
NotificationSetting.email_events(source_class).each do |event|
optional event, type: Boolean, desc: 'Enable/disable this notification'
end
end
......
......@@ -4919,6 +4919,9 @@ msgstr ""
msgid "NotificationEvent|Merge merge request"
msgstr ""
msgid "NotificationEvent|New epic"
msgstr ""
msgid "NotificationEvent|New issue"
msgstr ""
......
# frozen_string_literal: true
# Dynamic translations which needs to be marked by `N_` so they can be found by `rake gettext:find`, see:
# https://github.com/grosser/gettext_i18n_rails#unfound-translations-with-rake-gettextfind
# NotificationSetting.email_events
N_('NotificationEvent|New note')
N_('NotificationEvent|New issue')
N_('NotificationEvent|Reopen issue')
N_('NotificationEvent|Close issue')
N_('NotificationEvent|Reassign issue')
N_('NotificationEvent|New merge request')
N_('NotificationEvent|Close merge request')
N_('NotificationEvent|Reassign merge request')
N_('NotificationEvent|Merge merge request')
N_('NotificationEvent|Failed pipeline')
......@@ -21,10 +21,11 @@ describe NotificationSettingsController do
end
context 'when authorized' do
let(:notification_setting) { user.notification_settings_for(source) }
let(:custom_events) do
events = {}
NotificationSetting::EMAIL_EVENTS.each do |event|
NotificationSetting.email_events(source).each do |event|
events[event.to_s] = true
end
......@@ -36,7 +37,7 @@ describe NotificationSettingsController do
end
context 'for projects' do
let(:notification_setting) { user.notification_settings_for(project) }
let(:source) { project }
it 'creates notification setting' do
post :create,
......@@ -67,7 +68,7 @@ describe NotificationSettingsController do
end
context 'for groups' do
let(:notification_setting) { user.notification_settings_for(group) }
let(:source) { group }
it 'creates notification setting' do
post :create,
......@@ -145,7 +146,7 @@ describe NotificationSettingsController do
let(:custom_events) do
events = {}
NotificationSetting::EMAIL_EVENTS.each do |event|
notification_setting.email_events.each do |event|
events[event] = "true"
end
end
......
......@@ -94,9 +94,39 @@ RSpec.describe NotificationSetting do
end
end
context 'email events' do
it 'includes EXCLUDED_WATCHER_EVENTS in EMAIL_EVENTS' do
expect(described_class::EMAIL_EVENTS).to include(*described_class::EXCLUDED_WATCHER_EVENTS)
describe '.email_events' do
subject { described_class.email_events }
it 'returns email events' do
expect(subject).to include(
:new_note,
:new_issue,
:reopen_issue,
:close_issue,
:reassign_issue,
:new_merge_request,
:reopen_merge_request,
:close_merge_request,
:reassign_merge_request,
:merge_merge_request,
:failed_pipeline,
:success_pipeline
)
end
it 'includes EXCLUDED_WATCHER_EVENTS' do
expect(subject).to include(*described_class::EXCLUDED_WATCHER_EVENTS)
end
end
describe '#email_events' do
let(:source) { build(:group) }
subject { build(:notification_setting, source: source) }
it 'calls email_events' do
expect(described_class).to receive(:email_events).with(source)
subject.email_events
end
end
end
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