diff --git a/app/mailers/emails/issues.rb b/app/mailers/emails/issues.rb index b2b4b83d6c..3adb47dc5b 100644 --- a/app/mailers/emails/issues.rb +++ b/app/mailers/emails/issues.rb @@ -3,22 +3,27 @@ module Emails def new_issue_email(recipient_id, issue_id) @issue = Issue.find(issue_id) @project = @issue.project - mail(to: recipient(recipient_id), subject: subject("New issue ##{@issue.iid}", @issue.title)) + 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) + def reassigned_issue_email(recipient_id, issue_id, previous_assignee_id, updated_by_user_id) @issue = Issue.find(issue_id) @previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id @project = @issue.project - mail(to: recipient(recipient_id), subject: subject("Changed issue ##{@issue.iid}", @issue.title)) + 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) @issue = Issue.find issue_id @project = @issue.project @updated_by = User.find updated_by_user_id - mail(to: recipient(recipient_id), - subject: subject("Closed issue ##{@issue.iid}", @issue.title)) + 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) @@ -26,8 +31,9 @@ module Emails @issue_status = status @project = @issue.project @updated_by = User.find updated_by_user_id - mail(to: recipient(recipient_id), - subject: subject("Changed issue ##{@issue.iid}", @issue.title)) + 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 e60887d525..0845e14edc 100644 --- a/app/mailers/emails/merge_requests.rb +++ b/app/mailers/emails/merge_requests.rb @@ -3,27 +3,35 @@ module Emails def new_merge_request_email(recipient_id, merge_request_id) @merge_request = MergeRequest.find(merge_request_id) @project = @merge_request.project - mail(to: recipient(recipient_id), subject: subject("New merge request ##{@merge_request.iid}", @merge_request.title)) + 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) + def reassigned_merge_request_email(recipient_id, merge_request_id, previous_assignee_id, updated_by_user_id) @merge_request = MergeRequest.find(merge_request_id) @previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id @project = @merge_request.project - mail(to: recipient(recipient_id), subject: subject("Changed merge request ##{@merge_request.iid}", @merge_request.title)) + 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) @merge_request = MergeRequest.find(merge_request_id) @updated_by = User.find updated_by_user_id @project = @merge_request.project - mail(to: recipient(recipient_id), subject: subject("Closed merge request ##{@merge_request.iid}", @merge_request.title)) + 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) @merge_request = MergeRequest.find(merge_request_id) @project = @merge_request.project - mail(to: recipient(recipient_id), subject: subject("Accepted merge request ##{@merge_request.iid}", @merge_request.title)) + mail(from: sender(@merge_request.author_id_of_changes), + to: recipient(recipient_id), + subject: subject("#{@merge_request.title} (!#{@merge_request.iid})")) end end @@ -57,7 +65,7 @@ module Emails # >> subject('Lorem ipsum', 'Dolor sit amet') # => "GitLab Merge Request | Lorem ipsum | Dolor sit amet" def subject(*extra) - subject = "GitLab Merge Request |" + subject = "Merge Request | " if @merge_request.for_fork? subject << "#{@merge_request.source_project.name_with_namespace}:#{merge_request.source_branch} >> #{@merge_request.target_project.name_with_namespace}:#{merge_request.target_branch}" else diff --git a/app/mailers/emails/notes.rb b/app/mailers/emails/notes.rb index e967cf6dc7..00b127da42 100644 --- a/app/mailers/emails/notes.rb +++ b/app/mailers/emails/notes.rb @@ -4,27 +4,35 @@ module Emails @note = Note.find(note_id) @commit = @note.noteable @project = @note.project - mail(to: recipient(recipient_id), subject: subject("Note for commit #{@commit.short_id}", @commit.title)) + 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) @note = Note.find(note_id) @issue = @note.noteable @project = @note.project - mail(to: recipient(recipient_id), subject: subject("Note for issue ##{@issue.iid}")) + 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) @note = Note.find(note_id) @merge_request = @note.noteable @project = @note.project - mail(to: recipient(recipient_id), subject: subject("Note for merge request ##{@merge_request.iid}")) + mail(from: sender(@note.author_id), + to: recipient(recipient_id), + subject: subject("#{@merge_request.title} (!#{@merge_request.iid})")) end def note_wall_email(recipient_id, note_id) @note = Note.find(note_id) @project = @note.project - mail(to: recipient(recipient_id), subject: subject("Note on wall")) + mail(from: sender(@note.author_id), + to: recipient(recipient_id), + subject: subject("Note on wall")) end end end diff --git a/app/mailers/emails/projects.rb b/app/mailers/emails/projects.rb index 428d74d83c..46f24e9fb7 100644 --- a/app/mailers/emails/projects.rb +++ b/app/mailers/emails/projects.rb @@ -22,7 +22,9 @@ module Emails @diffs = compare.diffs @branch = branch - mail(to: recipient, subject: subject("New push to repository")) + mail(from: sender(author_id), + to: recipient, + subject: subject("New push to repository")) end end end diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb index 6c1a332896..554f53cf14 100644 --- a/app/mailers/notify.rb +++ b/app/mailers/notify.rb @@ -15,16 +15,33 @@ class Notify < ActionMailer::Base default_url_options[:port] = Gitlab.config.gitlab.port unless Gitlab.config.gitlab_on_standard_port? default_url_options[:script_name] = Gitlab.config.gitlab.relative_url_root - default from: Gitlab.config.gitlab.email_from + default from: Proc.new { default_sender_address.format } default reply_to: "noreply@#{Gitlab.config.gitlab.host}" - # Just send email with 3 seconds delay + # Just send email with 2 seconds delay def self.delay delay_for(2.seconds) end private + # The default email address to send emails from + def default_sender_address + address = Mail::Address.new(Gitlab.config.gitlab.email_from) + address.display_name = "GitLab" + address + 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) + if sender = User.find(sender_id) + address = default_sender_address + address.display_name = sender.name + address.format + end + end + # Look up a User by their ID and return their email address # # recipient_id - User ID @@ -43,21 +60,21 @@ class Notify < ActionMailer::Base # Examples # # >> subject('Lorem ipsum') - # => "GitLab | Lorem ipsum" + # => "Lorem ipsum" # # # Automatically inserts Project name when @project is set # >> @project = Project.last # => # # >> subject('Lorem ipsum') - # => "GitLab | Ruby on Rails | Lorem ipsum " + # => "Ruby on Rails | Lorem ipsum " # # # Accepts multiple arguments # >> subject('Lorem ipsum', 'Dolor sit amet') - # => "GitLab | Lorem ipsum | Dolor sit amet" + # => "Lorem ipsum | Dolor sit amet" def subject(*extra) - subject = "GitLab" - subject << (@project ? " | #{@project.name_with_namespace}" : "") - subject << " | " + extra.join(' | ') if extra.present? + subject = "" + subject << "#{@project.name} | " if @project + subject << extra.join(' | ') if extra.present? subject end end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 9d7bb9639a..5daf573630 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -257,7 +257,7 @@ class NotificationService recipients.delete(current_user) recipients.each do |recipient| - mailer.send(method, recipient.id, target.id, target.assignee_id_was) + mailer.send(method, recipient.id, target.id, target.assignee_id_was, current_user.id) end end diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index a02bf9d4ae..50669ece7a 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -4,7 +4,7 @@ Devise.setup do |config| # ==> Mailer Configuration # Configure the e-mail address which will be shown in Devise::Mailer, # note that it will be overwritten if you use your own mailer class with default "from" parameter. - config.mailer_sender = Gitlab.config.gitlab.email_from + config.mailer_sender = "GitLab <#{Gitlab.config.gitlab.email_from}>" # Configure the class responsible to send e-mails. diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 88cae0bb75..26b7ca876f 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -4,6 +4,7 @@ describe Notify do include EmailSpec::Helpers include EmailSpec::Matchers + let(:gitlab_sender) { Gitlab.config.gitlab.email_from } let(:recipient) { create(:user, email: 'recipient@example.com') } let(:project) { create(:project) } @@ -13,18 +14,28 @@ describe Notify do end end + shared_examples 'an email sent from GitLab' do + it 'is sent from GitLab' do + sender = subject.header[:from].addrs[0] + sender.display_name.should eq('GitLab') + sender.address.should eq(gitlab_sender) + 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) } subject { Notify.new_user_email(new_user.id, new_user.password) } + it_behaves_like 'an email sent from GitLab' + it 'is sent to the new user' do should deliver_to new_user.email end it 'has the correct subject' do - should have_subject /^gitlab \| Account was created for you$/i + should have_subject /^Account was created for you$/i end it 'contains the new user\'s login name' do @@ -47,12 +58,14 @@ describe Notify do subject { Notify.new_user_email(new_user.id, new_user.password) } + it_behaves_like 'an email sent from GitLab' + it 'is sent to the new user' do should deliver_to new_user.email end it 'has the correct subject' do - should have_subject /^gitlab \| Account was created for you$/i + should have_subject /^Account was created for you$/i end it 'contains the new user\'s login name' do @@ -73,12 +86,14 @@ describe Notify do subject { Notify.new_ssh_key_email(key.id) } + it_behaves_like 'an email sent from GitLab' + it 'is sent to the new user' do should deliver_to key.user.email end it 'has the correct subject' do - should have_subject /^gitlab \| SSH key was added to your account$/i + should have_subject /^SSH key was added to your account$/i end it 'contains the new ssh key title' do @@ -114,17 +129,24 @@ describe Notify do context 'for a project' do describe 'items that are assignable, the email' do + let(:current_user) { create(:user, email: "current@email.com") } let(:assignee) { create(:user, email: 'assignee@example.com') } let(:previous_assignee) { create(:user, name: 'Previous Assignee') } shared_examples 'an assignee email' do + it 'is sent as the author' do + sender = subject.header[:from].addrs[0] + sender.display_name.should eq(current_user.name) + sender.address.should eq(gitlab_sender) + end + it 'is sent to the assignee' do should deliver_to assignee.email end end context 'for issues' do - let(:issue) { create(:issue, assignee: assignee, project: project ) } + let(:issue) { create(:issue, author: current_user, assignee: assignee, project: project ) } describe 'that are new' do subject { Notify.new_issue_email(issue.assignee_id, issue.id) } @@ -132,7 +154,7 @@ describe Notify do it_behaves_like 'an assignee email' it 'has the correct subject' do - should have_subject /#{project.name} \| New issue ##{issue.iid} \| #{issue.title}/ + should have_subject /#{project.name} \| #{issue.title} \(##{issue.iid}\)/ end it 'contains a link to the new issue' do @@ -141,14 +163,18 @@ describe Notify do end describe 'that have been reassigned' do - before(:each) { issue.stub(:assignee_id_was).and_return(previous_assignee.id) } - - subject { Notify.reassigned_issue_email(recipient.id, issue.id, previous_assignee.id) } + subject { Notify.reassigned_issue_email(recipient.id, issue.id, previous_assignee.id, current_user) } it_behaves_like 'a multiple recipients email' + it 'is sent as the author' do + sender = subject.header[:from].addrs[0] + sender.display_name.should eq(current_user.name) + sender.address.should eq(gitlab_sender) + end + it 'has the correct subject' do - should have_subject /Changed issue ##{issue.iid} \| #{issue.title}/ + should have_subject /#{issue.title} \(##{issue.iid}\)/ end it 'contains the name of the previous assignee' do @@ -165,12 +191,17 @@ describe Notify do end describe 'status changed' do - let(:current_user) { create(:user, email: "current@email.com") } let(:status) { 'closed' } subject { Notify.issue_status_changed_email(recipient.id, issue.id, status, current_user) } + it 'is sent as the author' do + sender = subject.header[:from].addrs[0] + sender.display_name.should eq(current_user.name) + sender.address.should eq(gitlab_sender) + end + it 'has the correct subject' do - should have_subject /Changed issue ##{issue.iid} \| #{issue.title}/i + should have_subject /#{issue.title} \(##{issue.iid}\)/i end it 'contains the new status' do @@ -189,7 +220,7 @@ describe Notify do end context 'for merge requests' do - let(:merge_request) { create(:merge_request, assignee: assignee, source_project: project, target_project: project) } + let(:merge_request) { create(:merge_request, author: current_user, assignee: assignee, source_project: project, target_project: project) } describe 'that are new' do subject { Notify.new_merge_request_email(merge_request.assignee_id, merge_request.id) } @@ -197,7 +228,7 @@ describe Notify do it_behaves_like 'an assignee email' it 'has the correct subject' do - should have_subject /New merge request ##{merge_request.iid}/ + should have_subject /#{merge_request.title} \(!#{merge_request.iid}\)/ end it 'contains a link to the new merge request' do @@ -214,14 +245,18 @@ describe Notify do end describe 'that are reassigned' do - before(:each) { merge_request.stub(:assignee_id_was).and_return(previous_assignee.id) } - - subject { Notify.reassigned_merge_request_email(recipient.id, merge_request.id, previous_assignee.id) } + 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 'is sent as the author' do + sender = subject.header[:from].addrs[0] + sender.display_name.should eq(current_user.name) + sender.address.should eq(gitlab_sender) + end + it 'has the correct subject' do - should have_subject /Changed merge request ##{merge_request.iid}/ + should have_subject /#{merge_request.title} \(!#{merge_request.iid}\)/ end it 'contains the name of the previous assignee' do @@ -245,6 +280,8 @@ describe Notify do let(:user) { create(:user) } subject { Notify.project_was_moved_email(project.id, user.id) } + it_behaves_like 'an email sent from GitLab' + it 'has the correct subject' do should have_subject /Project was moved/ end @@ -265,6 +302,9 @@ describe Notify do project: project, user: user) } subject { Notify.project_access_granted_email(users_project.id) } + + it_behaves_like 'an email sent from GitLab' + it 'has the correct subject' do should have_subject /Access to project was granted/ end @@ -285,6 +325,12 @@ describe Notify do end shared_examples 'a note email' do + it 'is sent as the author' do + sender = subject.header[:from].addrs[0] + sender.display_name.should eq(note_author.name) + sender.address.should eq(gitlab_sender) + end + it 'is sent to the given recipient' do should deliver_to recipient.email end @@ -324,7 +370,7 @@ describe Notify do it_behaves_like 'a note email' it 'has the correct subject' do - should have_subject /Note for commit #{commit.short_id}/ + should have_subject /#{commit.title} \(#{commit.short_id}\)/ end it 'contains a link to the commit' do @@ -342,7 +388,7 @@ describe Notify do it_behaves_like 'a note email' it 'has the correct subject' do - should have_subject /Note for merge request ##{merge_request.iid}/ + should have_subject /#{merge_request.title} \(!#{merge_request.iid}\)/ end it 'contains a link to the merge request note' do @@ -360,7 +406,7 @@ describe Notify do it_behaves_like 'a note email' it 'has the correct subject' do - should have_subject /Note for issue ##{issue.iid}/ + should have_subject /#{issue.title} \(##{issue.iid}\)/ end it 'contains a link to the issue note' do @@ -377,6 +423,8 @@ describe Notify do subject { Notify.group_access_granted_email(membership.id) } + it_behaves_like 'an email sent from GitLab' + it 'has the correct subject' do should have_subject /Access to group was granted/ end @@ -401,6 +449,8 @@ describe Notify do subject { ActionMailer::Base.deliveries.last } + it_behaves_like 'an email sent from GitLab' + it 'is sent to the new user' do should deliver_to 'new-email@mail.com' end @@ -421,6 +471,12 @@ describe Notify do subject { Notify.repository_push_email(project.id, 'devs@company.name', user.id, 'master', compare) } + it 'is sent as the author' do + sender = subject.header[:from].addrs[0] + sender.display_name.should eq(user.name) + sender.address.should eq(gitlab_sender) + end + it 'is sent to recipient' do should deliver_to 'devs@company.name' end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index e378be0425..077ad8b6e1 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -137,11 +137,11 @@ describe NotificationService do end def should_email(user_id) - Notify.should_receive(:reassigned_issue_email).with(user_id, issue.id, issue.assignee_id) + Notify.should_receive(:reassigned_issue_email).with(user_id, issue.id, issue.assignee_id, @u_disabled.id) end def should_not_email(user_id) - Notify.should_not_receive(:reassigned_issue_email).with(user_id, issue.id, issue.assignee_id) + Notify.should_not_receive(:reassigned_issue_email).with(user_id, issue.id, issue.assignee_id, @u_disabled.id) end end @@ -201,11 +201,11 @@ describe NotificationService do end def should_email(user_id) - Notify.should_receive(:reassigned_merge_request_email).with(user_id, merge_request.id, merge_request.assignee_id) + Notify.should_receive(:reassigned_merge_request_email).with(user_id, merge_request.id, merge_request.assignee_id, merge_request.author_id) end def should_not_email(user_id) - Notify.should_not_receive(:reassigned_merge_request_email).with(user_id, merge_request.id, merge_request.assignee_id) + Notify.should_not_receive(:reassigned_merge_request_email).with(user_id, merge_request.id, merge_request.assignee_id, merge_request.author_id) end end