Remove unnecessary query to try to speed up ht_users index page. (#243)

* Remove unnecessary query to try to speed up ht_users index page.
- `@all_users` was being populated via SQL in all cases even though it was only used for CSV download.
- The query has been removed and the CSV now uses the active + inactive arrays so the query was redundant anyway.
* - (Controller) `HTUser.includes` changed from `HTUser.joins` to pick up the approval requests without having to make a query for each.
- `HTApprovalRequest.most_recent_order` made public so the correct request can be selected by the presenter.
- `HTUser#ht_approval_request` uses `most_recent_order` by default for cases (if any) not covered by the controller change.

Author: moseshll

app/controllers/ht_users_controller.rb

@@ -7,10 +7,9 @@ class HTUsersController < ApplicationController
     usertype role access expires expire_type iprestrict mfa].freeze
 
   def index
-    users = HTUser.joins(:ht_institution).order("ht_institutions.name")
+    users = HTUser.includes(:ht_institution, :ht_approval_request).order("ht_institutions.name").order(HTApprovalRequest.most_recent_order)
     @users = users.active.map { |u| presenter u }
     @expired_users = users.expired.map { |u| presenter u }
-    @all_users = users.map { |u| presenter u }
     respond_to do |format|
       format.html
       format.csv do
@@ -65,9 +64,10 @@ def user_params
 
   def users_csv
     require "csv"
+    all_users = @users + @expired_users
     CSV.generate do |csv|
-      csv << @all_users.first.csv_cols
-      @all_users.each do |user|
+      csv << all_users.first.csv_cols
+      all_users.each do |user|
         if params[:role_filter]&.include?(user.role)
           next
         end

app/models/ht_approval_request.rb

@@ -7,6 +7,16 @@ class HTApprovalRequest < ApplicationRecord
   def self.expiration_date
     Date.today - 1.week
   end
+
+  # Make sure that the most recent and most "incomplete" request comes first when fetching request for user
+  # If we had created/updated timestamps we could use those.
+  def self.most_recent_order
+    {
+      Arel.sql("sent IS NULL") => :desc, :sent => :desc,
+      Arel.sql("received IS NULL") => :desc, :received => :desc,
+      Arel.sql("renewed IS NULL") => :desc, :renewed => :desc
+    }
+  end
   # "Active" requests that may be subject to further action
   scope :not_renewed, -> { where(renewed: nil) }
   # "Inactive" requests that are only of historical interest
@@ -15,11 +25,6 @@ def self.expiration_date
   scope :active, -> { where(renewed: nil, received: nil).where.not(sent: nil).where("sent >= ?", HTApprovalRequest.expiration_date) }
   scope :for_approver, ->(approver) { where(approver: approver).order(:sent, :received, :renewed) }
   scope :for_user, ->(user) { where(userid: user).order(:sent, :received, :renewed) }
-  # Make sure that the most recent and most "incomplete" request comes first when fetching request for user
-  # If we had created/updated timestamps we could use those.
-  most_recent_order = {Arel.sql("sent IS NULL") => :desc, :sent => :desc,
-                       Arel.sql("received IS NULL") => :desc, :received => :desc,
-                       Arel.sql("renewed IS NULL") => :desc, :renewed => :desc}
   scope :most_recent, ->(user) { for_user(user).order(most_recent_order) }
   scope :not_approved, -> { where(received: nil) }
   scope :approved, -> { where.not(received: nil) }

app/models/ht_user.rb

@@ -23,7 +23,7 @@ class HTUser < ApplicationRecord
   belongs_to :ht_institution, foreign_key: :inst_id, primary_key: :inst_id
   has_one :ht_count, foreign_key: :userid, primary_key: :userid
   has_many :ht_logs, -> { HTLog.ht_user }, foreign_key: :objid, primary_key: :email
-  has_many :ht_approval_request, foreign_key: :userid, primary_key: :email
+  has_many :ht_approval_request, -> { order(HTApprovalRequest.most_recent_order) }, foreign_key: :userid, primary_key: :email
   has_many :ht_registration, foreign_key: :applicant_email, primary_key: :email
 
   validates :iprestrict, presence: true, unless: :mfa

app/presenters/ht_user_presenter.rb

@@ -92,6 +92,8 @@ def show_expires
   end
 
   def show_institution
+    return institution if ht_institution.nil?
+
     link_to institution, ht_institution_path(ht_institution.inst_id)
   end
 
@@ -130,7 +132,7 @@ def simple_email_link
   end
 
   def approval_request
-    @approval_request ||= HTApprovalRequest.most_recent(email).first
+    @approval_request ||= ht_approval_request.first
   end
 
   def renewal_status_badge