From 32bb7196e8f0d1751d850cc6d4b1eec4e8067fae Mon Sep 17 00:00:00 2001 From: Mysti Sadler Date: Mon, 5 Feb 2018 21:24:54 -0700 Subject: [PATCH] Run user search query on the slave Fixes ADMIN-687 Test plan - In the course, load up the user's page and verify that you can search for a user without any issues - In the logs, the queries that show should register as running on the slave instead of master - Verify /api/v1/courses/:id/search_users works as expected Change-Id: I013b666b12e90e9d4cd1978e6a61da805f91d0d3 Reviewed-on: https://gerrit.instructure.com/139979 Tested-by: Jenkins Reviewed-by: Jeremy Stanley QA-Review: Deepeeca Soundarrajan Product-Review: Mysti Sadler --- app/controllers/courses_controller.rb | 126 ++++++++++++++------------ lib/user_search.rb | 3 +- 2 files changed, 68 insertions(+), 61 deletions(-) diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb index a6f20888896..cf2e6ff5ea4 100644 --- a/app/controllers/courses_controller.rb +++ b/app/controllers/courses_controller.rb @@ -896,72 +896,78 @@ class CoursesController < ApplicationController search_params[:include_inactive_enrollments] = true if include_inactive search_term = search_params[:search_term].presence - if search_term - users = UserSearch.for_user_in_context(search_term, @context, @current_user, session, search_params) - else - users = UserSearch.scope_for(@context, @current_user, search_params) - end - # If a user_id is passed in, modify the page parameter so that the page - # that contains that user is returned. - # We delete it from params so that it is not maintained in pagination links. - user_id = params[:user_id] - if user_id.present? && user = users.where(:users => { :id => user_id }).first - position_scope = users.where("#{User.sortable_name_order_by_clause}<=#{User.best_unicode_collation_key('?')}", user.sortable_name) - position = position_scope.distinct.count(:all) - per_page = Api.per_page_for(self) - params[:page] = (position.to_f / per_page.to_f).ceil - end + Shackles.activate(:slave) do + users = if search_term + UserSearch.for_user_in_context(search_term, @context, @current_user, session, search_params) + else + UserSearch.scope_for(@context, @current_user, search_params) + end - user_ids = params[:user_ids] - if user_ids.present? - user_ids = user_ids.split(",") if user_ids.is_a?(String) - users = users.where(id: user_ids) - end - - users = Api.paginate(users, self, api_v1_course_users_url) - includes = Array(params[:include]) - - # user_json_preloads loads both active/accepted and deleted - # group_memberships when passed "group_memberships: true." In a - # known case in the wild, each student had thousands of deleted - # group memberships. Since we only care about active group - # memberships for this course, load the data in a more targeted way. - user_json_preloads(users, includes.include?('email')) - include_group_ids = includes.delete('group_ids').present? - - unless includes.include?('test_student') || Array(params[:enrollment_type]).include?("student_view") - users.reject! do |u| - u.preferences[:fake_student] + # If a user_id is passed in, modify the page parameter so that the page + # that contains that user is returned. + # We delete it from params so that it is not maintained in pagination links. + user_id = params[:user_id] + if user_id.present? && (user = users.where(:users => { :id => user_id }).first) + position_scope = users.where("#{User.sortable_name_order_by_clause}<=#{User.best_unicode_collation_key('?')}", + user.sortable_name) + position = position_scope.distinct.count(:all) + per_page = Api.per_page_for(self) + params[:page] = (position.to_f / per_page.to_f).ceil end - end - if includes.include?('enrollments') - enrollment_scope = @context.enrollments. - where(user_id: users). - preload(:course, :scores) - if search_params[:enrollment_state] - enrollment_scope = enrollment_scope.where(:workflow_state => search_params[:enrollment_state]) + user_ids = params[:user_ids] + if user_ids.present? + user_ids = user_ids.split(",") if user_ids.is_a?(String) + users = users.where(id: user_ids) + end + + users = Api.paginate(users, self, api_v1_course_users_url) + includes = Array(params[:include]) + + # user_json_preloads loads both active/accepted and deleted + # group_memberships when passed "group_memberships: true." In a + # known case in the wild, each student had thousands of deleted + # group memberships. Since we only care about active group + # memberships for this course, load the data in a more targeted way. + user_json_preloads(users, includes.include?('email')) + include_group_ids = includes.delete('group_ids').present? + + unless includes.include?('test_student') || Array(params[:enrollment_type]).include?("student_view") + users.reject! do |u| + u.preferences[:fake_student] + end + end + if includes.include?('enrollments') + enrollment_scope = @context.enrollments. + where(user_id: users). + preload(:course, :scores) + + enrollment_scope = if search_params[:enrollment_state] + enrollment_scope.where(:workflow_state => search_params[:enrollment_state]) + elsif include_inactive + enrollment_scope.all_active_or_pending + else + enrollment_scope.active_or_pending + end + enrollments_by_user = enrollment_scope.group_by(&:user_id) else - enrollment_scope = include_inactive ? enrollment_scope.all_active_or_pending : enrollment_scope.active_or_pending + confirmed_user_ids = @context.enrollments.where.not(:workflow_state => %w{invited creation_pending rejected}). + where(:user_id => users).distinct.pluck(:user_id) end - enrollments_by_user = enrollment_scope.group_by(&:user_id) - else - confirmed_user_ids = @context.enrollments.where.not(:workflow_state => %w{invited creation_pending rejected}). - where(:user_id => users).distinct.pluck(:user_id) - end - render :json => users.map { |u| - enrollments = enrollments_by_user[u.id] || [] if includes.include?('enrollments') - user_unconfirmed = if enrollments - enrollments.all?{|e| %w{invited creation_pending rejected}.include?(e.workflow_state)} - else - !confirmed_user_ids.include?(u.id) - end - excludes = user_unconfirmed ? %w{pseudonym personal_info} : [] - user_json(u, @current_user, session, includes, @context, enrollments, excludes).tap do |json| - json[:group_ids] = active_group_memberships(users)[u.id]&.map(&:group_id) || [] if include_group_ids - end - } + render :json => users.map { |u| + enrollments = enrollments_by_user[u.id] || [] if includes.include?('enrollments') + user_unconfirmed = if enrollments + enrollments.all?{|e| %w{invited creation_pending rejected}.include?(e.workflow_state)} + else + !confirmed_user_ids.include?(u.id) + end + excludes = user_unconfirmed ? %w{pseudonym personal_info} : [] + user_json(u, @current_user, session, includes, @context, enrollments, excludes).tap do |json| + json[:group_ids] = active_group_memberships(users)[u.id]&.map(&:group_id) || [] if include_group_ids + end + } + end end end diff --git a/lib/user_search.rb b/lib/user_search.rb index b640a134bdf..fb7627956d4 100644 --- a/lib/user_search.rb +++ b/lib/user_search.rb @@ -40,7 +40,8 @@ module UserSearch restrict_search = true end context.shard.activate do - base_scope = base_scope.where(conditions_statement(search_term, context.root_account, {:restrict_search => restrict_search})) + # TODO: Need to optimize this as it's not using the base scope filter for the conditions statement query + base_scope = base_scope.where(conditions_statement(search_term, context.root_account, {restrict_search: restrict_search})) if options[:role_filter_id] && options[:role_filter_id] != "" base_scope = base_scope.where("#{options[:role_filter_id]} IN (SELECT role_id FROM #{Enrollment.quoted_table_name}