diff --git a/app/models/split_users.rb b/app/models/split_users.rb index 59c0a6147d1..c8bbec2044b 100644 --- a/app/models/split_users.rb +++ b/app/models/split_users.rb @@ -16,6 +16,8 @@ # with this program. If not, see . class SplitUsers + class UnsafeSplitError < StandardError; end + ENROLLMENT_DATA_UPDATES = [ {table: 'asset_user_accesses', scope: -> { where(context_type: 'Course') }}.freeze, @@ -210,6 +212,33 @@ class SplitUsers ccs.delete_all end end + + # in cases where there are conflicting records + # between the source and target (of merge) comm records, + # we can eliminate some errors by detecting these and destroying + # the source record if it's already retired (because the one from + # the merge is about to overwrite it) + cc_records.where(previous_user_id: restored_user).each do |cr| + target_cc = cr.context + # if this cc didn't get moved, we don't need to worry + # about deconflicting it with the source users. + next unless target_cc.user_id == source_user.id + conflict_cc = restored_user.communication_channels.detect do |c| + c.path.downcase == target_cc.path.downcase && c.path_type == target_cc.path_type + end + if conflict_cc + # we need to resolve before we can un-merge + if conflict_cc.retired? || conflict_cc.unconfirmed? + # when the comm channel from the target record gets moved back, it will + # get restored to whatever state it needs. This one is in a useless state, + # so we could just blast this one away safely. + conflict_cc.destroy_permanently! + else + raise UnsafeSplitError, "Unsafe to decide automatically which CC to delete (for now): ( #{target_cc.id} , #{conflict_cc.id} ) from merge record #{cr.id}" + end + end + end + # move moved communication channels back max_position = restored_user.communication_channels.last.try(:position) || 0 scope = source_user.communication_channels.where(id: cc_records.where(previous_user_id: restored_user).pluck(:context_id)) diff --git a/spec/models/split_users_spec.rb b/spec/models/split_users_spec.rb index 5d7c1ba02ad..d264897fd7f 100644 --- a/spec/models/split_users_spec.rb +++ b/spec/models/split_users_spec.rb @@ -384,6 +384,23 @@ describe SplitUsers do map { |cc| [cc.path, cc.workflow_state] }.sort).to eq source_user_ccs end + it "deconflicts duplicated paths where it can" do + notification = Notification.where(name: "Report Generated").first_or_create + communication_channel(restored_user, {username: 'test@instructure.com'}) + restored_user_ccs = restored_user.communication_channels.where.not(workflow_state: 'retired'). + map { |cc| [cc.path, cc.workflow_state] }.sort + source_user_ccs = source_user.communication_channels.where.not(workflow_state: 'retired'). + map { |cc| [cc.path, cc.workflow_state] }.sort + UserMerge.from(restored_user).into(source_user) + communication_channel(restored_user, {username: 'test@instructure.com', cc_state: 'retired'}) + SplitUsers.split_db_users(source_user) + restored_user.reload + source_user.reload + expect(restored_user.communication_channels.where.not(workflow_state: 'retired'). + map { |cc| [cc.path, cc.workflow_state] }.sort).to eq restored_user_ccs + expect(source_user.communication_channels.where.not(workflow_state: 'retired'). + map { |cc| [cc.path, cc.workflow_state] }.sort).to eq source_user_ccs + end end it 'should restore submissions' do