Return admins in content share search

closes ADMIN-2907
flag=direct_share

Test plan
- Search should only return users by their name
- Should be able to search for any kind of admin
  (sub-account, root account, whatever)
- Should be able to search for teacher, ta and
  designers
- Should not be able to search for users with only
  deleted accounts/account admin membership/courses/
  or enrollments
- Should return all base Account Admin roles and
  any AccountMembership role that has the
  manage content permission at any account level

Change-Id: Ib2cb672fc6a44bbf201ae3bf6ee2c98a7b160018
Reviewed-on: https://gerrit.instructure.com/210328
Reviewed-by: Jeremy Stanley <jeremy@instructure.com>
Tested-by: Jenkins
QA-Review: Jeremy Stanley <jeremy@instructure.com>
Product-Review: Mysti Lilla <mysti@instructure.com>

app/controllers/courses_controller.rb

@@ -1100,11 +1100,30 @@ class CoursesController < ApplicationController
     return render json: { message: "Feature disabled" }, status: :forbidden unless @context.root_account.feature_enabled?(:direct_share)
     reject!('Search term required') unless params[:search_term]
     return unless authorized_action(@context, @current_user, :manage_content)
-    users = UserSearch.for_user_in_context(params[:search_term], @context.root_account,
-      @current_user, session, {enrollment_type: ['Ta', 'Teacher', 'Designer']}).
-      where.not(id: @current_user.id)
-    users = Api.paginate(users, self, api_v1_course_content_share_users_url)
-    render :json => users.map { |u| user_display_json(u, @context.root_account) }
+
+    users_scope = User.where(UserSearch.like_condition('users.name'), pattern: UserSearch.like_string_for(params[:search_term])).
+      where.not(id: @current_user.id).active.distinct
+    admin_users_scope = users_scope.joins(account_users: [:account, :role]).merge(AccountUser.active).merge(Account.active)
+
+    union_scope = users_scope.joins(enrollments: :course).merge(Enrollment.active.of_admin_type).merge(Course.active).
+      union(
+        root_account_scope(admin_users_scope.merge(Role.full_account_admin), @context.root_account_id),
+        root_account_scope(admin_users_scope.merge(Role.custom_account_admin_with_permission('manage_content')), @context.root_account_id),
+        sub_account_scope(admin_users_scope.merge(Role.full_account_admin), @context.root_account_id),
+        sub_account_scope(admin_users_scope.merge(Role.custom_account_admin_with_permission('manage_content')), @context.root_account_id)
+      ).
+      order(:name).
+      distinct
+    users = Api.paginate(union_scope, self, api_v1_course_content_share_users_url)
+    render :json => users_json(users, @current_user, session, ['avatar_url', 'email'])
+  end
+
+  def root_account_scope(scope, root_account_id)
+    scope.where(account_users: {account_id: root_account_id})
+  end
+
+  def sub_account_scope(scope, root_account_id)
+    scope.where(account_users: {accounts: {root_account_id: root_account_id}})
   end
 
   include Api::V1::PreviewHtml

app/models/role.rb

@@ -48,6 +48,7 @@ class Role < ActiveRecord::Base
 
   belongs_to :account
   belongs_to :root_account, :class_name => 'Account'
+  has_many :role_overrides
 
   before_validation :infer_root_account_id, :if => :belongs_to_account?
 
@@ -220,6 +221,17 @@ class Role < ActiveRecord::Base
   scope :inactive, -> { where(:workflow_state => 'inactive') }
   scope :for_courses, -> { where(:base_role_type => ENROLLMENT_TYPES) }
   scope :for_accounts, -> { where(:base_role_type => ACCOUNT_TYPES) }
+  scope :full_account_admin, -> { where(base_role_type: 'AccountAdmin') }
+  scope :custom_account_admin_with_permission, -> (permission) do
+    where(base_role_type: 'AccountMembership').
+    where("EXISTS (
+      SELECT 1
+      FROM #{RoleOverride.quoted_table_name}
+      WHERE role_overrides.role_id = roles.id
+        AND role_overrides.permission = ?
+        AND role_overrides.enabled = ?
+    )", permission, true)
+  end
 
   # Returns a list of hashes for each base enrollment type, and each will have a
   # custom_roles key, each will look like:

lib/api/v1/user.rb

@@ -161,6 +161,11 @@ module Api::V1::User
       ActiveRecord::Associations::Preloader.new.preload(context, :groups)
     end
 
+    if includes.include?('email') && !excludes.include?('personal_info') && context.grants_right?(current_user, session, :read_email_addresses)
+      ActiveRecord::Associations::Preloader.new.preload(users, :communication_channels)
+    end
+    ActiveRecord::Associations::Preloader.new.preload(users, :pseudonyms)
+
     users.map{ |user| user_json(user, current_user, session, includes, context, enrollments, excludes) }
   end
 

spec/controllers/courses_controller_spec.rb

@@ -2716,7 +2716,7 @@ describe CoursesController do
       expect(json[0]).to include({ "id" => student1.id, "uuid" => student1.uuid })
     end
 
