From 51266f41b3150b3a198bf8316d093a86d4d38cd1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Fri, 15 Jan 2016 15:28:16 +0100 Subject: [PATCH 1/3] Consider re-assign as a mention from a notification point of view This will ensure new assignee gets an email even if his notif level is "on mention". --- app/services/notification_service.rb | 16 +++++++++++----- spec/services/notification_service_spec.rb | 18 +++++++++++++++++- 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index e4edc55bf6..60b86914a3 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -379,7 +379,7 @@ class NotificationService 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 = build_recipients(target, project, current_user, action: :reassign, extra_recipients: [previous_assignee]) recipients.each do |recipient| mailer.send( @@ -400,22 +400,28 @@ class NotificationService end end - def build_recipients(target, project, current_user, extra_recipients = nil) + def build_recipients(target, project, current_user, action: nil, extra_recipients: nil) recipients = target.participants(current_user) recipients = recipients.concat(extra_recipients).compact.uniq if extra_recipients recipients = add_project_watchers(recipients, project) recipients = reject_mention_users(recipients, project) - recipients = reject_muted_users(recipients, project) + # Re-assign is considered as a mention of the new assignee so we add the + # new assignee to the list of recipients after we rejected users with + # the "on mention" notification level + if action == :reassign + recipients << target.assignee + end + + recipients = reject_muted_users(recipients, project) recipients = add_subscribed_users(recipients, target) recipients = reject_unsubscribed_users(recipients, target) recipients.delete(current_user) - recipients = recipients.uniq - recipients + recipients.uniq end def mailer diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 6d219f3589..b63903af41 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -227,9 +227,11 @@ describe NotificationService, services: true do end describe :reassigned_issue do - it 'should email new assignee' do + it 'emails new assignee' do + issue.update_attribute(:assignee, @u_mentioned) notification.reassigned_issue(issue, @u_disabled) + expect(issue.assignee).to be @u_mentioned should_email(issue.assignee) should_email(@u_watcher) should_email(@u_participant_mentioned) @@ -238,6 +240,20 @@ describe NotificationService, services: true do should_not_email(@u_participating) should_not_email(@u_disabled) end + + it 'does not email new assignee if they are the current user' do + issue.update_attribute(:assignee, @u_mentioned) + notification.reassigned_issue(issue, @u_mentioned) + + expect(issue.assignee).to be @u_mentioned + should_email(@u_watcher) + should_email(@u_participant_mentioned) + should_email(@subscriber) + should_not_email(issue.assignee) + should_not_email(@unsubscriber) + should_not_email(@u_participating) + should_not_email(@u_disabled) + end end describe :close_issue do From 55ae5394a05fbde53067336a17ff2f03733775a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Fri, 15 Jan 2016 16:38:18 +0100 Subject: [PATCH 2/3] Add changelog entry for fix-consider-re-assign-as-a-mention --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index d01576be67..f3916c39ec 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 8.4.0 (unreleased) + - Consider re-assign as a mention from a notification point of view - Add pagination headers to already paginated API resources - Properly generate diff of orphan commits, like the first commit in a repository - Improve the consistency of commit titles, branch names, tag names, issue/MR titles, on their respective project pages From 30a4f4c9e0cc44b67ea6041b8aef01196649b8d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Fri, 15 Jan 2016 17:57:45 +0100 Subject: [PATCH 3/3] This will ensure previous assignee gets an email even if his notif level is "on mention" --- app/services/notification_service.rb | 9 +++-- spec/services/notification_service_spec.rb | 40 ++++++++++++++++++++++ 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 60b86914a3..ca8a41d93b 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -376,10 +376,10 @@ class NotificationService end def reassign_resource_email(target, project, current_user, method) - previous_assignee_id = previous_record(target, "assignee_id") + 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, action: :reassign, extra_recipients: [previous_assignee]) + recipients = build_recipients(target, project, current_user, action: :reassign, previous_assignee: previous_assignee) recipients.each do |recipient| mailer.send( @@ -400,11 +400,9 @@ class NotificationService end end - def build_recipients(target, project, current_user, action: nil, extra_recipients: nil) + def build_recipients(target, project, current_user, action: nil, previous_assignee: nil) recipients = target.participants(current_user) - recipients = recipients.concat(extra_recipients).compact.uniq if extra_recipients - recipients = add_project_watchers(recipients, project) recipients = reject_mention_users(recipients, project) @@ -412,6 +410,7 @@ class NotificationService # new assignee to the list of recipients after we rejected users with # the "on mention" notification level if action == :reassign + recipients << previous_assignee if previous_assignee recipients << target.assignee end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index b63903af41..2d0b5df422 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -227,6 +227,46 @@ describe NotificationService, services: true do end describe :reassigned_issue do + it 'emails new assignee' do + notification.reassigned_issue(issue, @u_disabled) + + should_email(issue.assignee) + should_email(@u_watcher) + should_email(@u_participant_mentioned) + should_email(@subscriber) + should_not_email(@unsubscriber) + should_not_email(@u_participating) + should_not_email(@u_disabled) + end + + it 'emails previous assignee even if he has the "on mention" notif level' do + issue.update_attribute(:assignee, @u_mentioned) + issue.update_attributes(assignee: @u_watcher) + notification.reassigned_issue(issue, @u_disabled) + + should_email(@u_mentioned) + should_email(@u_watcher) + should_email(@u_participant_mentioned) + should_email(@subscriber) + should_not_email(@unsubscriber) + should_not_email(@u_participating) + should_not_email(@u_disabled) + end + + it 'emails new assignee even if he has the "on mention" notif level' do + issue.update_attributes(assignee: @u_mentioned) + notification.reassigned_issue(issue, @u_disabled) + + expect(issue.assignee).to be @u_mentioned + should_email(issue.assignee) + should_email(@u_watcher) + should_email(@u_participant_mentioned) + should_email(@subscriber) + should_not_email(@unsubscriber) + should_not_email(@u_participating) + should_not_email(@u_disabled) + end + it 'emails new assignee' do issue.update_attribute(:assignee, @u_mentioned) notification.reassigned_issue(issue, @u_disabled)