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

fix returning the correct pseudonym in the API 

test plain:
 * create a user with multiple pseudonyms in multiple accounts
 * API requests should return the pseudonym info only of
   pseudonyms that apply to the account related to the request
   being sent
 * SIS info should be returned even if a non SIS pseudonym is
   listed first

Change-Id: I895cc6fe2854dcd768590eeae16304b6092f2317
Reviewed-on: https://gerrit.instructure.com/8081
Tested-by: Hudson <hudson@instructure.com>
Reviewed-by: Brian Palmer <brianp@instructure.com>
This commit is contained in:
Cody Cutrer 2011-09-14 09:49:21 -06:00
parent 9f5735924a
commit e8c05e675d
6 changed files with 92 additions and 29 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
app/controllers/courses_controller.rb
View File

@ -227,7 +227,7 @@ class CoursesController < ApplicationController
if include_students
proxy = section.enrollments
if user_json_is_admin?
proxy = proxy.scoped(:include => { :user => :pseudonym })
proxy = proxy.scoped(:include => { :user => :pseudonyms })
else
proxy = proxy.scoped(:include => :user)
end
@ -256,7 +256,7 @@ class CoursesController < ApplicationController
if authorized_action(@context, @current_user, :read_roster)
proxy = @context.students
if user_json_is_admin?
proxy = proxy.scoped(:include => :pseudonym)
proxy = proxy.scoped(:include => :pseudonyms)
end
render :json => proxy.map { |u| user_json(u, @current_user, session) }
end

6
app/models/user.rb
View File

@ -914,7 +914,11 @@ class User < ActiveRecord::Base
def sis_pseudonym_for(context)
root_account = context.root_account
raise "could not resolve root account" unless root_account.is_a?(Account)
self.pseudonyms.active.find_by_account_id(root_account.id, :conditions => ["sis_user_id IS NOT NULL"])
if self.pseudonyms.loaded?
self.pseudonyms.detect { |p| p.active? && p.sis_user_id && p.account_id == root_account.id }
else
self.pseudonyms.active.find_by_account_id(root_account.id, :conditions => ["sis_user_id IS NOT NULL"])
end
end
set_policy do

22
lib/api/v1/user.rb
View File

@ -26,14 +26,18 @@ module Api::V1::User
def user_json(user, current_user, session, includes = [], context = @context)
api_json(user, current_user, session, API_USER_JSON_OPTS).tap do |json|
if user_json_is_admin?(context, current_user) && pseudonym = user.pseudonym
# the sis fields on pseudonym are poorly named -- sis_user_id is
# the id in the SIS import data, where on every other table
# that's called sis_source_id.
json.merge! :sis_user_id => pseudonym.sis_user_id,
# TODO: don't send sis_login_id; it's garbage data
:sis_login_id => pseudonym.sis_user_id ? pseudonym.unique_id : nil,
:login_id => pseudonym.unique_id
if user_json_is_admin?(context, current_user)
if sis_pseudonym = user.sis_pseudonym_for(@domain_root_account)
# the sis fields on pseudonym are poorly named -- sis_user_id is
# the id in the SIS import data, where on every other table
# that's called sis_source_id.
json.merge! :sis_user_id => sis_pseudonym.sis_user_id,
# TODO: don't send sis_login_id; it's garbage data
:sis_login_id => sis_pseudonym.unique_id
end
if pseudonym = sis_pseudonym || user.find_pseudonym_for_account(@domain_root_account)
json[:login_id] = pseudonym.unique_id
end
end
if service_enabled?(:avatars) && includes.include?('avatar_url')
json["avatar_url"] = avatar_image_url(user.id)
@ -41,7 +45,7 @@ module Api::V1::User
end
end
# optimization hint, currently user only needs to pull pseudonym from the db
# optimization hint, currently user only needs to pull pseudonyms from the db
# if a site admin is making the request or they can manage_students
def user_json_is_admin?(context = @context, current_user = @current_user)
@user_json_is_admin ||= {}

21
spec/apis/v1/enrollments_api_spec.rb
View File

