Merge branch 'pipeline-notifications' into 'master'

Integrate CI emails into notification system

Closes #21930

See merge request !6342

app/helpers/notifications_helper.rb

@@ -74,4 +74,13 @@ module NotificationsHelper
     return unless notification_setting.source_type
     hidden_field_tag "#{notification_setting.source_type.downcase}_id", notification_setting.source_id
   end
+
+  def notification_event_name(event)
+    case event
+    when :success_pipeline
+      'Successful pipeline'
+    else
+      event.to_s.humanize
+    end
+  end
 end

app/mailers/emails/pipelines.rb

 module Emails
   module Pipelines
-    def pipeline_success_email(pipeline, to)
-      pipeline_mail(pipeline, to, 'succeeded')
+    def pipeline_success_email(pipeline, recipients)
+      pipeline_mail(pipeline, recipients, 'succeeded')
     end
-    def pipeline_failed_email(pipeline, to)
-      pipeline_mail(pipeline, to, 'failed')
+    def pipeline_failed_email(pipeline, recipients)
+      pipeline_mail(pipeline, recipients, 'failed')
     end
     private
-    def pipeline_mail(pipeline, to, status)
+    def pipeline_mail(pipeline, recipients, status)
       @project = pipeline.project
       @pipeline = pipeline
       @merge_request = pipeline.merge_requests.first
       add_headers
-      mail(to: to, subject: pipeline_subject(status), skip_premailer: true) do |format|
+      # We use bcc here because we don't want to generate this emails for a
+      # thousand times. This could be potentially expensive in a loop, and
+      # recipients would contain all project watchers so it could be a lot.
+      mail(bcc: recipients,
+           subject: pipeline_subject(status),
+           skip_premailer: true) do |format|
         format.html { render layout: false }
         format.text
       end

app/models/ci/pipeline.rb

@@ -81,6 +81,12 @@ module Ci
           PipelineHooksWorker.perform_async(id)
         end
       end
+
+      after_transition any => [:success, :failed] do |pipeline|
+        pipeline.run_after_commit do
+          PipelineNotificationWorker.perform_async(pipeline.id)
+        end
+      end
     end
     # ref can't be HEAD or SHA, can only be branch/tag name
@@ -109,6 +115,11 @@ module Ci
       project.id
     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

@@ -32,7 +32,9 @@ class NotificationSetting < ActiveRecord::Base
     :reopen_merge_request,
     :close_merge_request,
     :reassign_merge_request,
-    :merge_merge_request
+    :merge_merge_request,
+    :failed_pipeline,
+    :success_pipeline
   ]
   store :events, accessors: EMAIL_EVENTS, coder: JSON

app/models/project_services/pipelines_email_service.rb

 class PipelinesEmailService < Service
   prop_accessor :recipients
-  boolean_accessor :add_pusher
   boolean_accessor :notify_only_broken_pipelines
-  validates :recipients,
-    presence: true,
-    if: ->(s) { s.activated? && !s.add_pusher? }
+  validates :recipients, presence: true, if: :activated?
   def initialize_properties
     self.properties ||= { notify_only_broken_pipelines: true }
@@ -34,8 +31,8 @@ class PipelinesEmailService < Service
     return unless all_recipients.any?
-    pipeline = Ci::Pipeline.find(data[:object_attributes][:id])
-    Ci::SendPipelineNotificationService.new(pipeline).execute(all_recipients)
+    pipeline_id = data[:object_attributes][:id]
+    PipelineNotificationWorker.new.perform(pipeline_id, all_recipients)
   end
   def can_test?
@@ -57,9 +54,6 @@ class PipelinesEmailService < Service
       { type: 'textarea',
         name: 'recipients',
         placeholder: 'Emails separated by comma' },
-      { type: 'checkbox',
-        name: 'add_pusher',
-        label: 'Add pusher to recipients list' },
       { type: 'checkbox',
         name: 'notify_only_broken_pipelines' },
     ]
@@ -85,12 +79,6 @@ class PipelinesEmailService < Service
   end
   def retrieve_recipients(data)
-    all_recipients = recipients.to_s.split(',').reject(&:blank?)
-
-    if add_pusher? && data[:user].try(:[], :email)
-      all_recipients << data[:user][:email]
-    end
-
-    all_recipients
+    recipients.to_s.split(',').reject(&:blank?)
   end
 end

app/policies/ci/build_policy.rb

@@ -5,7 +5,7 @@ module Ci
       # If we can't read build we should also not have that
       # ability when looking at this in context of commit_status
-      %w(read create update admin).each do |rule|
+      %w[read create update admin].each do |rule|
         cannot! :"#{rule}_commit_status" unless can? :"#{rule}_build"
       end
     end

app/policies/ci/pipeline_policy.rb

+module Ci
+  class PipelinePolicy < BuildPolicy
+  end
+end

app/services/ci/send_pipeline_notification_service.rb

-module Ci
-  class SendPipelineNotificationService
-    attr_reader :pipeline
-
-    def initialize(new_pipeline)
-      @pipeline = new_pipeline
-    end
-
-    def execute(recipients)
-      email_template = "pipeline_#{pipeline.status}_email"
-
-      return unless Notify.respond_to?(email_template)
-
-      recipients.each do |to|
-        Notify.public_send(email_template, pipeline, to).deliver_later
-      end
-    end
-  end
-end

app/services/notification_service.rb

@@ -312,6 +312,22 @@ class NotificationService
     mailer.project_was_not_exported_email(current_user, project, errors).deliver_later
   end
+  def pipeline_finished(pipeline, recipients = nil)
+    email_template = "pipeline_#{pipeline.status}_email"
+
+    return unless mailer.respond_to?(email_template)
+
+    recipients ||= build_recipients(
+      pipeline,
+      pipeline.project,
+      nil, # The acting user, who won't be added to recipients
+      action: pipeline.status).map(&:notification_email)
+
+    if recipients.any?
+      mailer.public_send(email_template, pipeline, recipients).deliver_later
+    end
+  end
+
   protected
   # Get project/group users with CUSTOM notification level
@@ -475,9 +491,14 @@ class NotificationService
   end
   def reject_users_without_access(recipients, target)
-    return recipients unless target.is_a?(Issuable)
-    ability = :"read_#{target.to_ability_name}"
+    ability = case target
+              when Issuable
+                :"read_#{target.to_ability_name}"
+              when Ci::Pipeline
+                :read_build # We have build trace in pipeline emails
+              end
+    return recipients unless ability
     recipients.select do |user|
       user.can?(ability, target)
@@ -624,6 +645,6 @@ class NotificationService
   # Build event key to search on custom notification level
   # Check NotificationSetting::EMAIL_EVENTS
   def build_custom_key(action, object)
-    "#{action}_#{object.class.name.underscore}".to_sym
+    "#{action}_#{object.class.model_name.name.underscore}".to_sym
   end
 end

app/views/shared/notifications/_custom_notifications.html.haml

@@ -27,5 +27,5 @@
                       %label{ for: field_id }
                         = check_box("notification_setting", event, id: field_id, class: "js-custom-notification-event", checked: notification_setting.events[event])
                         %strong
-                          = event.to_s.humanize
+                          = notification_event_name(event)
                           = icon("spinner spin", class: "custom-notification-event-loading")

app/workers/pipeline_notification_worker.rb

+class PipelineNotificationWorker
+  include Sidekiq::Worker
+  include PipelineQueue
+
+  def perform(pipeline_id, recipients = nil)
+    pipeline = Ci::Pipeline.find_by(id: pipeline_id)
+
+    return unless pipeline
+
+    NotificationService.new.pipeline_finished(pipeline, recipients)
+  end
+end

changelogs/unreleased/pipeline-notifications.yml

+---
+title: Add CI notifications. Who triggered a pipeline would receive an email after
+  the pipeline is succeeded or failed. Users could also update notification settings
+  accordingly
+merge_request: 6342
+author:

