compute total entries for UI course/user api index

This change will help prevent doing an n+1 of an expensive
query to fetch the next page in order to find the last page.
We are allowing Folio to compute the count for us _after_
we have made our expensive queries to filter down the result set
to be handed off for pagination. I made some optimizations
for particular cases when providing role ids for filtering user
role types in the user interface.

closes FOO-2931
flag = none

Test plan:
• You should *not* see the behavior outlined in FOO-2478
• Exercise both /accounts/1/users & /accounts/self api
  index actions and verify pagination works as expected
  (now /accounts/1/users will be populated with the last page
   since we compute the value and our UI sets it)

Change-Id: Ibd97406c6eb012c7f49c0a6a3968ca18b189a75b
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/294180
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Jacob Burroughs <jburroughs@instructure.com>
QA-Review: August Thornton <august@instructure.com>
Product-Review: August Thornton <august@instructure.com>

app/controllers/accounts_controller.rb

@@ -754,13 +754,18 @@ class AccountsController < ApplicationController
     # We only want to return the permissions for single courses and not lists of courses.
     # sections, needs_grading_count, and total_score not valid as enrollments are needed
     includes -= %w[permissions sections needs_grading_count total_scores]
-
-    # don't calculate a total count for this endpoint. total_entries: nil
     all_precalculated_permissions = nil
-    GuardRail.activate(:secondary) do
-      @courses = Api.paginate(@courses, self, api_v1_account_courses_url, { total_entries: nil })
 
-      ActiveRecord::Associations.preload(@courses, [:account, :root_account, course_account_associations: :account])
+    page_opts = { total_entries: nil }
+    if includes.include?("ui_invoked") && Setting.get("ui_invoked_count_pages", "true") == "true"
+      page_opts = {} # let Folio calculate total entries
+      includes.delete("ui_invoked")
+    end
+
+    GuardRail.activate(:secondary) do
+      @courses = Api.paginate(@courses, self, api_v1_account_courses_url, page_opts)
+
+      ActiveRecord::Associations.preload(@courses, [:account, :root_account, { course_account_associations: :account }])
       preload_teachers(@courses) if includes.include?("teachers")
       preload_teachers(@courses) if includes.include?("active_teachers")
       ActiveRecord::Associations.preload(@courses, [:enrollment_term]) if includes.include?("term") || includes.include?("concluded")

app/controllers/users_controller.rb

@@ -415,6 +415,9 @@ class UsersController < ApplicationController
     get_context
     return unless authorized_action(@context, @current_user, :read_roster)
 
+    includes = (params[:include] || []) & %w[avatar_url email last_login time_zone uuid ui_invoked]
+    includes << "last_login" if params[:sort] == "last_login" && !includes.include?("last_login")
+
     search_term = params[:search_term].presence
     if search_term
       users = UserSearch.for_user_in_context(search_term, @context, @current_user, session,
@@ -424,15 +427,22 @@ class UsersController < ApplicationController
                                              })
     else
       users = UserSearch.scope_for(@context, @current_user,
-                                   { order: params[:order], sort: params[:sort], enrollment_role_id: params[:role_filter_id],
-                                     enrollment_type: params[:enrollment_type] })
+                                   {
+                                     order: params[:order], sort: params[:sort], enrollment_role_id: params[:role_filter_id],
+                                     enrollment_type: params[:enrollment_type], ui_invoked: includes.include?("ui_invoked")
+                                   })
       users = users.with_last_login if params[:sort] == "last_login"
     end
 
-    includes = (params[:include] || []) & %w[avatar_url email last_login time_zone uuid]
-    includes << "last_login" if params[:sort] == "last_login" && !includes.include?("last_login")
+    page_opts = { total_entries: nil }
+    if includes.include?("ui_invoked") && Setting.get("ui_invoked_count_pages", "true") == "true"
+      page_opts = {} # let Folio calculate total entries
+      includes.delete("ui_invoked")
+    end
+
     GuardRail.activate(:secondary) do
-      users = Api.paginate(users, self, api_v1_account_users_url, { total_entries: nil })
+      users = Api.paginate(users, self, api_v1_account_users_url, page_opts)
+
       user_json_preloads(users, includes.include?("email"))
       User.preload_last_login(users, @context.resolved_root_account_id) if includes.include?("last_login") && params[:sort] != "last_login"
       render json: users.map { |u| user_json(u, @current_user, session, includes) }

lib/api.rb