@ -276,6 +276,17 @@ describe EnrollmentsApiController, :type => :integration do
res = res + @course.send("#{type}_enrollments").scoped(:include => :user)
end
json.should == enrollments.map do |e|
user_json = {
'name' => e.user.name,
'sortable_name' => e.user.sortable_name,
'short_name' => e.user.short_name,
'id' => e.user.id,
'login_id' => e.user.pseudonym ? e.user.pseudonym.unique_id : nil
}
user_json.merge!({
'sis_user_id' => e.user.pseudonym.sis_user_id,
'sis_login_id' => e.user.pseudonym.unique_id,
}) if e.user.pseudonym && e.user.pseudonym.sis_user_id
{
'root_account_id' => e.root_account_id,
'limit_privileges_to_course_section' => e.limit_privileges_to_course_section,
@ -285,15 +296,7 @@ describe EnrollmentsApiController, :type => :integration do
'type' => e.type,
'course_section_id' => e.course_section_id,
'course_id' => e.course_id,
'user' => {
'name' => e.user.name,
'sortable_name' => e.user.sortable_name,
'short_name' => e.user.short_name,
'id' => e.user.id,
'sis_user_id' => e.user.pseudonym ? e.user.pseudonym.sis_user_id : nil,
'sis_login_id' => e.user.pseudonym && e.user.sis_user_id ? e.user.pseudonym.unique_id : nil,
'login_id' => e.user.pseudonym ? e.user.pseudonym.unique_id : nil
}
'user' => user_json
}
end
end

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

@ -23,6 +23,9 @@ class TestUserApi
attr_accessor :services_enabled, :context, :current_user
def service_enabled?(service); @services_enabled.include? service; end
def avatar_image_url(user_id); "avatar_image_url(#{user_id})"; end
def initialize
@domain_root_account = Account.default
end
end
describe Api::V1::User do
@ -48,6 +51,54 @@ describe Api::V1::User do
})
end
it 'should use the correct SIS pseudonym' do
@user = User.create!(:name => 'User')
@account2 = Account.create!
@user.pseudonyms.create!(:unique_id => 'abc', :account => @account2) { |p| p.sis_user_id = 'abc' }
@user.pseudonyms.create!(:unique_id => 'xyz', :account => Account.default) { |p| p.sis_user_id = 'xyz' }
@test_api.user_json(@user, @admin, {}, [], Account.default).should == {
'name' => 'User',
'sortable_name' => 'User',
'sis_user_id' => 'xyz',
'id' => @user.id,
'short_name' => 'User',
'login_id' => 'xyz',
'sis_login_id' => 'xyz'
}
end
it 'should use the SIS pseudonym instead of another pseudonym' do
@user = User.create!(:name => 'User')
@account2 = Account.create!
@user.pseudonyms.create!(:unique_id => 'abc', :account => Account.default)
@user.pseudonyms.create!(:unique_id => 'xyz', :account => Account.default) { |p| p.sis_user_id = 'xyz' }
@test_api.user_json(@user, @admin, {}, [], Account.default).should == {
'name' => 'User',
'sortable_name' => 'User',
'sis_user_id' => 'xyz',
'id' => @user.id,
'short_name' => 'User',
'login_id' => 'xyz',
'sis_login_id' => 'xyz'
}
end
it 'should use the correct pseudonym' do
@user = User.create!(:name => 'User')
@account2 = Account.create!
@user.pseudonyms.create!(:unique_id => 'abc', :account => @account2)
@pseudonym = @user.pseudonyms.create!(:unique_id => 'xyz', :account => Account.default)
@user.stubs(:find_pseudonym_for_account).with(Account.default).returns(@pseudonym)
@test_api.user_json(@user, @admin, {}, [], Account.default).should == {
'name' => 'User',
'sortable_name' => 'User',
'id' => @user.id,
'short_name' => 'User',
'login_id' => 'xyz',
}
end
def test_context(mock_context, context_to_pass)
mock_context.expects(:account).returns(mock_context)
mock_context.expects(:id).returns(42)
@ -179,8 +230,6 @@ describe "Users API", :type => :integration do
'sortable_name' => 'User',
'short_name' => 'User',
'primary_email' => 'nobody@example.com',
'sis_user_id' => nil,
'sis_login_id' => nil,
'login_id' => 'nobody@example.com',
'avatar_url' => "http://www.example.com/images/users/#{@admin.id}",
'calendar' => { 'ics' => "http://www.example.com/feeds/calendars/user_#{@admin.uuid}.ics" },
@ -250,15 +299,14 @@ describe "Users API", :type => :integration do
describe "user account listing" do
it "should return users for an account" do
@account = @user.account
@account = Account.default
users = []
[['Test User1', 'test@example.com'], ['Test User2', 'test2@example.com'], ['Test User3', 'test3@example.com']].each_with_index do |u, i|
users << User.create(:name => u[0])
users[i].pseudonyms.create(:unique_id => u[1], :password => '123456', :password_confirmation => '123456')
users[i].pseudonym.update_attribute(:sis_user_id, (i + 1) * 100)
users[i].pseudonym.update_attribute(:account_id, @account.id)
users << User.create!(:name => u[0])
users[i].pseudonyms.create!(:unique_id => u[1], :account => @account) { |p| p.sis_user_id = u[1] }
end
@account.all_users.scoped(:order => :sortable_name).each_with_index do |user, i|
next unless users.find { |u| u == user }
json = api_call(:get, "/api/v1/accounts/#{@account.id}/users",
{ :controller => 'users', :action => 'index', :account_id => @account.id.to_param, :format => 'json' },
{ :per_page => 1, :page => i + 1 })
@ -269,7 +317,7 @@ describe "Users API", :type => :integration do
'id' => user.id,
'short_name' => user.short_name,
'login_id' => user.pseudonym.unique_id,
'sis_login_id' => user.pseudonym.sis_user_id ? user.pseudonym.unique_id : nil
'sis_login_id' => user.pseudonym.sis_user_id
}]
end
end

