diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb index 51a13920bb5..0b175c6dd8c 100644 --- a/app/controllers/accounts_controller.rb +++ b/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") diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 35a7717771e..2f9c9d7186e 100644 --- a/app/controllers/users_controller.rb +++ b/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) } diff --git a/lib/api.rb b/lib/api.rb index 12ec9473411..ed8309074a3 100644 --- a/lib/api.rb +++ b/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. diff --git a/lib/user_search.rb b/lib/user_search.rb index f55b7fc3176..81f8a3c5129 100644 --- a/lib/user_search.rb +++ b/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 diff --git a/spec/apis/v1/users_api_spec.rb b/spec/apis/v1/users_api_spec.rb index d0eae72c265..492e1685ba6 100644 --- a/spec/apis/v1/users_api_spec.rb +++ b/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 diff --git a/spec/controllers/accounts_controller_spec.rb b/spec/controllers/accounts_controller_spec.rb index f66b318f9e7..32881addf73 100644 --- a/spec/controllers/accounts_controller_spec.rb +++ b/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) diff --git a/ui/features/account_course_user_search/react/store/CoursesStore.js b/ui/features/account_course_user_search/react/store/CoursesStore.js index 83145052185..2a76f761052 100644 --- a/ui/features/account_course_user_search/react/store/CoursesStore.js +++ b/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' diff --git a/ui/features/account_course_user_search/react/store/UsersStore.js b/ui/features/account_course_user_search/react/store/UsersStore.js index fc9acc6d529..bcef30084c6 100644 --- a/ui/features/account_course_user_search/react/store/UsersStore.js +++ b/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' }