From 798873ca75e656ed0fbd2a3080022eb55a6f3106 Mon Sep 17 00:00:00 2001 From: adamliesko Date: Mon, 9 Nov 2015 17:26:01 +0100 Subject: [PATCH 1/6] Add notification to the former assignee upon unassignment --- CHANGELOG | 1 + app/services/notification_service.rb | 6 ++++-- spec/services/issues/update_service_spec.rb | 10 ++++++---- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 217dc4e004..c8db4da1d7 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -26,6 +26,7 @@ v 8.2.0 (unreleased) - Add "added", "modified" and "removed" properties to commit object in webhook - Rename "Back to" links to "Go to" because its not always a case it point to place user come from - Allow groups to appear in the search results if the group owner allows it + - Add email notification to former assignee upon unassignment v 8.1.3 - Spread out runner contacted_at updates diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index a6b2234865..16c84f4a05 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -362,7 +362,8 @@ class NotificationService def reassign_resource_email(target, project, current_user, method) assignee_id_was = previous_record(target, "assignee_id") - recipients = build_recipients(target, project, current_user) + previous_assignee = User.find(assignee_id_was) + recipients = build_recipients(target, project, current_user, [previous_assignee]) recipients.each do |recipient| mailer.send(method, recipient.id, target.id, assignee_id_was, current_user.id) @@ -377,8 +378,9 @@ class NotificationService end end - def build_recipients(target, project, current_user) + def build_recipients(target, project, current_user, previous_records = nil ) recipients = target.participants(current_user) + recipients.concat(previous_records).compact.uniq if previous_records recipients = add_project_watchers(recipients, project) recipients = reject_mention_users(recipients, project) diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index a91be3b447..4e79484f26 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Issues::UpdateService do let(:user) { create(:user) } let(:user2) { create(:user) } - let(:issue) { create(:issue, title: 'Old title') } + let(:issue) { create(:issue, title: 'Old title', assignee_id: user.id) } let(:label) { create(:label) } let(:project) { issue.project } @@ -34,9 +34,11 @@ describe Issues::UpdateService do it { expect(@issue.labels.count).to eq(1) } it { expect(@issue.labels.first.title).to eq('Bug') } - it 'should send email to user2 about assign of new issue' do - email = ActionMailer::Base.deliveries.last - expect(email.to.first).to eq(user2.email) + it 'should send email to user2 about assign of new issue and email to user about issue unassignment' do + deliveries = ActionMailer::Base.deliveries + email = deliveries.last + recipients = deliveries.map(&:to).uniq.flatten + expect(recipients.last(2)).to include(user.email,user2.email) expect(email.subject).to include(issue.title) end From 8c308e3df6b2c9b259f66172d13cc81009c1ae89 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 17 Nov 2015 16:51:35 +0100 Subject: [PATCH 2/6] Don't fail when there was no previous assignee --- app/services/notification_service.rb | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 16c84f4a05..27928c0945 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -113,7 +113,7 @@ class NotificationService end # Add all users participating in the thread (author, assignee, comment authors) - participants = + participants = if target.respond_to?(:participants) target.participants(note.author) else @@ -325,7 +325,7 @@ class NotificationService def reject_unsubscribed_users(recipients, target) return recipients unless target.respond_to? :subscriptions - + recipients.reject do |user| subscription = target.subscriptions.find_by_user_id(user.id) subscription && !subscription.subscribed @@ -343,7 +343,7 @@ class NotificationService recipients end end - + def new_resource_email(target, project, method) recipients = build_recipients(target, project, target.author) @@ -361,12 +361,13 @@ class NotificationService end def reassign_resource_email(target, project, current_user, method) - assignee_id_was = previous_record(target, "assignee_id") - previous_assignee = User.find(assignee_id_was) + previous_assignee_id = previous_record(target, "assignee_id") + previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id + recipients = build_recipients(target, project, current_user, [previous_assignee]) recipients.each do |recipient| - mailer.send(method, recipient.id, target.id, assignee_id_was, current_user.id) + mailer.send(method, recipient.id, target.id, previous_assignee_id, current_user.id) end end @@ -378,9 +379,10 @@ class NotificationService end end - def build_recipients(target, project, current_user, previous_records = nil ) + def build_recipients(target, project, current_user, extra_recipients = nil ) recipients = target.participants(current_user) - recipients.concat(previous_records).compact.uniq if previous_records + + recipients = recipients.concat(extra_recipients).compact.uniq if extra_recipients recipients = add_project_watchers(recipients, project) recipients = reject_mention_users(recipients, project) From 80a2eb5d50102ff8e0dcfb67994acc3037b0270e Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 17 Nov 2015 16:51:39 +0100 Subject: [PATCH 3/6] Fix spec --- spec/services/issues/update_service_spec.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 4e79484f26..6a9053753c 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -3,13 +3,15 @@ require 'spec_helper' describe Issues::UpdateService do let(:user) { create(:user) } let(:user2) { create(:user) } - let(:issue) { create(:issue, title: 'Old title', assignee_id: user.id) } + let(:user3) { create(:user) } + let(:issue) { create(:issue, title: 'Old title', assignee_id: user3.id) } let(:label) { create(:label) } let(:project) { issue.project } before do project.team << [user, :master] project.team << [user2, :developer] + project.team << [user3, :developer] end describe 'execute' do @@ -35,10 +37,10 @@ describe Issues::UpdateService do it { expect(@issue.labels.first.title).to eq('Bug') } it 'should send email to user2 about assign of new issue and email to user about issue unassignment' do - deliveries = ActionMailer::Base.deliveries + deliveries = ActionMailer::Base.deliveries email = deliveries.last - recipients = deliveries.map(&:to).uniq.flatten - expect(recipients.last(2)).to include(user.email,user2.email) + recipients = deliveries.last(2).map(&:to).flatten + expect(recipients).to include(user2.email, user3.email) expect(email.subject).to include(issue.title) end From 0fbf47a219c7c284b5728a4a6d066f1d0700f985 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 17 Nov 2015 16:51:49 +0100 Subject: [PATCH 4/6] Add credits to changelog --- CHANGELOG | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG b/CHANGELOG index 56c0daa61a..119185b8ce 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -32,7 +32,7 @@ v 8.2.0 (unreleased) - Add "added", "modified" and "removed" properties to commit object in webhook - Rename "Back to" links to "Go to" because its not always a case it point to place user come from - Allow groups to appear in the search results if the group owner allows it - - Add email notification to former assignee upon unassignment + - Add email notification to former assignee upon unassignment (Adam Lieskovský) - New design for project graphs page - Remove deprecated dumped yaml file generated from previous job definitions - Fix incoming email config defaults From 931c56f822c21e5f77743297603cfff5065eb772 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 17 Nov 2015 16:53:15 +0100 Subject: [PATCH 5/6] Add tests for merge request update. --- spec/services/issues/update_service_spec.rb | 2 +- spec/services/merge_requests/update_service_spec.rb | 11 +++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 6a9053753c..f55527ee9a 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -36,7 +36,7 @@ describe Issues::UpdateService do it { expect(@issue.labels.count).to eq(1) } it { expect(@issue.labels.first.title).to eq('Bug') } - it 'should send email to user2 about assign of new issue and email to user about issue unassignment' do + it 'should send email to user2 about assign of new issue and email to user3 about issue unassignment' do deliveries = ActionMailer::Base.deliveries email = deliveries.last recipients = deliveries.last(2).map(&:to).flatten diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index c75173c145..2ed51d223b 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -3,7 +3,8 @@ require 'spec_helper' describe MergeRequests::UpdateService do let(:user) { create(:user) } let(:user2) { create(:user) } - let(:merge_request) { create(:merge_request, :simple, title: 'Old title') } + let(:user3) { create(:user) } + let(:merge_request) { create(:merge_request, :simple, title: 'Old title', assignee_id: user3.id) } let(:project) { merge_request.project } let(:label) { create(:label) } @@ -47,9 +48,11 @@ describe MergeRequests::UpdateService do with(@merge_request, 'update') end - it 'should send email to user2 about assign of new merge_request' do - email = ActionMailer::Base.deliveries.last - expect(email.to.first).to eq(user2.email) + it 'should send email to user2 about assign of new merge request and email to user3 about merge request unassignment' do + deliveries = ActionMailer::Base.deliveries + email = deliveries.last + recipients = deliveries.last(2).map(&:to).flatten + expect(recipients).to include(user2.email, user3.email) expect(email.subject).to include(merge_request.title) end From f008b2d2dfd1b8030ec0634c223ca9f11d304e91 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 17 Nov 2015 17:19:31 +0100 Subject: [PATCH 6/6] Remove extra space --- app/services/notification_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 27928c0945..d9aa6bc98c 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -379,7 +379,7 @@ class NotificationService end end - def build_recipients(target, project, current_user, extra_recipients = nil ) + def build_recipients(target, project, current_user, extra_recipients = nil) recipients = target.participants(current_user) recipients = recipients.concat(extra_recipients).compact.uniq if extra_recipients