Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge users through the console #3327

Merged
merged 22 commits into from
Feb 1, 2022
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 67 additions & 0 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,73 @@ def set_search
self.search = "#{username || ''} #{first_name || ''} #{last_name || ''}"
end

def merge_into(other, force: false)
errors.add(:merge, 'User belongs to different institution') if other.institution_id != institution_id && other.institution_id.present? && institution_id.present?
errors.add(:merge, 'User has different permissions') if other.permission != permission && !force
return false if errors.any?

transaction do
jorg-vr marked this conversation as resolved.
Show resolved Hide resolved
other.permission = permission if (permission == 'staff' && other.permission == 'student') \
|| (permission == 'zeus' && other.permission != 'zeus')

other.institution_id = institution_id if other.institution_id.nil?

rights_request.update(user: other) if !rights_request.nil? && other.permission == 'student' && other.rights_request.nil?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these "if modifiers" enforced by rubocop? If they are more complex like the first and third one, a classical if seems more appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they are indeed enforced by rubocop


submissions.each { |s| s.update(user: other) }
api_tokens.each { |at| at.update(user: other) }
events.each { |e| e.update(user: other) }
exports.each { |e| e.update(user: other) }
notifications.each { |n| n.update(user: other) }
annotations.each { |a| a.update(user: other, last_updated_by_id: other.id) }
questions.each { |q| q.update(user: other) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these updates and the ones below cover most aspects of our users. Some open thoughts:

  • How do we handle grading?
  • Calling update on each of these will generate a huge number of queries, especially for submissions. Using update_all will do this in a single query, but will skip all validations and callbacks. It might be worth checking if update_all can be used for submissions and read states.
  • Somewhat linked with the previous point: the necessary caches should be invalidated (e.g. the number of users that solved an exercise within a course). This might happen automatically using the callbacks/validations but it should be double checked.
  • You of course added some tests, but I was curious if you checked the order of the updates. For example, validations (or cache recalculations) that might fail if a users has submissions in a course which he is at that moment not a member off (the membership is updated last).

Copy link
Contributor Author

@jorg-vr jorg-vr Jan 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I indeed seem to have forgotten about grading (As it was not mentioned as a relationship at the top of the model) Will add code for evaluations

Will explore the update_all statements

I have not yet checked any cached values, will do this now :)

Order of updates has not caused me any trouble in my manual testing. But I see this is not explicitly tested in the test cases, so I will expand upon this to make sure this does not cause any errors

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Grading:
I have added a merge of evaluation users. When two users appear in the same evaluation I keep the evaluation of other.
I am hesitating to just block the merge of users in this case, because removing the grades of a user without manually checking which grade is most applicable can led to real world issues. It also should not happen very often, so it is acceptable to break here, and force this merge to be done manually.

On update_all:
This indeed forgoes validations, which breaks caching. I am weary to combine an update all statement with a custom rewrite of the applicable validations, as this code will diverge over time from the actual validations that need to happen .

On caching:
I have added tests to validate some cached parameters. These broke when testing the update_all statements for example

On order:
In general it seems possible to have submissions linked to a course which you are not a member of. So i was unable to produce failing tests. Yet I did update the order a bit to be more logical in case later validations are written that break this. (Although there will always be inconsistencies while the merge is in progress)


activity_read_states.each do |ars|
if other.activity_read_states.find { |oars| oars.activity_id == ars.activity_id }
ars.delete
else
ars.update(user: other)
end
end

identities.each do |i|
if other.identities.find { |oi| oi.provider_id == i.provider_id }
i.delete
else
i.update(user: other)
end
end

repository_admins.each do |ra|
if other.repository_admins.find { |ora| ora.repository_id == ra.repository_id }
ra.delete
else
ra.update(user: other)
end
end

course_memberships.each do |cm|
other_cm = other.course_memberships.find { |ocm| ocm.course_id == cm.course_id }
if other_cm.nil?
cm.update(user: other)
elsif other_cm.status == cm.status \
|| other_cm.status == 'course_admin' \
|| (other_cm.status == 'student' && cm.status != 'course_admin') \
|| (other_cm.status == 'unsubscribed' && cm.status == 'pending')
other_cm.update(favorite: true) if cm.favorite
cm.delete
else
cm.update(favorite: true) if other_cm.favorite
other_cm.delete
cm.update(user: other)
end
end

reload
destroy
end
end

private

def set_token
Expand Down
62 changes: 62 additions & 0 deletions lib/tasks/merge_users.rake
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@

task :merge_users, [:arg1, :arg2] => :environment do |task, args|
u1_id = args[:arg1].to_i
u2_id = args[:arg2].to_i

MergeUsers.new.merge_users_interactive u1_id, u2_id
end

