From ed0dce66c5bc869fae30ce863763986f07c822a4 Mon Sep 17 00:00:00 2001 From: Manuel Raynaud Date: Thu, 18 Jun 2026 17:50:29 +0200 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8(backend)=20specific=20user=20delete?= =?UTF-8?q?=20method=20to=20delete=20its=20relations?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting a user is not possible when it has created docs because the on_delete on the Document class id RESTRICT and we don't want to change it. We decided to have a specific workflow for correctly delete a user. The document where the user is the sole owner must be deleted, the other only the owner access must be deleted. For the remaining Documents where the user is the creator, we set it to `null`, then the user can be delete, remaining relations are deleted in cascade. --- CHANGELOG.md | 1 + src/backend/core/models.py | 50 ++++ src/backend/core/tests/test_models_users.py | 279 +++++++++++++++++++- 3 files changed, 329 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 06e06f8c5..2d6e0721c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ and this project adheres to - ✨(frontend) leave a document #2410 - ✨(frontend) add top parent on sub docs search #1952 - ✨(frontend) unauthenticated users can search #2407 +- ✨(backend) specific user delete method to delete its relations ### Changed diff --git a/src/backend/core/models.py b/src/backend/core/models.py index 8ae5b1312..e2cd88de2 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -20,6 +20,7 @@ from django.core.files.base import ContentFile from django.core.files.storage import default_storage from django.core.mail import send_mail from django.db import models, transaction +from django.db.models import Count from django.db.models.functions import Left, Length from django.template.loader import render_to_string from django.utils import timezone @@ -225,6 +226,55 @@ class User(AbstractBaseUser, BaseModel, auth_models.PermissionsMixin): self._duplicate_onboarding_sandbox_document() self._convert_valid_invitations() + def delete(self, using=None, keep_parents=False): + """Completely delete a user and its relations.""" + with transaction.atomic(): + self._delete_user_shared_documents_accesses() + self._delete_documents_single_owner() + self._clear_user_created_documents() + + return super().delete(using=using, keep_parents=keep_parents) + + def _delete_user_shared_documents_accesses(self): + """ + accesses to delete where there are more than one owner. + + Create first a subquery to filter all the the accesses having more than one + owner. Then use this subquery to filter the accesses belonging to this list of + documents and to the user to delete + """ + docs_ids = ( + DocumentAccess.objects.filter(role=RoleChoices.OWNER) + .values("document_id") + .annotate(owner_count=Count("id")) + .filter(owner_count__gte=2) + .values("document_id") + ) + DocumentAccess.objects.filter(user=self, document_id__in=docs_ids).delete() + + logger.info( + "user_delete: shared documents accesses for user %s have been deleted", + self.id, + ) + + def _delete_documents_single_owner(self): + """Delete the documents where the user is the single owner.""" + Document.objects.filter( + accesses__user=self, accesses__role=RoleChoices.OWNER + ).delete() + logger.info( + "user_delete: documents where the user %s is the sole owner deleted", + self.id, + ) + + def _clear_user_created_documents(self): + """Set creator to Null for documents where the user is the creator.""" + Document.objects.filter(creator=self).update(creator=None) + logger.info( + "user_delete: documents created by user %s have been cleared", + self.id, + ) + def _handle_onboarding_documents_access(self): """ If the user is new and there are documents configured to be given to new users, diff --git a/src/backend/core/tests/test_models_users.py b/src/backend/core/tests/test_models_users.py index b8bc20ffc..2cf0d50d3 100644 --- a/src/backend/core/tests/test_models_users.py +++ b/src/backend/core/tests/test_models_users.py @@ -7,7 +7,7 @@ from concurrent.futures import ThreadPoolExecutor from unittest.mock import patch from django.core.exceptions import ValidationError -from django.db import connection +from django.db import DatabaseError, connection from django.test.utils import override_settings import pytest @@ -366,3 +366,280 @@ def test_models_users_duplicate_onboarding_sandbox_race_condition(): assert isinstance(user1, models.User) assert isinstance(user2, models.User) + + +@pytest.mark.django_db(transaction=True) +def test_task_user_delete(): + """test user_delete task with a complete scenario""" + + user_to_delete = factories.UserFactory() + other_user = factories.UserFactory() + + # Documents where the user is the sole owner + documents_to_delete = factories.DocumentFactory.create_batch( + 3, creator=user_to_delete, users=[(user_to_delete, models.RoleChoices.OWNER)] + ) + # Documents where the user is not the sole owner + documents_to_keep = factories.DocumentFactory.create_batch( + 4, + creator=other_user, + users=[ + (user_to_delete, models.RoleChoices.OWNER), + (other_user, models.RoleChoices.OWNER), + ], + ) + # descendants of a document to delete should also be removed + depth = 5 + for _ in range(depth + 1): + documents_to_delete.append( + factories.DocumentFactory(parent=documents_to_delete[-1]) + ) + + # Documents created by the user to delete and should be set to null + documents_creator_set_to_null = factories.DocumentFactory.create_batch( + 3, creator=user_to_delete + ) + + # Non creator nor owner document but with accesses to delete + for role in models.RoleChoices.values: + if role == models.RoleChoices.OWNER.value: + continue + + factories.DocumentFactory(users=[(user_to_delete, role)]) + + # create 3 link traces + # + factories.DocumentFactory.create_batch(3, link_traces=[user_to_delete, other_user]) + # Create other link traces for the other user + # + factories.DocumentFactory.create_batch(3, link_traces=[other_user]) + + # Threads created by the user to delete should be set to null + threads_owned_by_user_to_delete = factories.ThreadFactory.create_batch( + 2, creator=user_to_delete + ) + # Threads created by the other user should not change + threads_owned_by_other_user = factories.ThreadFactory.create_batch( + 2, creator=other_user + ) + + # create comments for both users in all existing threads + comments = [] + for thread in threads_owned_by_user_to_delete + threads_owned_by_other_user: + comments.append(factories.CommentFactory(thread=thread, user=user_to_delete)) + comments.append(factories.CommentFactory(thread=thread, user=other_user)) + + assert models.DocumentAccess.objects.filter(user=user_to_delete).count() == 11 + assert models.Document.objects.all().count() == 30 + assert models.LinkTrace.objects.all().count() == 9 + assert models.Thread.objects.all().count() == 4 + assert models.Comment.objects.all().count() == 8 + assert len(documents_to_delete) == 9 + + user_to_delete.delete() + + for document in documents_to_delete: + assert models.Document.objects.filter(id=document.id).exists() is False + + for document in documents_to_keep: + assert models.Document.objects.filter(id=document.id).exists() + + for document in documents_creator_set_to_null: + document.refresh_from_db() + assert document.creator is None + + assert models.DocumentAccess.objects.filter(user_id=user_to_delete.id).count() == 0 + assert ( + models.Document.objects.all().count() == 21 + ) # models.Document.objects.all().count() - len(documents_to_delete) + assert models.LinkTrace.objects.all().count() == 6 + + # Number of threads and comments should not have changed + assert models.Thread.objects.all().count() == 4 + assert models.Comment.objects.all().count() == 8 + + for thread in threads_owned_by_user_to_delete: + thread.refresh_from_db() + assert thread.creator is None + + assert ( + models.Comment.objects.filter(thread=thread, user__isnull=True).count() == 1 + ) + assert ( + models.Comment.objects.filter(thread=thread, user=other_user).count() == 1 + ) + + for thread in threads_owned_by_other_user: + thread.refresh_from_db() + assert thread.creator == other_user + assert ( + models.Comment.objects.filter(thread=thread, user__isnull=True).count() == 1 + ) + assert ( + models.Comment.objects.filter(thread=thread, user=other_user).count() == 1 + ) + + assert models.User.objects.filter(id=user_to_delete.id).exists() is False + + +@pytest.mark.django_db(transaction=True) +def test_tasks_user_delete_nothing_to_delete(): + """Delete a user not having creating data yet.""" + + user_to_delete = factories.UserFactory() + other_user = factories.UserFactory() + + factories.DocumentFactory.create_batch( + 4, + creator=other_user, + users=[ + (other_user, models.RoleChoices.OWNER), + ], + ) + + # Create other link traces for the other user + # + factories.DocumentFactory.create_batch(3, link_traces=[other_user]) + + threads_owned_by_other_user = factories.ThreadFactory.create_batch( + 2, creator=other_user + ) + + # create comments for bot users in all existing threads + comments = [] + for thread in threads_owned_by_other_user: + comments.append(factories.CommentFactory(thread=thread, user=other_user)) + + assert models.DocumentAccess.objects.count() == 4 + assert models.Document.objects.all().count() == 9 + assert models.LinkTrace.objects.all().count() == 3 + assert models.Thread.objects.all().count() == 2 + assert models.Comment.objects.all().count() == 2 + + user_to_delete.delete() + + assert models.DocumentAccess.objects.count() == 4 + assert models.Document.objects.all().count() == 9 + assert models.LinkTrace.objects.all().count() == 3 + assert models.Thread.objects.all().count() == 2 + assert models.Comment.objects.all().count() == 2 + assert models.User.objects.filter(id=user_to_delete.id).exists() is False + + +@pytest.mark.django_db(transaction=True) +def test_tasks_user_delete_only_owner_but_not_creator(): + """Test delete a user who is sole owner of a document but not the creator should work.""" + + user_to_delete = factories.UserFactory() + other_user = factories.UserFactory() + + document_to_delete = factories.DocumentFactory( + creator=other_user, users=[(user_to_delete, models.RoleChoices.OWNER)] + ) + + document_to_keep = factories.DocumentFactory( + creator=other_user, + users=[ + (other_user, models.RoleChoices.OWNER), + (user_to_delete, models.RoleChoices.OWNER), + ], + ) + + assert models.DocumentAccess.objects.filter(user=user_to_delete).count() == 2 + assert models.Document.objects.all().count() == 2 + + user_to_delete.delete() + + assert models.DocumentAccess.objects.filter(user_id=user_to_delete.id).count() == 0 + assert models.Document.objects.all().count() == 1 + + assert models.Document.objects.filter(id=document_to_delete.id).exists() is False + assert models.Document.objects.filter(id=document_to_keep.id).exists() is True + + assert models.User.objects.filter(id=user_to_delete.id).exists() is False + + +@pytest.mark.django_db(transaction=True) +def test_tasks_user_delete_error_during_deletion_should_rollback_deletion(monkeypatch): + """Test transaction is correctly working by forcing an error during user deletion.""" + + user_to_delete = factories.UserFactory() + other_user = factories.UserFactory() + + # Documents where the user is the sole owner + documents_to_delete = factories.DocumentFactory.create_batch( + 3, creator=user_to_delete, users=[(user_to_delete, models.RoleChoices.OWNER)] + ) + # Documents where the user is not the sole owner + factories.DocumentFactory.create_batch( + 4, + creator=other_user, + users=[ + (user_to_delete, models.RoleChoices.OWNER), + (other_user, models.RoleChoices.OWNER), + ], + ) + # descendants of a document to delete should also be removed + depth = 5 + for _ in range(depth + 1): + documents_to_delete.append( + factories.DocumentFactory(parent=documents_to_delete[-1]) + ) + + # Documents created by the user to delete and should be set to null + factories.DocumentFactory.create_batch(3, creator=user_to_delete) + + # Non creator nor owner document but with accesses to delete + for role in models.RoleChoices.values: + if role == models.RoleChoices.OWNER.value: + continue + + factories.DocumentFactory(users=[(user_to_delete, role)]) + + # create 3 link traces + # + factories.DocumentFactory.create_batch(3, link_traces=[user_to_delete, other_user]) + # Create other link traces for the other user + # + factories.DocumentFactory.create_batch(3, link_traces=[other_user]) + + # Threads created by the user to delete should be set to null + threads_owned_by_user_to_delete = factories.ThreadFactory.create_batch( + 2, creator=user_to_delete + ) + # Threads created by the other user should not change + threads_owned_by_other_user = factories.ThreadFactory.create_batch( + 2, creator=other_user + ) + + # create comments for bot users in all existing threads + comments = [] + for thread in threads_owned_by_user_to_delete + threads_owned_by_other_user: + comments.append(factories.CommentFactory(thread=thread, user=user_to_delete)) + comments.append(factories.CommentFactory(thread=thread, user=other_user)) + + assert models.DocumentAccess.objects.filter(user=user_to_delete).count() == 11 + assert models.Document.objects.all().count() == 30 + assert models.LinkTrace.objects.all().count() == 9 + assert models.Thread.objects.all().count() == 4 + assert models.Comment.objects.all().count() == 8 + assert len(documents_to_delete) == 9 + + def mock_clear_user_created_documents(self): + raise DatabaseError() + + monkeypatch.setattr( + models.User, "_clear_user_created_documents", mock_clear_user_created_documents + ) + + with pytest.raises(DatabaseError): + user_to_delete.delete() + + assert models.DocumentAccess.objects.filter(user=user_to_delete).count() == 11 + assert models.Document.objects.all().count() == 30 + assert models.LinkTrace.objects.all().count() == 9 + assert models.Thread.objects.all().count() == 4 + assert models.Comment.objects.all().count() == 8 + assert len(documents_to_delete) == 9 + + assert models.User.objects.filter(id=user_to_delete.id).exists() is True