Improve and finish the fallback to the In-Reply-To and References header for the reply-by-email feature2364-fallback-to-in-reply-to-header

A few things to note:
- The IncomingEmail feature is now enabled even without a
  correctly-formatted sub-address
- Message-ID for new thread mail are kept the same so that subsequent
  notifications to this thread are grouped in the thread by the email
  service that receives the notification
  (i.e. In-Reply-To of the answer == Message-ID of the first thread message)
- To maximize our chance to be able to retrieve the reply key, we look
  for it in the In-Reply-To header and the References header
- The pattern for the fallback reply message id is "reply-[key]@[gitlab_host]"
- Improve docs thanks to Axil

2 files changed, 111 insertions, 36 deletions

diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb
index 0f3de33f361..631b5094f42 100644
--- a/spec/mailers/notify_spec.rb
+++ b/spec/mailers/notify_spec.rb
@@ -35,7 +35,9 @@ describe Notify do
           subject { Notify.new_issue_email(issue.assignee_id, issue.id) }
 
           it_behaves_like 'an assignee email'
-          it_behaves_like 'an email starting a new thread', 'issue'
+          it_behaves_like 'an email starting a new thread with reply-by-email enabled' do
+            let(:model) { issue }
+          end
           it_behaves_like 'it should show Gmail Actions View Issue link'
           it_behaves_like 'an unsubscribeable thread'
 
@@ -73,9 +75,11 @@ describe Notify do
           subject { Notify.reassigned_issue_email(recipient.id, issue.id, previous_assignee.id, current_user.id) }
 
           it_behaves_like 'a multiple recipients email'
-          it_behaves_like 'an answer to an existing thread', 'issue'
+          it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do
+            let(:model) { issue }
+          end
           it_behaves_like 'it should show Gmail Actions View Issue link'
-          it_behaves_like "an unsubscribeable thread"
+          it_behaves_like 'an unsubscribeable thread'
 
           it 'is sent as the author' do
             sender = subject.header[:from].addrs[0]
@@ -104,7 +108,9 @@ describe Notify do
           subject { Notify.relabeled_issue_email(recipient.id, issue.id, %w[foo bar baz], current_user.id) }
 
           it_behaves_like 'a multiple recipients email'
-          it_behaves_like 'an answer to an existing thread', 'issue'
+          it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do
+            let(:model) { issue }
+          end
           it_behaves_like 'it should show Gmail Actions View Issue link'
           it_behaves_like 'a user cannot unsubscribe through footer link'
           it_behaves_like 'an email with a labels subscriptions link in its footer'
@@ -132,7 +138,9 @@ describe Notify do
           let(:status) { 'closed' }
           subject { Notify.issue_status_changed_email(recipient.id, issue.id, status, current_user.id) }
 
-          it_behaves_like 'an answer to an existing thread', 'issue'
+          it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do
+            let(:model) { issue }
+          end
           it_behaves_like 'it should show Gmail Actions View Issue link'
           it_behaves_like 'an unsubscribeable thread'
 
@@ -163,7 +171,9 @@ describe Notify do
           let(:new_issue) { create(:issue) }
           subject { Notify.issue_moved_email(recipient, issue, new_issue, current_user) }
 
-          it_behaves_like 'an answer to an existing thread', 'issue'
+          it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do
+            let(:model) { issue }
+          end
           it_behaves_like 'it should show Gmail Actions View Issue link'
           it_behaves_like 'an unsubscribeable thread'
 
@@ -196,9 +206,11 @@ describe Notify do
           subject { Notify.new_merge_request_email(merge_request.assignee_id, merge_request.id) }
 
           it_behaves_like 'an assignee email'
-          it_behaves_like 'an email starting a new thread', 'merge_request'
+          it_behaves_like 'an email starting a new thread with reply-by-email enabled' do
+            let(:model) { merge_request }
+          end
           it_behaves_like 'it should show Gmail Actions View Merge request link'
-          it_behaves_like "an unsubscribeable thread"
+          it_behaves_like 'an unsubscribeable thread'
 
           it 'has the correct subject' do
             is_expected.to have_subject /#{merge_request.title} \(##{merge_request.iid}\)/
@@ -216,14 +228,6 @@ describe Notify do
             is_expected.to have_body_text /#{merge_request.target_branch}/
           end
 
