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