Merge branch 'fix-consider-re-assign-as-a-mention'

3 files changed, 71 insertions, 9 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 5795395beeb..9d58f5cea59 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -4,6 +4,7 @@ v 8.5.0 (unreleased)
 
 v 8.4.0 (unreleased)
   - Ensure Gravatar host looks like an actual host
+  - 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
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index e4edc55bf69..ca8a41d93b8 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, [previous_assignee])
+    recipients = build_recipients(target, project, current_user, action: :reassign, previous_assignee: previous_assignee)
 
     recipients.each do |recipient|
       mailer.send(
@@ -400,22 +400,27 @@ class NotificationService
     end
   end
 
-  def build_recipients(target, project, current_user, 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)
-    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 << previous_assignee if previous_assignee
+      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 6d219f35895..2d0b5df4224 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -227,7 +227,7 @@ describe NotificationService, services: true do
     end
 
     describe :reassigned_issue do
-      it 'should email new assignee' do
+      it 'emails new assignee' do
         notification.reassigned_issue(issue, @u_disabled)
 
         should_email(issue.assignee)
@@ -238,6 +238,62 @@ describe NotificationService, services: true do
         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)
+
+        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 '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