-          it 'has the correct message-id set' do
-            is_expected.to have_header 'Message-ID', /<reply\+(.*)@#{Gitlab.config.gitlab.host}>/
-          end
-
-          it 'has the correct references set' do
-            is_expected.to have_header 'References', "<merge_request_#{merge_request.id}@#{Gitlab.config.gitlab.host}>"
-          end
-
           context 'when enabled email_author_in_body' do
             before do
               allow(current_application_settings).to receive(:email_author_in_body).and_return(true)
@@ -251,7 +255,9 @@ describe Notify do
           subject { Notify.reassigned_merge_request_email(recipient.id, merge_request.id, previous_assignee.id, current_user.id) }
 
           it_behaves_like 'a multiple recipients email'
-          it_behaves_like 'an answer to an existing thread', 'merge_request'
+          it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do
+            let(:model) { merge_request }
+          end
           it_behaves_like 'it should show Gmail Actions View Merge request link'
           it_behaves_like "an unsubscribeable thread"
 
@@ -282,7 +288,9 @@ describe Notify do
           subject { Notify.relabeled_merge_request_email(recipient.id, merge_request.id, %w[foo bar baz], current_user.id) }
 
           it_behaves_like 'a multiple recipients email'
-          it_behaves_like 'an answer to an existing thread', 'merge_request'
+          it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do
+            let(:model) { merge_request }
+          end
           it_behaves_like 'it should show Gmail Actions View Merge request link'
           it_behaves_like 'a user cannot unsubscribe through footer link'
           it_behaves_like 'an email with a labels subscriptions link in its footer'
@@ -310,9 +318,11 @@ describe Notify do
           let(:status) { 'reopened' }
           subject { Notify.merge_request_status_email(recipient.id, merge_request.id, status, current_user.id) }
 
-          it_behaves_like 'an answer to an existing thread', 'merge_request'
+          it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do
+            let(:model) { merge_request }
+          end
           it_behaves_like 'it should show Gmail Actions View Merge request link'
-          it_behaves_like "an unsubscribeable thread"
+          it_behaves_like 'an unsubscribeable thread'
 
           it 'is sent as the author' do
             sender = subject.header[:from].addrs[0]
@@ -341,9 +351,11 @@ describe Notify do
           subject { Notify.merged_merge_request_email(recipient.id, merge_request.id, merge_author.id) }
 
           it_behaves_like 'a multiple recipients email'
-          it_behaves_like 'an answer to an existing thread', 'merge_request'
+          it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do
+            let(:model) { merge_request }
+          end
           it_behaves_like 'it should show Gmail Actions View Merge request link'
-          it_behaves_like "an unsubscribeable thread"
+          it_behaves_like 'an unsubscribeable thread'
 
           it 'is sent as the merge author' do
             sender = subject.header[:from].addrs[0]
@@ -460,9 +472,11 @@ describe Notify do
         subject { Notify.note_commit_email(recipient.id, note.id) }
 
         it_behaves_like 'a note email'
-        it_behaves_like 'an answer to an existing thread', 'commit'
+        it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do
+          let(:model) { commit }
+        end
         it_behaves_like 'it should show Gmail Actions View Commit link'
-        it_behaves_like "a user cannot unsubscribe through footer link"
+        it_behaves_like 'a user cannot unsubscribe through footer link'
 
         it 'has the correct subject' do
           is_expected.to have_subject /#{commit.title} \(#{commit.short_id}\)/
@@ -481,7 +495,9 @@ describe Notify do
         subject { Notify.note_merge_request_email(recipient.id, note.id) }
 
         it_behaves_like 'a note email'
-        it_behaves_like 'an answer to an existing thread', 'merge_request'
+        it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do
+          let(:model) { merge_request }
+        end
         it_behaves_like 'it should show Gmail Actions View Merge request link'
         it_behaves_like 'an unsubscribeable thread'
 
@@ -502,7 +518,9 @@ describe Notify do
         subject { Notify.note_issue_email(recipient.id, note.id) }
 
         it_behaves_like 'a note email'
-        it_behaves_like 'an answer to an existing thread', 'issue'
+        it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do
+          let(:model) { issue }
+        end
         it_behaves_like 'it should show Gmail Actions View Issue link'
         it_behaves_like 'an unsubscribeable thread'
 
diff --git a/spec/mailers/shared/notify.rb b/spec/mailers/shared/notify.rb
index eafc5e5044b..56a6dbf96f9 100644
--- a/spec/mailers/shared/notify.rb
+++ b/spec/mailers/shared/notify.rb
@@ -10,6 +10,13 @@ shared_context 'gitlab email notification' do
     ActionMailer::Base.deliveries.clear
     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}")
