diff options
author | Sean McGivern <sean@gitlab.com> | 2018-10-02 15:39:46 +0300 |
---|---|---|
committer | Sean McGivern <sean@gitlab.com> | 2018-10-04 16:27:36 +0300 |
commit | 819ecd5fb01ab64cacc31253c51984a7564949d7 (patch) | |
tree | dd9a64ff81186105161950715c1e9ab1b3d1b2e6 | |
parent | eac20b74a7e92c34a42fe0658b745521cf2e862f (diff) |
Fix N+1 for notification recipients in subscribers
-rw-r--r-- | app/models/concerns/subscribable.rb | 8 | ||||
-rw-r--r-- | spec/services/notification_recipient_service_spec.rb | 47 |
2 files changed, 40 insertions, 15 deletions
diff --git a/app/models/concerns/subscribable.rb b/app/models/concerns/subscribable.rb index 1d0a61364b0..92a5c1112af 100644 --- a/app/models/concerns/subscribable.rb +++ b/app/models/concerns/subscribable.rb @@ -31,9 +31,11 @@ module Subscribable end def subscribers(project) - subscriptions_available(project) - .where(subscribed: true) - .map(&:user) + relation = subscriptions_available(project) + .where(subscribed: true) + .select(:user_id) + + User.where(id: relation) end def toggle_subscription(user, project = nil) diff --git a/spec/services/notification_recipient_service_spec.rb b/spec/services/notification_recipient_service_spec.rb index 14ba6b7bed2..9a24821860e 100644 --- a/spec/services/notification_recipient_service_spec.rb +++ b/spec/services/notification_recipient_service_spec.rb @@ -10,27 +10,50 @@ describe NotificationRecipientService do let(:issue) { create(:issue, project: project, assignees: [assignee]) } let(:note) { create(:note_on_issue, noteable: issue, project_id: issue.project_id) } - def create_watcher - watcher = create(:user) - create(:notification_setting, source: project, user: watcher, level: :watch) + 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 + + it 'avoids N+1 queries', :request_store do + create_watcher + + service.build_new_note_recipients(note) + + control_count = ActiveRecord::QueryRecorder.new do + service.build_new_note_recipients(note) + end - other_projects.each do |other_project| - create(:notification_setting, source: other_project, user: watcher, level: :watch) + create_watcher + + expect { service.build_new_note_recipients(note) }.not_to exceed_query_limit(control_count) end end - it 'avoids N+1 queries', :request_store do - create_watcher + context 'when there are multiple subscribers' do + def create_subscriber + subscriber = create(:user) + issue.subscriptions.create(user: subscriber, project: project, subscribed: true) + end - service.build_new_note_recipients(note) + it 'avoids N+1 queries' do + create_subscriber - control_count = ActiveRecord::QueryRecorder.new do service.build_new_note_recipients(note) - end - create_watcher + control_count = ActiveRecord::QueryRecorder.new do + service.build_new_note_recipients(note) + end - expect { service.build_new_note_recipients(note) }.not_to exceed_query_limit(control_count) + create_subscriber + + expect { service.build_new_note_recipients(note) }.not_to exceed_query_limit(control_count) + end end end end |