class MergeUsers
niknetniko marked this conversation as resolved.
Show resolved Hide resolved
LABEL_WIDTH = 20
USER_WIDTH = 50

def initialize(input = $stdin, output = $stdout)
@input = input
@output = output
end

def print_users(*users)
@output.puts 'Username: '.ljust(LABEL_WIDTH) + users.map { |u| u.username.to_s.ljust(USER_WIDTH) }.join
@output.puts 'Email: '.ljust(LABEL_WIDTH) + users.map { |u| u.email.to_s.ljust(USER_WIDTH) }.join
@output.puts 'Name: '.ljust(LABEL_WIDTH) + users.map { |u| u.full_name.to_s.ljust(USER_WIDTH) }.join
@output.puts 'Permission: '.ljust(LABEL_WIDTH) + users.map { |u| u.permission.to_s.ljust(USER_WIDTH) }.join

@output.puts 'Courses: '.ljust(LABEL_WIDTH) + users.map { |u| u.courses.count.to_s.ljust(USER_WIDTH) }.join
@output.puts 'Submissions: '.ljust(LABEL_WIDTH) + users.map { |u| u.submissions.count.to_s.ljust(USER_WIDTH) }.join
@output.puts 'Read states: '.ljust(LABEL_WIDTH) + users.map { |u| u.activity_read_states.count.to_s.ljust(USER_WIDTH) }.join
@output.puts 'Repositories: '.ljust(LABEL_WIDTH) + users.map { |u| u.repositories.count.to_s.ljust(USER_WIDTH) }.join
end

def merge_users_interactive(u1_id, u2_id)
u1 = User.find(u1_id)
u2 = User.find(u2_id)

print_users u1, u2

@output.puts ''

c = ''
until %W[\r \n y n f].include? c
u1, u2 = u2, u1 if c == 's'
@output.puts "Invalid input #{c}" unless ['s', ''].include?(c)
@output.puts "Are you sure you want to merge #{u1.username} into #{u2.username}? (y)es|(N)o|(f)orce|(s)wap"
c = @input.getch.downcase
end

@output.puts ''

if %W[\r \n n].include? c
@output.puts 'Merge cancelled'
return
end

success = u1.merge_into(u2, force: c == 'f')
if success
@output.puts "Successfully merged #{u1.username} into #{u2.username}"
print_users u2
else
@output.puts 'Merge failed'
@output.puts u1.errors.full_messages.join('\n')
end
end
end
167 changes: 167 additions & 0 deletions test/models/user_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -365,4 +365,171 @@ def setup
@user.subscribed_courses.first.update(year: '2')
assert_equal [@user.subscribed_courses.first], @user.drawer_courses
end

test 'user should be removed after merge' do
u1 = create :user
u2 = create :user
u1.merge_into(u2)

assert_not u1.persisted?
end

test 'merge should fail if institutions are different' do
i1 = create :institution
i2 = create :institution
u1 = create :user, institution: i1
u2 = create :user, institution: i2

result = u1.merge_into(u2)

assert_not result
assert u1.persisted?
end

test 'merge should succeed if only one institution is set' do
i1 = create :institution
u1 = create :user, institution: i1
u2 = create :user

result = u1.merge_into(u2)

assert result
assert_not u1.persisted?
assert_equal i1, u2.institution
end

test 'merge should fail if permissions are different' do
u1 = create :user, permission: 'student'
u2 = create :user, permission: 'staff'

result = u1.merge_into(u2)

assert_not result
assert u1.persisted?
end

test 'merge should take highest permission if force is used' do
u1 = create :user, permission: 'zeus'
u2 = create :user, permission: 'staff'

result = u1.merge_into(u2, force: true)

assert result
assert_not u1.persisted?
assert_equal 'zeus', u2.permission
end

test 'merge should transfer all associated objects to the other user' do
u1 = create :user
u2 = create :user

[u1, u2].each do |u|
c = create :course
s = create :submission, user: u, course: c
create :api_token, user: u
create :event, user: u
create :export, user: u
create :notification, user: u
create :annotation, user: u, submission: s
create :question, submission: s
end

result = u1.merge_into(u2)

assert result
assert_not u1.persisted?
assert_equal 2, u2.submissions.count
assert_equal 2, u2.api_tokens.count
assert_equal 2, u2.events.count
assert_equal 2, u2.exports.count
assert_equal 4, u2.notifications.count
assert_equal 4, u2.annotations.count
assert_equal 2, u2.questions.count
end

test 'merge should only transfer unique read states to the other user' do
u1 = create :user
u2 = create :user

a1 = create :content_page
a2 = create :content_page
create :activity_read_state, user: u1, activity: a1
create :activity_read_state, user: u2, activity: a1
create :activity_read_state, user: u1, activity: a2

