Commit 58254088 authored by Pierre de La Morinerie's avatar Pierre de La Morinerie Committed by Jacob Vosmaer

Fix the merge notification email not being sent

The 'author_id_of_changes' attribute is not persisted in the database.
As we retrieve the merge request from the DB just before sending the
email, this attribute was always nil.

Also there was no tests for the merge notification code - tests have
been added.

Fix #6605
parent dbbf4ea2
...@@ -29,11 +29,11 @@ module Emails ...@@ -29,11 +29,11 @@ module Emails
subject: subject("#{@merge_request.title} (!#{@merge_request.iid})")) subject: subject("#{@merge_request.title} (!#{@merge_request.iid})"))
end end
def merged_merge_request_email(recipient_id, merge_request_id) def merged_merge_request_email(recipient_id, merge_request_id, updated_by_user_id)
@merge_request = MergeRequest.find(merge_request_id) @merge_request = MergeRequest.find(merge_request_id)
@project = @merge_request.project @project = @merge_request.project
@target_url = project_merge_request_url(@project, @merge_request) @target_url = project_merge_request_url(@project, @merge_request)
mail(from: sender(@merge_request.author_id_of_changes), mail(from: sender(updated_by_user_id),
to: recipient(recipient_id), to: recipient(recipient_id),
subject: subject("#{@merge_request.title} (!#{@merge_request.iid})")) subject: subject("#{@merge_request.title} (!#{@merge_request.iid})"))
end end
......
...@@ -91,7 +91,7 @@ class NotificationService ...@@ -91,7 +91,7 @@ class NotificationService
recipients = recipients.concat(project_watchers(merge_request.target_project)).uniq recipients = recipients.concat(project_watchers(merge_request.target_project)).uniq
recipients.each do |recipient| recipients.each do |recipient|
mailer.merged_merge_request_email(recipient.id, merge_request.id) mailer.merged_merge_request_email(recipient.id, merge_request.id, merge_request.author_id_of_changes)
end end
end end
......
...@@ -229,6 +229,7 @@ describe Notify do ...@@ -229,6 +229,7 @@ describe Notify do
end end
context 'for merge requests' do context 'for merge requests' do
let(:merge_author) { create(:user) }
let(:merge_request) { create(:merge_request, author: current_user, 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) }
let(:merge_request_with_description) { create(:merge_request, author: current_user, assignee: assignee, source_project: project, target_project: project, description: Faker::Lorem.sentence) } let(:merge_request_with_description) { create(:merge_request, author: current_user, assignee: assignee, source_project: project, target_project: project, description: Faker::Lorem.sentence) }
...@@ -288,7 +289,30 @@ describe Notify do ...@@ -288,7 +289,30 @@ describe Notify do
it 'contains a link to the merge request' do it 'contains a link to the merge request' do
should have_body_text /#{project_merge_request_path project, merge_request}/ should have_body_text /#{project_merge_request_path project, merge_request}/
end end
end
describe 'that are merged' do
subject { Notify.merged_merge_request_email(recipient.id, merge_request.id, merge_author.id) }
it_behaves_like 'a multiple recipients email'
it 'is sent as the merge author' do
sender = subject.header[:from].addrs[0]
sender.display_name.should eq(merge_author.name)
sender.address.should eq(gitlab_sender)
end
it 'has the correct subject' do
should have_subject /#{merge_request.title} \(!#{merge_request.iid}\)/
end
it 'contains the new status' do
should have_body_text /merged/i
end
it 'contains a link to the merge request' do
should have_body_text /#{project_merge_request_path project, merge_request}/
end
end end
end end
end end
......
...@@ -233,15 +233,15 @@ describe NotificationService do ...@@ -233,15 +233,15 @@ describe NotificationService do
should_email(@u_watcher.id) should_email(@u_watcher.id)
should_not_email(@u_participating.id) should_not_email(@u_participating.id)
should_not_email(@u_disabled.id) should_not_email(@u_disabled.id)
notification.merge_mr(merge_request) notification.merge_mr(merge_request, @u_disabled)
end end
def should_email(user_id) def should_email(user_id)
Notify.should_receive(:merged_merge_request_email).with(user_id, merge_request.id) Notify.should_receive(:merged_merge_request_email).with(user_id, merge_request.id, @u_disabled.id)
end end
def should_not_email(user_id) def should_not_email(user_id)
Notify.should_not_receive(:merged_merge_request_email).with(user_id, merge_request.id) Notify.should_not_receive(:merged_merge_request_email).with(user_id, merge_request.id, @u_disabled.id)
end end
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment