diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb index 51e2d9d318e..dc118302d69 100644 --- a/app/controllers/courses_controller.rb +++ b/app/controllers/courses_controller.rb @@ -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 diff --git a/app/models/user.rb b/app/models/user.rb index 1c1c86a90b2..6da63ab0d56 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -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 diff --git a/lib/api/v1/user.rb b/lib/api/v1/user.rb index f1de7503f9c..d0e6162e82c 100644 --- a/lib/api/v1/user.rb +++ b/lib/api/v1/user.rb @@ -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 ||= {} diff --git a/spec/apis/v1/enrollments_api_spec.rb b/spec/apis/v1/enrollments_api_spec.rb index db1cd71e776..d815b8c78b9 100644 --- a/spec/apis/v1/enrollments_api_spec.rb +++ b/spec/apis/v1/enrollments_api_spec.rb @@ -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 diff --git a/spec/apis/v1/users_api_spec.rb b/spec/apis/v1/users_api_spec.rb index 1d89c644ed2..39444bcd4c9 100644 --- a/spec/apis/v1/users_api_spec.rb +++ b/spec/apis/v1/users_api_spec.rb @@ -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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 50080aa5254..39995b68c9e 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -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