BigW Consortium Gitlab

Commit 60bd2ae3 by Sean McGivern

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.
parent 18a7fa55
...@@ -11,7 +11,7 @@ class NotificationRecipientService ...@@ -11,7 +11,7 @@ class NotificationRecipientService
def build_recipients(target, current_user, action:, previous_assignee: nil, skip_current_user: true) def build_recipients(target, current_user, action:, previous_assignee: nil, skip_current_user: true)
custom_action = build_custom_key(action, target) custom_action = build_custom_key(action, target)
recipients = target.participants(current_user) recipients = participants(target, current_user)
recipients = add_project_watchers(recipients) recipients = add_project_watchers(recipients)
recipients = add_custom_notifications(recipients, custom_action) recipients = add_custom_notifications(recipients, custom_action)
recipients = reject_mention_users(recipients) recipients = reject_mention_users(recipients)
...@@ -86,12 +86,7 @@ class NotificationRecipientService ...@@ -86,12 +86,7 @@ class NotificationRecipientService
mentioned_users = note.mentioned_users.select { |user| user.can?(ability, subject) } mentioned_users = note.mentioned_users.select { |user| user.can?(ability, subject) }
# Add all users participating in the thread (author, assignee, comment authors) # Add all users participating in the thread (author, assignee, comment authors)
recipients = recipients = participants(target, note.author) || mentioned_users
if target.respond_to?(:participants)
target.participants(note.author)
else
mentioned_users
end
unless note.for_personal_snippet? unless note.for_personal_snippet?
# Merge project watchers # Merge project watchers
...@@ -123,6 +118,14 @@ class NotificationRecipientService ...@@ -123,6 +118,14 @@ class NotificationRecipientService
protected 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 # Get project/group users with CUSTOM notification level
def add_custom_notifications(recipients, action) def add_custom_notifications(recipients, action)
user_ids = [] user_ids = []
......
---
title: Ensure participants for issues, merge requests, etc. are calculated correctly
when sending notifications
merge_request:
author:
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
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment