chamilo / chamilo-lms

Chamilo is a learning management system focused on ease of use and accessibility
https://chamilo.org
GNU General Public License v3.0
785 stars 478 forks source link

C2: Create "fallback" user role and account to attach orphan content to #5221

Closed ywarnier closed 6 months ago

ywarnier commented 6 months ago

When (really) deleting a user in C2, all related resources are deleted at the moment. This is not the desired behaviour, but due to foreign keys, we don't really have a choice to leave a large number of objects with a null reference. This would make them really difficult to clean-up afterwards. Instead, we are going to use the "hack" of creating a FALLBACK role and a 'fallback' user that cannot be deleted.

Whenever we delete user 123, we will scan the contents linked to that user, and replace the reference to the id of the FALLBACK user. This way, all content of user 123 is not deleted, only reassigned.

This role and user should be created on install and on migration from 1.11.

christianbeeznest commented 6 months ago

While implementing the solution to reassign resources from deleted users to a fallback user, I've realized I might encounter unique constraint violations in the database. This is particularly true for tables with unique constraints involving the user ID and other fields.

For example :

In SessionRelUser

[ORM\UniqueConstraint(name: 'session_user_unique', columns: ['session_id', 'user_id', 'relation_type'])]

UserRelUser

[ORM\UniqueConstraint(name: 'user_friend_relation', columns: ['user_id', 'friend_user_id', 'relation_type'])]

Does anyone have suggestions on how to anticipate and handle these conflicts effectively? I'm considering reviewing the affected entities and relationships and adding specific tests to ensure database integrity. Any advice on balancing data preservation with database performance and integrity would be greatly appreciated.

ywarnier commented 6 months ago

Regarding the listing of affected entities and relationships, I would suggest just scanning the entities for any user_id reference.

Regarding anticipating these conflicts : automated testing is the answer, although these are complex tests, there could be just one big test to test all of this: go through Chamilo, create some stuff as a new user, then delete the user and go around the tools as admin to see if an error pops up...

This being said, I don't think you would hav a lot of constraint integrity issues if you just update an object to point to a new user_id. The constraint will protest if you delete a record (which is what we are trying to avoid), not if you update it.

Any additional opinion, @AngelFQC ?

ywarnier commented 6 months ago

OK, so to be clear and thanks to @christianbeeznest 's list of user fields in the database, here is a list of all fields we should keep (replace with fallback) or delete on user deletion.

The general rule is that a field should be converted to use the fallback user ID if it relates to some content we don't necessarily want to delete, or important history that might affect other users (like text feedback or grading on some other user's work). In any other case, we should delete the record.

Test results, although their deletion can affect average scores and the statistics in general, are considered to be an acceptable loss, agreed upon and fully understood by the administrator at the time of deletion. Those should not affect the learning of other users, only surface elements.

It's important to remember that admins have several other options available to maintain a user's history on the platform:

Field Action Reason
AccessUrlRelUser.user Delete Relationship data, not important for other users.
Admin.user Delete Relationship data, not important for other users.
AgendaEventInvitee.user Delete Personal invitation. Only affects deleted user.
AttemptFeedback.user Convert The feedback provided might affect other users.
Chat.toUser Convert Activity between two people. The other one needs to keep the history.
ChatVideo.toUser Convert Activity between two people. The other one needs to keep the history.
CourseRelUser.user Delete Relationship data, not important for other users.
CourseRelUserCatalogue.user Delete Relationship data, not important for other users.
CourseRequest.user Convert Course requests should remain in the portal history.
CAttendanceResult.user Delete Only affects deleted user.
CAttendanceResultComment.userId Convert Might affect feedback given to other users.
CAttendanceSheet.user Delete Only affects deleted user.
CAttendanceSheetLog.user Delete Only affects deleted user.
CChatConnected.userId Delete Only affects deleted user.
CDropboxCategory.userId Convert Might affect content shown to other users.
CDropboxFeedback.authorUserId Convert Might affect feedback given to other users.
CDropboxPerson.userId Convert Might affect content shown to other users.
CDropboxPost.destUserId Convert Might affect content shown to other users.
CForumMailcue.userId Delete Only affects deleted user.
CForumNotification.userId Delete Only affects deleted user.
CForumPost.user Convert Might affect content shown to other users.
CForumThread.user Convert Might affect content shown to other users.
CForumThreadQualify.user Convert Might affect content shown to other users.
CForumThreadQualifyLog.userId Convert Might affect content shown to other users.
CGroupRelTutor.user Convert Might affect content shown to other users.
CGroupRelUser.user Convert Might affect content shown to other users.
CLpCategoryRelUser.user Delete Only affects deleted user.
CLpRelUser.user Delete Only affects deleted user.
CLpView.user Delete Only affects deleted user.
CStudentPublicationComment.user Convert Might affect feedback given to other users.
CStudentPublicationRelUser.user Delete Should only affects deleted user (there's some blurry cases here).
CSurveyInvitation.user Convert Might affect content shown to other users.
CWiki.userId Convert Might affect content shown to other users.
CWikiMailcue.userId Delete Only affects deleted user.
ExtraFieldSavedSearch.user Delete Only affects deleted user.
GradebookCategory.user Convert Might affect content shown to other users.
GradebookCertificate.user Delete Only affects deleted user, stats and links to the certificate, if public.
GradebookComment.user Convert Might affect feedback given to other users.
GradebookEvaluation.user Delete Only affects deleted user.
GradebookLink.user Delete Only affects deleted user.
GradebookLinkevalLog.user Delete Only affects deleted user.
GradebookResult.user Delete Only affects deleted user and stats.
GradebookResultLog.user Delete Only affects deleted user.
GradebookScoreLog.user Delete Only affects deleted user.
Message.sender Convert Might affect content shown to other users.
MessageFeedback.user Convert Might affect content shown to other users.
MessageRelUser.receiver Convert Might affect content shown to other users.
MessageTag.user Delete Only affects deleted user.
Notification.destUserId Delete Only affects deleted user (a notification is automated, vs a message).
PageCategory.creator Convert Might affect content shown to other users.
PersonalAgenda.user Delete Only affects deleted user.
Portfolio.user Convert Might affect content shown to other users.
PortfolioCategory.user Convert Might affect content shown to other users.
PortfolioComment.author Convert Might affect content shown to other users.
ResourceComment.author Convert Might affect content shown to other users.
SequenceValue.user Convert Might affect content shown to other users.
SessionRelCourseRelUser.user Delete Only affects deleted user.
SessionRelUser.user Delete Only affects deleted user.
SkillRelItemRelUser.user Delete Only affects deleted user and links to public skills.
SkillRelUser.user Delete Only affects deleted user and links to public skills.
SkillRelUserComment.feedbackGiver Convert
SocialPost.sender Convert Might affect content shown to other users.
SocialPost.userReceiver Convert Might affect content shown to other users.
SocialPostAttachment.insertUserId Convert Might affect content shown to other users.
SocialPostFeedback.user Convert Might affect feedback given to other users.
Templates.user Convert Might affect content shown to other users.
TicketAssignedLog.user Convert Might affect content shown to other users.
TicketCategory.insertUserId Convert Might affect content shown to other users.
TicketCategoryRelUser.user Delete By removing the auto-assignment of a ticket category to a deleted user, we put the lack of responsible person for this category in evidence.
TicketMessage.insertUserId Convert Might affect content shown to other users.
TicketMessageAttachment.lastEditUserId Convert Might affect content shown to other users.
TicketPriority.insertUserId Convert Might affect content shown to other users.
TicketProject.insertUserId Convert Might affect content shown to other users.
TicketProject.lastEditUserId Convert Might affect content shown to other users.
TrackEAccess.accessUserId Delete Only affects deleted user and stats.
TrackEAccessComplete.user Delete Only affects deleted user and stats.
TrackEAttempt.user Delete Only affects deleted user and stats.
TrackECourseAccess.user Delete Only affects deleted user and stats.
TrackEDefault.defaultUserId Convert We should not lose track_e_default records when a user has been deleted, for audition reasons.
TrackEDownloads.downUserId Delete Only affects deleted user and stats.
TrackEExercise.user Delete Only affects deleted user and stats.
TrackEExerciseConfirmation.user Delete Only affects deleted user and stats.
TrackEHotpotatoes.exeUserId Delete Only affects deleted user and stats.
TrackEHotspot.hotspotUserId Delete Only affects deleted user and stats.
TrackELastaccess.accessUserId Delete Only affects deleted user and stats.
TrackELinks.linksUserId Delete Only affects deleted user and stats.
TrackELogin.user Delete Only affects deleted user and stats.
TrackEOnline.loginUserId Delete Only affects deleted user and stats.
TrackEUploads.uploadUserId Delete Only affects deleted user and stats.
UsergroupRelUser.user Convert Important if this was a group admin.
UserRelTag.user Delete Only affects deleted user.
UserRelUser.user Convert Important to keep history of the relationships.
christianbeeznest commented 6 months ago

It is added in this PR https://github.com/chamilo/chamilo-lms/pull/5232 , ready for testing.

ywarnier commented 6 months ago

A bit tricky to test, but this all works properly. Thanks!