Merge pull request #3878 from gdpelican/optimize-all-allowed-users

Optimize all_allowed_users query

app/models/topic.rb

@@ -253,11 +253,8 @@ class Topic < ActiveRecord::Base
 
   # all users (in groups or directly targetted) that are going to get the pm
   def all_allowed_users
-    # TODO we should probably change this to 1 query
-    allowed_user_ids = allowed_users.select('users.id').to_a
-    allowed_group_user_ids = allowed_group_users.select('users.id').to_a
-    allowed_staff_ids = private_message? && has_flags? ? User.where(moderator: true).pluck(:id).to_a : []
-    User.where('id IN (?)', allowed_user_ids + allowed_group_user_ids + allowed_staff_ids)
+    moderators_sql = " UNION #{User.moderators.to_sql}" if private_message? && has_flags?
+    User.from("(#{allowed_users.to_sql} UNION #{allowed_group_users.to_sql}#{moderators_sql}) as users")
   end
 
   # Additional rate limits on topics: per day and private messages per day

spec/models/topic_spec.rb

@@ -1315,6 +1315,47 @@ describe Topic do
     end
   end
 
+  describe 'all_allowed_users' do
+    let(:group) { Fabricate(:group) }
+    let(:topic) { Fabricate(:topic, allowed_groups: [group]) }
+    let!(:allowed_user) { Fabricate(:user) }
+    let!(:allowed_group_user) { Fabricate(:user) }
+    let!(:moderator) { Fabricate(:user, moderator: true) }
+    let!(:rando) { Fabricate(:user) }
+
+    before do
+      topic.allowed_users << allowed_user
+      group.users << allowed_group_user
+    end
+
+    it 'includes allowed_users' do
+      expect(topic.all_allowed_users).to include allowed_user
+    end
+
+    it 'includes allowed_group_users' do
+      expect(topic.all_allowed_users).to include allowed_group_user
+    end
+
+    it 'includes moderators if flagged and a pm' do
+      topic.stubs(:has_flags?).returns(true)
+      topic.stubs(:private_message?).returns(true)
+      expect(topic.all_allowed_users).to include moderator
+    end
+
+    it 'does not include moderators if pm without flags' do
+      topic.stubs(:private_message?).returns(true)
+      expect(topic.all_allowed_users).not_to include moderator
+    end
+
+    it 'does not include moderators for regular topic' do
+      expect(topic.all_allowed_users).not_to include moderator
+    end
+
+    it 'does not include randos' do
+      expect(topic.all_allowed_users).not_to include rando
+    end
+  end
+
   describe '#listable_count_per_day' do
     before(:each) do
       Timecop.freeze