Merge users through the console #3327
Conversation
jorg-vr
requested review from
bmesuere and
niknetniko
and removed request for
a team
app/models/user.rb
Outdated
| 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? |
There was a problem hiding this comment.
Are these "if modifiers" enforced by rubocop? If they are more complex like the first and third one, a classical if seems more appropriate.
There was a problem hiding this comment.
they are indeed enforced by rubocop
app/models/user.rb
Outdated
| 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) } |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Ok for me, but I would like approval from @niknetniko before merging.
This pull request adds a function
merge_intoto the user model. This function can be used to merge two user from the command line.Also added an error if the two users don't have the same permissions. This error can be circumvented by using force, in which case the highest permissions will be kept for the new user.
For overlapping unique properties, such as right_requests, read_states and identities the values of other are kept.
For overlapping course memberships, the highest permission is kept.
I have added a rake task to run this from a command line interface: (Without rails console)
example input: invalid, swap, yes
Closes #2668 .