Merge branch 'stop-notification-recipient-service-modifying-participants' into 'master'

Ensure NotificationRecipientService doesn't modify participants

See merge request !12517

app/services/notification_recipient_service.rb

@@ -11,7 +11,7 @@ class NotificationRecipientService
   def build_recipients(target, current_user, action:, previous_assignee: nil, skip_current_user: true)
     custom_action = build_custom_key(action, target)
 
-    recipients = target.participants(current_user)
+    recipients = participants(target, current_user)
     recipients = add_project_watchers(recipients)
     recipients = add_custom_notifications(recipients, custom_action)
     recipients = reject_mention_users(recipients)
@@ -86,12 +86,7 @@ class NotificationRecipientService
     mentioned_users = note.mentioned_users.select { |user| user.can?(ability, subject) }
 
     # Add all users participating in the thread (author, assignee, comment authors)
-    recipients =
-      if target.respond_to?(:participants)
-        target.participants(note.author)
-      else
-        mentioned_users
-      end
+    recipients = participants(target, note.author) || mentioned_users
 
     unless note.for_personal_snippet?
       # Merge project watchers
@@ -123,6 +118,14 @@ class NotificationRecipientService
 
   protected
 
+  # Ensure that if we modify this array, we aren't modifying the memoised
+  # participants on the target.
+  def participants(target, user)
+    return unless target.respond_to?(:participants)
+
+    target.participants(user).dup
+  end
+
   # Get project/group users with CUSTOM notification level
   def add_custom_notifications(recipients, action)
     user_ids = []

changelogs/unreleased/stop-notification-recipient-service-modifying-participants.yml

+---
+title: Ensure participants for issues, merge requests, etc. are calculated correctly
+  when sending notifications
+merge_request:
+author:

spec/services/notification_recipient_service_spec.rb

+require 'spec_helper'
+
+describe NotificationRecipientService, services: true do
+  set(:user) { create(:user) }
+  set(:project) { create(:empty_project, :public) }
+  set(:issue) { create(:issue, project: project) }
+
+  set(:watcher) do
+    watcher = create(:user)
+    setting = watcher.notification_settings_for(project)
+    setting.level = :watch
+    setting.save
+
+    watcher
+  end
+
+  subject { described_class.new(project) }
+
+  describe '#build_recipients' do
+    it 'does not modify the participants of the target' do
+      expect { subject.build_recipients(issue, user, action: :new_issue) }
+        .not_to change { issue.participants(user) }
+    end
+  end
+
+  describe '#build_new_note_recipients' do
+    set(:note) { create(:note_on_issue, noteable: issue, project: project) }
+
+    it 'does not modify the participants of the target' do
+      expect { subject.build_new_note_recipients(note) }
+        .not_to change { note.noteable.participants(note.author) }
+    end
+  end
+end