Download as
Browse Code »

Commit 65f5496e0210be96b4803f655d1a4c94ef1e07fe

Authored by Pierre de La Morinerie
1 parent 36ea8645
Exists in spb-stable and in 3 other branches 6-8-stable, 7-0-stable, 7-0-stable-spb

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
Showing 4 changed files with 30 additions and 6 deletions   Show diff stats
app/mailers/emails/merge_requests.rb
  Diff comments   View file @65f5496
@@ -29,11 +29,11 @@ module Emails @@ -29,11 +29,11 @@ module Emails
29 subject: subject("#{@merge_request.title} (!#{@merge_request.iid})")) 29 subject: subject("#{@merge_request.title} (!#{@merge_request.iid})"))
30 end 30 end
31 31
32 - def merged_merge_request_email(recipient_id, merge_request_id) 32 + def merged_merge_request_email(recipient_id, merge_request_id, updated_by_user_id)
33 @merge_request = MergeRequest.find(merge_request_id) 33 @merge_request = MergeRequest.find(merge_request_id)
34 @project = @merge_request.project 34 @project = @merge_request.project
35 @target_url = project_merge_request_url(@project, @merge_request) 35 @target_url = project_merge_request_url(@project, @merge_request)
36 - mail(from: sender(@merge_request.author_id_of_changes), 36 + mail(from: sender(updated_by_user_id),
37 to: recipient(recipient_id), 37 to: recipient(recipient_id),
38 subject: subject("#{@merge_request.title} (!#{@merge_request.iid})")) 38 subject: subject("#{@merge_request.title} (!#{@merge_request.iid})"))
39 end 39 end
app/services/notification_service.rb
  Diff comments   View file @65f5496
@@ -91,7 +91,7 @@ class NotificationService @@ -91,7 +91,7 @@ class NotificationService
91 recipients = recipients.concat(project_watchers(merge_request.target_project)).uniq 91 recipients = recipients.concat(project_watchers(merge_request.target_project)).uniq
92 92
93 recipients.each do |recipient| 93 recipients.each do |recipient|
94 - mailer.merged_merge_request_email(recipient.id, merge_request.id) 94 + mailer.merged_merge_request_email(recipient.id, merge_request.id, merge_request.author_id_of_changes)
95 end 95 end
96 end 96 end
97 97
spec/mailers/notify_spec.rb
  Diff comments   View file @65f5496
@@ -229,6 +229,7 @@ describe Notify do @@ -229,6 +229,7 @@ describe Notify do
229 end 229 end
230 230
231 context 'for merge requests' do 231 context 'for merge requests' do
  232 + let(:merge_author) { create(:user) }
232 let(:merge_request) { create(:merge_request, author: current_user, assignee: assignee, source_project: project, target_project: project) } 233 let(:merge_request) { create(:merge_request, author: current_user, assignee: assignee, source_project: project, target_project: project) }
233 let(:merge_request_with_description) { create(:merge_request, author: current_user, assignee: assignee, source_project: project, target_project: project, description: Faker::Lorem.sentence) } 234 let(:merge_request_with_description) { create(:merge_request, author: current_user, assignee: assignee, source_project: project, target_project: project, description: Faker::Lorem.sentence) }
234 235
@@ -288,7 +289,30 @@ describe Notify do @@ -288,7 +289,30 @@ describe Notify do
288 it 'contains a link to the merge request' do 289 it 'contains a link to the merge request' do
289 should have_body_text /#{project_merge_request_path project, merge_request}/ 290 should have_body_text /#{project_merge_request_path project, merge_request}/
290 end 291 end
  292 + end
  293 +
  294 + describe 'that are merged' do
  295 + subject { Notify.merged_merge_request_email(recipient.id, merge_request.id, merge_author.id) }
  296 +
  297 + it_behaves_like 'a multiple recipients email'
  298 +
  299 + it 'is sent as the merge author' do
  300 + sender = subject.header[:from].addrs[0]
  301 + sender.display_name.should eq(merge_author.name)
  302 + sender.address.should eq(gitlab_sender)
  303 + end
  304 +
  305 + it 'has the correct subject' do
  306 + should have_subject /#{merge_request.title} \(!#{merge_request.iid}\)/
  307 + end
291 308
  309 + it 'contains the new status' do
  310 + should have_body_text /merged/i
  311 + end
  312 +
  313 + it 'contains a link to the merge request' do
  314 + should have_body_text /#{project_merge_request_path project, merge_request}/
  315 + end
292 end 316 end
293 end 317 end
294 end 318 end
spec/services/notification_service_spec.rb
  Diff comments   View file @65f5496
@@ -233,15 +233,15 @@ describe NotificationService do @@ -233,15 +233,15 @@ describe NotificationService do
233 should_email(@u_watcher.id) 233 should_email(@u_watcher.id)
234 should_not_email(@u_participating.id) 234 should_not_email(@u_participating.id)
235 should_not_email(@u_disabled.id) 235 should_not_email(@u_disabled.id)
236 - notification.merge_mr(merge_request) 236 + notification.merge_mr(merge_request, @u_disabled)
237 end 237 end
238 238
239 def should_email(user_id) 239 def should_email(user_id)
240 - Notify.should_receive(:merged_merge_request_email).with(user_id, merge_request.id) 240 + Notify.should_receive(:merged_merge_request_email).with(user_id, merge_request.id, @u_disabled.id)
241 end 241 end
242 242
243 def should_not_email(user_id) 243 def should_not_email(user_id)
244 - Notify.should_not_receive(:merged_merge_request_email).with(user_id, merge_request.id) 244 + Notify.should_not_receive(:merged_merge_request_email).with(user_id, merge_request.id, @u_disabled.id)
245 end 245 end
246 end 246 end
247 end 247 end