@@ -433,19 +433,6 @@ module Api
     wrap_pagination_args!(pagination_args, controller)
     begin
       paginated = collection.paginate(pagination_args)
-      # If we aren't told the last page (because the AR .count was suppressed), then see if we
-      # can figure it out based on the contents of the next page. if it is short, then that's
-      # definitely the last page. Notice that we can only do this trick if we can perform
-      # arithmetic on the page numbers vs the page size, so if the pages are bookmark: urls
-      # then we just can't do this. TODO: the real fix for this is in the Folio gem
-      if paginated.ordinal_pages? && paginated.last_page.nil?
-        page_size = paginated.per_page
-        look_ahead = collection.paginate(pagination_args.merge({ page: paginated.next_page }))
-        next_page_len = look_ahead.length
-        if next_page_len < page_size # the next page (or possibly even this one) is the very last one
-          paginated.total_entries = (look_ahead.current_page.pred * page_size) + next_page_len
-        end # if the next page is full-sized, then we still don't know what the last page is
-      end
     rescue Folio::InvalidPage
       # Have to .try(:build_page) because we use some collections (like
       # PaginatedCollection) that do not conform to the full will_paginate API.

lib/user_search.rb

@@ -55,9 +55,9 @@ module UserSearch
     end
 
     def scope_for(context, searcher, options = {})
-      users_scope = context_scope(context, searcher, options.slice(:enrollment_state, :include_inactive_enrollments))
-      users_scope = order_scope(users_scope, context, options.slice(:order, :sort))
-      roles_scope(users_scope, context, options.slice(:enrollment_role, :enrollment_role_id, :enrollment_type, :exclude_groups))
+      users_scope = context_scope(context, searcher, options.slice(:enrollment_state, :include_inactive_enrollments, :enrollment_role_id, :ui_invoked))
+      users_scope = roles_scope(users_scope, context, options.slice(:enrollment_role, :enrollment_role_id, :enrollment_type, :exclude_groups, :ui_invoked))
+      order_scope(users_scope, context, options.slice(:order, :sort))
     end
 
     def context_scope(context, searcher, options = {})
@@ -66,7 +66,17 @@ module UserSearch
       include_inactive_enrollments = !!options[:include_inactive_enrollments]
       case context
       when Account
-        User.of_account(context).active
+        if options[:enrollment_role_id].present? && options[:ui_invoked].present?
+          # specifically don't use multishard scope for enrollments
+          # filter to the appropriate account where the enrollment role resides
+          User.joins(:enrollments)
+              .where.not(enrollments: { workflow_state: %w[rejected deleted inactive] })
+              .joins(:all_courses)
+              .where(courses: { account_id: context.id })
+              .distinct
+        else
+          User.of_account(context).active
+        end
       when Course
         context.users_visible_to(searcher, include_prior_enrollments,
                                  enrollment_state: enrollment_states, include_inactive: include_inactive_enrollments).distinct
@@ -113,19 +123,21 @@ module UserSearch
       exclude_groups = Array(options[:exclude_groups]) if options[:exclude_groups]
 
       if enrollment_role_ids || enrollment_roles
-        users_scope = users_scope.joins(:not_removed_enrollments).distinct if context.is_a?(Account)
-        roles = if enrollment_role_ids
-                  enrollment_role_ids.filter_map { |id| Role.get_role_by_id(id) }
-                else
-                  enrollment_roles.filter_map do |name|
-                    if context.is_a?(Account)
-                      context.get_course_role_by_name(name)
-                    else
-                      context.account.get_course_role_by_name(name)
-                    end
-                  end
-                end
-        users_scope = users_scope.where(enrollments: { role_id: roles.map(&:id) })
+        if context.is_a?(Account) && options[:ui_invoked].blank?
+          users_scope = users_scope.joins(:not_removed_enrollments).distinct
+        end
+        role_ids = if enrollment_role_ids
+                     enrollment_role_ids.filter_map { |id| Role.get_role_by_id(id).id }
+                   else
+                     enrollment_roles.filter_map do |name|
+                       if context.is_a?(Account)
+                         context.get_course_role_by_name(name).id
+                       else
+                         context.account.get_course_role_by_name(name).id
+                       end
+                     end
+                   end
+        users_scope = users_scope.where(enrollments: { role_id: role_ids })
       elsif enrollment_types
         enrollment_types = enrollment_types.map do |e|
           ce = e.camelize

