From 239342fbbd216ad2005f245f04e2555332cf31f3 Mon Sep 17 00:00:00 2001 From: Samuel Paccoud - DINUM Date: Sun, 5 Jan 2025 14:43:15 +0100 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8(backend)=20add=20API=20endpoint=20act?= =?UTF-8?q?ion=20to=20restore=20a=20soft=20deleted=20document?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only owners can see and restore deleted documents. They can only do it during the grace period before the document is considered hard deleted and hidden from everybody on the API. --- CHANGELOG.md | 2 +- src/backend/core/api/viewsets.py | 16 +++ src/backend/core/models.py | 47 +++++++ .../documents/test_api_documents_restore.py | 126 ++++++++++++++++++ .../documents/test_api_documents_retrieve.py | 5 + .../core/tests/test_models_documents.py | 8 ++ 6 files changed, 203 insertions(+), 1 deletion(-) create mode 100644 src/backend/core/tests/documents/test_api_documents_restore.py 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,