Merge branch 'handle-smtp-input-errors' into 'master'

Gracefully handle SMTP user input errors (e.g. incorrect email addresses) to prevent Sidekiq retries

### What does this MR do?

This MR gracefully handles SMTP input errors (e.g. incorrect or invalid e-mail addresses) to prevent these types of exceptions from causing Sidekiq to retry the task. If these specific exceptions occur, they will be logged, and the e-mail will be dropped from the queue.

### Why was this MR needed?

If you include an author that has a misspelled e-mail address, Sidekiq will keep sending e-mail to all the recipients even if they have already received the e-mail. The only way to recover is to clear the Sidekiq queue.

Note that other exceptions can still be thrown (e.g. `IOError`, `Net::SMTPAuthenticationError`, `Net::SMTPServerBusy`, `Net::SMTPUnknownError`, and `TimeoutError`). If the worker encounters these, Sidekiq should retry the task.

### What are the relevant issue numbers?

Closes https://github.com/gitlabhq/gitlabhq/issues/9560

See merge request !1163

3 files changed, 54 insertions, 14 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 9fddcf82783..35b5803d341 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -13,6 +13,7 @@ v 8.0.0 (unreleased)
   - Search for comments should be case insensetive
   - Create cross-reference for closing references on commits pushed to non-default branches (Maël Valais)
   - Ability to search milestones
+  - Gracefully handle SMTP user input errors (e.g. incorrect email addresses) to prevent Sidekiq retries (Stan Hu)
 
 v 7.14.1 (unreleased)
   - Only include base URL in OmniAuth full_host parameter (Stan Hu)
diff --git a/app/workers/emails_on_push_worker.rb b/app/workers/emails_on_push_worker.rb
index 1d21addece6..916a99bb273 100644
--- a/app/workers/emails_on_push_worker.rb
+++ b/app/workers/emails_on_push_worker.rb
@@ -4,7 +4,7 @@ class EmailsOnPushWorker
   def perform(project_id, recipients, push_data, options = {})
     options.symbolize_keys!
     options.reverse_merge!(
-      send_from_committer_email:  false, 
+      send_from_committer_email:  false,
       disable_diffs:              false
     )
     send_from_committer_email = options[:send_from_committer_email]
@@ -16,9 +16,9 @@ class EmailsOnPushWorker
     ref = push_data["ref"]
     author_id = push_data["user_id"]
 
-    action = 
+    action =
       if Gitlab::Git.blank_ref?(before_sha)
-        :create 
+        :create
       elsif Gitlab::Git.blank_ref?(after_sha)
         :delete
       else
@@ -42,17 +42,22 @@ class EmailsOnPushWorker
     end
 
     recipients.split(" ").each do |recipient|
-      Notify.repository_push_email(
-        project_id, 
-        recipient, 
-        author_id:                  author_id, 
-        ref:                        ref, 
-        action:                     action,
-        compare:                    compare, 
-        reverse_compare:            reverse_compare,
-        send_from_committer_email:  send_from_committer_email,
-        disable_diffs:              disable_diffs
-      ).deliver
+      begin
+        Notify.repository_push_email(
+          project_id,
+          recipient,
+          author_id:                  author_id,
+          ref:                        ref,
+          action:                     action,
+          compare:                    compare,
+          reverse_compare:            reverse_compare,
+          send_from_committer_email:  send_from_committer_email,
+          disable_diffs:              disable_diffs
+        ).deliver
+      # These are input errors and won't be corrected even if Sidekiq retries
+      rescue Net::SMTPFatalError, Net::SMTPSyntaxError => e
+        logger.info("Failed to send e-mail for project '#{project.name_with_namespace}' to #{recipient}: #{e}")
+      end
     end
   ensure
     compare = nil
diff --git a/spec/workers/emails_on_push_worker_spec.rb b/spec/workers/emails_on_push_worker_spec.rb
new file mode 100644
index 00000000000..3600c771075
--- /dev/null
+++ b/spec/workers/emails_on_push_worker_spec.rb
@@ -0,0 +1,34 @@
+require 'spec_helper'
+
+describe EmailsOnPushWorker do
+  include RepoHelpers
+
+  let(:project) { create(:project) }
+  let(:user) { create(:user) }
+  let(:data) { Gitlab::PushDataBuilder.build_sample(project, user) }
+
+  subject { EmailsOnPushWorker.new }
+
+  before do
+    allow(Project).to receive(:find).and_return(project)
+  end
+
+  describe "#perform" do
+    it "sends mail" do
+      subject.perform(project.id, user.email, data.stringify_keys)
+
+      email = ActionMailer::Base.deliveries.last
+      expect(email.subject).to include('Change some files')
+      expect(email.to).to eq([user.email])
+    end
+
+    it "gracefully handles an input SMTP error" do
+      ActionMailer::Base.deliveries.clear
+      allow(Notify).to receive(:repository_push_email).and_raise(Net::SMTPFatalError)
+
+      subject.perform(project.id, user.email, data.stringify_keys)
+
+      expect(ActionMailer::Base.deliveries.count).to eq(0)
+    end
+  end
+end