Merge branch 'quiet-pipelines' into 'master'

Quiet pipeline emails

Closes #24845

See merge request !10333

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

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

app/services/notification_recipient_service.rb

@@ -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_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

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

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

changelogs/unreleased/quiet-pipelines.yml

+---
+title: Only email pipeline creators; only email for successful pipelines with custom
+  settings
+merge_request:
+author:

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

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

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

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
+      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
+
+          it 'emails only the creator' do
+            should_only_email(u_custom_notification_enabled, kind: :bcc)
+          end
+        end
+      end
+
+      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
 
-          should_only_email(current_user, kind: :bcc)
+          it 'emails only the creator' do
+            should_only_email(u_watcher, kind: :bcc)
           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 '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
 
-          should_only_email(*users, kind: :bcc)
+        context 'when the creator has custom notifications disabled' do
+          before do
+            pipeline = create_pipeline(u_custom_notification_disabled, :failed)
+            notification.pipeline_finished(pipeline)
+          end
+
+          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
 

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
+    it 'calls NotificationService#pipeline_finished when the pipeline exists' do
+      expect(NotificationService).to receive_message_chain(:new, :pipeline_finished)
 
-    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] }
-
-            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'
+    it 'does nothing when the pipeline does not exist' do
+      expect(NotificationService).not_to receive(:new)
 
-          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
-    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 not send emails' do
-        should_only_email(pusher, kind: :bcc)
-      end
+      subject.perform(Ci::Pipeline.maximum(:id).to_i.succ)
     end
   end
 end