From 157883a15a7fcb821f76b450b2235b0eac2d2b44 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 13 Mar 2017 21:42:12 +0800 Subject: [PATCH 1/6] Make successful pipeline a special case for notifying Problems -------- * People don't want to receive any successful pipeline notifications, even if they're the pusher. Current status -------------- * Watchers do not receive successful pipeline notifications. * People with custom notification with successful pipeline notifications on would receive them. For **all pipelines** regardless who is the pusher. * People receive their own successful pipeline notifications if they're the pusher. **No way to turn this off** for now. Proposed solution ----------------- * Make it that the only way to receive successful pipeline notifications would be using custom level, turning successful pipeline notifications on. Also, only send it if they're the pusher. Disregard pipelines from other people. Closes #24845 --- app/services/notification_service.rb | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index d12692ecc901..2a51e96ab17c 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -356,6 +356,19 @@ def add_custom_notifications(recipients, project, action) recipients.concat(User.find(user_ids)) end + def add_special_custom_notifications(target, project, action) + target.participants.inject([]) do |recipients, user| + setting = [user.notification_settings_for(project), + user.notification_settings_for(project.group), + user.global_notification_setting].find do |setting| + setting&.level == :custom + end + + recipients << user if setting.events[action] + recipients + end + end + # Get project users with WATCH notification level def project_watchers(project) project_members = notification_settings_for(project) @@ -599,13 +612,15 @@ def reopen_resource_email(target, project, current_user, method, status) def build_recipients(target, project, current_user, action: nil, previous_assignee: nil, skip_current_user: true) custom_action = build_custom_key(action, target) - recipients = target.participants(current_user) - - unless NotificationSetting::EXCLUDED_WATCHER_EVENTS.include?(custom_action) + if NotificationSetting::EXCLUDED_WATCHER_EVENTS.include?(custom_action) + recipients = add_special_custom_notifications( + target, project, custom_action) + else + recipients = target.participants(current_user) recipients = add_project_watchers(recipients, project) + recipients = add_custom_notifications(recipients, project, custom_action) end - recipients = add_custom_notifications(recipients, project, custom_action) recipients = reject_mention_users(recipients, project) recipients = recipients.uniq -- GitLab From 602a1588df4969b0a8ff3c4341111f3c9c67aba9 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 14 Mar 2017 01:25:34 +0800 Subject: [PATCH 2/6] Use strings to compare, and add a test --- app/services/notification_service.rb | 4 ++-- spec/models/ci/pipeline_spec.rb | 14 ++++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 2a51e96ab17c..4015c01254cd 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -361,10 +361,10 @@ def add_special_custom_notifications(target, project, action) setting = [user.notification_settings_for(project), user.notification_settings_for(project.group), user.global_notification_setting].find do |setting| - setting&.level == :custom + setting&.level == 'custom' end - recipients << user if setting.events[action] + recipients << user if setting && setting.events[action.to_s] recipients end end diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 9962c987110b..84aa4ad99f1f 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -1058,6 +1058,20 @@ def create_build(name, stage_idx) end end + it_behaves_like 'not sending any notification' + end + + context 'with success pipeline and custom notification on' do + before do + perform_enqueued_jobs do + pipeline.user.global_notification_setting + .update(level: NotificationSetting.levels[:custom], + events: {success_pipeline: true}) + + pipeline.succeed + end + end + it_behaves_like 'sending a notification' end -- GitLab From 8782c3a6593388854ab9ca736e00bc727cd37706 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 14 Mar 2017 01:51:48 +0800 Subject: [PATCH 3/6] Add space between hash literal --- spec/models/ci/pipeline_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 84aa4ad99f1f..302cc8103acd 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -1066,7 +1066,7 @@ def create_build(name, stage_idx) perform_enqueued_jobs do pipeline.user.global_notification_setting .update(level: NotificationSetting.levels[:custom], - events: {success_pipeline: true}) + events: { success_pipeline: true }) pipeline.succeed end -- GitLab From 1dcd3cce3e973c93cecc98209970ab672b8c7d58 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 14 Mar 2017 02:11:02 +0800 Subject: [PATCH 4/6] now only the pusher could possibly receive it --- spec/models/ci/pipeline_spec.rb | 3 +- .../pipeline_notification_worker_spec.rb | 37 +++++-------------- 2 files changed, 11 insertions(+), 29 deletions(-) diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 302cc8103acd..c088f13ea9d7 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -1065,8 +1065,7 @@ def create_build(name, stage_idx) before do perform_enqueued_jobs do pipeline.user.global_notification_setting - .update(level: NotificationSetting.levels[:custom], - events: { success_pipeline: true }) + .update(level: 'custom', success_pipeline: true) pipeline.succeed end diff --git a/spec/workers/pipeline_notification_worker_spec.rb b/spec/workers/pipeline_notification_worker_spec.rb index 603ae52ed1e8..b1149ef28cd6 100644 --- a/spec/workers/pipeline_notification_worker_spec.rb +++ b/spec/workers/pipeline_notification_worker_spec.rb @@ -46,33 +46,16 @@ context 'with success pipeline' do let(:status) { 'success' } let(:email_subject) { "Pipeline ##{pipeline.id} has succeeded" } - let(:receivers) { [pusher, watcher] } + let(:receivers) { [pusher] } - it_behaves_like 'sending emails' - context 'with pipeline from someone else' do - let(:pusher) { create(:user) } - let(:watcher) { user } - - context 'with success pipeline notification on' do - before do - watcher.global_notification_setting. - update(level: 'custom', success_pipeline: true) - end - - it_behaves_like 'sending emails' + context 'with custom notification success pipeline on' do + before do + pusher.global_notification_setting + .update(level: 'custom', success_pipeline: true) end - context 'with success pipeline notification off' do - let(:receivers) { [pusher] } - - before do - watcher.global_notification_setting. - update(level: 'custom', success_pipeline: false) - end - - it_behaves_like 'sending emails' - end + it_behaves_like 'sending emails' end context 'with failed pipeline' do @@ -87,8 +70,8 @@ context 'with failed pipeline notification on' do before do - watcher.global_notification_setting. - update(level: 'custom', failed_pipeline: true) + watcher.global_notification_setting + .update(level: 'custom', failed_pipeline: true) end it_behaves_like 'sending emails' @@ -98,8 +81,8 @@ let(:receivers) { [pusher] } before do - watcher.global_notification_setting. - update(level: 'custom', failed_pipeline: false) + watcher.global_notification_setting + .update(level: 'custom', failed_pipeline: false) end it_behaves_like 'sending emails' -- GitLab From 423aac61ca62526d2c2cbfd14741e2f5c9697b66 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 14 Mar 2017 02:31:10 +0800 Subject: [PATCH 5/6] Remove extra blank line --- spec/workers/pipeline_notification_worker_spec.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/workers/pipeline_notification_worker_spec.rb b/spec/workers/pipeline_notification_worker_spec.rb index b1149ef28cd6..8b0acf578e32 100644 --- a/spec/workers/pipeline_notification_worker_spec.rb +++ b/spec/workers/pipeline_notification_worker_spec.rb @@ -48,7 +48,6 @@ let(:email_subject) { "Pipeline ##{pipeline.id} has succeeded" } let(:receivers) { [pusher] } - context 'with custom notification success pipeline on' do before do pusher.global_notification_setting -- GitLab From 4e1f19257621a7653e3ae5dd7e0d85bab0a8f699 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 14 Mar 2017 21:08:56 +0800 Subject: [PATCH 6/6] Fix the tests and add changelog entry --- ...pipeline-emails-only-with-custom-level.yml | 5 ++ .../pipeline_notification_worker_spec.rb | 48 ++++++++++--------- 2 files changed, 30 insertions(+), 23 deletions(-) create mode 100644 changelogs/unreleased/enable-successful-pipeline-emails-only-with-custom-level.yml diff --git a/changelogs/unreleased/enable-successful-pipeline-emails-only-with-custom-level.yml b/changelogs/unreleased/enable-successful-pipeline-emails-only-with-custom-level.yml new file mode 100644 index 000000000000..1e2c15edaa9e --- /dev/null +++ b/changelogs/unreleased/enable-successful-pipeline-emails-only-with-custom-level.yml @@ -0,0 +1,5 @@ +--- +title: Make it that the only way to turn on successful pipeline emails would be using + custom notification with successful pipeline on +merge_request: 9907 +author: diff --git a/spec/workers/pipeline_notification_worker_spec.rb b/spec/workers/pipeline_notification_worker_spec.rb index 8b0acf578e32..9fe25e55e161 100644 --- a/spec/workers/pipeline_notification_worker_spec.rb +++ b/spec/workers/pipeline_notification_worker_spec.rb @@ -17,6 +17,9 @@ let(:watcher) { pusher } describe '#execute' do + let(:status) { 'success' } + let(:email_subject) { "Pipeline ##{pipeline.id} has succeeded" } + before do reset_delivered_emails! pipeline.project.team << [pusher, Gitlab::Access::DEVELOPER] @@ -44,8 +47,6 @@ end context 'with success pipeline' do - let(:status) { 'success' } - let(:email_subject) { "Pipeline ##{pipeline.id} has succeeded" } let(:receivers) { [pusher] } context 'with custom notification success pipeline on' do @@ -56,36 +57,37 @@ it_behaves_like 'sending emails' end + end - context 'with failed pipeline' do - let(:status) { 'failed' } - let(:email_subject) { "Pipeline ##{pipeline.id} has failed" } - - it_behaves_like 'sending emails' + context 'with failed pipeline' do + let(:status) { 'failed' } + let(:email_subject) { "Pipeline ##{pipeline.id} has failed" } + let(:receivers) { [pusher, watcher] } - context 'with pipeline from someone else' do - let(:pusher) { create(:user) } - let(:watcher) { user } + it_behaves_like 'sending emails' - context 'with failed pipeline notification on' do - before do - watcher.global_notification_setting - .update(level: 'custom', failed_pipeline: true) - end + context 'with pipeline from someone else' do + let(:pusher) { create(:user) } + let(:watcher) { user } - it_behaves_like 'sending emails' + context 'with failed pipeline notification on' do + before do + watcher.global_notification_setting + .update(level: 'custom', failed_pipeline: true) end - context 'with failed pipeline notification off' do - let(:receivers) { [pusher] } + it_behaves_like 'sending emails' + end - before do - watcher.global_notification_setting - .update(level: 'custom', failed_pipeline: false) - end + context 'with failed pipeline notification off' do + let(:receivers) { [pusher] } - it_behaves_like 'sending emails' + before do + watcher.global_notification_setting + .update(level: 'custom', failed_pipeline: false) end + + it_behaves_like 'sending emails' end end end -- GitLab