spec/apis/v1/users_api_spec.rb

@@ -1085,6 +1085,17 @@ describe "Users API", type: :request do
       expect(json.map { |r| r["id"] }).to eq [@student.id]
     end
 
+    it "sets pagination total_pages/last page link if includes ui_invoked is set" do
+      Setting.set("ui_invoked_count_pages", "true")
+      @account = Account.default
+      @user = @admin
+      api_call(:get, "/api/v1/accounts/#{@account.id}/users",
+               { controller: "users", action: "api_index", format: "json", account_id: @account.id.to_param },
+               { role_filter_id: student_role.id.to_s, include: ["ui_invoked"] })
+      expect(response).to be_successful
+      expect(response.headers["Link"]).to include("last")
+    end
+
     context "includes last login info" do
       before :once do
         @account = Account.default
@@ -1186,13 +1197,29 @@ describe "Users API", type: :request do
       end
     end
 
-    it "does not return a next-page link on the last page" do
+    it "does return a next header on the last page" do
       @account = Account.default
       u = User.create!(name: "test user")
       u.pseudonyms.create!(account: @account, unique_id: "user")
 
       json = api_call(:get, "/api/v1/accounts/#{@account.id}/users", { controller: "users", action: "api_index", format: "json", account_id: @account.id.to_param }, { search_term: u.id.to_s, per_page: "1", page: "1" })
       expect(json.length).to eq 1
+      expect(response.headers["Link"]).to include("rel=\"next\"")
+      json = api_call(:get, "/api/v1/accounts/#{@account.id}/users", { controller: "users", action: "api_index", format: "json", account_id: @account.id.to_param }, { search_term: u.id.to_s, per_page: "1", page: "2" })
+      expect(json).to be_empty
+      expect(response.headers["Link"]).to_not include("rel=\"next\"")
+    end
+
+    it "does not return a next-page link on the last page" do
+      Setting.set("ui_invoked_count_pages", "true")
+      @account = Account.default
+      u = User.create!(name: "test user")
+      u.pseudonyms.create!(account: @account, unique_id: "user")
+
+      json = api_call(:get, "/api/v1/accounts/#{@account.id}/users",
+                      { controller: "users", action: "api_index", format: "json", account_id: @account.id.to_param },
+                      { search_term: u.id.to_s, per_page: "1", page: "1", include: ["ui_invoked"] })
+      expect(json.length).to eq 1
       expect(response.headers["Link"]).to_not include("rel=\"next\"")
     end
   end

spec/controllers/accounts_controller_spec.rb

@@ -1301,6 +1301,15 @@ describe AccountsController do
       expect(response.headers.to_a.find { |a| a.first == "Link" }.last).to_not include("last")
     end
 
+    it "sets pagination total_pages/last page link if includes ui_invoked is set" do
+      Setting.set("ui_invoked_count_pages", "true")
+      admin_logged_in(@account)
+      get "courses_api", params: { account_id: @account.id, per_page: 1, include: ["ui_invoked"] }
+
+      expect(response).to be_successful
+      expect(response.headers.to_a.find { |a| a.first == "Link" }.last).to include("last")
+    end
+
     it "properly removes sections from includes" do
       @s1 = @course.course_sections.create!
       @course.enroll_student(user_factory(active_all: true), section: @s1, allow_multiple_enrollments: true)

ui/features/account_course_user_search/react/store/CoursesStore.js

@@ -28,7 +28,14 @@ export default createStore({
   normalizeParams(originalParams) {
     const params = {
       ...originalParams,
-      include: ['total_students', 'active_teachers', 'subaccount', 'term', 'concluded'],
+      include: [
+        'total_students',
+        'active_teachers',
+        'subaccount',
+        'term',
+        'concluded',
+        'ui_invoked'
+      ],
       teacher_limit: 25,
       per_page: COURSES_TO_FETCH_PER_PAGE,
       no_avatar_fallback: '1'

ui/features/account_course_user_search/react/store/UsersStore.js

@@ -21,7 +21,7 @@ import createStore from './createStore'
 
 const USERS_TO_FETCH_PER_PAGE = 15
 const defaultParms = {
-  include: ['last_login', 'avatar_url', 'email', 'time_zone'],
+  include: ['last_login', 'avatar_url', 'email', 'time_zone', 'ui_invoked'],
   per_page: USERS_TO_FETCH_PER_PAGE,
   no_avatar_fallback: '1'
 }