Merge branch 'buildsemailservice-should-filter-out-blank-email-addresses-14883' into 'master'

Ensure empty recipients are rejected in BuildsEmailService

Fixes #14883.

See merge request !3543

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