4
spec/models/user_spec.rb
View File

@ -1139,6 +1139,7 @@ describe User do
u = User.create!
pseudonyms = mock()
u.stubs(:pseudonyms).returns(pseudonyms)
pseudonyms.stubs(:loaded?).returns(false)
pseudonyms.stubs(:active).returns(pseudonyms)
pseudonyms.expects(:find_by_account_id).with(@account.id, :conditions => ["sis_user_id IS NOT NULL"]).returns(42)
u.sis_pseudonym_for(@course).should == 42
@ -1151,6 +1152,7 @@ describe User do
u = User.create!
pseudonyms = mock()
u.stubs(:pseudonyms).returns(pseudonyms)
pseudonyms.stubs(:loaded?).returns(false)
pseudonyms.stubs(:active).returns(pseudonyms)
pseudonyms.expects(:find_by_account_id).with(@account.id, :conditions => ["sis_user_id IS NOT NULL"]).returns(42)
u.sis_pseudonym_for(@group).should == 42
@ -1162,6 +1164,7 @@ describe User do
u = User.create!
pseudonyms = mock()
u.stubs(:pseudonyms).returns(pseudonyms)
pseudonyms.stubs(:loaded?).returns(false)
pseudonyms.stubs(:active).returns(pseudonyms)
pseudonyms.expects(:find_by_account_id).with(@root_account.id, :conditions => ["sis_user_id IS NOT NULL"]).returns(42)
u.sis_pseudonym_for(@account).should == 42
@ -1172,6 +1175,7 @@ describe User do
u = User.create!
pseudonyms = mock()
u.stubs(:pseudonyms).returns(pseudonyms)
pseudonyms.stubs(:loaded?).returns(false)
pseudonyms.stubs(:active).returns(pseudonyms)
pseudonyms.expects(:find_by_account_id).with(@account.id, :conditions => ["sis_user_id IS NOT NULL"]).returns(42)
u.sis_pseudonym_for(@account).should == 42