diff options
| -rw-r--r-- | app/policies/project_policy.rb | 6 | ||||
| -rw-r--r-- | spec/services/notification_recipient_service_spec.rb | 44 |
2 files changed, 27 insertions, 23 deletions
diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index d0e84b1aa38..f2c246cd969 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -390,7 +390,11 @@ class ProjectPolicy < BasePolicy greedy_load_subject ||= !@user.persisted? if greedy_load_subject - project.team.members.include?(user) + # We want to load all the members with one query. Calling #include? on + # project.team.members will perform a separate query for each user, unless + # project.team.members was loaded before somewhere else. Calling #to_a + # ensures it's always loaded before checking for membership. + project.team.members.to_a.include?(user) else # otherwise we just make a specific query for # this particular user. diff --git a/spec/services/notification_recipient_service_spec.rb b/spec/services/notification_recipient_service_spec.rb index 9a24821860e..cea5ea125b9 100644 --- a/spec/services/notification_recipient_service_spec.rb +++ b/spec/services/notification_recipient_service_spec.rb @@ -10,18 +10,9 @@ describe NotificationRecipientService do let(:issue) { create(:issue, project: project, assignees: [assignee]) } let(:note) { create(:note_on_issue, noteable: issue, project_id: issue.project_id) } - context 'when there are multiple watchers' do - def create_watcher - watcher = create(:user) - create(:notification_setting, source: project, user: watcher, level: :watch) - - other_projects.each do |other_project| - create(:notification_setting, source: other_project, user: watcher, level: :watch) - end - end - + shared_examples 'no N+1 queries' do it 'avoids N+1 queries', :request_store do - create_watcher + create_user service.build_new_note_recipients(note) @@ -29,30 +20,39 @@ describe NotificationRecipientService do service.build_new_note_recipients(note) end - create_watcher + create_user expect { service.build_new_note_recipients(note) }.not_to exceed_query_limit(control_count) end end + context 'when there are multiple watchers' do + def create_user + watcher = create(:user) + create(:notification_setting, source: project, user: watcher, level: :watch) + + other_projects.each do |other_project| + create(:notification_setting, source: other_project, user: watcher, level: :watch) + end + end + + include_examples 'no N+1 queries' + end + context 'when there are multiple subscribers' do - def create_subscriber + def create_user subscriber = create(:user) issue.subscriptions.create(user: subscriber, project: project, subscribed: true) end - it 'avoids N+1 queries' do - create_subscriber + include_examples 'no N+1 queries' - service.build_new_note_recipients(note) - - control_count = ActiveRecord::QueryRecorder.new do - service.build_new_note_recipients(note) + context 'when the project is private' do + before do + project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) end - create_subscriber - - expect { service.build_new_note_recipients(note) }.not_to exceed_query_limit(control_count) + include_examples 'no N+1 queries' end end end |