doc/api/notification_settings.md

@@ -4,7 +4,7 @@
 **Valid notification levels**
-The notification levels are defined in the `NotificationSetting::level` model enumeration. Currently, these levels are recognized:
+The notification levels are defined in the `NotificationSetting.level` model enumeration. Currently, these levels are recognized:
 ```
 disabled
@@ -28,6 +28,8 @@ reopen_merge_request
 close_merge_request
 reassign_merge_request
 merge_merge_request
+failed_pipeline
+success_pipeline
 ```
 ## Global notification settings
@@ -77,6 +79,8 @@ curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab
 | `close_merge_request` | boolean | no | Enable/disable this notification |
 | `reassign_merge_request` | boolean | no | Enable/disable this notification |
 | `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 |
 Example response:
@@ -141,6 +145,8 @@ curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab
 | `close_merge_request` | boolean | no | Enable/disable this notification |
 | `reassign_merge_request` | boolean | no | Enable/disable this notification |
 | `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 |
 Example responses:
@@ -161,7 +167,9 @@ Example responses:
     "reopen_merge_request": false,
     "close_merge_request": false,
     "reassign_merge_request": false,
-    "merge_merge_request": false
+    "merge_merge_request": false,
+    "failed_pipeline": false,
+    "success_pipeline": false
   }
 }
 ```

doc/workflow/notifications.md

@@ -66,6 +66,7 @@ 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
@@ -88,6 +89,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 |
 In addition, if the title or description of an Issue or Merge Request is

spec/models/ci/pipeline_spec.rb

@@ -524,4 +524,78 @@ describe Ci::Pipeline, models: true do
       expect(pipeline.merge_requests).to be_empty
     end
   end
+
+  describe 'notifications when pipeline success or failed' do
+    let(:project) { create(:project) }
+
+    let(:pipeline) do
+      create(:ci_pipeline,
+             project: project,
+             sha: project.commit('master').sha,
+             user: create(:user))
+    end
+
+    before do
+      reset_delivered_emails!
+
+      project.team << [pipeline.user, Gitlab::Access::DEVELOPER]
+
+      perform_enqueued_jobs do
+        pipeline.enqueue
+        pipeline.run
+      end
+    end
+
+    shared_examples 'sending a notification' do
+      it 'sends an email' do
+        should_only_email(pipeline.user, kind: :bcc)
+      end
+    end
+
+    shared_examples 'not sending any notification' do
+      it 'does not send any email' do
+        should_not_email_anyone
+      end
+    end
+
+    context 'with success pipeline' do
+      before do
+        perform_enqueued_jobs do
+          pipeline.succeed
+        end
+      end
+
+      it_behaves_like 'sending a notification'
+    end
+
+    context 'with failed pipeline' do
+      before do
+        perform_enqueued_jobs do
+          pipeline.drop
+        end
+      end
+
+      it_behaves_like 'sending a notification'
+    end
+
+    context 'with skipped pipeline' do
+      before do
+        perform_enqueued_jobs do
+          pipeline.skip
+        end
+      end
+
+      it_behaves_like 'not sending any notification'
+    end
+
+    context 'with cancelled pipeline' do
+      before do
+        perform_enqueued_jobs do
+          pipeline.cancel
+        end
+      end
+
+      it_behaves_like 'not sending any notification'
+    end
+  end
 end

spec/models/project_services/pipeline_email_service_spec.rb

@@ -13,7 +13,7 @@ describe PipelinesEmailService do
   end
   before do
-    ActionMailer::Base.deliveries.clear
+    reset_delivered_emails!
   end
   describe 'Validations' do
@@ -23,14 +23,6 @@ describe PipelinesEmailService do
       end
       it { is_expected.to validate_presence_of(:recipients) }
-
-      context 'when pusher is added' do
-        before do
-          subject.add_pusher = true
-        end
-
-        it { is_expected.not_to validate_presence_of(:recipients) }
-      end
     end
     context 'when service is inactive' do
@@ -66,8 +58,7 @@ describe PipelinesEmailService do
     end
     it 'sends email' do
-      sent_to = ActionMailer::Base.deliveries.flat_map(&:to)
-      expect(sent_to).to contain_exactly(recipient)
+      should_only_email(double(notification_email: recipient), kind: :bcc)
     end
   end
@@ -79,7 +70,7 @@ describe PipelinesEmailService do
     end
     it 'does not send email' do
-      expect(ActionMailer::Base.deliveries).to be_empty
+      should_not_email_anyone
     end
   end

spec/services/ci/send_pipeline_notification_service_spec.rb

-require 'spec_helper'
-
-describe Ci::SendPipelineNotificationService, services: true do
-  let(:pipeline) do
-    create(:ci_pipeline,
-           project: project,
-           sha: project.commit('master').sha,
-           user: user,
-           status: status)
-  end
-
-  let(:project) { create(:project) }
-  let(:user) { create(:user) }
-
-  subject{ described_class.new(pipeline) }
-
-  describe '#execute' do
-    before do
-      reset_delivered_emails!
-    end
-
-    shared_examples 'sending emails' do
-      it 'sends an email to pipeline user' do
-        perform_enqueued_jobs do
-          subject.execute([user.email])
-        end
-
-        email = ActionMailer::Base.deliveries.last
-        expect(email.subject).to include(email_subject)
-        expect(email.to).to eq([user.email])
-      end
-    end
-
-    context 'with success pipeline' do
-      let(:status) { 'success' }
-      let(:email_subject) { "Pipeline ##{pipeline.id} has succeeded" }
-
-      it_behaves_like 'sending emails'
-    end
-
-    context 'with failed pipeline' do
-      let(:status) { 'failed' }
-      let(:email_subject) { "Pipeline ##{pipeline.id} has failed" }
-
-      it_behaves_like 'sending emails'
-    end
-  end
-end

spec/services/notification_service_spec.rb

@@ -17,7 +17,7 @@ describe NotificationService, services: true do
     it 'sends no emails when no new mentions are present' do
       send_notifications
-      expect(ActionMailer::Base.deliveries).to be_empty
+      should_not_email_anyone
     end
     it 'emails new mentions with a watch level higher than participant' do
@@ -27,7 +27,7 @@ describe NotificationService, services: true do
     it 'does not email new mentions with a watch level equal to or less than participant' do
       send_notifications(@u_participating, @u_mentioned)
-      expect(ActionMailer::Base.deliveries).to be_empty
+      should_not_email_anyone
     end
   end
@@ -79,7 +79,7 @@ describe NotificationService, services: true do
           # Ensure create SentNotification by noteable = issue 6 times, not noteable = note
           expect(SentNotification).to receive(:record).with(issue, any_args).exactly(8).times
-          ActionMailer::Base.deliveries.clear
+          reset_delivered_emails!
           notification.new_note(note)
@@ -111,7 +111,7 @@ describe NotificationService, services: true do
         context 'participating' do
           context 'by note' do
             before do
-              ActionMailer::Base.deliveries.clear
+              reset_delivered_emails!
               note.author = @u_lazy_participant
               note.save
               notification.new_note(note)
@@ -134,7 +134,7 @@ describe NotificationService, services: true do
           @u_watcher.notification_settings_for(note.project).participating!
           @u_watcher.notification_settings_for(note.project.group).global!
           update_custom_notification(:new_note, @u_custom_global)
-          ActionMailer::Base.deliveries.clear
+          reset_delivered_emails!
         end
         it do
@@ -173,7 +173,7 @@ describe NotificationService, services: true do
         expect(SentNotification).to receive(:record).with(confidential_issue, any_args).exactly(4).times
-        ActionMailer::Base.deliveries.clear
+        reset_delivered_emails!
         notification.new_note(note)
@@ -196,7 +196,7 @@ describe NotificationService, services: true do
       before do
         build_team(note.project)
         note.project.team << [note.author, :master]
-        ActionMailer::Base.deliveries.clear
+        reset_delivered_emails!
       end
       describe '#new_note' do
@@ -238,7 +238,7 @@ describe NotificationService, services: true do
       before do
         build_team(note.project)
         note.project.team << [note.author, :master]
-        ActionMailer::Base.deliveries.clear
+        reset_delivered_emails!
       end
       describe '#new_note' do
@@ -273,7 +273,7 @@ describe NotificationService, services: true do
       before do
         build_team(note.project)
-        ActionMailer::Base.deliveries.clear
+        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_custom_global)
@@ -348,7 +348,7 @@ describe NotificationService, services: true do
     before do
       build_team(issue.project)
       add_users_with_subscription(issue.project, issue)
-      ActionMailer::Base.deliveries.clear
+      reset_delivered_emails!
       update_custom_notification(:new_issue, @u_guest_custom, project)
       update_custom_notification(:new_issue, @u_custom_global)
     end
@@ -408,7 +408,7 @@ describe NotificationService, services: true do
           label.toggle_subscription(guest)
           label.toggle_subscription(admin)
-          ActionMailer::Base.deliveries.clear
+          reset_delivered_emails!
           notification.new_issue(confidential_issue, @u_disabled)
@@ -604,7 +604,7 @@ describe NotificationService, services: true do
           label_2.toggle_subscription(guest)
           label_2.toggle_subscription(admin)
-          ActionMailer::Base.deliveries.clear
+          reset_delivered_emails!
           notification.relabeled_issue(confidential_issue, [label_2], @u_disabled)
@@ -733,7 +733,7 @@ describe NotificationService, services: true do
       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_custom_global)
-      ActionMailer::Base.deliveries.clear
+      reset_delivered_emails!
     end
     describe '#new_merge_request' do
@@ -1111,7 +1111,7 @@ describe NotificationService, services: true do
     before do
       build_team(project)
-      ActionMailer::Base.deliveries.clear
+      reset_delivered_emails!
     end
     describe '#project_was_moved' do

spec/support/email_helpers.rb

 module EmailHelpers
-  def sent_to_user?(user)
-    ActionMailer::Base.deliveries.map(&:to).flatten.count(user.email) == 1
+  def sent_to_user?(user, recipients = email_recipients)
+    recipients.include?(user.notification_email)
   end
   def reset_delivered_emails!
     ActionMailer::Base.deliveries.clear
   end
-  def should_only_email(*users)
-    users.each {|user| should_email(user) }
-    recipients = ActionMailer::Base.deliveries.flat_map(&:to)
+  def should_only_email(*users, kind: :to)
+    recipients = email_recipients(kind: kind)
+
+    users.each { |user| should_email(user, recipients) }
+
     expect(recipients.count).to eq(users.count)
   end
-  def should_email(user)
-    expect(sent_to_user?(user)).to be_truthy
+  def should_email(user, recipients = email_recipients)
+    expect(sent_to_user?(user, recipients)).to be_truthy
+  end
+
+  def should_not_email(user, recipients = email_recipients)
+    expect(sent_to_user?(user, recipients)).to be_falsey
+  end
+
+  def should_not_email_anyone
+    expect(ActionMailer::Base.deliveries).to be_empty
   end
-  def should_not_email(user)
-    expect(sent_to_user?(user)).to be_falsey
+  def email_recipients(kind: :to)
+    ActionMailer::Base.deliveries.flat_map(&kind)
   end
 end

spec/support/notify_shared_examples.rb

@@ -7,7 +7,7 @@ shared_context 'gitlab email notification' do
   let(:new_user_address) { 'newguy@example.com' }
   before do
-    ActionMailer::Base.deliveries.clear
+    reset_delivered_emails!
     email = recipient.emails.create(email: "notifications@example.com")
     recipient.update_attribute(:notification_email, email.email)
     stub_incoming_email_setting(enabled: true, address: "reply+%{key}@#{Gitlab.config.gitlab.host}")