From a1805cbcd55a28658049b42c12a87a50ab9ab977 Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Thu, 30 Mar 2017 12:29:52 +0100
Subject: [PATCH] Quiet pipeline emails
1. Never send a pipeline email to anyone other than the user who created
the pipeline.
2. Only send pipeline success emails to people with the custom
notification setting for enabled. Watchers and participants will
never receive this.
3. When custom settings are unset (for new settings and legacy ones),
act as if failed_pipeline is set.
---
app/models/ci/pipeline.rb | 5 -
app/models/notification_setting.rb | 17 +-
.../notification_recipient_service.rb | 42 +++-
app/services/notification_service.rb | 4 +-
.../_custom_notifications.html.haml | 2 +-
changelogs/unreleased/quiet-pipelines.yml | 5 +
doc/workflow/notifications.md | 7 +-
spec/factories/ci/pipelines.rb | 4 +
spec/models/ci/pipeline_spec.rb | 7 +-
spec/services/notification_service_spec.rb | 190 +++++++++++++++---
.../pipeline_notification_worker_spec.rb | 126 +-----------
11 files changed, 237 insertions(+), 172 deletions(-)
create mode 100644 changelogs/unreleased/quiet-pipelines.yml
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index ad7e0b23ff4..49dec770096 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -164,11 +164,6 @@ module Ci
builds.latest.with_artifacts_not_expired.includes(project: [:namespace])
end
- # For now the only user who participates is the user who triggered
- def participants(_current_user = nil)
- Array(user)
- end
-
def valid_commit_sha
if self.sha == Gitlab::Git::BLANK_SHA
self.errors.add(:sha, " cant be 00000000 (branch removal)")
diff --git a/app/models/notification_setting.rb b/app/models/notification_setting.rb
index 52577bd52ea..e4726e62e93 100644
--- a/app/models/notification_setting.rb
+++ b/app/models/notification_setting.rb
@@ -60,16 +60,25 @@ class NotificationSetting < ActiveRecord::Base
def set_events
return if custom?
- EMAIL_EVENTS.each do |event|
- events[event] = false
- end
+ self.events = {}
end
# Validates store accessors values as boolean
# It is a text field so it does not cast correct boolean values in JSON
def events_to_boolean
EMAIL_EVENTS.each do |event|
- events[event] = ActiveRecord::ConnectionAdapters::Column::TRUE_VALUES.include?(events[event])
+ bool = ActiveRecord::ConnectionAdapters::Column::TRUE_VALUES.include?(public_send(event))
+
+ events[event] = bool
end
end
+
+ # Allow people to receive failed pipeline notifications if they already have
+ # custom notifications enabled, as these are more like mentions than the other
+ # custom settings.
+ def failed_pipeline
+ bool = super
+
+ bool.nil? || bool
+ end
end
diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 940e850600f..8bb995158de 100644
--- a/app/services/notification_recipient_service.rb
+++ b/app/services/notification_recipient_service.rb
@@ -3,7 +3,7 @@
#
class NotificationRecipientService
attr_reader :project
-
+
def initialize(project)
@project = project
end
@@ -12,11 +12,7 @@ class NotificationRecipientService
custom_action = build_custom_key(action, target)
recipients = target.participants(current_user)
-
- unless NotificationSetting::EXCLUDED_WATCHER_EVENTS.include?(custom_action)
- recipients = add_project_watchers(recipients)
- end
-
+ recipients = add_project_watchers(recipients)
recipients = add_custom_notifications(recipients, custom_action)
recipients = reject_mention_users(recipients)
@@ -43,6 +39,28 @@ class NotificationRecipientService
recipients.uniq
end
+ def build_pipeline_recipients(target, current_user, action:)
+ return [] unless current_user
+
+ custom_action =
+ case action.to_s
+ when 'failed'
+ :failed_pipeline
+ when 'success'
+ :success_pipeline
+ end
+
+ notification_setting = notification_setting_for_user_project(current_user, target.project)
+
+ return [] if notification_setting.mention? || notification_setting.disabled?
+
+ return [] if notification_setting.custom? && !notification_setting.public_send(custom_action)
+
+ return [] if (notification_setting.watch? || notification_setting.participating?) && NotificationSetting::EXCLUDED_WATCHER_EVENTS.include?(custom_action)
+
+ reject_users_without_access([current_user], target)
+ end
+
def build_relabeled_recipients(target, current_user, labels:)
recipients = add_labels_subscribers([], target, labels: labels)
recipients = reject_unsubscribed_users(recipients, target)
@@ -290,4 +308,16 @@ class NotificationRecipientService
def build_custom_key(action, object)
"#{action}_#{object.class.model_name.name.underscore}".to_sym
end
+
+ def notification_setting_for_user_project(user, project)
+ project_setting = user.notification_settings_for(project)
+
+ return project_setting unless project_setting.global?
+
+ group_setting = user.notification_settings_for(project.group)
+
+ return group_setting unless group_setting.global?
+
+ user.global_notification_setting
+ end
end
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index 2c6f849259e..6b186263bd1 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -278,11 +278,11 @@ class NotificationService
return unless mailer.respond_to?(email_template)
- recipients ||= NotificationRecipientService.new(pipeline.project).build_recipients(
+ recipients ||= NotificationRecipientService.new(pipeline.project).build_pipeline_recipients(
pipeline,
pipeline.user,
action: pipeline.status,
- skip_current_user: false).map(&:notification_email)
+ ).map(&:notification_email)
if recipients.any?
mailer.public_send(email_template, pipeline, recipients).deliver_later
diff --git a/app/views/shared/notifications/_custom_notifications.html.haml b/app/views/shared/notifications/_custom_notifications.html.haml
index a736bfd91e2..708adbc38f1 100644
--- a/app/views/shared/notifications/_custom_notifications.html.haml
+++ b/app/views/shared/notifications/_custom_notifications.html.haml
@@ -25,7 +25,7 @@
.form-group
.checkbox{ class: ("prepend-top-0" if index == 0) }
%label{ for: field_id }
- = check_box("notification_setting", event, id: field_id, class: "js-custom-notification-event", checked: notification_setting.events[event])
+ = check_box("notification_setting", event, id: field_id, class: "js-custom-notification-event", checked: notification_setting.public_send(event))
%strong
= notification_event_name(event)
= icon("spinner spin", class: "custom-notification-event-loading")
diff --git a/changelogs/unreleased/quiet-pipelines.yml b/changelogs/unreleased/quiet-pipelines.yml
new file mode 100644
index 00000000000..c02eb59b824
--- /dev/null
+++ b/changelogs/unreleased/quiet-pipelines.yml
@@ -0,0 +1,5 @@
+---
+title: Only email pipeline creators; only email for successful pipelines with custom
+ settings
+merge_request:
+author:
diff --git a/doc/workflow/notifications.md b/doc/workflow/notifications.md
index 4c52974e103..e91d36987a9 100644
--- a/doc/workflow/notifications.md
+++ b/doc/workflow/notifications.md
@@ -66,14 +66,13 @@ Below is the table of events users can be notified of:
In all of the below cases, the notification will be sent to:
- Participants:
- the author and assignee of the issue/merge request
- - the author of the pipeline
- authors of comments on the issue/merge request
- anyone mentioned by `@username` in the issue/merge request title or description
- anyone mentioned by `@username` in any of the comments on the issue/merge request
...with notification level "Participating" or higher
-- Watchers: users with notification level "Watch" (however successful pipeline would be off for watchers)
+- Watchers: users with notification level "Watch"
- Subscribers: anyone who manually subscribed to the issue/merge request
- Custom: Users with notification level "custom" who turned on notifications for any of the events present in the table below
@@ -89,8 +88,8 @@ In all of the below cases, the notification will be sent to:
| Reopen merge request | |
| Merge merge request | |
| New comment | The above, plus anyone mentioned by `@username` in the comment, with notification level "Mention" or higher |
-| Failed pipeline | The above, plus the author of the pipeline |
-| Successful pipeline | The above, plus the author of the pipeline |
+| Failed pipeline | The author of the pipeline |
+| Successful pipeline | The author of the pipeline, if they have the custom notification setting for successful pipelines set |
In addition, if the title or description of an Issue or Merge Request is
diff --git a/spec/factories/ci/pipelines.rb b/spec/factories/ci/pipelines.rb
index b67c96bc00d..561fbc8e247 100644
--- a/spec/factories/ci/pipelines.rb
+++ b/spec/factories/ci/pipelines.rb
@@ -48,6 +48,10 @@ FactoryGirl.define do
trait :success do
status :success
end
+
+ trait :failed do
+ status :failed
+ end
end
end
end
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 53282b999dc..e4a24fd63c2 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -1055,10 +1055,13 @@ describe Ci::Pipeline, models: true do
end
before do
- reset_delivered_emails!
-
project.team << [pipeline.user, Gitlab::Access::DEVELOPER]
+ pipeline.user.global_notification_setting.
+ update(level: 'custom', failed_pipeline: true, success_pipeline: true)
+
+ reset_delivered_emails!
+
perform_enqueued_jobs do
pipeline.enqueue
pipeline.run
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index 5c841843b40..e3146a56495 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -113,7 +113,7 @@ describe NotificationService, services: true do
project.add_master(issue.assignee)
project.add_master(note.author)
create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: '@subscribed_participant cc this guy')
- update_custom_notification(:new_note, @u_guest_custom, project)
+ update_custom_notification(:new_note, @u_guest_custom, resource: project)
update_custom_notification(:new_note, @u_custom_global)
end
@@ -379,7 +379,7 @@ describe NotificationService, services: true do
build_team(note.project)
reset_delivered_emails!
allow_any_instance_of(Commit).to receive(:author).and_return(@u_committer)
- update_custom_notification(:new_note, @u_guest_custom, project)
+ update_custom_notification(:new_note, @u_guest_custom, resource: project)
update_custom_notification(:new_note, @u_custom_global)
end
@@ -457,7 +457,7 @@ describe NotificationService, services: true do
add_users_with_subscription(issue.project, issue)
reset_delivered_emails!
- update_custom_notification(:new_issue, @u_guest_custom, project)
+ update_custom_notification(:new_issue, @u_guest_custom, resource: project)
update_custom_notification(:new_issue, @u_custom_global)
end
@@ -567,7 +567,7 @@ describe NotificationService, services: true do
describe '#reassigned_issue' do
before do
- update_custom_notification(:reassign_issue, @u_guest_custom, project)
+ update_custom_notification(:reassign_issue, @u_guest_custom, resource: project)
update_custom_notification(:reassign_issue, @u_custom_global)
end
@@ -760,7 +760,7 @@ describe NotificationService, services: true do
describe '#close_issue' do
before do
- update_custom_notification(:close_issue, @u_guest_custom, project)
+ update_custom_notification(:close_issue, @u_guest_custom, resource: project)
update_custom_notification(:close_issue, @u_custom_global)
end
@@ -791,7 +791,7 @@ describe NotificationService, services: true do
describe '#reopen_issue' do
before do
- update_custom_notification(:reopen_issue, @u_guest_custom, project)
+ update_custom_notification(:reopen_issue, @u_guest_custom, resource: project)
update_custom_notification(:reopen_issue, @u_custom_global)
end
@@ -856,14 +856,14 @@ describe NotificationService, services: true do
before do
build_team(merge_request.target_project)
add_users_with_subscription(merge_request.target_project, merge_request)
- update_custom_notification(:new_merge_request, @u_guest_custom, project)
+ update_custom_notification(:new_merge_request, @u_guest_custom, resource: project)
update_custom_notification(:new_merge_request, @u_custom_global)
reset_delivered_emails!
end
describe '#new_merge_request' do
before do
- update_custom_notification(:new_merge_request, @u_guest_custom, project)
+ update_custom_notification(:new_merge_request, @u_guest_custom, resource: project)
update_custom_notification(:new_merge_request, @u_custom_global)
end
@@ -952,7 +952,7 @@ describe NotificationService, services: true do
describe '#reassigned_merge_request' do
before do
- update_custom_notification(:reassign_merge_request, @u_guest_custom, project)
+ update_custom_notification(:reassign_merge_request, @u_guest_custom, resource: project)
update_custom_notification(:reassign_merge_request, @u_custom_global)
end
@@ -1026,7 +1026,7 @@ describe NotificationService, services: true do
describe '#closed_merge_request' do
before do
- update_custom_notification(:close_merge_request, @u_guest_custom, project)
+ update_custom_notification(:close_merge_request, @u_guest_custom, resource: project)
update_custom_notification(:close_merge_request, @u_custom_global)
end
@@ -1056,7 +1056,7 @@ describe NotificationService, services: true do
describe '#merged_merge_request' do
before do
- update_custom_notification(:merge_merge_request, @u_guest_custom, project)
+ update_custom_notification(:merge_merge_request, @u_guest_custom, resource: project)
update_custom_notification(:merge_merge_request, @u_custom_global)
end
@@ -1108,7 +1108,7 @@ describe NotificationService, services: true do
describe '#reopen_merge_request' do
before do
- update_custom_notification(:reopen_merge_request, @u_guest_custom, project)
+ update_custom_notification(:reopen_merge_request, @u_guest_custom, resource: project)
update_custom_notification(:reopen_merge_request, @u_custom_global)
end
@@ -1281,40 +1281,172 @@ describe NotificationService, services: true do
describe 'Pipelines' do
describe '#pipeline_finished' do
let(:project) { create(:project, :public, :repository) }
- let(:current_user) { create(:user) }
let(:u_member) { create(:user) }
- let(:u_other) { create(:user) }
+ let(:u_watcher) { create_user_with_notification(:watch, 'watcher') }
+
+ let(:u_custom_notification_unset) do
+ create_user_with_notification(:custom, 'custom_unset')
+ end
+
+ let(:u_custom_notification_enabled) do
+ user = create_user_with_notification(:custom, 'custom_enabled')
+ update_custom_notification(:success_pipeline, user, resource: project)
+ update_custom_notification(:failed_pipeline, user, resource: project)
+ user
+ end
+
+ let(:u_custom_notification_disabled) do
+ user = create_user_with_notification(:custom, 'custom_disabled')
+ update_custom_notification(:success_pipeline, user, resource: project, value: false)
+ update_custom_notification(:failed_pipeline, user, resource: project, value: false)
+ user
+ end
let(:commit) { project.commit }
- let(:pipeline) do
- create(:ci_pipeline, :success,
+
+ def create_pipeline(user, status)
+ create(:ci_pipeline, status,
project: project,
- user: current_user,
+ user: user,
ref: 'refs/heads/master',
sha: commit.id,
before_sha: '00000000')
end
before do
- project.add_master(current_user)
project.add_master(u_member)
+ project.add_master(u_watcher)
+ project.add_master(u_custom_notification_unset)
+ project.add_master(u_custom_notification_enabled)
+ project.add_master(u_custom_notification_disabled)
+
reset_delivered_emails!
end
- context 'without custom recipients' do
- it 'notifies the pipeline user' do
- notification.pipeline_finished(pipeline)
+ context 'with a successful pipeline' do
+ context 'when the creator has default settings' do
+ before do
+ pipeline = create_pipeline(u_member, :success)
+ notification.pipeline_finished(pipeline)
+ end
+
+ it 'notifies nobody' do
+ should_not_email_anyone
+ end
+ end
+
+ context 'when the creator has watch set' do
+ before do
+ pipeline = create_pipeline(u_watcher, :success)
+ notification.pipeline_finished(pipeline)
+ end
+
+ it 'notifies nobody' do
+ should_not_email_anyone
+ end
+ end
+
+ context 'when the creator has custom notifications, but without any set' do
+ before do
+ pipeline = create_pipeline(u_custom_notification_unset, :success)
+ notification.pipeline_finished(pipeline)
+ end
+
+ it 'notifies nobody' do
+ should_not_email_anyone
+ end
+ end
+
+ context 'when the creator has custom notifications disabled' do
+ before do
+ pipeline = create_pipeline(u_custom_notification_disabled, :success)
+ notification.pipeline_finished(pipeline)
+ end
+
+ it 'notifies nobody' do
+ should_not_email_anyone
+ end
+ end
+
+ context 'when the creator has custom notifications enabled' do
+ before do
+ pipeline = create_pipeline(u_custom_notification_enabled, :success)
+ notification.pipeline_finished(pipeline)
+ end
- should_only_email(current_user, kind: :bcc)
+ it 'emails only the creator' do
+ should_only_email(u_custom_notification_enabled, kind: :bcc)
+ end
end
end
- context 'with custom recipients' do
- it 'notifies the custom recipients' do
- users = [u_member, u_other]
- notification.pipeline_finished(pipeline, users.map(&:notification_email))
+ context 'with a failed pipeline' do
+ context 'when the creator has no custom notification set' do
+ before do
+ pipeline = create_pipeline(u_member, :failed)
+ notification.pipeline_finished(pipeline)
+ end
+
+ it 'emails only the creator' do
+ should_only_email(u_member, kind: :bcc)
+ end
+ end
+
+ context 'when the creator has watch set' do
+ before do
+ pipeline = create_pipeline(u_watcher, :failed)
+ notification.pipeline_finished(pipeline)
+ end
+
+ it 'emails only the creator' do
+ should_only_email(u_watcher, kind: :bcc)
+ end
+ end
+
+ context 'when the creator has custom notifications, but without any set' do
+ before do
+ pipeline = create_pipeline(u_custom_notification_unset, :failed)
+ notification.pipeline_finished(pipeline)
+ end
+
+ it 'emails only the creator' do
+ should_only_email(u_custom_notification_unset, kind: :bcc)
+ end
+ end
+
+ context 'when the creator has custom notifications disabled' do
+ before do
+ pipeline = create_pipeline(u_custom_notification_disabled, :failed)
+ notification.pipeline_finished(pipeline)
+ end
- should_only_email(*users, kind: :bcc)
+ it 'notifies nobody' do
+ should_not_email_anyone
+ end
+ end
+
+ context 'when the creator has custom notifications set' do
+ before do
+ pipeline = create_pipeline(u_custom_notification_enabled, :failed)
+ notification.pipeline_finished(pipeline)
+ end
+
+ it 'emails only the creator' do
+ should_only_email(u_custom_notification_enabled, kind: :bcc)
+ end
+ end
+
+ context 'when the creator has no read_build access' do
+ before do
+ pipeline = create_pipeline(u_member, :failed)
+ project.update(public_builds: false)
+ project.team.truncate
+ notification.pipeline_finished(pipeline)
+ end
+
+ it 'does not send emails' do
+ should_not_email_anyone
+ end
end
end
end
@@ -1385,9 +1517,9 @@ describe NotificationService, services: true do
# Create custom notifications
# When resource is nil it means global notification
- def update_custom_notification(event, user, resource = nil)
+ def update_custom_notification(event, user, resource: nil, value: true)
setting = user.notification_settings_for(resource)
- setting.events[event] = true
+ setting.events[event] = value
setting.save
end
diff --git a/spec/workers/pipeline_notification_worker_spec.rb b/spec/workers/pipeline_notification_worker_spec.rb
index 5a7ce2e08c4..139032d77bd 100644
--- a/spec/workers/pipeline_notification_worker_spec.rb
+++ b/spec/workers/pipeline_notification_worker_spec.rb
@@ -3,131 +3,19 @@ require 'spec_helper'
describe PipelineNotificationWorker do
include EmailHelpers
- let(:pipeline) do
- create(:ci_pipeline,
- project: project,
- sha: project.commit('master').sha,
- user: pusher,
- status: status)
- end
-
- let(:project) { create(:project, :repository, public_builds: false) }
- let(:user) { create(:user) }
- let(:pusher) { user }
- let(:watcher) { pusher }
+ let(:pipeline) { create(:ci_pipeline) }
describe '#execute' do
- before do
- reset_delivered_emails!
- pipeline.project.team << [pusher, Gitlab::Access::DEVELOPER]
- end
-
- context 'when watcher has developer access' do
- before do
- pipeline.project.team << [watcher, Gitlab::Access::DEVELOPER]
- end
-
- shared_examples 'sending emails' do
- it 'sends emails' do
- perform_enqueued_jobs do
- subject.perform(pipeline.id)
- end
-
- emails = ActionMailer::Base.deliveries
- actual = emails.flat_map(&:bcc).sort
- expected_receivers = receivers.map(&:email).uniq.sort
-
- expect(actual).to eq(expected_receivers)
- expect(emails.size).to eq(1)
- expect(emails.last.subject).to include(email_subject)
- end
- end
-
- context 'with success pipeline' do
- let(:status) { 'success' }
- let(:email_subject) { "Pipeline ##{pipeline.id} has succeeded" }
- let(:receivers) { [pusher, watcher] }
-
- 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'
- end
-
- context 'with success pipeline notification off' do
- let(:receivers) { [pusher] }
+ it 'calls NotificationService#pipeline_finished when the pipeline exists' do
+ expect(NotificationService).to receive_message_chain(:new, :pipeline_finished)
- before do
- watcher.global_notification_setting.
- update(level: 'custom', success_pipeline: false)
- end
-
- 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 pipeline from someone else' do
- let(:pusher) { create(:user) }
- let(:watcher) { user }
-
- context 'with failed pipeline notification on' do
- before do
- watcher.global_notification_setting.
- update(level: 'custom', failed_pipeline: true)
- end
-
- it_behaves_like 'sending emails'
- end
-
- context 'with failed pipeline notification off' do
- let(:receivers) { [pusher] }
-
- before do
- watcher.global_notification_setting.
- update(level: 'custom', failed_pipeline: false)
- end
-
- it_behaves_like 'sending emails'
- end
- end
- end
- end
+ subject.perform(pipeline.id)
end
- context 'when watcher has no read_build access' do
- let(:status) { 'failed' }
- let(:email_subject) { "Pipeline ##{pipeline.id} has failed" }
- let(:watcher) { create(:user) }
-
- before do
- pipeline.project.team << [watcher, Gitlab::Access::GUEST]
-
- watcher.global_notification_setting.
- update(level: 'custom', failed_pipeline: true)
-
- perform_enqueued_jobs do
- subject.perform(pipeline.id)
- end
- end
+ it 'does nothing when the pipeline does not exist' do
+ expect(NotificationService).not_to receive(:new)
- it 'does not send emails' do
- should_only_email(pusher, kind: :bcc)
- end
+ subject.perform(Ci::Pipeline.maximum(:id).to_i.succ)
end
end
end
--
GitLab