From d0b713ddcde41dc8188cb8a6de60b70401ba73e5 Mon Sep 17 00:00:00 2001
From: Sebastian Serth <Sebastian.Serth@hpi.de>
Date: Tue, 21 Feb 2023 00:42:29 +0100
Subject: [PATCH] Dramatically improve RfC query

While the code gets more complex, the new query will first filter all relevant RfCs and only use the `with_last_activity` scope for the current page.

Using `distinct` in the policy is not required (no duplicated RfCs), but causes issues for the ordering otherwise. Therefore, it is removed.

Fixes CODEOCEAN-J2
---
 .../request_for_comments_controller.rb        | 39 ++++++++++++++-----
 app/policies/request_for_comment_policy.rb    |  4 +-
 2 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/app/controllers/request_for_comments_controller.rb b/app/controllers/request_for_comments_controller.rb
index 0011e3f1..40b701cb 100644
--- a/app/controllers/request_for_comments_controller.rb
+++ b/app/controllers/request_for_comments_controller.rb
@@ -17,15 +17,24 @@ class RequestForCommentsController < ApplicationController
   def index
     @search = policy_scope(RequestForComment)
       .last_per_user(2)
-      .with_last_activity
-      .ransack(params[:q])
-    @request_for_comments = @search.result
       .joins(:exercise)
       .where(exercises: {unpublished: false})
+      .order(created_at: :desc) # Order for the LIMIT part of the query
+      .ransack(params[:q])
+
+    # This total is used later to calculate the total number of entries
+    request_for_comments = @search.result
+      # All conditions are included in the query, so get the number of requested records
+      .paginate(page: params[:page], per_page: per_page_param)
+
+    @request_for_comments = RequestForComment.where(id: request_for_comments)
+      .with_last_activity # expensive query, so we only do it for the current page
       .includes(submission: %i[study_group exercise])
       .includes(:file, :comments, :user)
-      .order(created_at: :desc)
-      .paginate(page: params[:page], per_page: per_page_param)
+      .order(created_at: :desc) # Order for the view
+      # We need to "paginate" again to enable the pagination links.
+      # This `paginate` call is not expensive, because the total number of entries is already known.
+      .paginate(page: params[:page], per_page: per_page_param, total_entries: @search.result.length)
 
     authorize!
   end
@@ -33,20 +42,32 @@ class RequestForCommentsController < ApplicationController
   # GET /my_request_for_comments
   def my_comment_requests
     @search = policy_scope(RequestForComment)
-      .with_last_activity
       .where(user: current_user)
+      .order(created_at: :desc) # Order for the LIMIT part of the query
       .ransack(params[:q])
-    @request_for_comments = @search.result
+
+    # This total is used later to calculate the total number of entries
+    request_for_comments = @search.result
+      # All conditions are included in the query, so get the number of requested records
+      .paginate(page: params[:page], per_page: per_page_param)
+
+    @request_for_comments = RequestForComment.where(id: request_for_comments)
+      .with_last_activity
       .includes(submission: %i[study_group exercise])
       .includes(:file, :comments, :user)
-      .order(created_at: :desc)
-      .paginate(page: params[:page], per_page: per_page_param)
+      .order(created_at: :desc) # Order for the view
+      # We need to "paginate" again to enable the pagination links.
+      # This `paginate` call is not expensive, because the total number of entries is already known.
+      .paginate(page: params[:page], per_page: per_page_param, total_entries: @search.result.length)
+
     authorize!
     render 'index'
   end
 
   # GET /my_rfc_activity
   def rfcs_with_my_comments
+    # As we order by `last_comment`, we need to include `with_last_activity` in the original query.
+    # Therefore, the optimization chosen above doesn't work here.
     @search = policy_scope(RequestForComment)
       .with_last_activity
       .joins(:comments) # we don't need to outer join here, because we know the user has commented on these
diff --git a/app/policies/request_for_comment_policy.rb b/app/policies/request_for_comment_policy.rb
index ca759f82..160fbc15 100644
--- a/app/policies/request_for_comment_policy.rb
+++ b/app/policies/request_for_comment_policy.rb
@@ -63,14 +63,14 @@ class RequestForCommentPolicy < ApplicationPolicy
           when 'all'
             @scope.all
           when 'consumer'
-            rfcs_with_users = @scope.distinct
+            rfcs_with_users = @scope
               .joins('LEFT OUTER JOIN external_users ON request_for_comments.user_type = \'ExternalUser\' AND request_for_comments.user_id = external_users.id')
               .joins('LEFT OUTER JOIN internal_users ON request_for_comments.user_type = \'InternalUser\' AND request_for_comments.user_id = internal_users.id')
 
             rfcs_with_users.where(external_users: {consumer_id: @user.consumer.id})
               .or(rfcs_with_users.where(internal_users: {consumer_id: @user.consumer.id}))
           when 'study_group'
-            @scope.distinct
+            @scope
               .joins(:submission)
               .where(submission: {study_group: @user.current_study_group_id})
           else