-    it 'can sort uesrs' do
+    it 'can sort users' do
       student1.update!(name: 'Student B')
       student2.update!(name: 'Student A')
 
@@ -2745,7 +2745,7 @@ describe CoursesController do
       expect(response).to be_bad_request
     end
 
-    it 'requires the user to have manage content permission for the course' do
+    it 'requires the user to have an admin role for the course' do
       course_with_student_logged_in
       get 'content_share_users', params: {course_id: @course.id, search_term: 'teacher'}
       expect(response).to be_unauthorized
@@ -2757,6 +2757,16 @@ describe CoursesController do
       expect(response).to be_forbidden
     end
 
+    it 'should return email, url avatar, and name' do
+      user_session(@teacher)
+      @search_context = @course
+      course_with_teacher(name: 'course teacher')
+      @teacher.account.enable_service(:avatars)
+      get 'content_share_users', params: {course_id: @search_context.id, search_term: 'course'}
+      json = json_parse(response.body)
+      expect(json[0]).to include({'email' => nil, 'name' => 'course teacher', 'avatar_url' => "http://test.host/images/messages/avatar-50.png"})
+    end
+
     it 'searches for teachers, TAs, and designers' do
       user_session(@teacher)
       @search_context = @course
@@ -2767,7 +2777,50 @@ describe CoursesController do
       course_with_observer(name: 'course observer')
       get 'content_share_users', params: {course_id: @search_context.id, search_term: 'course'}
       json = json_parse(response.body)
-      expect(json.map{|user| user['display_name']}).to match_array(['course teacher', 'course ta', 'course designer'])
+      expect(json.map{|user| user['name']}).to eq(['course designer', 'course ta', 'course teacher'])
+    end
+
+    it 'should not return users with only deleted enrollments or deleted courses' do
+      user_session(@teacher)
+      @search_context = @course
+      course_with_teacher(name: 'course teacher').destroy
+      get 'content_share_users', params: {course_id: @search_context.id, search_term: 'course'}
+      json = json_parse(response.body)
+      expect(json.map{|user| user['name']}).not_to include('course teacher')
+
+      course_with_ta(name: 'course ta')
+      @course.destroy
+      get 'content_share_users', params: {course_id: @search_context.id, search_term: 'course'}
+      json = json_parse(response.body)
+      expect(json.map{|user| user['name']}).not_to include('course ta')
+    end
+
+    it 'search for root and sub-account admins' do
+      user_session(@teacher)
+      @search_context = @course
+      sub_account = account_model(parent_account: @course.root_account)
+      account_admin = user_factory(name: 'account admin')
+      sub_account_admin = user_factory(name: 'sub-account admin')
+      account_admin_user(account: @course.root_account, user: account_admin)
+      account_admin_user(account: sub_account, user: sub_account_admin)
+
+      get 'content_share_users', params: {course_id: @search_context.id, search_term: 'admin'}
+      json = json_parse(response.body)
+      expect(json.map{|user| user['name']}).to eq(['account admin', 'sub-account admin'])
+    end
+
+    it 'should not return users with deleted admin accounts' do
+      user_session(@teacher)
+      sub_account = account_model(parent_account: @course.root_account)
+      account_admin = user_factory(name: 'account admin')
+      sub_account_admin = user_factory(name: 'sub-account admin')
+      account_admin_user(account: @course.root_account, user: account_admin).destroy
+      account_admin_user(account: sub_account, user: sub_account_admin)
+      sub_account.destroy
+
+      get 'content_share_users', params: {course_id: @course.id, search_term: 'admin'}
+      json = json_parse(response.body)
+      expect(json.map{|user| user['name']}).not_to include('account admin', 'sub-account admin')
     end
 
     it 'should not return the searching user' do
@@ -2776,7 +2829,23 @@ describe CoursesController do
       course_with_teacher(name: 'course teacher')
       get 'content_share_users', params: {course_id: @search_context.id, search_term: 'teacher'}
       json = json_parse(response.body)
-      expect(json.map{|user| user['display_name']}).to match_array(['course teacher'])
+      expect(json.map{|user| user['name']}).to match_array(['course teacher'])
+    end
+
+    it 'should not return admin roles that do not have the "manage_content" permission' do
+      user_session(@teacher)
+      account_admin = user_factory(name: 'less privileged account admin')
+      role = custom_account_role('manage_content', account: @course.root_account)
+      account_admin_user(account: @course.root_account, user: account_admin, role: role)
+
+      get 'content_share_users', params: {course_id: @course.id, search_term: 'less privileged'}
+      json = json_parse(response.body)
+      expect(json.map{|user| user['name']}).not_to include('less privileged account admin')
+
+      role.role_overrides.create!(enabled: true, permission: 'manage_content', context: @course.root_account)
+      get 'content_share_users', params: {course_id: @course.id, search_term: 'less privileged'}
+      json = json_parse(response.body)
+      expect(json.map{|user| user['name']}).to include('less privileged account admin')
     end
   end
 end