ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

return maximum current_login_at of all relevant pseudonyms 

For trust links sometimes we see users logging into another
institution that trusts their current for a particular
course enrollment. They may have an active pseudonym on their
home shard, but not on the trust shard. Instead of querying
on non-existent pseudonyms look at all relevant pseudonyms
regardless of shard

fixes FOO-2066
flag = none

test plan:
• Have at least a trust with two accounts
• Create a user in account A
• Attempt to use the last_login API for the user in account B
  • /api/v1/users/<global_id>/?include[]=last_login
• Verify no error occurs and you get a value for the
  "last_login" attribute in the JSON response

Change-Id: I8b19721abd18ba05a76c85502b8c88ec86b692d9
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/269439
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Cody Cutrer <cody@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>
QA-Review: August Thornton <august@instructure.com>
This commit is contained in:
August Thornton 2021-07-19 14:39:48 -06:00
parent f92e92393a
commit 387161737a
4 changed files with 170 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
app/controllers/users_controller.rb
View File

@ -390,6 +390,7 @@ class UsersController < ApplicationController
def api_index
get_context
return unless authorized_action(@context, @current_user, :read_roster)
search_term = params[:search_term].presence
page_opts = {}
if search_term
@ -1290,12 +1291,19 @@ class UsersController < ApplicationController
@user = api_find(User, params[:id])
if @user.grants_right?(@current_user, session, :api_show_user)
includes = api_show_includes
# would've preferred to pass User.with_last_login as the collection to
# api_find but the implementation of that scope appears to be incompatible
# with what api_find does
if includes.include?('last_login')
@user.last_login = User.with_last_login.find(@user.id).read_attribute(:last_login)
pseudonyms =
SisPseudonym.for(
@user,
@domain_root_account,
type: :implicit,
require_sis: false,
include_all_pseudonyms: true
)
@user.last_login = pseudonyms.max_by(&:current_login_at).current_login_at
end
render json: user_json(@user, @current_user, session, includes, @domain_root_account),

79
app/models/sis_pseudonym.rb
View File

@ -19,16 +19,19 @@
class SisPseudonym
# type: :exact, :trusted, or :implicit
def self.for(user, context, type: :exact, require_sis: true, include_deleted: false, root_account: nil, in_region: false)
def self.for(user, context, type: :exact, require_sis: true, include_deleted: false, root_account: nil, in_region: false, include_all_pseudonyms: false)
raise ArgumentError("type must be :exact, :trusted, or :implicit") unless [:exact, :trusted, :implicit].include?(type)
raise ArgumentError("invalid root_account") if root_account && !root_account.root_account?
raise ArgumentError("context must respond to .root_account") unless root_account&.root_account? || context.respond_to?(:root_account)
self.new(user, context, type, require_sis, include_deleted, root_account, in_region: in_region).pseudonym
sis_pseudonym =
self.new(user, context, type, require_sis, include_deleted, root_account, in_region: in_region, include_all_pseudonyms: include_all_pseudonyms)
include_all_pseudonyms ? sis_pseudonym.all_pseudonyms : sis_pseudonym.pseudonym
end
attr_reader :user, :context, :type, :require_sis, :include_deleted
attr_reader :user, :context, :type, :require_sis, :include_deleted, :include_all_pseudonyms
def initialize(user, context, type, require_sis, include_deleted, root_account, in_region: false)
def initialize(user, context, type, require_sis, include_deleted, root_account, in_region: false, include_all_pseudonyms: false)
@user = user
@context = context
@type = type
@ -36,6 +39,7 @@ class SisPseudonym
@include_deleted = include_deleted
@root_account = root_account
@in_region = in_region
@include_all_pseudonyms = include_all_pseudonyms
end
def pseudonym
@ -50,6 +54,23 @@ class SisPseudonym
result
end
def all_pseudonyms
results = []
results << @context.sis_pseudonym if @context.class <= Enrollment
results << find_on_enrollment_for_context
results << find_in_home_account
results << find_in_other_accounts
results = results.flatten.compact.uniq
results.reject! {|result| exclude_deleted?(result)}
if results.present?
results.each do |result|
result.account = root_account if result.account_id == root_account.id
end
return results
end
nil
end
private
def exclude_deleted?(result)
@ -66,23 +87,22 @@ class SisPseudonym
# it will grab that one instead.
return nil if pseudonym&.sis_user_id.nil?
return nil if pseudonym&.workflow_state == 'deleted' && !@include_deleted
pseudonym
end
end
def find_in_other_accounts
return nil if type == :exact
if include_deleted
if user.all_pseudonyms_loaded?
return pick_user_pseudonym(user.all_pseudonyms, type == :trusted ? root_account.trusted_account_ids : nil)
end
else
if user.all_active_pseudonyms_loaded?
return pick_user_pseudonym(user.all_active_pseudonyms, type == :trusted ? root_account.trusted_account_ids : nil)
end
elsif user.all_active_pseudonyms_loaded?
return pick_user_pseudonym(user.all_active_pseudonyms, type == :trusted ? root_account.trusted_account_ids : nil)
end
trusted_account_ids = root_account.trusted_account_ids.group_by { |id| Shard.shard_for(id) } if type == :trusted
# try the user's home shard first if it's fast
@ -105,6 +125,7 @@ class SisPseudonym
Shard.with_each_shard(shards.sort) do
next if Shard.current == user.shard && user_shard_is_in_region
account_ids = trusted_account_ids[Shard.current] if type == :trusted
result = find_in_trusted_accounts(account_ids)
return result if result
@ -145,6 +166,7 @@ class SisPseudonym
@root_account ||= begin
account = context.root_account
raise "could not resolve root account" unless account.is_a?(Account)
account
end
end
@ -152,16 +174,22 @@ class SisPseudonym
def pick_pseudonym(account_ids)
relation = Pseudonym.active.where(user_id: user)
relation = relation.where(account_id: account_ids) if account_ids
relation = if require_sis
relation.where.not(sis_user_id: nil)
else
# false sorts before true
relation.order(Arel.sql("sis_user_id IS NULL"))
end
relation =
if require_sis
relation.where.not(sis_user_id: nil)
else
# false sorts before true
relation.order(Arel.sql("sis_user_id IS NULL"))
end
relation.primary_shard.activate do
relation = relation.order(Pseudonym.best_unicode_collation_key(:unique_id))
end
if type == :implicit
if include_all_pseudonyms
return relation.select { |p| p.works_for_account?(root_account, true) }
end
relation.detect { |p| p.works_for_account?(root_account, true) }
else
relation.first
@ -169,11 +197,22 @@ class SisPseudonym
end
def pick_user_pseudonym(collection, account_ids)
collection.sort_by {|p| [p.workflow_state, p.sis_user_id ? 0 : 1, Canvas::ICU.collation_key(p.unique_id)] }.detect do |p|
next if account_ids && !account_ids.include?(p.account_id)
next if !account_ids && !p.works_for_account?(root_account, type == :implicit)
next if require_sis && !p.sis_user_id
include_deleted || p.workflow_state != 'deleted'
if include_all_pseudonyms
collection.select do |p|
next if account_ids && !account_ids.include?(p.account_id)
next if !account_ids && !p.works_for_account?(root_account, type == :implicit)
next if require_sis && !p.sis_user_id
true
end
else
collection.sort_by {|p| [p.workflow_state, p.sis_user_id ? 0 : 1, Canvas::ICU.collation_key(p.unique_id)] }.detect do |p|
next if account_ids && !account_ids.include?(p.account_id)
next if !account_ids && !p.works_for_account?(root_account, type == :implicit)
next if require_sis && !p.sis_user_id
include_deleted || p.workflow_state != 'deleted'
end
end
end
end

