Scope account/course user search better
fixes ADMIN-751 Test plan - Specs pass - Do smoke test on account and course search functions Change-Id: I2c7e32c32bc52c1a364ed40a89db0b7dc9557ffb Reviewed-on: https://gerrit.instructure.com/200682 Tested-by: Jenkins Reviewed-by: Jeremy Stanley <jeremy@instructure.com> QA-Review: Jeremy Stanley <jeremy@instructure.com> Product-Review: Mysti Lilla <mysti@instructure.com>
This commit is contained in:
Mysti Lilla
parent
ffd4743577
commit
0352326afa
|
|
@ -462,7 +462,7 @@ class UsersController < ApplicationController
|
|||
|
||||
includes = (params[:include] || []) & %w{avatar_url email last_login time_zone uuid}
|
||||
includes << 'last_login' if params[:sort] == 'last_login' && !includes.include?('last_login')
|
||||
users = users.with_last_login if includes.include?('last_login')
|
||||
users = users.with_last_login if includes.include?('last_login') && !search_term
|
||||
users = Api.paginate(users, self, api_v1_account_users_url, page_opts)
|
||||
user_json_preloads(users, includes.include?('email'))
|
||||
return render :json => users.map { |u| user_json(u, @current_user, session, includes)}
|
||||
|
|
|
|||
|
|
@ -28,31 +28,19 @@ module UserSearch
|
|||
@include_sis = context.grants_any_right?(searcher, session, :read_sis, :manage_sis)
|
||||
|
||||
context.shard.activate do
|
||||
base_scope = scope_for(context, searcher, options.slice(:enrollment_type, :enrollment_role,
|
||||
:enrollment_role_id, :exclude_groups, :enrollment_state, :include_inactive_enrollments, :sort, :order))
|
||||
users_scope = context_scope(context, searcher, options.slice(:enrollment_state, :include_inactive_enrollments))
|
||||
|
||||
# TODO: Need to optimize this as it's not using the base scope filter for the conditions statement query
|
||||
base_scope.where(conditions_statement(search_term, context.root_account))
|
||||
users_scope = users_scope.from("(#{conditions_statement(search_term, context.root_account, users_scope)}) AS users")
|
||||
users_scope = order_scope(users_scope, context, options.slice(:order, :sort))
|
||||
roles_scope(users_scope, context, options.slice(:enrollment_type, :enrollment_role,
|
||||
:enrollment_role_id, :exclude_groups))
|
||||
end
|
||||
end
|
||||
|
||||
def self.complex_search?
|
||||
@include_login || @include_email || @include_sis || @is_id
|
||||
end
|
||||
|
||||
def self.conditions_statement(search_term, root_account, options={})
|
||||
def self.conditions_statement(search_term, root_account, users_scope)
|
||||
pattern = like_string_for(search_term)
|
||||
conditions = []
|
||||
|
||||
if complex_search_enabled? && complex_search?
|
||||
# db_id is only used if the search_term.to_s =~ Api::ID_REGEX
|
||||
params = {pattern: pattern, account: root_account, path_type: CommunicationChannel::TYPE_EMAIL, db_id: search_term}
|
||||
conditions << complex_sql << params
|
||||
else
|
||||
conditions << like_condition('users.name') << {pattern: pattern}
|
||||
end
|
||||
|
||||
conditions
|
||||
params = {pattern: pattern, account: root_account, path_type: CommunicationChannel::TYPE_EMAIL, db_id: search_term}
|
||||
complex_sql(users_scope, params)
|
||||
end
|
||||
|
||||
def self.like_string_for(search_term)
|
||||
|
|
@ -61,74 +49,76 @@ module UserSearch
|
|||
end
|
||||
|
||||
def self.scope_for(context, searcher, options={})
|
||||
enrollment_roles = Array(options[:enrollment_role]) if options[:enrollment_role]
|
||||
enrollment_role_ids = Array(options[:enrollment_role_id]) if options[:enrollment_role_id]
|
||||
enrollment_types = Array(options[:enrollment_type]) if options[:enrollment_type]
|
||||
users_scope = context_scope(context, searcher, options.slice(:enrollment_state, :include_inactive_enrollments))
|
||||
users_scope = users_scope.select("users.*")
|
||||
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))
|
||||
end
|
||||
|
||||
def self.context_scope(context, searcher, options={})
|
||||
enrollment_states = Array(options[:enrollment_state]) if options[:enrollment_state]
|
||||
include_prior_enrollments = !options[:enrollment_state].nil?
|
||||
include_inactive_enrollments = !!options[:include_inactive_enrollments]
|
||||
if context.is_a?(Account)
|
||||
User.of_account(context).active
|
||||
elsif context.is_a?(Course)
|
||||
context.users_visible_to(searcher, include_prior_enrollments,
|
||||
enrollment_state: enrollment_states, include_inactive: include_inactive_enrollments).distinct
|
||||
else
|
||||
context.users_visible_to(searcher).distinct
|
||||
end
|
||||
end
|
||||
|
||||
def self.order_scope(users_scope, context, options={})
|
||||
order = ' DESC NULLS LAST, id DESC' if options[:order] == 'desc'
|
||||
if options[:sort] == "last_login"
|
||||
users_scope.order(Arel.sql("last_login#{order}"))
|
||||
elsif options[:sort] == "username"
|
||||
users_scope.order_by_sortable_name(direction: options[:order] == 'desc' ? :descending : :ascending)
|
||||
elsif options[:sort] == "email"
|
||||
users_scope = users_scope.select("users.*, (SELECT path FROM #{CommunicationChannel.quoted_table_name}
|
||||
WHERE communication_channels.user_id = users.id AND
|
||||
communication_channels.path_type = 'email' AND
|
||||
communication_channels.workflow_state <> 'retired'
|
||||
ORDER BY communication_channels.position ASC
|
||||
LIMIT 1)
|
||||
AS email")
|
||||
users_scope.order(Arel.sql("email#{order}"))
|
||||
elsif options[:sort] == "sis_id"
|
||||
users_scope = users_scope.select(User.send(:sanitize_sql, [
|
||||
"users.*, (SELECT sis_user_id FROM #{Pseudonym.quoted_table_name}
|
||||
WHERE pseudonyms.user_id = users.id AND
|
||||
pseudonyms.workflow_state <> 'deleted' AND
|
||||
pseudonyms.account_id = ?
|
||||
LIMIT 1) AS sis_user_id",
|
||||
context.root_account_id || context.id
|
||||
]))
|
||||
users_scope.order(Arel.sql("sis_user_id#{order}"))
|
||||
else
|
||||
users_scope.order_by_sortable_name
|
||||
end
|
||||
end
|
||||
|
||||
def self.roles_scope(users_scope, context, options={})
|
||||
enrollment_roles = Array(options[:enrollment_role]) if options[:enrollment_role]
|
||||
enrollment_role_ids = Array(options[:enrollment_role_id]) if options[:enrollment_role_id]
|
||||
enrollment_types = Array(options[:enrollment_type]) if options[:enrollment_type]
|
||||
exclude_groups = Array(options[:exclude_groups]) if options[:exclude_groups]
|
||||
|
||||
users = if context.is_a?(Account)
|
||||
User.of_account(context).active
|
||||
elsif context.is_a?(Course)
|
||||
context.users_visible_to(searcher, include_prior_enrollments,
|
||||
enrollment_state: enrollment_states, include_inactive: include_inactive_enrollments).distinct
|
||||
else
|
||||
context.users_visible_to(searcher).distinct
|
||||
end
|
||||
|
||||
users = if options[:sort] == "last_login"
|
||||
if options[:order] == 'desc'
|
||||
users.order(Arel.sql("MAX(current_login_at) DESC NULLS LAST, id DESC"))
|
||||
else
|
||||
users.order(Arel.sql("MAX(current_login_at), id"))
|
||||
end
|
||||
elsif options[:sort] == "username"
|
||||
if options[:order] == 'desc'
|
||||
users.order_by_sortable_name(direction: :descending)
|
||||
else
|
||||
users.order_by_sortable_name
|
||||
end
|
||||
elsif options[:sort] == "email"
|
||||
users = users.select("users.*, (SELECT path FROM #{CommunicationChannel.quoted_table_name}
|
||||
WHERE communication_channels.user_id = users.id AND
|
||||
communication_channels.path_type = 'email' AND
|
||||
communication_channels.workflow_state <> 'retired'
|
||||
ORDER BY communication_channels.position ASC
|
||||
LIMIT 1)
|
||||
AS email")
|
||||
if options[:order] == 'desc'
|
||||
users.order(Arel.sql("email DESC, id DESC"))
|
||||
else
|
||||
users.order(Arel.sql("email"))
|
||||
end
|
||||
elsif options[:sort] == "sis_id"
|
||||
users = users.select(User.send(:sanitize_sql, [
|
||||
"users.*, (SELECT sis_user_id FROM #{Pseudonym.quoted_table_name}
|
||||
WHERE pseudonyms.user_id = users.id AND
|
||||
pseudonyms.workflow_state <> 'deleted' AND
|
||||
pseudonyms.account_id = ?
|
||||
LIMIT 1) AS sis_user_id",
|
||||
context.root_account_id || context.id]))
|
||||
if options[:order] == 'desc'
|
||||
users.order(Arel.sql("sis_user_id DESC, id DESC"))
|
||||
else
|
||||
users.order(Arel.sql("sis_user_id"))
|
||||
end
|
||||
else
|
||||
users.order_by_sortable_name
|
||||
end
|
||||
|
||||
if enrollment_role_ids || enrollment_roles
|
||||
users = users.joins(:not_removed_enrollments).distinct if context.is_a?(Account)
|
||||
users_scope = users_scope.joins(:not_removed_enrollments).distinct if context.is_a?(Account)
|
||||
roles = if enrollment_role_ids
|
||||
enrollment_role_ids.map{|id| Role.get_role_by_id(id)}.compact
|
||||
else
|
||||
enrollment_roles.map{|name| context.is_a?(Account) ? context.get_course_role_by_name(name) :
|
||||
context.account.get_course_role_by_name(name)}.compact
|
||||
enrollment_roles.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.compact
|
||||
end
|
||||
users = users.where("role_id IN (?)", roles.map(&:id))
|
||||
users_scope = users_scope.where("role_id IN (?)", roles.map(&:id))
|
||||
elsif enrollment_types
|
||||
enrollment_types = enrollment_types.map { |e| "#{e.camelize}Enrollment" }
|
||||
if enrollment_types.any?{ |et| !Enrollment.readable_types.keys.include?(et) }
|
||||
|
|
@ -138,56 +128,74 @@ module UserSearch
|
|||
if context.is_a?(Account)
|
||||
# for example, one user can have multiple teacher enrollments, but
|
||||
# we only want one such a user record in results
|
||||
users = users.joins(:enrollments).merge(Enrollment.active.where(type: enrollment_types)).distinct
|
||||
users_scope = users_scope.joins(:enrollments).merge(Enrollment.active.where(type: enrollment_types)).distinct
|
||||
else
|
||||
if context.is_a?(Group) && context.context_type == "Course"
|
||||
users = users.joins(:enrollments).where(:enrollments => {:course_id => context.context_id})
|
||||
users_scope = users_scope.joins(:enrollments).where(:enrollments => {:course_id => context.context_id})
|
||||
end
|
||||
users = users.where(:enrollments => { :type => enrollment_types })
|
||||
users_scope = users_scope.where(:enrollments => { :type => enrollment_types })
|
||||
end
|
||||
end
|
||||
|
||||
if exclude_groups
|
||||
users = users.where(Group.not_in_group_sql_fragment(exclude_groups))
|
||||
users_scope = users_scope.where(Group.not_in_group_sql_fragment(exclude_groups))
|
||||
end
|
||||
|
||||
users
|
||||
users_scope
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
def self.complex_sql
|
||||
id_queries = ["SELECT id FROM #{User.quoted_table_name} WHERE (#{like_condition('users.name')})"]
|
||||
|
||||
if @include_login || @include_sis
|
||||
pseudonym_conditions = []
|
||||
pseudonym_conditions << like_condition('pseudonyms.unique_id') if @include_login
|
||||
pseudonym_conditions << like_condition('pseudonyms.sis_user_id') if @include_sis
|
||||
id_queries << <<-SQL
|
||||
SELECT user_id FROM #{Pseudonym.quoted_table_name}
|
||||
WHERE (#{pseudonym_conditions.join(' OR ')})
|
||||
AND pseudonyms.workflow_state='active'
|
||||
AND pseudonyms.account_id=:account
|
||||
SQL
|
||||
def self.complex_sql(users_scope, params)
|
||||
users_scope = users_scope.group(:id)
|
||||
queries = [name_sql(users_scope, params)]
|
||||
if complex_search_enabled?
|
||||
queries << id_sql(users_scope, params) if @is_id
|
||||
queries << login_sql(users_scope, params) if @include_login
|
||||
queries << sis_sql(users_scope, params) if @include_sis
|
||||
queries << email_sql(users_scope, params) if @include_email
|
||||
end
|
||||
queries.map(&:to_sql).join("\nUNION\n")
|
||||
end
|
||||
|
||||
if @is_id
|
||||
id_queries << "SELECT id FROM #{User.quoted_table_name} WHERE users.id IN (:db_id)"
|
||||
end
|
||||
def self.id_sql(users_scope, params)
|
||||
users_scope.select("users.*, MAX(current_login_at) as last_login").
|
||||
joins("LEFT JOIN #{Pseudonym.quoted_table_name} ON pseudonyms.user_id = users.id
|
||||
AND pseudonyms.account_id = #{User.connection.quote(params[:account])}
|
||||
AND pseudonyms.workflow_state = 'active'").
|
||||
where(id: params[:db_id]).
|
||||
group(:id)
|
||||
end
|
||||
|
||||
if @include_email
|
||||
id_queries << <<-SQL
|
||||
SELECT communication_channels.user_id FROM #{CommunicationChannel.quoted_table_name}
|
||||
WHERE EXISTS (SELECT 1 FROM #{UserAccountAssociation.quoted_table_name} AS uaa
|
||||
WHERE uaa.account_id= :account
|
||||
AND uaa.user_id=communication_channels.user_id)
|
||||
AND communication_channels.path_type = :path_type
|
||||
AND #{like_condition('communication_channels.path')}
|
||||
AND communication_channels.workflow_state IN ('active', 'unconfirmed')
|
||||
SQL
|
||||
end
|
||||
def self.name_sql(users_scope, params)
|
||||
users_scope.select("users.*, MAX(current_login_at) as last_login").
|
||||
joins("LEFT JOIN #{Pseudonym.quoted_table_name} ON pseudonyms.user_id = users.id
|
||||
AND pseudonyms.account_id = #{User.connection.quote(params[:account])}
|
||||
AND pseudonyms.workflow_state = 'active'").
|
||||
where(like_condition('users.name'), pattern: params[:pattern])
|
||||
end
|
||||
|
||||
"users.id IN (#{id_queries.join("\nUNION\n")})"
|
||||
def self.login_sql(users_scope, params)
|
||||
users_scope.select("users.*, MAX(current_login_at) as last_login").
|
||||
joins(:pseudonyms).
|
||||
where(pseudonyms: {account_id: params[:account], workflow_state: 'active'}).
|
||||
where(like_condition('pseudonyms.unique_id'), pattern: params[:pattern])
|
||||
end
|
||||
|
||||
def self.sis_sql(users_scope, params)
|
||||
users_scope.select("users.*, MAX(current_login_at) as last_login").
|
||||
joins(:pseudonyms).
|
||||
where(pseudonyms: {account_id: params[:account], workflow_state: 'active'}).
|
||||
where(like_condition('pseudonyms.sis_user_id'), pattern: params[:pattern])
|
||||
end
|
||||
|
||||
def self.email_sql(users_scope, params)
|
||||
users_scope.select("users.*, MAX(current_login_at) as last_login").
|
||||
joins(:communication_channels).
|
||||
joins("LEFT JOIN #{Pseudonym.quoted_table_name} ON pseudonyms.user_id = users.id
|
||||
AND pseudonyms.account_id = #{User.connection.quote(params[:account])}
|
||||
AND pseudonyms.workflow_state = 'active'").
|
||||
where(communication_channels: {workflow_state: ['active', 'unconfirmed'], path_type: params[:path_type]}).
|
||||
where(like_condition('communication_channels.path'), pattern: params[:pattern])
|
||||
end
|
||||
|
||||
def self.gist_search_enabled?
|
||||
|
|
|
|||
|
|
@ -206,6 +206,14 @@ describe UserSearch do
|
|||
expect(results).to include(other_user)
|
||||
end
|
||||
|
||||
it 'sorts by sis id' do
|
||||
User.find_by(name: 'Rose Tyler').pseudonyms.create!(unique_id: 'rose.tyler@example.com',
|
||||
sis_user_id: '25rose', account_id: course.root_account_id)
|
||||
User.find_by(name: 'Tyler Pickett').pseudonyms.create!(unique_id: 'tyler.pickett@example.com',
|
||||
sis_user_id: '1tyler', account_id: course.root_account_id)
|
||||
users = UserSearch.for_user_in_context('Tyler', course, user, nil, sort: 'sis_id')
|
||||
expect(users.map(&:name)).to eq ['Tyler Pickett', 'Rose Tyler', 'Tyler Teacher']
|
||||
end
|
||||
end
|
||||
|
||||
describe 'searching on emails' do
|
||||
|
|
@ -246,6 +254,13 @@ describe UserSearch do
|
|||
user.communication_channels.create!(path: 'unconfirmed@example.com')
|
||||
expect(UserSearch.for_user_in_context("unconfirmed", course, user)).to eq [user]
|
||||
end
|
||||
|
||||
it 'sorts by email' do
|
||||
User.find_by(name: 'Tyler Pickett').communication_channels.create!(path: '1tyler@example.com')
|
||||
User.find_by(name: 'Tyler Teacher').communication_channels.create!(path: '25teacher@example.com')
|
||||
users = UserSearch.for_user_in_context('Tyler', course, user, nil, sort: 'email')
|
||||
expect(users.map(&:name)).to eq ['Tyler Pickett', 'Tyler Teacher', 'Rose Tyler']
|
||||
end
|
||||
end
|
||||
|
||||
describe 'searching by a DB ID' do
|
||||
|
|
|
|||
Loading…
Reference in New Issue