diff options
19 files changed, 279 insertions, 131 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 8cb848c3957..2b65f5842b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,7 @@ Please view this file on the master branch, on stable branches it's out of date. - The instrumentation for Banzai::Renderer has been restored - Change user & group landing page routing from /u/:username to /:username - Added documentation for .gitattributes files + - 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. !6342 - Move Pipeline Metrics to separate worker - AbstractReferenceFilter caches project_refs on RequestStore when active - Replaced the check sign to arrow in the show build view. !6501 diff --git a/app/helpers/notifications_helper.rb b/app/helpers/notifications_helper.rb index 7e8369d0a05..03cc8f2b6bd 100644 --- a/app/helpers/notifications_helper.rb +++ b/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 diff --git a/app/mailers/emails/pipelines.rb b/app/mailers/emails/pipelines.rb index 601c8b5cd62..9460a6cd2be 100644 --- a/app/mailers/emails/pipelines.rb +++ b/app/mailers/emails/pipelines.rb @@ -1,22 +1,27 @@ 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 diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index e84c91b417d..66bab722110 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -84,6 +84,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 @@ -112,6 +118,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)") diff --git a/app/models/notification_setting.rb b/app/models/notification_setting.rb index 121b598b8f3..43fc218de2b 100644 --- a/app/models/notification_setting.rb +++ b/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 diff --git a/app/models/project_services/pipelines_email_service.rb b/app/models/project_services/pipelines_email_service.rb index ec3c1bc85ee..745f9bd1b43 100644 --- a/app/models/project_services/pipelines_email_service.rb +++ b/app/models/project_services/pipelines_email_service.rb @@ -1,10 +1,7 @@ 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? @@ -58,9 +55,6 @@ class PipelinesEmailService < Service 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' }, ] end @@ -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 diff --git a/app/services/ci/send_pipeline_notification_service.rb b/app/services/ci/send_pipeline_notification_service.rb deleted file mode 100644 index ceb182801f7..00000000000 --- a/app/services/ci/send_pipeline_notification_service.rb +++ /dev/null @@ -1,19 +0,0 @@ -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 diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 72712afc07e..d5d69248af7 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -312,6 +312,20 @@ 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(&:email) + + mailer.public_send(email_template, pipeline, recipients).deliver_later + end + protected # Get project/group users with CUSTOM notification level @@ -624,6 +638,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 diff --git a/app/views/shared/notifications/_custom_notifications.html.haml b/app/views/shared/notifications/_custom_notifications.html.haml index b704981e3db..a82fc95df84 100644 --- a/app/views/shared/notifications/_custom_notifications.html.haml +++ b/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") diff --git a/app/workers/pipeline_notification_worker.rb b/app/workers/pipeline_notification_worker.rb new file mode 100644 index 00000000000..68e72f7382f --- /dev/null +++ b/app/workers/pipeline_notification_worker.rb @@ -0,0 +1,11 @@ +class PipelineNotificationWorker + include Sidekiq::Worker + + 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 diff --git a/spec/mailers/shared/notify.rb b/spec/mailers/shared/notify.rb index 3956d05060b..49867aa5cc4 100644 --- a/spec/mailers/shared/notify.rb +++ b/spec/mailers/shared/notify.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}") diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 43397c5ae39..ab05150c97e 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -528,4 +528,76 @@ 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! + + 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 diff --git a/spec/models/project_services/pipeline_email_service_spec.rb b/spec/models/project_services/pipeline_email_service_spec.rb index 1368a2925e8..57baff3354f 100644 --- a/spec/models/project_services/pipeline_email_service_spec.rb +++ b/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(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 diff --git a/spec/services/ci/send_pipeline_notification_service_spec.rb b/spec/services/ci/send_pipeline_notification_service_spec.rb deleted file mode 100644 index 288302cc94f..00000000000 --- a/spec/services/ci/send_pipeline_notification_service_spec.rb +++ /dev/null @@ -1,48 +0,0 @@ -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 diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 699b9925b4e..8ce35354c22 100644 --- a/spec/services/notification_service_spec.rb +++ b/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 diff --git a/spec/support/email_helpers.rb b/spec/support/email_helpers.rb index 0bfc4685532..a946f10f71a 100644 --- a/spec/support/email_helpers.rb +++ b/spec/support/email_helpers.rb @@ -1,23 +1,33 @@ 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.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 diff --git a/spec/workers/build_email_worker_spec.rb b/spec/workers/build_email_worker_spec.rb index 788b92c1b84..a1aa336361a 100644 --- a/spec/workers/build_email_worker_spec.rb +++ b/spec/workers/build_email_worker_spec.rb @@ -24,7 +24,7 @@ describe BuildEmailWorker do end it "gracefully handles an input SMTP error" do - ActionMailer::Base.deliveries.clear + reset_delivered_emails! allow(Notify).to receive(:build_success_email).and_raise(Net::SMTPFatalError) subject.perform(build.id, [user.email], data.stringify_keys) diff --git a/spec/workers/emails_on_push_worker_spec.rb b/spec/workers/emails_on_push_worker_spec.rb index 036d037f3f9..fc652f6f4c3 100644 --- a/spec/workers/emails_on_push_worker_spec.rb +++ b/spec/workers/emails_on_push_worker_spec.rb @@ -87,7 +87,7 @@ describe EmailsOnPushWorker do context "when there is an SMTP error" do before do - ActionMailer::Base.deliveries.clear + reset_delivered_emails! allow(Notify).to receive(:repository_push_email).and_raise(Net::SMTPFatalError) allow(subject).to receive_message_chain(:logger, :info) perform @@ -112,7 +112,7 @@ describe EmailsOnPushWorker do original.call(Mail.new(mail.encoded)) end - ActionMailer::Base.deliveries.clear + reset_delivered_emails! end it "sends the mail to each of the recipients" do diff --git a/spec/workers/pipeline_notification_worker_spec.rb b/spec/workers/pipeline_notification_worker_spec.rb new file mode 100644 index 00000000000..c334b2057a6 --- /dev/null +++ b/spec/workers/pipeline_notification_worker_spec.rb @@ -0,0 +1,101 @@ +require 'spec_helper' + +describe PipelineNotificationWorker do + let(:pipeline) do + create(:ci_pipeline, + project: project, + sha: project.commit('master').sha, + user: pusher, + status: status) + end + + let(:project) { create(:project) } + let(:user) { create(:user) } + let(:pusher) { user } + let(:watcher) { pusher } + + describe '#execute' do + before do + reset_delivered_emails! + 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 = [pusher, watcher].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" } + + it_behaves_like 'sending emails' + + context 'with pipeline from someone else' do + let(:pusher) { create(:user) } + + context 'with success pipeline notification on' do + let(:watcher) { user } + + 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 + before do + watcher.global_notification_setting. + update(level: 'custom', success_pipeline: false) + end + + it_behaves_like 'sending emails' + end + 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) } + + context 'with failed pipeline notification on' do + let(:watcher) { user } + + 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 + before do + watcher.global_notification_setting. + update(level: 'custom', failed_pipeline: false) + end + + it_behaves_like 'sending emails' + end + end + end + end +end |