diff --git a/CHANGELOG.md b/CHANGELOG.md index ea87977f..9e6bd3fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ and this project adheres to ## Added -- ✨(backend) add soft delete API endpoint to documents #516 +- ✨(backend) add soft delete and restore API endpoints to documents #516 - ✨(backend) allow organizing documents in a tree structure #516 - ✨(backend) add "excerpt" field to document list serializer #516 - ✨(backend) add github actions to manage Crowdin workflow #559 & #563 diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index fdc56014..93f8be0f 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -714,6 +714,22 @@ class DocumentViewSet( {"message": "Document moved successfully."}, status=status.HTTP_200_OK ) + @drf.decorators.action( + detail=True, + methods=["post"], + ) + def restore(self, request, *args, **kwargs): + """ + Restore a soft-deleted document if it was deleted less than x days ago. + """ + document = self.get_object() + document.restore() + + return drf_response.Response( + {"detail": "Document has been successfully restored."}, + status=status.HTTP_200_OK, + ) + @drf.decorators.action( detail=True, methods=["get", "post"], diff --git a/src/backend/core/models.py b/src/backend/core/models.py index fca9cfcc..4aa4de87 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -662,6 +662,7 @@ class Document(MP_Node, BaseModel): "invite_owner": is_owner, "move": is_owner_or_admin and not self.ancestors_deleted_at, "partial_update": can_update, + "restore": is_owner, "retrieve": can_get, "media_auth": can_get, "update": can_update, @@ -753,6 +754,52 @@ class Document(MP_Node, BaseModel): ancestors_deleted_at=self.ancestors_deleted_at ) + @transaction.atomic + def restore(self): + """Cancelling a soft delete with checks.""" + # This should not happen + if self.deleted_at is None: + raise ValidationError({"deleted_at": [_("This document is not deleted.")]}) + + if self.deleted_at < get_trashbin_cutoff(): + raise ValidationError( + { + "deleted_at": [ + _( + "This document was permanently deleted and cannot be restored." + ) + ] + } + ) + + # Restore the current document + self.deleted_at = None + + # Calculate the minimum `deleted_at` among all ancestors + ancestors_deleted_at = ( + self.get_ancestors() + .filter(deleted_at__isnull=False) + .values_list("deleted_at", flat=True) + ) + self.ancestors_deleted_at = min(ancestors_deleted_at, default=None) + self.save() + + # Update descendants excluding those who were deleted prior to the deletion of the + # current document (the ancestor_deleted_at date for those should already by good) + # The number of deleted descendants should not be too big so we can handcraft a union + # clause for them: + deleted_descendants_paths = ( + self.get_descendants() + .filter(deleted_at__isnull=False) + .values_list("path", flat=True) + ) + exclude_condition = models.Q( + *(models.Q(path__startswith=path) for path in deleted_descendants_paths) + ) + self.get_descendants().exclude(exclude_condition).update( + ancestors_deleted_at=self.ancestors_deleted_at + ) + class LinkTrace(BaseModel): """ diff --git a/src/backend/core/tests/documents/test_api_documents_restore.py b/src/backend/core/tests/documents/test_api_documents_restore.py new file mode 100644 index 00000000..5ae64aec --- /dev/null +++ b/src/backend/core/tests/documents/test_api_documents_restore.py @@ -0,0 +1,126 @@ +""" +Test restoring documents after a soft delete via the detail action API endpoint. +""" + +from datetime import timedelta + +from django.utils import timezone + +import pytest +from rest_framework.test import APIClient + +from core import factories + +pytestmark = pytest.mark.django_db + + +def test_api_documents_restore_anonymous_user(): + """Anonymous users should not be able to restore deleted documents.""" + now = timezone.now() - timedelta(days=15) + document = factories.DocumentFactory(deleted_at=now) + + response = APIClient().post(f"/api/v1.0/documents/{document.id!s}/restore/") + + assert response.status_code == 404 + assert response.json() == {"detail": "Not found."} + + document.refresh_from_db() + assert document.deleted_at == now + assert document.ancestors_deleted_at == now + + +@pytest.mark.parametrize("role", [None, "reader", "editor", "administrator"]) +def test_api_documents_restore_authenticated_no_permission(role): + """ + Authenticated users who are not owners of a deleted document should + not be allowed to restore it. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + now = timezone.now() - timedelta(days=15) + document = factories.DocumentFactory( + deleted_at=now, link_reach="public", link_role="editor" + ) + if role: + factories.UserDocumentAccessFactory(document=document, user=user, role=role) + + response = client.post(f"/api/v1.0/documents/{document.id!s}/restore/") + + assert response.status_code == 404 + assert response.json() == {"detail": "Not found."} + + document.refresh_from_db() + assert document.deleted_at == now + assert document.ancestors_deleted_at == now + + +def test_api_documents_restore_authenticated_owner_success(): + """The owner of a deleted document should be able to restore it.""" + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + now = timezone.now() - timedelta(days=15) + document = factories.DocumentFactory(deleted_at=now) + factories.UserDocumentAccessFactory(document=document, user=user, role="owner") + + response = client.post(f"/api/v1.0/documents/{document.id!s}/restore/") + + assert response.status_code == 200 + assert response.json() == {"detail": "Document has been successfully restored."} + + document.refresh_from_db() + assert document.deleted_at is None + assert document.ancestors_deleted_at is None + + +def test_api_documents_restore_authenticated_owner_ancestor_deleted(): + """ + The restored document should still be marked as deleted if one of its + ancestors is soft deleted as well. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + grand_parent = factories.DocumentFactory() + parent = factories.DocumentFactory(parent=grand_parent) + document = factories.DocumentFactory(parent=parent) + factories.UserDocumentAccessFactory(document=document, user=user, role="owner") + + document.soft_delete() + document_deleted_at = document.deleted_at + assert document_deleted_at is not None + + grand_parent.soft_delete() + grand_parent_deleted_at = grand_parent.deleted_at + assert grand_parent_deleted_at is not None + + response = client.post(f"/api/v1.0/documents/{document.id!s}/restore/") + + assert response.status_code == 200 + assert response.json() == {"detail": "Document has been successfully restored."} + + document.refresh_from_db() + assert document.deleted_at is None + # document is still marked as deleted + assert document.ancestors_deleted_at == grand_parent_deleted_at + assert grand_parent_deleted_at > document_deleted_at + + +def test_api_documents_restore_authenticated_owner_expired(): + """It should not be possible to restore a document beyond the allowed time limit.""" + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + now = timezone.now() - timedelta(days=40) + document = factories.DocumentFactory(deleted_at=now) + factories.UserDocumentAccessFactory(document=document, user=user, role="owner") + + response = client.post(f"/api/v1.0/documents/{document.id!s}/restore/") + + assert response.status_code == 404 + assert response.json() == {"detail": "Not found."} diff --git a/src/backend/core/tests/documents/test_api_documents_retrieve.py b/src/backend/core/tests/documents/test_api_documents_retrieve.py index 99682f65..215164df 100644 --- a/src/backend/core/tests/documents/test_api_documents_retrieve.py +++ b/src/backend/core/tests/documents/test_api_documents_retrieve.py @@ -42,6 +42,7 @@ def test_api_documents_retrieve_anonymous_public_standalone(): "media_auth": True, "move": False, "partial_update": document.link_role == "editor", + "restore": False, "retrieve": True, "update": document.link_role == "editor", "versions_destroy": False, @@ -97,6 +98,7 @@ def test_api_documents_retrieve_anonymous_public_parent(): "media_auth": True, "move": False, "partial_update": grand_parent.link_role == "editor", + "restore": False, "retrieve": True, "update": grand_parent.link_role == "editor", "versions_destroy": False, @@ -185,6 +187,7 @@ def test_api_documents_retrieve_authenticated_unrelated_public_or_authenticated( "media_auth": True, "move": False, "partial_update": document.link_role == "editor", + "restore": False, "retrieve": True, "update": document.link_role == "editor", "versions_destroy": False, @@ -247,6 +250,7 @@ def test_api_documents_retrieve_authenticated_public_or_authenticated_parent(rea "move": False, "media_auth": True, "partial_update": grand_parent.link_role == "editor", + "restore": False, "retrieve": True, "update": grand_parent.link_role == "editor", "versions_destroy": False, @@ -418,6 +422,7 @@ def test_api_documents_retrieve_authenticated_related_parent(): "media_auth": True, "move": access.role in ["administrator", "owner"], "partial_update": access.role != "reader", + "restore": access.role == "owner", "retrieve": True, "update": access.role != "reader", "versions_destroy": access.role in ["administrator", "owner"], diff --git a/src/backend/core/tests/test_models_documents.py b/src/backend/core/tests/test_models_documents.py index a28194f3..028740cb 100644 --- a/src/backend/core/tests/test_models_documents.py +++ b/src/backend/core/tests/test_models_documents.py @@ -160,6 +160,7 @@ def test_models_documents_get_abilities_forbidden( "move": False, "link_configuration": False, "partial_update": False, + "restore": False, "retrieve": False, "update": False, "versions_destroy": False, @@ -207,6 +208,7 @@ def test_models_documents_get_abilities_reader( "media_auth": True, "move": False, "partial_update": False, + "restore": False, "retrieve": True, "update": False, "versions_destroy": False, @@ -254,6 +256,7 @@ def test_models_documents_get_abilities_editor( "media_auth": True, "move": False, "partial_update": True, + "restore": False, "retrieve": True, "update": True, "versions_destroy": False, @@ -288,6 +291,7 @@ def test_models_documents_get_abilities_owner(django_assert_num_queries): "media_auth": True, "move": True, "partial_update": True, + "restore": True, "retrieve": True, "update": True, "versions_destroy": True, @@ -322,6 +326,7 @@ def test_models_documents_get_abilities_administrator(django_assert_num_queries) "media_auth": True, "move": True, "partial_update": True, + "restore": False, "retrieve": True, "update": True, "versions_destroy": True, @@ -355,6 +360,7 @@ def test_models_documents_get_abilities_editor_user(django_assert_num_queries): "media_auth": True, "move": False, "partial_update": True, + "restore": False, "retrieve": True, "update": True, "versions_destroy": False, @@ -391,6 +397,7 @@ def test_models_documents_get_abilities_reader_user(django_assert_num_queries): "media_auth": True, "move": False, "partial_update": access_from_link, + "restore": False, "retrieve": True, "update": access_from_link, "versions_destroy": False, @@ -430,6 +437,7 @@ def test_models_documents_get_abilities_preset_role(django_assert_num_queries): "media_auth": True, "move": False, "partial_update": False, + "restore": False, "retrieve": True, "update": False, "versions_destroy": False,