result = u1.merge_into(u2)

assert result
assert_not u1.persisted?
assert_equal 2, u2.activity_read_states.count
end

test 'merge should only transfer unique identities to the other user' do
u1 = create :user
u2 = create :user

p1 = create :provider
p2 = create :provider
Identity.create user: u1, provider: p1, identifier: 'a'
Identity.create user: u2, provider: p1, identifier: 'b'
Identity.create user: u1, provider: p2, identifier: 'c'

result = u1.merge_into(u2)

assert result
assert_not u1.persisted?
assert_equal 2, u2.identities.count
end

test 'merge should only transfer unique repositories to the other user' do
u1 = create :user
u2 = create :user

r1 = create :repository, :git_stubbed
r2 = create :repository, :git_stubbed
RepositoryAdmin.create user: u1, repository: r1
RepositoryAdmin.create user: u2, repository: r1
RepositoryAdmin.create user: u1, repository: r2

result = u1.merge_into(u2)

assert result
assert_not u1.persisted?
assert_equal 2, u2.repository_admins.count
end

test 'merge should transfer course membership with most rights to the other user' do
u1 = create :user
u2 = create :user

c1 = create :course
c2 = create :course
c3 = create :course
c4 = create :course
c5 = create :course
CourseMembership.create user: u1, course: c1, status: 'student', favorite: true
CourseMembership.create user: u1, course: c2, status: 'pending'
CourseMembership.create user: u1, course: c3, status: 'unsubscribed'
CourseMembership.create user: u1, course: c4, status: 'student'
CourseMembership.create user: u1, course: c5, status: 'course_admin'

CourseMembership.create user: u2, course: c2, status: 'pending'
CourseMembership.create user: u2, course: c3, status: 'course_admin', favorite: true
CourseMembership.create user: u2, course: c4, status: 'unsubscribed'
CourseMembership.create user: u2, course: c5, status: 'student'

result = u1.merge_into(u2)

assert_equal 0, u1.course_memberships.count

assert result
assert_not u1.persisted?
assert_equal 5, u2.course_memberships.count
assert_equal 4, u2.subscribed_courses.count
assert_equal 2, u2.favorite_courses.count
assert_equal 2, u2.administrating_courses.count
assert_equal 2, u2.enrolled_courses.count
assert_equal 1, u2.pending_courses.count
assert_equal 0, u2.unsubscribed_courses.count
end
end
89 changes: 89 additions & 0 deletions test/tasks/merge_users_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
require 'test_helper'
require 'stringio'
require 'rake'
load './lib/tasks/merge_users.rake'

class MergeUserTest < ActiveSupport::TestCase
def stub_stdin(*chars)
string_io = StringIO.new
chars.each { |c| string_io.print c }
string_io.rewind
string_io
end

def merge_users_interactive(u1_id, u2_id, *chars)
input = stub_stdin(*chars)
output = File.open(File::NULL, 'w')
MergeUsers.new(input, output).merge_users_interactive(u1_id, u2_id)
end

test 'The script should cancel on no' do
u1 = create :user
u2 = create :user

merge_users_interactive u1.id, u2.id, 'n'

assert User.exists?(u1.id)
assert User.exists?(u2.id)
end

test 'The script should cancel on enter' do
u1 = create :user
u2 = create :user

merge_users_interactive u1.id, u2.id, '\n'

assert User.exists?(u1.id)
assert User.exists?(u2.id)
end

test 'The script should merge on yes' do
u1 = create :user
u2 = create :user

merge_users_interactive u1.id, u2.id, 'y'

assert_not User.exists?(u1.id)
assert User.exists?(u2.id)
end

test 'The script should swap on swap' do
u1 = create :user
u2 = create :user

merge_users_interactive u1.id, u2.id, 's', 'y'

assert User.exists?(u1.id)
assert_not User.exists?(u2.id)
end

test 'The script should ignore incorrect input' do
u1 = create :user
u2 = create :user

merge_users_interactive u1.id, u2.id, 'a', 'z', 'd', 'y'

assert_not User.exists?(u1.id)
assert User.exists?(u2.id)
end

test 'The script should fail on yes with different permissions' do
u1 = create :user, permission: 'student'
u2 = create :user, permission: 'zeus'

merge_users_interactive u1.id, u2.id, 'y'

assert User.exists?(u1.id)
assert User.exists?(u2.id)
end

test 'The script should merge on force with different permissions' do
u1 = create :user, permission: 'student'
u2 = create :user, permission: 'zeus'

merge_users_interactive u1.id, u2.id, 'f'

assert_not User.exists?(u1.id)
assert User.exists?(u2.id)
end
end