+  end
+end
+
+shared_context 'reply-by-email is enabled with incoming address without %{key}' do
+  before do
+    stub_incoming_email_setting(enabled: true, address: "reply@#{Gitlab.config.gitlab.host}")
   end
 end
 
@@ -46,26 +53,76 @@ shared_examples 'an email with X-GitLab headers containing project details' do
   end
 end
 
-shared_examples 'an email starting a new thread' do |message_id_prefix|
-  include_examples 'an email with X-GitLab headers containing project details'
+shared_examples 'a new thread email with reply-by-email enabled' do
+  let(:regex) { /\A<reply\-(.*)@#{Gitlab.config.gitlab.host}>\Z/ }
+
+  it 'has a Message-ID header' do
+    is_expected.to have_header 'Message-ID', "<#{model.class.model_name.singular_route_key}_#{model.id}@#{Gitlab.config.gitlab.host}>"
+  end
 
-  it 'has a discussion identifier' do
-    is_expected.to have_header 'Message-ID',  /<reply\+(.*)@#{Gitlab.config.gitlab.host}>/
-    is_expected.to have_header 'References',  /<#{message_id_prefix}(.*)@#{Gitlab.config.gitlab.host}>/
+  it 'has a References header' do
+    is_expected.to have_header 'References', regex
   end
 end
 
-shared_examples 'an answer to an existing thread' do |thread_id_prefix|
+shared_examples 'a thread answer email with reply-by-email enabled' do
   include_examples 'an email with X-GitLab headers containing project details'
+  let(:regex) { /\A<#{model.class.model_name.singular_route_key}_#{model.id}@#{Gitlab.config.gitlab.host}> <reply\-(.*)@#{Gitlab.config.gitlab.host}>\Z/ }
+
+  it 'has a Message-ID header' do
+    is_expected.to have_header 'Message-ID', /\A<(.*)@#{Gitlab.config.gitlab.host}>\Z/
+  end
+
+  it 'has a In-Reply-To header' do
+    is_expected.to have_header 'In-Reply-To', "<#{model.class.model_name.singular_route_key}_#{model.id}@#{Gitlab.config.gitlab.host}>"
+  end
+
+  it 'has a References header' do
+    is_expected.to have_header 'References', regex
+  end
 
   it 'has a subject that begins with Re: ' do
     is_expected.to have_subject /^Re: /
   end
+end
+
+shared_examples 'an email starting a new thread with reply-by-email enabled' do
+  include_examples 'an email with X-GitLab headers containing project details'
+  include_examples 'a new thread email with reply-by-email enabled'
+
+  context 'when reply-by-email is enabled with incoming address with %{key}' do
+    it 'has a Reply-To header' do
+      is_expected.to have_header 'Reply-To', /<reply+(.*)@#{Gitlab.config.gitlab.host}>\Z/
+    end
+  end
+
+  context 'when reply-by-email is enabled with incoming address without %{key}' do
+    include_context 'reply-by-email is enabled with incoming address without %{key}'
+    include_examples 'a new thread email with reply-by-email enabled'
+
+    it 'has a Reply-To header' do
+      is_expected.to have_header 'Reply-To', /<reply@#{Gitlab.config.gitlab.host}>\Z/
+    end
+  end
+end
+
+shared_examples 'an answer to an existing thread with reply-by-email enabled' do
+  include_examples 'an email with X-GitLab headers containing project details'
+  include_examples 'a thread answer email with reply-by-email enabled'
+
+  context 'when reply-by-email is enabled with incoming address with %{key}' do
+    it 'has a Reply-To header' do
+      is_expected.to have_header 'Reply-To', /<reply+(.*)@#{Gitlab.config.gitlab.host}>\Z/
+    end
+  end
+
+  context 'when reply-by-email is enabled with incoming address without %{key}' do
+    include_context 'reply-by-email is enabled with incoming address without %{key}'
+    include_examples 'a thread answer email with reply-by-email enabled'
 
-  it 'has headers that reference an existing thread' do
-    is_expected.to have_header 'Message-ID',  /<(.*)@#{Gitlab.config.gitlab.host}>/
-    is_expected.to have_header 'References',  /<#{thread_id_prefix}(.*)@#{Gitlab.config.gitlab.host}>/
-    is_expected.to have_header 'In-Reply-To', /<#{thread_id_prefix}(.*)@#{Gitlab.config.gitlab.host}>/
+    it 'has a Reply-To header' do
+      is_expected.to have_header 'Reply-To', /<reply@#{Gitlab.config.gitlab.host}>\Z/
+    end
   end
 end