Commit e79b867d authored by Rémy Coutable's avatar Rémy Coutable

Ensure empty recipients are rejected in BuildsEmailService

Signed-off-by: Rémy Coutable's avatarRémy Coutable <remy@rymai.me>
parent 1749bd3b
Pipeline #1494925 passed with stage
......@@ -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)
......
......@@ -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]}"
......
......@@ -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
  • @rymai If recipients can't be empty, how can I enable sending mails only to the committer? Want to use "Add pusher" standalone.

    Edited by Daniel Haß
  • @danielhass This will still work, I didn't change that if you look at the diff closely. ;) Though, there is no spec for that (that's bad!)!

  • @rymai Thanks for the fast reply! If I check active with no recipients in the list I get this error: "Recipients can't be blank" and settings are not saved (v8.6.4). What to do for standalone "Add pusher"?

  • @danielhass Hmm, this is not an issue I addressed in this commit. Please create a new issue in the issue tracker if none already exist, thanks in advance!

    Edited by Rémy Coutable
  • @rymai aahh.. see what you mean. Just read the heading at thought it would fit :D I'll open an issue. Thanks for your time!

  • Just for reference and anyone else facing this problem: found matching issue #13574 (closed)

  • @danielhass Thanks for not having created a duplicate! :)

Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment