From 45c5f56ffcf0c120a13ca2dcd7e7119c6eaeadda Mon Sep 17 00:00:00 2001
From: David Taylor <david@taylorhq.com>
Date: Thu, 2 Jan 2020 13:04:08 +0000
Subject: [PATCH] PERF: Reduce DB queries when serializing ignore/mute
 information (#8629)

* PERF: Cache ignored and muted user ids in the current_user object
* PERF: Avoid DB queries when checking ignore/mute permission in guardian
---
 app/controllers/users_controller.rb      |  4 +--
 app/models/user.rb                       | 13 +++++++++
 app/serializers/user_serializer.rb       | 10 ++++---
 lib/guardian.rb                          | 10 +++----
 spec/components/guardian_spec.rb         | 24 ++++++++--------
 spec/serializers/user_serializer_spec.rb | 35 ++++++++++++++++++++++++
 6 files changed, 73 insertions(+), 23 deletions(-)

diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index ac6993568c9..8d64202630d 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -1118,7 +1118,7 @@ class UsersController < ApplicationController
     user = fetch_user_from_params
 
     if params[:notification_level] == "ignore"
-      guardian.ensure_can_ignore_user!(user.id)
+      guardian.ensure_can_ignore_user!(user)
       MutedUser.where(user: current_user, muted_user: user).delete_all
       ignored_user = IgnoredUser.find_by(user: current_user, ignored_user: user)
       if ignored_user.present?
@@ -1127,7 +1127,7 @@ class UsersController < ApplicationController
         IgnoredUser.create!(user: current_user, ignored_user: user, expiring_at: Time.parse(params[:expiring_at]))
       end
     elsif params[:notification_level] == "mute"
-      guardian.ensure_can_mute_user!(user.id)
+      guardian.ensure_can_mute_user!(user)
       IgnoredUser.where(user: current_user, ignored_user: user).delete_all
       MutedUser.find_or_create_by!(user: current_user, muted_user: user)
     elsif params[:notification_level] == "normal"
diff --git a/app/models/user.rb b/app/models/user.rb
index 1c06d191609..8a7d48fcba2 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -83,6 +83,9 @@ class User < ActiveRecord::Base
   has_many :muted_user_records, class_name: 'MutedUser'
   has_many :muted_users, through: :muted_user_records
 
+  has_many :ignored_user_records, class_name: 'IgnoredUser'
+  has_many :ignored_users, through: :ignored_user_records
+
   has_many :api_keys, dependent: :destroy
 
   has_many :push_subscriptions, dependent: :destroy
@@ -468,9 +471,19 @@ class User < ActiveRecord::Base
     @unread_total_notifications = nil
     @unread_pms = nil
     @user_fields = nil
+    @ignored_user_ids = nil
+    @muted_user_ids = nil
     super
   end
 
+  def ignored_user_ids
+    @ignored_user_ids ||= ignored_users.pluck(:id)
+  end
+
+  def muted_user_ids
+    @muted_user_ids ||= muted_users.pluck(:id)
+  end
+
   def unread_notifications_of_type(notification_type)
     # perf critical, much more efficient than AR
     sql = <<~SQL
diff --git a/app/serializers/user_serializer.rb b/app/serializers/user_serializer.rb
index 43bbc569768..2eb9ae16dd3 100644
--- a/app/serializers/user_serializer.rb
+++ b/app/serializers/user_serializer.rb
@@ -255,19 +255,21 @@ class UserSerializer < BasicUserSerializer
   end
 
   def ignored
-    IgnoredUser.where(user_id: scope.user&.id, ignored_user_id: object.id).exists?
+    scope_ignored_user_ids = scope.user&.ignored_user_ids || []
+    scope_ignored_user_ids.include?(object.id)
   end
 
   def muted
-    MutedUser.where(user_id: scope.user&.id, muted_user_id: object.id).exists?
+    scope_muted_user_ids = scope.user&.muted_user_ids || []
+    scope_muted_user_ids.include?(object.id)
   end
 
   def can_mute_user
-    scope.can_mute_user?(object.id)
+    scope.can_mute_user?(object)
   end
 
   def can_ignore_user
-    scope.can_ignore_user?(object.id)
+    scope.can_ignore_user?(object)
   end
 
   # Needed because 'send_private_message_to_user' will always return false
diff --git a/lib/guardian.rb b/lib/guardian.rb
index 9920fa89b28..4405a860234 100644
--- a/lib/guardian.rb
+++ b/lib/guardian.rb
@@ -463,10 +463,10 @@ class Guardian
     UserExport.where(user_id: @user.id, created_at: (Time.zone.now.beginning_of_day..Time.zone.now.end_of_day)).count == 0
   end
 
-  def can_mute_user?(user_id)
+  def can_mute_user?(target_user)
     can_mute_users? &&
-      @user.id != user_id &&
-      User.where(id: user_id, admin: false, moderator: false).exists?
+      @user.id != target_user.id &&
+      !target_user.staff?
   end
 
   def can_mute_users?
@@ -474,8 +474,8 @@ class Guardian
     @user.staff? || @user.trust_level >= TrustLevel.levels[:basic]
   end
 
-  def can_ignore_user?(user_id)
-    can_ignore_users? && @user.id != user_id && User.where(id: user_id, admin: false, moderator: false).exists?
+  def can_ignore_user?(target_user)
+    can_ignore_users? && @user.id != target_user.id && !target_user.staff?
   end
 
   def can_ignore_users?
diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb
index 46ef188bccd..8aa4e497378 100644
--- a/spec/components/guardian_spec.rb
+++ b/spec/components/guardian_spec.rb
@@ -2759,7 +2759,7 @@ describe Guardian do
 
     context "when ignored user is the same as guardian user" do
       it 'does not allow ignoring user' do
-        expect(guardian.can_ignore_user?(trust_level_2.id)).to eq(false)
+        expect(guardian.can_ignore_user?(trust_level_2)).to eq(false)
       end
     end
 
@@ -2767,13 +2767,13 @@ describe Guardian do
       let!(:admin) { Fabricate(:user, admin: true) }
 
       it 'does not allow ignoring user' do
-        expect(guardian.can_ignore_user?(admin.id)).to eq(false)
+        expect(guardian.can_ignore_user?(admin)).to eq(false)
       end
     end
 
     context "when ignored user is a normal user" do
       it 'allows ignoring user' do
-        expect(guardian.can_ignore_user?(another_user.id)).to eq(true)
+        expect(guardian.can_ignore_user?(another_user)).to eq(true)
       end
     end
 
@@ -2782,7 +2782,7 @@ describe Guardian do
       let!(:trust_level_1) { build(:user, trust_level: 1) }
 
       it 'does not allow ignoring user' do
-        expect(guardian.can_ignore_user?(another_user.id)).to eq(false)
+        expect(guardian.can_ignore_user?(another_user)).to eq(false)
       end
     end
 
@@ -2790,7 +2790,7 @@ describe Guardian do
       let(:guardian) { Guardian.new(admin) }
 
       it 'allows ignoring user' do
-        expect(guardian.can_ignore_user?(another_user.id)).to eq(true)
+        expect(guardian.can_ignore_user?(another_user)).to eq(true)
       end
     end
 
@@ -2798,7 +2798,7 @@ describe Guardian do
       let(:guardian) { Guardian.new(trust_level_2) }
 
       it 'allows ignoring user' do
-        expect(guardian.can_ignore_user?(another_user.id)).to eq(true)
+        expect(guardian.can_ignore_user?(another_user)).to eq(true)
       end
     end
   end
@@ -2809,7 +2809,7 @@ describe Guardian do
 
     context "when muted user is the same as guardian user" do
       it 'does not allow muting user' do
-        expect(guardian.can_mute_user?(trust_level_1.id)).to eq(false)
+        expect(guardian.can_mute_user?(trust_level_1)).to eq(false)
       end
     end
 
@@ -2817,13 +2817,13 @@ describe Guardian do
       let!(:admin) { Fabricate(:user, admin: true) }
 
       it 'does not allow muting user' do
-        expect(guardian.can_mute_user?(admin.id)).to eq(false)
+        expect(guardian.can_mute_user?(admin)).to eq(false)
       end
     end
 
     context "when muted user is a normal user" do
       it 'allows muting user' do
-        expect(guardian.can_mute_user?(another_user.id)).to eq(true)
+        expect(guardian.can_mute_user?(another_user)).to eq(true)
       end
     end
 
@@ -2832,7 +2832,7 @@ describe Guardian do
       let!(:trust_level_0) { build(:user, trust_level: 0) }
 
       it 'does not allow muting user' do
-        expect(guardian.can_mute_user?(another_user.id)).to eq(false)
+        expect(guardian.can_mute_user?(another_user)).to eq(false)
       end
     end
 
@@ -2840,7 +2840,7 @@ describe Guardian do
       let(:guardian) { Guardian.new(admin) }
 
       it 'allows muting user' do
-        expect(guardian.can_mute_user?(another_user.id)).to eq(true)
+        expect(guardian.can_mute_user?(another_user)).to eq(true)
       end
     end
 
@@ -2848,7 +2848,7 @@ describe Guardian do
       let(:guardian) { Guardian.new(trust_level_1) }
 
       it 'allows muting user' do
-        expect(guardian.can_mute_user?(another_user.id)).to eq(true)
+        expect(guardian.can_mute_user?(another_user)).to eq(true)
       end
     end
   end
diff --git a/spec/serializers/user_serializer_spec.rb b/spec/serializers/user_serializer_spec.rb
index fbe25589db0..9aa80e5011b 100644
--- a/spec/serializers/user_serializer_spec.rb
+++ b/spec/serializers/user_serializer_spec.rb
@@ -192,6 +192,41 @@ describe UserSerializer do
         end
       end
     end
+
+    describe "ignored and muted" do
+      fab!(:viewing_user) { Fabricate(:user) }
+      let(:scope) { Guardian.new(viewing_user) }
+
+      it 'returns false values for muted and ignored' do
+        expect(json[:ignored]).to eq(false)
+        expect(json[:muted]).to eq(false)
+      end
+
+      context 'when ignored' do
+        before do
+          Fabricate(:ignored_user, user: viewing_user, ignored_user: user)
+          viewing_user.reload
+        end
+
+        it 'returns true for ignored' do
+          expect(json[:ignored]).to eq(true)
+          expect(json[:muted]).to eq(false)
+        end
+      end
+
+      context 'when muted' do
+        before do
+          Fabricate(:muted_user, user: viewing_user, muted_user: user)
+          viewing_user.reload
+        end
+
+        it 'returns true for muted' do
+          expect(json[:muted]).to eq(true)
+          expect(json[:ignored]).to eq(false)
+        end
+      end
+
+    end
   end
 
   context "with custom_fields" do