From 60bd2ae372a9cab09990a6d2d5522ad3aeed8a40 Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Wed, 28 Jun 2017 11:44:32 +0100
Subject: Ensure NotificationRecipientService doesn't modify 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.
---
 app/services/notification_recipient_service.rb     | 17 ++++++-----
 ...on-recipient-service-modifying-participants.yml |  5 ++++
 .../notification_recipient_service_spec.rb         | 34 ++++++++++++++++++++++
 3 files changed, 49 insertions(+), 7 deletions(-)
 create mode 100644 changelogs/unreleased/stop-notification-recipient-service-modifying-participants.yml
 create mode 100644 spec/services/notification_recipient_service_spec.rb

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..dfe1ee7c41e
--- /dev/null
+++ b/spec/services/notification_recipient_service_spec.rb
@@ -0,0 +1,34 @@
+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
-- 
cgit v1.2.1