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

proper handling of cross-shard admin visibility 

when querying common contexts between user A and messageable user B, "0"
is used as a special course id to indicate user B has at least one
enrollment in an account A admins -- no actual course involved.

to manage common contexts cross shard, they are stored with global ids.
this was "globalizing" 0 to no longer be zero, causing problems. fix
that edge case.

additionally, if there were multiple shards with accounts in which B has
and enrollment and A is an admin, it would overwrite (given corrected
handling of the 0) with the last queried shard's values, rather than
combining them.

fixes CNVS-6303

test-plan:
 - have at least two shards
 - create user A on shard 1, user B on shard 2
 - enroll user B as a teacher in a course under account X on shard 2
 - add user A as an admin of account X
 - on shard 1, reload user B via user A's messageable users, e.g.:
     userB = userA.load_messageable_user(userB)
 - userB's common courses should include a key of 0 with
   'TeacherEnrollment' as the only value
 - repeat on shard 2
 - enroll user B as a student in a course under account Y on shard 1
 - add user A as an admin of account Y
 - on shard 1, reload user B via user A's messageable users (again)
 - userB's common courses should include a key of 0 with both
   'TeacherEnrollment' and 'StudentEnrollment' as values
 - repeat on shard 2

Change-Id: I014d1f0d7793889de512a4a0262e32027ee5702e
Reviewed-on: https://gerrit.instructure.com/21542
Reviewed-by: Cody Cutrer <cody@instructure.com>
Tested-by: Jenkins <jenkins@instructure.com>
QA-Review: Jeremy Putnam <jeremyp@instructure.com>
Product-Review: Jacob Fugal <jacob@instructure.com>
This commit is contained in:
Jacob Fugal 2013-06-17 14:13:43 -06:00
parent 0ed607f8fc
commit 043b797257
3 changed files with 57 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
lib/messageable_user.rb
View File

@ -125,7 +125,10 @@ class MessageableUser < User
if common_courses = read_attribute(:common_courses)
common_courses.to_s.split(',').each do |common_course|
course_id, role = common_course.split(':')
course_id = Shard.global_id_for(course_id.to_i)
course_id = course_id.to_i
# a course id of 0 indicates admin visibility without an actual shared
# course; don't "globalize" it
course_id = Shard.global_id_for(course_id) unless course_id.zero?
@global_common_courses[course_id] ||= []
@global_common_courses[course_id] << role
end
@ -141,6 +144,11 @@ class MessageableUser < User
end
end
def include_common_contexts_from(other)
combine_common_contexts(self.global_common_courses, other.global_common_courses)
combine_common_contexts(self.global_common_groups, other.global_common_groups)
end
private
def common_contexts_on_current_shard(common_contexts)
@ -149,7 +157,9 @@ class MessageableUser < User
return local_common_contexts if common_contexts.empty?
Shard.partition_by_shard(common_contexts.keys) do |sharded_ids|
sharded_ids.each do |id|
global_id = Shard.global_id_for(id)
# a context id of 0 indicates admin visibility without an actual shared
# context; don't "globalize" it
global_id = id == 0 ? id : Shard.global_id_for(id)
id = global_id unless Shard.current == target_shard
local_common_contexts[id] = common_contexts[global_id]
end
@ -157,6 +167,10 @@ class MessageableUser < User
local_common_contexts
end
def combine_common_contexts(left, right)
right.each{ |key,values| (left[key] ||= []).concat(values) }
end
# both bookmark_for and restrict_scope should always be executed on the
# same shard (not guaranteed, but we don't have to guarantee correctness if
# they aren't). so local ids here and local ids there have identical

13
lib/messageable_user/calculator.rb
View File

@ -215,8 +215,7 @@ class MessageableUser
end
BookmarkedCollection.merge(*collections) do |u1, u2|
u1.global_common_courses.merge!(u2.global_common_courses)
u1.global_common_groups.merge!(u2.global_common_groups)
u1.include_common_contexts_from(u2)
end
end
end
@ -258,7 +257,7 @@ class MessageableUser
reverse_lookup = users.index_by(&:id)
user_ids = reverse_lookup.keys
visible_enrollment_scope(scope_options).where(:id => user_ids).each do |user|
reverse_lookup[user.id].global_common_courses.merge!(user.global_common_courses)
reverse_lookup[user.id].include_common_contexts_from(user)
end
end
end
@ -272,7 +271,7 @@ class MessageableUser
reverse_lookup = missing_users.index_by(&:id)
missing_user_ids = reverse_lookup.keys
enrollment_scope(scope_options).where(:id => missing_user_ids, 'courses.id' => course.id).each do |user|
reverse_lookup[user.id].global_common_courses.merge!(user.global_common_courses)
reverse_lookup[user.id].include_common_contexts_from(user)
end
end
end
@ -285,7 +284,7 @@ class MessageableUser
reverse_lookup = users.index_by(&:id)
user_ids = reverse_lookup.keys
visible_account_user_scope(scope_options).where(:id => user_ids).each do |user|
reverse_lookup[user.id].global_common_courses.merge!(user.global_common_courses)
reverse_lookup[user.id].include_common_contexts_from(user)
end
end
end
@ -311,7 +310,7 @@ class MessageableUser
reverse_lookup = users.index_by(&:id)
user_ids = reverse_lookup.keys
fully_visible_group_user_scope(scope_options).where(:id => user_ids).each do |user|
reverse_lookup[user.id].global_common_groups.merge!(user.global_common_groups)
reverse_lookup[user.id].include_common_contexts_from(user)
end
end
end
@ -325,7 +324,7 @@ class MessageableUser
reverse_lookup = missing_users.index_by(&:id)
missing_user_ids = reverse_lookup.keys
group_user_scope(scope_options).where(:id => missing_user_ids, 'group_memberships.group_id' => group.id).each do |user|
reverse_lookup[user.id].global_common_groups.merge!(user.global_common_groups)
reverse_lookup[user.id].include_common_contexts_from(user)
end
end
end

35
spec/lib/messageable_user_spec.rb
View File

@ -190,6 +190,15 @@ describe "MessageableUser" do
end
end
end
it "should not translate a 0 key" do
user = MessageableUser.prepped(:common_course_column => 0, :common_role_column => "'StudentEnrollment'").first
[Shard.default, @shard1, @shard2].each do |shard|
shard.activate do
user.common_courses.should == {0 => ['StudentEnrollment']}
end
end
end
end
end
@ -221,4 +230,30 @@ describe "MessageableUser" do
end
end
end
describe "#include_common_contexts_from" do
before do
user(:active_all => 1)
end
it "should merge disparate ids" do
# e.g. two copies of the user from different shards with course
# visibility on each
user1 = MessageableUser.prepped(:common_course_column => 1, :common_role_column => "'StudentEnrollment'").first
user2 = MessageableUser.prepped(:common_course_column => 2, :common_role_column => "'StudentEnrollment'").first
user1.include_common_contexts_from(user2)
user1.common_courses[1].should include('StudentEnrollment')
user1.common_courses[2].should include('StudentEnrollment')
end
it "should stack coinciding ids" do
# e.g. two copies of the user from different shards with admin visibility
# on each
user1 = MessageableUser.prepped(:common_course_column => 0, :common_role_column => "'StudentEnrollment'").first
user2 = MessageableUser.prepped(:common_course_column => 0, :common_role_column => "'TeacherEnrollment'").first
user1.include_common_contexts_from(user2)
user1.common_courses[0].should include('StudentEnrollment')
user1.common_courses[0].should include('TeacherEnrollment')
end
end
end