Merge branch 'revert_email_thread' into 'master'

Revert email threading rewrite

The 'to:' address introduced by this merge caused a flood of email going to an admin account for GitLab.com. This feature should not be shipped as-is.

app/mailers/emails/groups.rb

@@ -4,7 +4,7 @@ module Emails
       @membership = UsersGroup.find(user_group_id)
       @group = @membership.group
       @target_url = group_url(@group)
-      mail(cc: @membership.user.email,
+      mail(to: @membership.user.email,
            subject: subject("Access to group was granted"))
     end
   end

app/mailers/emails/issues.rb

@@ -4,10 +4,10 @@ module Emails
       @issue = Issue.find(issue_id)
       @project = @issue.project
       @target_url = project_issue_url(@project, @issue)
-      mail_new_thread(@issue,
-                      from: sender(@issue.author_id),
-                      cc: recipient(recipient_id),
-                      subject: subject("#{@issue.title} (##{@issue.iid})"))
+      set_message_id("issue_#{issue_id}")
+      mail(from: sender(@issue.author_id),
+           to: recipient(recipient_id),
+           subject: subject("#{@issue.title} (##{@issue.iid})"))
     end
     def reassigned_issue_email(recipient_id, issue_id, previous_assignee_id, updated_by_user_id)
@@ -15,10 +15,10 @@ module Emails
       @previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id
       @project = @issue.project
       @target_url = project_issue_url(@project, @issue)
-      mail_answer_thread(@issue,
-                         from: sender(updated_by_user_id),
-                         cc: recipient(recipient_id),
-                         subject: subject("#{@issue.title} (##{@issue.iid})"))
+      set_reference("issue_#{issue_id}")
+      mail(from: sender(updated_by_user_id),
+           to: recipient(recipient_id),
+           subject: subject("#{@issue.title} (##{@issue.iid})"))
     end
     def closed_issue_email(recipient_id, issue_id, updated_by_user_id)
@@ -26,10 +26,10 @@ module Emails
       @project = @issue.project
       @updated_by = User.find updated_by_user_id
       @target_url = project_issue_url(@project, @issue)
-      mail_answer_thread(@issue,
-                         from: sender(updated_by_user_id),
-                         cc: recipient(recipient_id),
-                         subject: subject("#{@issue.title} (##{@issue.iid})"))
+      set_reference("issue_#{issue_id}")
+      mail(from: sender(updated_by_user_id),
+           to: recipient(recipient_id),
+           subject: subject("#{@issue.title} (##{@issue.iid})"))
     end
     def issue_status_changed_email(recipient_id, issue_id, status, updated_by_user_id)
@@ -38,10 +38,10 @@ module Emails
       @project = @issue.project
       @updated_by = User.find updated_by_user_id
       @target_url = project_issue_url(@project, @issue)
-      mail_answer_thread(@issue,
-                         from: sender(updated_by_user_id),
-                         cc: recipient(recipient_id),
-                         subject: subject("#{@issue.title} (##{@issue.iid})"))
+      set_reference("issue_#{issue_id}")
+      mail(from: sender(updated_by_user_id),
+           to: recipient(recipient_id),
+           subject: subject("#{@issue.title} (##{@issue.iid})"))
     end
   end
 end

app/mailers/emails/merge_requests.rb

@@ -4,10 +4,10 @@ module Emails
       @merge_request = MergeRequest.find(merge_request_id)
       @project = @merge_request.project
       @target_url = project_merge_request_url(@project, @merge_request)
-      mail_new_thread(@merge_request,
-                      from: sender(@merge_request.author_id),
-                      cc: recipient(recipient_id),
-                      subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
+      set_message_id("merge_request_#{merge_request_id}")
+      mail(from: sender(@merge_request.author_id),
+           to: recipient(recipient_id),
+           subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
     end
     def reassigned_merge_request_email(recipient_id, merge_request_id, previous_assignee_id, updated_by_user_id)
@@ -15,10 +15,10 @@ module Emails
       @previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id
       @project = @merge_request.project
       @target_url = project_merge_request_url(@project, @merge_request)
-      mail_answer_thread(@merge_request,
-                         from: sender(updated_by_user_id),
-                         cc: recipient(recipient_id),
-                         subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
+      set_reference("merge_request_#{merge_request_id}")
+      mail(from: sender(updated_by_user_id),
+           to: recipient(recipient_id),
+           subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
     end
     def closed_merge_request_email(recipient_id, merge_request_id, updated_by_user_id)
@@ -26,20 +26,20 @@ module Emails
       @updated_by = User.find updated_by_user_id
       @project = @merge_request.project
       @target_url = project_merge_request_url(@project, @merge_request)
-      mail_answer_thread(@merge_request,
-                         from: sender(updated_by_user_id),
-                         cc: recipient(recipient_id),
-                         subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
+      set_reference("merge_request_#{merge_request_id}")
+      mail(from: sender(updated_by_user_id),
+           to: recipient(recipient_id),
+           subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
     end
     def merged_merge_request_email(recipient_id, merge_request_id, updated_by_user_id)
       @merge_request = MergeRequest.find(merge_request_id)
       @project = @merge_request.project
       @target_url = project_merge_request_url(@project, @merge_request)
-      mail_answer_thread(@merge_request,
-                         from: sender(updated_by_user_id),
-                         cc: recipient(recipient_id),
-                         subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
+      set_reference("merge_request_#{merge_request_id}")
+      mail(from: sender(updated_by_user_id),
+           to: recipient(recipient_id),
+           subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
     end
   end

app/mailers/emails/notes.rb

@@ -5,10 +5,9 @@ module Emails
       @commit = @note.noteable
       @project = @note.project
       @target_url = project_commit_url(@project, @commit, anchor: "note_#{@note.id}")
-      mail_answer_thread(@commit,
-                         from: sender(@note.author_id),
-                         cc: recipient(recipient_id),
-                         subject: subject("#{@commit.title} (#{@commit.short_id})"))
+      mail(from: sender(@note.author_id),
+           to: recipient(recipient_id),
+           subject: subject("#{@commit.title} (#{@commit.short_id})"))
     end
     def note_issue_email(recipient_id, note_id)
@@ -16,10 +15,10 @@ module Emails
       @issue = @note.noteable
       @project = @note.project
       @target_url = project_issue_url(@project, @issue, anchor: "note_#{@note.id}")
-      mail_answer_thread(@issue,
-                         from: sender(@note.author_id),
-                         cc: recipient(recipient_id),
-                         subject: subject("#{@issue.title} (##{@issue.iid})"))
+      set_reference("issue_#{@issue.id}")
+      mail(from: sender(@note.author_id),
+           to: recipient(recipient_id),
+           subject: subject("#{@issue.title} (##{@issue.iid})"))
     end
     def note_merge_request_email(recipient_id, note_id)
@@ -27,10 +26,10 @@ module Emails
       @merge_request = @note.noteable
       @project = @note.project
       @target_url = project_merge_request_url(@project, @merge_request, anchor: "note_#{@note.id}")
-      mail_answer_thread(@merge_request,
-                         from: sender(@note.author_id),
-                         cc: recipient(recipient_id),
-                         subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
+      set_reference("merge_request_#{@merge_request.id}")
+      mail(from: sender(@note.author_id),
+           to: recipient(recipient_id),
+           subject: subject("#{@merge_request.title} (##{@merge_request.iid})"))
     end
   end
 end

app/mailers/emails/projects.rb

@@ -4,7 +4,7 @@ module Emails
       @users_project = UsersProject.find user_project_id
       @project = @users_project.project
       @target_url = project_url(@project)
-      mail(cc: @users_project.user.email,
+      mail(to: @users_project.user.email,
            subject: subject("Access to project was granted"))
     end
@@ -12,7 +12,7 @@ module Emails
       @user = User.find user_id
       @project = Project.find project_id
       @target_url = project_url(@project)
-      mail(cc: @user.email,
+      mail(to: @user.email,
            subject: subject("Project was moved"))
     end
@@ -32,7 +32,7 @@ module Emails
       end
       mail(from: sender(author_id),
-           cc: recipient,
+           to: recipient,
            subject: subject(@subject))
     end
   end

app/mailers/notify.rb

 class Notify < ActionMailer::Base
-  include ActionDispatch::Routing::PolymorphicRoutes
-
   include Emails::Issues
   include Emails::MergeRequests
   include Emails::Notes
@@ -18,7 +16,6 @@ class Notify < ActionMailer::Base
   default_url_options[:script_name] = Gitlab.config.gitlab.relative_url_root
   default from: Proc.new { default_sender_address.format }
-  default to:   Proc.new { project_sender_address.format }
   default reply_to: "noreply@#{Gitlab.config.gitlab.host}"
   # Just send email with 2 seconds delay
@@ -35,17 +32,6 @@ class Notify < ActionMailer::Base
     address
   end
-  # The default email address to send emails to. Includes the project name if possible.
-  def project_sender_address
-    if @project
-      address = default_sender_address
-      address.display_name = @project.name_with_namespace
-      address
-    else
-      default_sender_address
-    end
-  end
-
   # Return an email address that displays the name of the sender.
   # Only the displayed name changes; the actual email address is always the same.
   def sender(sender_id)
@@ -67,6 +53,14 @@ class Notify < ActionMailer::Base
     end
   end
+  # Set the Message-ID header field
+  #
+  # local_part - The local part of the message ID
+  #
+  def set_message_id(local_part)
+    headers["Message-ID"] = "<#{local_part}@#{Gitlab.config.gitlab.host}>"
+  end
+
   # Set the References header field
   #
   # local_part - The local part of the referenced message ID
@@ -99,48 +93,4 @@ class Notify < ActionMailer::Base
     subject << extra.join(' | ') if extra.present?
     subject
   end
-
-  # Return a string suitable for inclusion in the 'Message-Id' mail header.
-  #
-  # The message-id is generated from the unique URL to a model object.
-  def message_id(model)
-    model_name = model.class.model_name.singular_route_key
-    "<#{model_name}_#{model.id}@#{Gitlab.config.gitlab.host}>"
-  end
-
-  # Send an email that starts a new conversation thread,
-  # with headers suitable for grouping by thread in email clients.
-  #
-  # See: mail_answer_thread
-  def mail_new_thread(model, headers = {}, &block)
-    raise ArgumentError, '"To:" header will be overwritten; use "Cc:" or "Bcc:"' unless headers[:to].nil?
-    headers[:to] = project_sender_address.format
-
-    headers['Message-ID'] = message_id(model)
-
-    mail(headers, &block)
-  end
-
-  # Send an email that responds to an existing conversation thread,
-  # with headers suitable for grouping by thread in email clients.
-  #
-  # For grouping emails by thread, email clients heuristics require the answers to:
-  #
-  #  * have a subject that begin by 'Re: '
-  #  * have a 'In-Reply-To' or 'References' header that references the original 'Message-ID'
-  #  * have stable 'From' and 'To' headers between messages of the same thread
-  #
-  def mail_answer_thread(model, headers = {}, &block)
-    raise ArgumentError, '"To:" header will be overwritten; use "Cc:" or "Bcc:"' unless headers[:to].nil?
-    headers[:to] = project_sender_address.format
-
-    headers['In-Reply-To'] = message_id(model)
-    headers['References'] = message_id(model)
-
-    if (headers[:subject])
-      headers[:subject].prepend('Re: ')
-    end
-
-    mail(headers, &block)
-  end
 end

spec/mailers/notify_spec.rb

@@ -10,7 +10,7 @@ describe Notify do
   shared_examples 'a multiple recipients email' do
     it 'is sent to the given recipient' do
-      should cc_to recipient.email
+      should deliver_to recipient.email
     end
   end
@@ -22,23 +22,6 @@ describe Notify do
     end
   end
-  shared_examples 'an email starting a new thread' do |message_id_prefix|
-    it 'has a discussion identifier' do
-      should have_header 'Message-ID',  /<#{message_id_prefix}(.*)@#{Gitlab.config.gitlab.host}>/
-    end
-  end
-
-  shared_examples 'an answer to an existing thread' do |thread_id_prefix|
-    it 'has a subject that begins with Re: ' do
-      should have_subject /^Re: /
-    end
-
-    it 'has headers that reference an existing thread' do
-      should have_header 'References',  /<#{thread_id_prefix}(.*)@#{Gitlab.config.gitlab.host}>/
-      should have_header 'In-Reply-To', /<#{thread_id_prefix}(.*)@#{Gitlab.config.gitlab.host}>/
-    end
-  end
-
   describe 'for new users, the email' do
     let(:example_site_path) { root_path }
     let(:new_user) { create(:user, email: 'newguy@example.com', created_by_id: 1) }
@@ -158,7 +141,7 @@ describe Notify do
         end
         it 'is sent to the assignee' do
-          should cc_to assignee.email
+          should deliver_to assignee.email
         end
       end
@@ -170,7 +153,6 @@ 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 'has the correct subject' do
             should have_subject /#{project.name} \| #{issue.title} \(##{issue.iid}\)/
@@ -179,6 +161,10 @@ describe Notify do
           it 'contains a link to the new issue' do
             should have_body_text /#{project_issue_path project, issue}/
           end
+
+          it 'has the correct message-id set' do
+            should have_header 'Message-ID', "<issue_#{issue.id}@#{Gitlab.config.gitlab.host}>"
+          end
         end
         describe 'that are new with a description' do
@@ -193,7 +179,6 @@ describe Notify do
           subject { Notify.reassigned_issue_email(recipient.id, issue.id, previous_assignee.id, current_user) }
           it_behaves_like 'a multiple recipients email'
-          it_behaves_like 'an answer to an existing thread', 'issue'
           it 'is sent as the author' do
             sender = subject.header[:from].addrs[0]
@@ -216,14 +201,16 @@ describe Notify do
           it 'contains a link to the issue' do
             should have_body_text /#{project_issue_path project, issue}/
           end
+
+          it 'has the correct reference set' do
+            should have_header 'References', "<issue_#{issue.id}@#{Gitlab.config.gitlab.host}>"
+          end
         end
         describe 'status changed' do
           let(:status) { 'closed' }
           subject { Notify.issue_status_changed_email(recipient.id, issue.id, status, current_user) }
-          it_behaves_like 'an answer to an existing thread', 'issue'
-
           it 'is sent as the author' do
             sender = subject.header[:from].addrs[0]
             sender.display_name.should eq(current_user.name)
@@ -245,6 +232,10 @@ describe Notify do
           it 'contains a link to the issue' do
             should have_body_text /#{project_issue_path project, issue}/
           end
+
+          it 'has the correct reference set' do
+            should have_header 'References', "<issue_#{issue.id}@#{Gitlab.config.gitlab.host}>"
+          end
         end
       end
@@ -258,7 +249,6 @@ 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 'has the correct subject' do
             should have_subject /#{merge_request.title} \(##{merge_request.iid}\)/
@@ -293,7 +283,6 @@ 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 'is sent as the author' do
             sender = subject.header[:from].addrs[0]
@@ -322,7 +311,6 @@ 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 'is sent as the merge author' do
             sender = subject.header[:from].addrs[0]
@@ -341,6 +329,10 @@ describe Notify do
           it 'contains a link to the merge request' do
             should have_body_text /#{project_merge_request_path project, merge_request}/
           end
+
+          it 'has the correct reference set' do
+            should have_header 'References', "<merge_request_#{merge_request.id}@#{Gitlab.config.gitlab.host}>"
+          end
         end
       end
     end
@@ -402,7 +394,7 @@ describe Notify do
         end
         it 'is sent to the given recipient' do
-          should cc_to recipient.email
+          should deliver_to recipient.email
         end
         it 'contains the message from the note' do
@@ -418,7 +410,6 @@ 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', 'commits'
         it 'has the correct subject' do
           should have_subject /#{commit.title} \(#{commit.short_id}\)/
@@ -437,7 +428,6 @@ 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 'has the correct subject' do
           should have_subject /#{merge_request.title} \(##{merge_request.iid}\)/
@@ -456,7 +446,6 @@ 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 'has the correct subject' do
           should have_subject /#{issue.title} \(##{issue.iid}\)/
@@ -533,7 +522,7 @@ describe Notify do
     end
     it 'is sent to recipient' do
-      should cc_to 'devs@company.name'
+      should deliver_to 'devs@company.name'
     end
     it 'has the correct subject' do
@@ -569,7 +558,7 @@ describe Notify do
     end
     it 'is sent to recipient' do
-      should cc_to 'devs@company.name'
+      should deliver_to 'devs@company.name'
     end
     it 'has the correct subject' do

spec/services/issues/close_service_spec.rb

@@ -22,7 +22,7 @@ describe Issues::CloseService do
       it 'should send email to user2 about assign of new issue' do
         email = ActionMailer::Base.deliveries.last
-        email.cc.first.should == user2.email
+        email.to.first.should == user2.email
         email.subject.should include(issue.title)
       end

spec/services/issues/update_service_spec.rb

@@ -31,7 +31,7 @@ describe Issues::UpdateService do
       it 'should send email to user2 about assign of new issue' do
         email = ActionMailer::Base.deliveries.last
-        email.cc.first.should == user2.email
+        email.to.first.should == user2.email
         email.subject.should include(issue.title)
       end

spec/services/merge_requests/close_service_spec.rb

@@ -22,7 +22,7 @@ describe MergeRequests::CloseService do
       it 'should send email to user2 about assign of new merge_request' do
         email = ActionMailer::Base.deliveries.last
-        email.cc.first.should == user2.email
+        email.to.first.should == user2.email
         email.subject.should include(merge_request.title)
       end

spec/services/merge_requests/update_service_spec.rb

@@ -31,7 +31,7 @@ describe MergeRequests::UpdateService do
       it 'should send email to user2 about assign of new merge_request' do
         email = ActionMailer::Base.deliveries.last
-        email.cc.first.should == user2.email
+        email.to.first.should == user2.email
         email.subject.should include(merge_request.title)
       end