Merge branch '2364-fallback-to-in-reply-to-header' into 'master'

Fall back to In-Reply-To and References headers when sub-addressing is not available

_Originally opened at !3024 by @dabit._

- - -

Fixes #2364 

Summary of the changes:
- No more need to have the `%{key}` placeholder in the `incoming_email.address`
- The fallback message id format is `reply-[key]@[gitlab_host]` (reminder: it doesn't have to be a real email address)
- The fallback message id that includes the reply key is added to both `References` header
- Documentation for the "Reply by email" feature updated

See merge request !3305

6 files changed, 187 insertions, 50 deletions

diff --git a/spec/fixtures/emails/reply_without_subaddressing_and_key_inside_references.eml b/spec/fixtures/emails/reply_without_subaddressing_and_key_inside_references.eml
new file mode 100644
index 00000000000..39d5cefbc2a
--- /dev/null
+++ b/spec/fixtures/emails/reply_without_subaddressing_and_key_inside_references.eml
@@ -0,0 +1,42 @@
+Return-Path: <jake@adventuretime.ooo>
+Received: from iceking.adventuretime.ooo ([unix socket]) by iceking (Cyrus v2.2.13-Debian-2.2.13-19+squeeze3) with LMTPA; Thu, 13 Jun 2013 17:03:50 -0400
+Received: from mail-ie0-x234.google.com (mail-ie0-x234.google.com [IPv6:2607:f8b0:4001:c03::234]) by iceking.adventuretime.ooo (8.14.3/8.14.3/Debian-9.4) with ESMTP id r5DL3nFJ016967 (version=TLSv1/SSLv3 cipher=RC4-SHA bits=128 verify=NOT) for <reply+59d8df8370b7e95c5a49fbf86aeb2c93@appmail.adventuretime.ooo>; Thu, 13 Jun 2013 17:03:50 -0400
+Received: by mail-ie0-f180.google.com with SMTP id f4so21977375iea.25 for <reply@appmail.adventuretime.ooo>; Thu, 13 Jun 2013 14:03:48 -0700
+Received: by 10.0.0.1 with HTTP; Thu, 13 Jun 2013 14:03:48 -0700
+Date: Thu, 13 Jun 2013 17:03:48 -0400
+From: Jake the Dog <jake@adventuretime.ooo>
+To: reply@appmail.adventuretime.ooo
+Message-ID: <CADkmRc+rNGAGGbV2iE5p918UVy4UyJqVcXRO2=otppgzduJSg@mail.gmail.com>
+In-Reply-To: <issue_1@localhost>
+References: <issue_1@localhost> <reply-59d8df8370b7e95c5a49fbf86aeb2c93@localhost>
+Subject: re: [Discourse Meta] eviltrout posted in 'Adventure Time Sux'
+Mime-Version: 1.0
+Content-Type: text/plain;
+ charset=ISO-8859-1
+Content-Transfer-Encoding: 7bit
+X-Sieve: CMU Sieve 2.2
+X-Received: by 10.0.0.1 with SMTP id n7mr11234144ipb.85.1371157428600; Thu,
+ 13 Jun 2013 14:03:48 -0700 (PDT)
+X-Scanned-By: MIMEDefang 2.69 on IPv6:2001:470:1d:165::1
+
+I could not disagree more. I am obviously biased but adventure time is the
+greatest show ever created. Everyone should watch it.
+
+- Jake out
+
+
+On Sun, Jun 9, 2013 at 1:39 PM, eviltrout via Discourse Meta
+<reply+59d8df8370b7e95c5a49fbf86aeb2c93@appmail.adventuretime.ooo> wrote:
+>
+>
+>
+> eviltrout posted in 'Adventure Time Sux' on Discourse Meta:
+>
+> ---
+> hey guys everyone knows adventure time sucks!
+>
+> ---
+> Please visit this link to respond: http://localhost:3000/t/adventure-time-sux/1234/3
+>
+> To unsubscribe from these emails, visit your [user preferences](http://localhost:3000/user_preferences).
+>
diff --git a/spec/fixtures/emails/valid_reply.eml b/spec/fixtures/emails/valid_reply.eml
index 1e696389954..980e10a8812 100644
--- a/spec/fixtures/emails/valid_reply.eml
+++ b/spec/fixtures/emails/valid_reply.eml
@@ -7,6 +7,8 @@ Date: Thu, 13 Jun 2013 17:03:48 -0400
 From: Jake the Dog <jake@adventuretime.ooo>
 To: reply+59d8df8370b7e95c5a49fbf86aeb2c93@appmail.adventuretime.ooo
 Message-ID: <CADkmRc+rNGAGGbV2iE5p918UVy4UyJqVcXRO2=otppgzduJSg@mail.gmail.com>
+In-Reply-To: <issue_1@localhost>
+References: <issue_1@localhost> <reply-59d8df8370b7e95c5a49fbf86aeb2c93@localhost>
 Subject: re: [Discourse Meta] eviltrout posted in 'Adventure Time Sux'
 Mime-Version: 1.0
 Content-Type: text/plain;
@@ -37,4 +39,4 @@ On Sun, Jun 9, 2013 at 1:39 PM, eviltrout via Discourse Meta
 > Please visit this link to respond: http://localhost:3000/t/adventure-time-sux/1234/3
 >
 > To unsubscribe from these emails, visit your [user preferences](http://localhost:3000/user_preferences).
->
\ No newline at end of file
+>
diff --git a/spec/lib/gitlab/email/receiver_spec.rb b/spec/lib/gitlab/email/receiver_spec.rb
index abe179cd4af..36267faeb93 100644
--- a/spec/lib/gitlab/email/receiver_spec.rb
+++ b/spec/lib/gitlab/email/receiver_spec.rb
@@ -3,6 +3,7 @@ require "spec_helper"
 describe Gitlab::Email::Receiver, lib: true do
   before do
     stub_incoming_email_setting(enabled: true, address: "reply+%{key}@appmail.adventuretime.ooo")
+    stub_config_setting(host: 'localhost')
   end
 
   let(:reply_key) { "59d8df8370b7e95c5a49fbf86aeb2c93" }
@@ -137,5 +138,27 @@ describe Gitlab::Email::Receiver, lib: true do
 
       expect(note.note).to include(markdown)
     end
+
+    context 'when sub-addressing is not supported' do
+      before do
+        stub_incoming_email_setting(enabled: true, address: nil)
+      end
+
+      shared_examples 'an email that contains a reply key' do |header|
+        it "fetches the reply key from the #{header} header and creates a comment" do
+          expect { receiver.execute }.to change { noteable.notes.count }.by(1)
+          note = noteable.notes.last
+
+          expect(note.author).to eq(sent_notification.recipient)
+          expect(note.note).to include('I could not disagree more.')
+        end
+      end
+
+      context 'reply key is in the References header' do
+        let(:email_raw) { fixture_file('emails/reply_without_subaddressing_and_key_inside_references.eml') }
+
+        it_behaves_like 'an email that contains a reply key', 'References'
+      end
+    end
   end
 end
diff --git a/spec/lib/gitlab/incoming_email_spec.rb b/spec/lib/gitlab/incoming_email_spec.rb
index bcdba8d4c12..afb3e26f8fb 100644
--- a/spec/lib/gitlab/incoming_email_spec.rb
+++ b/spec/lib/gitlab/incoming_email_spec.rb
@@ -7,24 +7,8 @@ describe Gitlab::IncomingEmail, lib: true do
         stub_incoming_email_setting(enabled: true)
       end
 
-      context "when the address is valid" do
-        before do
-          stub_incoming_email_setting(address: "replies+%{key}@example.com")
-        end
-
-        it "returns true" do
-          expect(described_class.enabled?).to be_truthy
-        end
-      end
-
-      context "when the address is invalid" do
-        before do
-          stub_incoming_email_setting(address: "replies@example.com")
-        end
-
-        it "returns false" do
-          expect(described_class.enabled?).to be_falsey
-        end
+      it 'returns true' do
+        expect(described_class.enabled?).to be_truthy
       end
     end
 
@@ -58,4 +42,10 @@ describe Gitlab::IncomingEmail, lib: true do
       expect(described_class.key_from_address("replies+key@example.com")).to eq("key")
     end
   end
+
+  context 'self.key_from_fallback_reply_message_id' do
+    it 'returns reply key' do
+      expect(described_class.key_from_fallback_reply_message_id('reply-key@localhost')).to eq('key')
+    end
+  end
 end
diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb
index 9b47acfe0cd..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,10 +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', "<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)
@@ -247,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"
 
@@ -278,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'
@@ -306,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]
@@ -337,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]
@@ -456,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}\)/
@@ -477,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'
 
@@ -498,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 6019af544d3..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,25 +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',  /<#{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