47
spec/apis/v1/users_api_spec.rb
View File

@ -1077,6 +1077,53 @@ describe "Users API", type: :request do
{ include: ['last_login'], order: 'desc', search_term: 'test'})
expect(json.first['last_login']).to eq @p.current_login_at.iso8601
end
context "sharding" do
specs_require_sharding
it "should take all relevant pseudonyms and return the maximum current_login_at" do
@shard3.activate do
p4 = @u.pseudonyms.create!(account: @account, unique_id: 'p4')
p4.current_login_at = 4.minutes.ago
p4.save!
end
@shard2.activate do
account = Account.create!
allow(account).to receive(:trust_exists?).and_return(true)
allow(account).to receive(:trusted_account_ids).and_return([@account.id])
course = account.courses.create!
course.enroll_student(@u)
p2 = @u.pseudonyms.create!(account: account, unique_id: 'p2')
p2.current_login_at = 5.minutes.ago
p2.save!
p3 = @u.pseudonyms.create!(account: account, unique_id: 'p3')
p3.current_login_at = 6.minutes.ago
p3.save!
end
@shard1.activate do
account = Account.create!
course = account.courses.create!
course.enroll_student(@u)
account_admin_user
user_session(@user)
json =
api_call(
:get,
"/api/v1/users/#{@u.id}",
{
controller: 'users',
action: 'api_show',
id: @u.id.to_param,
format: 'json'
},
{ include: ['last_login'] }
)
expect(json.fetch('last_login')).to eq @p.current_login_at.iso8601
end
end
end
end
it "does return a next header on the last page" do

54
spec/models/sis_pseudonym_spec.rb
View File

@ -282,6 +282,60 @@ describe SisPseudonym do
expect(SisPseudonym.for(@user, Account.default, type: :implicit)).to eq @pseudonym
expect(SisPseudonym.for(@user, Account.default, type: :implicit, in_region: true)).to eq @pseudonym
end
it 'returns a collection of all relevant pseudonyms' do
@shard1.activate { @user = User.create! }
@pseudonym =
Account
.default
.pseudonyms
.create!(user: @user, unique_id: 'user') { |p| p.sis_user_id = 'abc' }
@shard2.activate do
expect(
SisPseudonym.for(@user, Account.default, type: :implicit, include_all_pseudonyms: true)
).to eq [@pseudonym]
end
end
it 'returns a collection of all relevant pre-loaded pseudonyms' do
@shard1.activate { @user = User.create! }
@pseudonym = Account.default.pseudonyms.create!(user: @user, unique_id: 'user')
@shard2.activate do
@user.pseudonyms.to_a
expect(
SisPseudonym.for(
@user,
Account.default,
type: :implicit,
require_sis: false,
include_all_pseudonyms: true
)
).to eq [@pseudonym]
end
end
it 'returns a collection of all relevant non-duplicated pseudonyms' do
@user = u
p1 = @user.pseudonyms.create!(pseud_params('a'))
p2 = @user.pseudonyms.create!(pseud_params('b'))
@shard1.activate do
account = account_model
course = account.courses.create!
course.enroll_student(@user)
@user.pseudonyms.create!(pseud_params('a', account))
@user.pseudonyms.create!(pseud_params('b', account))
@user.pseudonyms.create!(pseud_params('c', account))
end
expect(
SisPseudonym.for(
@user,
Account.default,
type: :implicit,
require_sis: false,
include_all_pseudonyms: true
)
).to eq [p1, p2]
end
end
end