From e79b867d2b24f97cdf91315abd9dbbb22dc2d0e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Tue, 5 Apr 2016 18:15:13 +0200 Subject: Ensure empty recipients are rejected in BuildsEmailService MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- CHANGELOG | 1 + .../project_services/builds_email_service.rb | 11 ++++++---- .../project_services/builds_email_service_spec.rb | 24 ++++++++++++++++++++-- 3 files changed, 30 insertions(+), 6 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index e0ae6b79751..3febc65da69 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -14,6 +14,7 @@ v 8.7.0 (unreleased) - Add links to CI setup documentation from project settings and builds pages - Handle nil descriptions in Slack issue messages (Stan Hu) - Add default scope to projects to exclude projects pending deletion + - Ensure empty recipients are rejected in BuildsEmailService - Implement 'Groups View' as an option for dashboard preferences !3379 (Elias W.) - Implement 'TODOs View' as an option for dashboard preferences !3379 (Elias W.) - Gracefully handle notes on deleted commits in merge requests (Stan Hu) diff --git a/app/models/project_services/builds_email_service.rb b/app/models/project_services/builds_email_service.rb index f6313255cbb..f9f04838766 100644 --- a/app/models/project_services/builds_email_service.rb +++ b/app/models/project_services/builds_email_service.rb @@ -50,12 +50,15 @@ class BuildsEmailService < Service def execute(push_data) return unless supported_events.include?(push_data[:object_kind]) + return unless should_build_be_notified?(push_data) - if should_build_be_notified?(push_data) + recipients = all_recipients(push_data) + + if recipients.any? BuildEmailWorker.perform_async( push_data[:build_id], - all_recipients(push_data), - push_data, + recipients, + push_data ) end end @@ -84,7 +87,7 @@ class BuildsEmailService < Service end def all_recipients(data) - all_recipients = recipients.split(',') + all_recipients = recipients.split(',').compact.reject(&:blank?) if add_pusher? && data[:user][:email] all_recipients << "#{data[:user][:email]}" diff --git a/spec/models/project_services/builds_email_service_spec.rb b/spec/models/project_services/builds_email_service_spec.rb index 905379a64e3..2ccbff553f0 100644 --- a/spec/models/project_services/builds_email_service_spec.rb +++ b/spec/models/project_services/builds_email_service_spec.rb @@ -6,18 +6,38 @@ describe BuildsEmailService do let(:service) { BuildsEmailService.new } describe :execute do - it "sends email" do + it 'sends email' do service.recipients = 'test@gitlab.com' data[:build_status] = 'failed' expect(BuildEmailWorker).to receive(:perform_async) service.execute(data) end - it "does not sends email with failed build and allowed_failure on" do + it 'does not send email with succeeded build and notify_only_broken_builds on' do + expect(service).to receive(:notify_only_broken_builds).and_return(true) + data[:build_status] = 'success' + expect(BuildEmailWorker).not_to receive(:perform_async) + service.execute(data) + end + + it 'does not send email with failed build and build_allow_failure is true' do data[:build_status] = 'failed' data[:build_allow_failure] = true expect(BuildEmailWorker).not_to receive(:perform_async) service.execute(data) end + + it 'does not send email with unknown build status' do + data[:build_status] = 'foo' + expect(BuildEmailWorker).not_to receive(:perform_async) + service.execute(data) + end + + it 'does not send email when recipients list is empty' do + service.recipients = ' ,, ' + data[:build_status] = 'failed' + expect(BuildEmailWorker).not_to receive(:perform_async) + service.execute(data) + end end end -- cgit v1.2.1