Download as
Browse Code »

Commit bbca0351f7028f52900ef0855615821b03a2c3f7

Authored by Dmitriy Zaporozhets
2 parents 55be026e 65f5496e
Exists in spb-stable and in 3 other branches 6-8-stable, 7-0-stable, 7-0-stable-spb

Merge pull request #6606 from kemenaran/fix-merge-notifications 

Fix the merge notification email not being sent
Showing 4 changed files with 30 additions and 6 deletions   Show diff stats
app/mailers/emails/merge_requests.rb
  Diff comments   View file @bbca035
@@ -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 @bbca035
@@ -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 @bbca035
@@ -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 @bbca035
@@ -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