From 4e6aa4375967043d0e68160ee03b2bc6a59aec03 Mon Sep 17 00:00:00 2001 From: Rob Orton Date: Fri, 21 Aug 2020 21:07:21 -0600 Subject: [PATCH] use correct id for merging observation links when merging a user if done from the source shard the target user.id is global, but from target shard is is local and the update call will fail and update the record to a local id. This could potentially be a user that should not be associated to that record test plan - specs should pass - merge users with observation links from target shard fixes VICE-734 flag=none Change-Id: Iad6007bfb4bc596f21a69a59555d5622f08d4721 Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/245751 Tested-by: Service Cloud Jenkins Reviewed-by: Ben Nelson QA-Review: Caleb Guanzon Product-Review: Caleb Guanzon --- app/models/user_observation_link.rb | 2 +- lib/user_merge.rb | 4 ++-- spec/models/split_users_spec.rb | 12 ++++++++++++ 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/app/models/user_observation_link.rb b/app/models/user_observation_link.rb index 25e2c1a03f9..709107d4df1 100644 --- a/app/models/user_observation_link.rb +++ b/app/models/user_observation_link.rb @@ -44,7 +44,7 @@ class UserObservationLink < ActiveRecord::Base MISSING_ROOT_ACCOUNT_ID = -1 - # shadow_record param is private + # cross_shard_record param is private def self.create_or_restore(student: , observer: , root_account: , cross_shard_record: false) raise ArgumentError, 'student, observer and root_account are required' unless student && observer && root_account shard = cross_shard_record ? observer.shard : student.shard diff --git a/lib/user_merge.rb b/lib/user_merge.rb index 03248e073b5..5ee4d31762b 100644 --- a/lib/user_merge.rb +++ b/lib/user_merge.rb @@ -411,9 +411,9 @@ class UserMerge from_user.as_observer_observation_links.where(user_id: target_user).destroy_all target_user.as_observer_observation_links.where(user_id: from_user).destroy_all from_user.as_observer_observation_links.update_all(observer_id: target_user.id) - xor_observer_ids = UserObservationLink.where(student: [from_user, target_user]).distinct.pluck(:observer_id) + xor_observer_ids = UserObservationLink.shard(from_user, target_user).where(student: [from_user, target_user]).distinct.pluck(:observer_id) from_user.as_student_observation_links.where(observer_id: target_user.as_student_observation_links.map(&:observer_id)).destroy_all - from_user.as_student_observation_links.update_all(user_id: target_user.id) + from_user.shard.activate { from_user.as_student_observation_links.update_all(user_id: target_user.id) } # for any observers not already watching both users, make sure they have # any missing observer enrollments added if from_user.shard != target_user.shard diff --git a/spec/models/split_users_spec.rb b/spec/models/split_users_spec.rb index 0dd2322e6ab..c6c76882546 100644 --- a/spec/models/split_users_spec.rb +++ b/spec/models/split_users_spec.rb @@ -520,6 +520,18 @@ describe SplitUsers do expect(shard1_source_user.reload.linked_observers).to eq [observer2] end + it 'should handle user_observers cross shard from target shard' do + observer1 = user_model + add_linked_observer(restored_user, observer1) + @shard1.activate do + UserMerge.from(restored_user).into(shard1_source_user) + end + expect(restored_user.linked_observers).to eq [] + expect(shard1_source_user.linked_observers.pluck(:id).sort).to eq [observer1.id].sort + SplitUsers.split_db_users(shard1_source_user) + expect(restored_user.reload.linked_observers).to eq [observer1] + end + it 'should handle conflicting submissions for cross shard users' do course1.enroll_student(restored_user, enrollment_state: 'active') course1.enroll_student(shard1_source_user, enrollment_state: 'active')