From 780d868fec936327cbdf688cf6e2571b07d780c9 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 18 Jun 2014 13:41:12 +0200 Subject: [PATCH] Revert "Merge branch 'fix-email-threading' into 'master'" This reverts commit 97fd990ecde387290be269ef7daafa5761f94af6, reversing changes made to f451a697e0c018359e6d8ff3aaba4eb0484c4bee. Conflicts: app/mailers/emails/notes.rb app/mailers/emails/projects.rb --- app/mailers/emails/groups.rb | 2 +- app/mailers/emails/issues.rb | 32 ++++----- app/mailers/emails/merge_requests.rb | 32 ++++----- app/mailers/emails/notes.rb | 23 ++++--- app/mailers/emails/projects.rb | 6 +- app/mailers/notify.rb | 66 +++---------------- spec/mailers/notify_spec.rb | 53 ++++++--------- spec/services/issues/close_service_spec.rb | 2 +- spec/services/issues/update_service_spec.rb | 2 +- .../merge_requests/close_service_spec.rb | 2 +- .../merge_requests/update_service_spec.rb | 2 +- 11 files changed, 80 insertions(+), 142 deletions(-) diff --git a/app/mailers/emails/groups.rb b/app/mailers/emails/groups.rb index 39527b00e9f..1654fc55bca 100644 --- a/app/mailers/emails/groups.rb +++ b/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 diff --git a/app/mailers/emails/issues.rb b/app/mailers/emails/issues.rb index 516e3da84c8..a096df9dc0d 100644 --- a/app/mailers/emails/issues.rb +++ b/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 diff --git a/app/mailers/emails/merge_requests.rb b/app/mailers/emails/merge_requests.rb index c4ca3a3b69b..ea5671c4502 100644 --- a/app/mailers/emails/merge_requests.rb +++ b/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 diff --git a/app/mailers/emails/notes.rb b/app/mailers/emails/notes.rb index acc6d03b5e4..b7fedbd3707 100644 --- a/app/mailers/emails/notes.rb +++ b/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 diff --git a/app/mailers/emails/projects.rb b/app/mailers/emails/projects.rb index 64878568ef8..6b13a1d746d 100644 --- a/app/mailers/emails/projects.rb +++ b/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 diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb index d65bead9cfe..84a0da0129d 100644 --- a/app/mailers/notify.rb +++ b/app/mailers/notify.rb @@ -1,6 +1,4 @@ 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 diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index ab26b290d77..224b613b477 100644 --- a/spec/mailers/notify_spec.rb +++ b/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', "" + 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', "" + 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', "" + 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', "" + 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 diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index 7324650d534..d4f2cc1339b 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/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 diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 47d18565034..347560414e7 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/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 diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb index 2c5a7c4f26c..a504f916b08 100644 --- a/spec/services/merge_requests/close_service_spec.rb +++ b/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 diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 42ac05862b3..af5d3a3dc81 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/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