devilry / devilry-django

Devilry project main repository
http://devilry.org
BSD 3-Clause "New" or "Revised" License
51 stars 24 forks source link

Username "merge into" #1122

Closed torgeirl closed 2 years ago

torgeirl commented 5 years ago

We're noticing an increase in requests for switching username mid-semester. The current workaround for this is very time-consuming, error prone, and «dirty»:

It would be better if we could merge all previous activity into the new user, allowing reuse of results from previous, and a much smoother move of assignments from the old to the new user.

The function should only be available for sysadmins so no need for UI elements. Would be good if the CLI command has an extra confirm though:

$ venv/bin/python manage.py devilry_usermerge <old_username> <new_username> Are you sure you want to merge '<old_username>' into '<new_username>'? This action can't be undone! Type the new username to confirm:

StianJul commented 3 years ago

@torgeirl, I'm a bit confused.

Are you talking about changed the username?

We're noticing an increase in requests for switching username mid-semester.

Or are you talking about merging separate users?

... manage.py devilry_usermerge ...

torgeirl commented 3 years ago

@StianJul: their primary UiO user are switched mid-semester, and since UiO only export primary users to Feide the student becomes unable to log into their old Devilry user for the rest of the semester.

We therefore have a need to merge their old Devilry user into their new Devilry user. (This is mostly an issue due to UiO's systems operate on a person level while we are stuck work on a user level due to the lack of actual APIs for student data operations.)

Since some of these user switches are a result of other personal attributes also having changes it would be nice if the usermerge replaced most references to the old user with the new user.

StianJul commented 3 years ago

I think we should have a discussion about how to best handle this, meaning that this is not within the scope of milestone 5.1.

Just so we can keep this issue within the "planned" tasks, I'll move it to milestone 5.2.

espenak commented 3 years ago

I think this is done, but this is a complex solution to get "perfect".

The current solution does the following:

We DO NOT HANDLE these things:

Risk of data loss

As far as I can see, we do not have ForeignKeys to RelatedStudent or RelatedExaminer from any other models than the ones we already handle in the merge. If we do have such foreignkeys, data will be lost in the merge when we remove the dangling RelatedStudent and RelatedExaminer objects from source user after merging all the data into the target user. This is a fairly small risk since we have used quite some time looking for this.

torgeirl commented 3 years ago

current solution does the following (...)

RelatedStudent and FeedbackSet objects are probably the most important to get right so this looks very good.

Merging AssignmentGroups that are left when the two merged users was already in their own AssignmentGroup on the same assignment. (...) It will look a bit strange from the students perspective since they will get the same assignment twice in their list of assignments.

So merge into will change the name and username of the assignment, in practice leaving duplicates on every assignment where both users where present? If so, the sysadmin should probably handle those duplicates after running the script. Could you list assignments with duplicates once the CLI command is finished?

espenak commented 2 years ago

@torgeirl, the "duplicates" is already printed when run in verbose mode, and this verbose output is stored in the database and can be viewed later in django admin by superusers if you forget to run the command with --verbose. The output is not super readable, but it looks like this:

{
  "Comment": {
    "moved_comments_count": 0,
    "note": "Comments made by the source user - foreign key moved to the target user."
  },
  "FeedbackSet": {
    "created_by_count": 0,
    "last_updated_by_count": 0,
    "note": "Feedback sets created, updated or published by the source user. Foreign keys moved to the target user.",
    "published_by_count": 0
  },
  "PermissionGroupUser": {
    "details": {
      "permission_group_users_updated": []
    },
    "note": "Administrator permission groups. Update user foreign key so source user is replaced with target user.",
    "permission_group_user_count": 0
  },
  "RelatedExaminer": {
    "details": {
      "merged_examiners": [],
      "merged_qualifiesforfinalexams": [],
      "relatedexaminers_with_user_fk_changed": []
    },
    "merged_relatedexaminers_and_subobjects_count": 0,
    "note": "Students on a period. Merge source RelatedExaminer and all sub-objects (Examiners) into target RelatedExaminer.",
    "relatedexaminers_with_user_fk_changed_count": 0
  },
  "RelatedStudent": {
    "details": {
      "merged_candidates": [
        {
          "assignment": "duck1010.springaaaa.assignment0",
          "candidate_id": 15
        }
      ],
      "merged_qualifiesforfinalexams": [],
      "relatedstudents_with_user_fk_changed": []
    },
    "merged_relatedstudents_and_subobjects_count": 1,
    "note": "Students on a period. Merge source RelatedStudent and all sub-objects (Candidates and QualifiesForFinalExams) into target RelatedStudent.",
    "relatedstudents_with_user_fk_changed_count": 0
  }
}

In this case, the RelatedStudent.details.merged_candidates key shows that they where both on duck1010.springaaaa.assignment0.

espenak commented 2 years ago

Docs: https://github.com/devilry/devilry-django/blob/milestone-5.5/not_for_deploy/guides/cli/devilry_usermerge.md

torgeirl commented 2 years ago

We DO NOT HANDLE these things:

  • (...)
  • Collisions with QualifiedForFinalExam. The same user can not be part of the same Status in the qualified for final exam system. So in the very strange case where we need to merge two users that both have qualified for final exam statuses for the same Period, it will crash. This is not something we can handle automatically (we can not determine which grade is the correct one safely), and that means that it must be fixed manually before merging. The crash will lead to a full revert (we do everything in a transaction), so there will not be a partial merge caused by this crash.

Unclear how these collisions should be fixed. Unpublishing a QualifiedForFinalExam does not solve it, and as far as I can tell there isn't any way of editing an unpublishing (by design I assume) so maybe we either how to find another way to handle or ignore this when doing an user merge?

And as far as I can tell, this is not that uncommon in our production environment because a subset of secondary users avoid the filtration in our import scripts since they don't follow the established username notations for secondary users (-drift, -test and -adm).

torgeirl commented 2 years ago

Suggested solution for collision handling

espenak commented 2 years ago

The solution here is a separate script that can be used to handle qualifies for final exam collisions. It works works like this:

$ python manage.py devilry_qualifiedforfinalexam_delete_duplicates --user-a user_a --user-b user_b

## Period: subject1.period2

user_a (subject1.period2):
- Qualifies: False [status:ready, created time:2022-09-19 13:53:13.356797+00:00, exported time:None]

user_b (subject1.period2):
- Qualifies: False [status:ready, created time:2022-09-19 13:53:13.363757+00:00, exported time:None]

Select which user to DELETE qualifies for final exam results for in subject1.period2 [user_a|user_b]: user_a

## Period: subject1.period3

user_a (subject1.period3):
- Qualifies: False [status:notready, created time:2022-09-19 13:53:13.360672+00:00, exported time:None]

user_b (subject1.period3):
- Qualifies: False [status:almostready, created time:2022-09-19 13:53:13.366257+00:00, exported time:None]

Select which user to DELETE qualifies for final exam results for in subject1.period3 [user_a|user_b]: user_b

Can also be run with --preview to just preview all the collisions.

espenak commented 2 years ago

Should be mostly done now. Just missing testing the new check added to devilry_usermerge to inform about the collision in qualifies for final exam.