Ensure NotificationRecipientService doesn't modify participantsstop-notification-recipient-service-modifying-participants

Even though it does modify the participants of the notification target in some
cases, this should have been safe, as different workers are responsible for
creating the notifications for each target. However, this is at best confusing,
and we should ensure we don't do that.

3 files changed, 50 insertions, 7 deletions

diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb
index 8d1820bc504..9ac561e4bd2 100644
--- a/app/services/notification_recipient_service.rb
+++ b/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 = []
diff --git a/changelogs/unreleased/stop-notification-recipient-service-modifying-participants.yml b/changelogs/unreleased/stop-notification-recipient-service-modifying-participants.yml
new file mode 100644
index 00000000000..7e66ea4ca8b
--- /dev/null
+++ b/changelogs/unreleased/stop-notification-recipient-service-modifying-participants.yml
@@ -0,0 +1,5 @@
+---
+title: Ensure participants for issues, merge requests, etc. are calculated correctly
+  when sending notifications
+merge_request:
+author:
diff --git a/spec/services/notification_recipient_service_spec.rb b/spec/services/notification_recipient_service_spec.rb
new file mode 100644
index 00000000000..e18e87a8e75
--- /dev/null
+++ b/spec/services/notification_recipient_service_spec.rb
@@ -0,0 +1,35 @@
+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