From b639254de0f41f754e33ee259969a0fd0d1a89c5 Mon Sep 17 00:00:00 2001 From: James Williams Date: Mon, 6 Aug 2018 14:16:23 -0600 Subject: [PATCH] sort users not logged in last when sorting by last login test plan: * on the account user search page, when searching by login descending, it should put users without any login time last closes #CORE-1688 Change-Id: I2c13c52d6ada64c31865944f4a1bb32ebc2625ad Reviewed-on: https://gerrit.instructure.com/159807 Tested-by: Jenkins QA-Review: Jeremy Putnam Reviewed-by: Rob Orton Product-Review: Rob Orton --- app/controllers/courses_controller.rb | 2 +- app/models/user.rb | 2 +- lib/user_search.rb | 4 ++-- spec/apis/v1/users_api_spec.rb | 6 ++++++ 4 files changed, 10 insertions(+), 4 deletions(-) diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb index ef52e8f5584..bec81cfd5da 100644 --- a/app/controllers/courses_controller.rb +++ b/app/controllers/courses_controller.rb @@ -990,7 +990,7 @@ class CoursesController < ApplicationController get_context if authorized_action(@context, @current_user, :read_reports) scope = User.for_course_with_last_login(@context, @context.root_account_id, 'StudentEnrollment') - scope = scope.order('login_info_exists, last_login DESC') + scope = scope.order('last_login DESC NULLS LAST') users = Api.paginate(scope, self, api_v1_course_recent_students_url) user_json_preloads(users) render :json => users.map { |u| user_json(u, @current_user, session, ['last_login']) } diff --git a/app/models/user.rb b/app/models/user.rb index a855ce55ca5..5919f0bb46d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -305,7 +305,7 @@ class User < ActiveRecord::Base scope :for_course_with_last_login, lambda { |course, root_account_id, enrollment_type| # add a field to each user that is the aggregated max from current_login_at and last_login_at from their pseudonyms - scope = select("users.*, MAX(current_login_at) as last_login, MAX(current_login_at) IS NULL as login_info_exists"). + scope = select("users.*, MAX(current_login_at) as last_login"). # left outer join ensures we get the user even if they don't have a pseudonym joins(sanitize_sql([<<-SQL, root_account_id])).where(:enrollments => { :course_id => course }) LEFT OUTER JOIN #{Pseudonym.quoted_table_name} ON pseudonyms.user_id = users.id AND pseudonyms.account_id = ? diff --git a/lib/user_search.rb b/lib/user_search.rb index 3ea7de64ae8..4db8319346e 100644 --- a/lib/user_search.rb +++ b/lib/user_search.rb @@ -80,9 +80,9 @@ module UserSearch users = if options[:sort] == "last_login" if options[:order] == 'desc' - users.order("MAX(current_login_at) desc, id desc") + users.order(Arel.sql("MAX(current_login_at) DESC NULLS LAST, id DESC")) else - users.order("MAX(current_login_at), id") + users.order(Arel.sql("MAX(current_login_at), id")) end elsif options[:sort] == "username" if options[:order] == 'desc' diff --git a/spec/apis/v1/users_api_spec.rb b/spec/apis/v1/users_api_spec.rb index 3e27439ceb9..310f284b24c 100644 --- a/spec/apis/v1/users_api_spec.rb +++ b/spec/apis/v1/users_api_spec.rb @@ -647,6 +647,12 @@ describe "Users API", type: :request do expect(json.count).to eq 1 expect(json.first['last_login']).to eq p.current_login_at.iso8601 + + # it should sort too + json = api_call(:get, "/api/v1/accounts/#{@account.id}/users", + { :controller => 'users', :action => "index", :format => 'json', :account_id => @account.id.to_param }, + { include: ['last_login'], sort: "last_login", order: 'desc'}) + expect(json.first['last_login']).to eq p.current_login_at.iso8601 end end