From 573a2b673b2af0b90bf10adafd0d80adfd8e87d3 Mon Sep 17 00:00:00 2001 From: Rob Orton Date: Mon, 22 Feb 2016 10:49:12 -0700 Subject: [PATCH] create user merge table fixes CNVS-27291 test plan - merge users - data should be populated Change-Id: I5720933484494f582bad4b294a7e6b4edea66b92 Reviewed-on: https://gerrit.instructure.com/72700 Tested-by: Jenkins Reviewed-by: Cody Cutrer QA-Review: Jeremy Putnam Product-Review: Rob Orton --- app/models/user_merge_data.rb | 33 +++++++++ app/models/user_merge_data_record.rb | 24 ++++++ .../20160222035553_create_user_merge_data.rb | 29 ++++++++ lib/user_merge.rb | 73 ++++++++++++------- spec/lib/user_merge_spec.rb | 25 ++++++- 5 files changed, 158 insertions(+), 26 deletions(-) create mode 100644 app/models/user_merge_data.rb create mode 100644 app/models/user_merge_data_record.rb create mode 100644 db/migrate/20160222035553_create_user_merge_data.rb diff --git a/app/models/user_merge_data.rb b/app/models/user_merge_data.rb new file mode 100644 index 00000000000..798a8475aa5 --- /dev/null +++ b/app/models/user_merge_data.rb @@ -0,0 +1,33 @@ +# +# Copyright (C) 2016 Instructure, Inc. +# +# This file is part of Canvas. +# +# Canvas is free software: you can redistribute it and/or modify it under +# the terms of the GNU Affero General Public License as published by the Free +# Software Foundation, version 3 of the License. +# +# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY +# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR +# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more +# details. +# +# You should have received a copy of the GNU Affero General Public License along +# with this program. If not, see . +# +class UserMergeData < ActiveRecord::Base + belongs_to :user + belongs_to :from_user, class_name: 'User' + has_many :user_merge_data_records + + strong_params + + def add_more_data(objects) + objects.each do |o| + r = self.user_merge_data_records.new(context: o, previous_user_id: o.user_id) + r.previous_workflow_state = o.workflow_state if o.class.columns_hash.key?('workflow_state') + r.save! + end + end + +end diff --git a/app/models/user_merge_data_record.rb b/app/models/user_merge_data_record.rb new file mode 100644 index 00000000000..a0c61cbc106 --- /dev/null +++ b/app/models/user_merge_data_record.rb @@ -0,0 +1,24 @@ +# +# Copyright (C) 2016 Instructure, Inc. +# +# This file is part of Canvas. +# +# Canvas is free software: you can redistribute it and/or modify it under +# the terms of the GNU Affero General Public License as published by the Free +# Software Foundation, version 3 of the License. +# +# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY +# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR +# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more +# details. +# +# You should have received a copy of the GNU Affero General Public License along +# with this program. If not, see . +# +class UserMergeDataRecord < ActiveRecord::Base + belongs_to :previous_user, class_name: 'User' + belongs_to :user_merge_data + belongs_to :context, polymorphic: [:account_user, :enrollment, :pseudonym] + + strong_params +end diff --git a/db/migrate/20160222035553_create_user_merge_data.rb b/db/migrate/20160222035553_create_user_merge_data.rb new file mode 100644 index 00000000000..c44aa661a76 --- /dev/null +++ b/db/migrate/20160222035553_create_user_merge_data.rb @@ -0,0 +1,29 @@ +class CreateUserMergeData < ActiveRecord::Migration + tag :predeploy + + def change + create_table :user_merge_data do |t| + t.integer :user_id, limit: 8, null: false + t.integer :from_user_id, limit: 8, null: false + t.timestamps null: false + t.string :workflow_state, null: false, default: 'active' + end + + create_table :user_merge_data_records do |t| + t.integer :user_merge_data_id, limit: 8, null: false + t.integer :context_id, limit: 8, null: false + t.integer :previous_user_id, limit: 8, null: false + t.string :context_type, null: false + t.string :previous_workflow_state + end + + add_index :user_merge_data, :user_id + add_index :user_merge_data, :from_user_id + add_index :user_merge_data_records, :user_merge_data_id + add_index :user_merge_data_records, [:context_id, :context_type, :user_merge_data_id, :previous_user_id], + unique: true, name: "index_user_merge_data_records_on_context_id_and_context_type" + + add_foreign_key :user_merge_data, :users + add_foreign_key :user_merge_data_records, :user_merge_data, column: :user_merge_data_id + end +end diff --git a/lib/user_merge.rb b/lib/user_merge.rb index 7b0a88a8781..4e02030b5c3 100644 --- a/lib/user_merge.rb +++ b/lib/user_merge.rb @@ -13,6 +13,9 @@ class UserMerge def into(target_user) return unless target_user return if target_user == from_user + user_merge_data = target_user.shard.activate do + UserMergeData.create!(user: target_user, from_user: from_user) + end if target_user.avatar_state == :none && from_user.avatar_state != :none [:avatar_image_source, :avatar_image_url, :avatar_image_updated_at, :avatar_state].each do |attr| @@ -96,11 +99,13 @@ class UserMerge destroy_conflicting_module_progressions(@from_user, target_user) - move_enrollments(@from_user, target_user) + move_enrollments(target_user, user_merge_data) Shard.with_each_shard(from_user.associated_shards + from_user.associated_shards(:weak) + from_user.associated_shards(:shadow)) do - max_position = Pseudonym.where(:user_id => target_user).order(:position).last.try(:position) || 0 - Pseudonym.where(:user_id => from_user).update_all(["user_id=?, position=position+?", target_user, max_position]) + max_position = Pseudonym.where(user_id: target_user).order(:position).last.try(:position) || 0 + pseudonyms_to_move = Pseudonym.where(user_id: from_user) + user_merge_data.add_more_data(pseudonyms_to_move) + pseudonyms_to_move.update_all(["user_id=?, position=position+?", target_user, max_position]) target_user.communication_channels.email.unretired.each do |cc| Rails.cache.delete([cc.path, 'invited_enrollments2'].cache_key) @@ -165,9 +170,12 @@ class UserMerge Rails.logger.error "migrating discussions failed: #{e}" end + account_users = AccountUser.where(user_id: from_user) + user_merge_data.add_more_data(account_users) + account_users.update_all(user_id: target_user) + updates = {} - ['account_users', 'access_tokens', 'asset_user_accesses', - 'attachments', + ['access_tokens', 'asset_user_accesses', 'attachments', 'calendar_events', 'collaborations', 'context_module_progressions', 'group_memberships', 'page_comments', @@ -291,31 +299,46 @@ class UserMerge END, sis_batch_id DESC, updated_at DESC").first end - def move_enrollments(from_user, target_user) + def handle_conflicts(column, target_user, user_merge_data) + users = [from_user, target_user] + conflict_scope(column).where(column => users).find_each do |e| + + scope = enrollment_conflicts(e, column, users) + keeper = enrollment_keeper(scope) + + # delete all conflicts from target user + to_delete = scope.where("id<>?", keeper).where(column => target_user) + user_merge_data.add_more_data(to_delete) + to_delete.delete_all + + # mark all conflicts on from_user as deleted so they will be left + to_delete = scope.active.where("id<>?", keeper).where(column => from_user) + user_merge_data.add_more_data(to_delete) + to_delete.destroy_all + end + end + + def remove_self_observers(target_user, user_merge_data) + # prevent observing self by marking them as deleted + to_delete = Enrollment.active.where("type = 'ObserverEnrollment' AND + (associated_user_id = :target_user AND user_id = :from_user OR + associated_user_id = :from_user AND user_id = :target_user)", + {target_user: target_user, from_user: from_user}) + user_merge_data.add_more_data(to_delete) + to_delete.destroy_all + end + + def move_enrollments(target_user, user_merge_data) [:associated_user_id, :user_id].each do |column| - users = [from_user, target_user] Shard.with_each_shard(from_user.associated_shards) do Enrollment.transaction do - conflict_scope(column).where(column => users).find_each do |e| - - scope = enrollment_conflicts(e, column, users) - keeper = enrollment_keeper(scope) - - # delete all conflicts from target user - scope.where("id<>?", keeper).where(column => target_user).delete_all - - # mark all conflicts on from_user as deleted so they will be left - scope.active.where("id<>?", keeper).where(column => from_user).destroy_all - end - - # prevent observing self by marking them as deleted - Enrollment.active.where("type = 'ObserverEnrollment' AND - (associated_user_id = :target_user AND user_id = :from_user OR - associated_user_id = :from_user AND user_id = :target_user)", - {target_user: target_user, from_user: from_user}).destroy_all + handle_conflicts(column, target_user, user_merge_data) + remove_self_observers(target_user, user_merge_data) # move all the enrollments that are not marked as deleted to the target user - Enrollment.active.where(column => from_user).update_all(column => target_user) + to_move = Enrollment.active.where(column => from_user) + user_merge_data.add_more_data(to_move) + to_move.update_all(column => target_user) end end end diff --git a/spec/lib/user_merge_spec.rb b/spec/lib/user_merge_spec.rb index 48e37fd4a9a..c15cb8d473a 100644 --- a/spec/lib/user_merge_spec.rb +++ b/spec/lib/user_merge_spec.rb @@ -17,14 +17,29 @@ describe UserMerge do end it "should move pseudonyms to the new user" do - user2.pseudonyms.create!(:unique_id => 'sam@yahoo.com') + pseudonym = user2.pseudonyms.create!(unique_id: 'sam@yahoo.com') + pseudonym2 = user2.pseudonyms.create!(unique_id: 'sam2@yahoo.com') UserMerge.from(user2).into(user1) + merge_data = UserMergeData.where(user_id: user1).first + expect(merge_data.from_user_id).to eq user2.id + expect(merge_data.user_merge_data_records.pluck(:context_id).sort).to eq [pseudonym.id, pseudonym2.id].sort user2.reload expect(user2.pseudonyms).to be_empty user1.reload expect(user1.pseudonyms.map(&:unique_id)).to be_include('sam@yahoo.com') end + it "should move admins to the new user" do + account1 = account_model + admin = account1.account_users.create(user: user2) + UserMerge.from(user2).into(user1) + merge_data = UserMergeData.where(user_id: user1).first + expect(merge_data.from_user_id).to eq user2.id + expect(merge_data.user_merge_data_records.where(context_type: 'AccountUser').first.context_id).to eq admin.id + user1.reload + expect(user1.account_users.first.id).to eq admin.id + end + it "should use avatar information from merged user if none exists" do user2.avatar_image = {'type' => 'external', 'url' => 'https://example.com/image.png'} user2.save! @@ -219,9 +234,14 @@ describe UserMerge do enrollment4 = course1.enroll_teacher(user1) UserMerge.from(user1).into(user2) + merge_data = UserMergeData.where(user_id: user2).first + expect(merge_data.user_merge_data_records.pluck(:context_id).sort). + to eq [enrollment1.id, enrollment3.id, enrollment4.id].sort enrollment1.reload expect(enrollment1.user).to eq user1 expect(enrollment1).to be_deleted + merge_data_record = merge_data.user_merge_data_records.where(context_id: enrollment1).first + expect(merge_data_record.previous_workflow_state).to eq 'invited' enrollment2.reload expect(enrollment2).to be_active expect(enrollment2.user).to eq user2 @@ -271,6 +291,9 @@ describe UserMerge do enrollment2 = course1.enroll_user(user2, 'ObserverEnrollment', enrollment_state: 'active', associated_user_id: user1.id) UserMerge.from(user1).into(user2) + merge_data = UserMergeData.where(user_id: user2).first + o = merge_data.user_merge_data_records.where(context_id: enrollment2).first + expect(o.previous_workflow_state).to eq 'active' expect(enrollment1.reload.user).to eql user2 expect(enrollment2.reload.workflow_state).to eql 'deleted' end