From 21fb88e3f084388ecc6fd5840a50be56037cbc1c Mon Sep 17 00:00:00 2001 From: shamoon <4887959+shamoon@users.noreply.github.com> Date: Mon, 7 Sep 2026 19:11:20 -0700 Subject: [PATCH] Fix: prevent orphaned versions from trashing --- src/documents/bulk_edit.py | 3 +- src/documents/models.py | 12 +++++- src/documents/tests/test_api_trash.py | 62 +++++++++++++++++++++++++++ src/documents/tests/test_bulk_edit.py | 5 +++ src/documents/views.py | 15 ++++++- 5 files changed, 92 insertions(+), 5 deletions(-) diff --git a/src/documents/bulk_edit.py b/src/documents/bulk_edit.py index 4053ee8cb..79a79e6cf 100644 --- a/src/documents/bulk_edit.py +++ b/src/documents/bulk_edit.py @@ -2,6 +2,7 @@ from __future__ import annotations import logging import tempfile +import uuid from pathlib import Path from typing import TYPE_CHECKING from typing import Literal @@ -379,7 +380,7 @@ def delete(doc_ids: list[int]) -> Literal["OK"]: ) delete_ids = list({*doc_ids, *version_ids}) - Document.objects.filter(id__in=delete_ids).delete() + Document.objects.filter(id__in=delete_ids).delete(transaction_id=uuid.uuid4()) from documents.search import get_backend diff --git a/src/documents/models.py b/src/documents/models.py index 0cc4decda..521f43b6a 100644 --- a/src/documents/models.py +++ b/src/documents/models.py @@ -1,4 +1,5 @@ import datetime +import uuid from pathlib import Path from typing import Final @@ -514,13 +515,20 @@ class Document(SoftDeleteModel, ModelWithOwner): # type: ignore[django-manager- def delete( self, *args, + transaction_id=None, **kwargs, ): - # If deleting a root document, move all its versions to trash as well. + # Versions must share the root's transaction ID so they are restored + # together by django-softdelete. + if transaction_id is None: + transaction_id = uuid.uuid4() if self.root_document_id is None: - Document.objects.filter(root_document=self).delete() + Document.objects.filter(root_document=self).delete( + transaction_id=transaction_id, + ) return super().delete( *args, + transaction_id=transaction_id, **kwargs, ) diff --git a/src/documents/tests/test_api_trash.py b/src/documents/tests/test_api_trash.py index 420b7727d..1d55f61aa 100644 --- a/src/documents/tests/test_api_trash.py +++ b/src/documents/tests/test_api_trash.py @@ -207,3 +207,65 @@ class TestTrashAPI(DirectoriesMixin, APITestCase): ) self.assertEqual(resp.status_code, status.HTTP_400_BAD_REQUEST) self.assertIn("have not yet been deleted", resp.data["documents"][0]) + + def _make_versioned_document(self) -> tuple[Document, list[Document]]: + root = Document.objects.create( + title="root", + content="root-content", + checksum="root", + mime_type="application/pdf", + ) + versions = [ + Document.objects.create( + title=f"v{index}", + content=f"v{index}-content", + checksum=f"v{index}", + mime_type="application/pdf", + root_document=root, + version_index=index, + ) + for index in range(1, 3) + ] + return root, versions + + def test_api_trash_restore_document_restores_its_versions(self) -> None: + """ + GIVEN: + - Existing document with two versions + WHEN: + - API request to delete the document + - API request to restore it from the trash + THEN: + - Only the document itself is listed in the trash + - A version cannot be restored without its root + - The document is restored together with all of its versions + """ + root, versions = self._make_versioned_document() + + self.client.force_login(user=self.user) + self.client.delete(f"/api/documents/{root.pk}/") + self.assertEqual(Document.deleted_objects.count(), 3) + + resp = self.client.get("/api/trash/") + self.assertEqual(resp.status_code, status.HTTP_200_OK) + self.assertEqual(resp.data["count"], 1) + self.assertEqual(resp.data["results"][0]["id"], root.pk) + + # A version cannot be restored while its root remains in the trash. + resp = self.client.post( + "/api/trash/", + {"action": "restore", "documents": [versions[0].pk]}, + ) + self.assertEqual(resp.status_code, status.HTTP_400_BAD_REQUEST) + self.assertIn("Restore the root document", resp.data["documents"][0]) + + resp = self.client.post( + "/api/trash/", + {"action": "restore", "documents": [root.pk]}, + ) + self.assertEqual(resp.status_code, status.HTTP_200_OK) + self.assertEqual(Document.deleted_objects.count(), 0) + self.assertCountEqual( + Document.objects.filter(root_document=root).values_list("id", flat=True), + [version.pk for version in versions], + ) diff --git a/src/documents/tests/test_bulk_edit.py b/src/documents/tests/test_bulk_edit.py index 6063a9bbc..1315dd84b 100644 --- a/src/documents/tests/test_bulk_edit.py +++ b/src/documents/tests/test_bulk_edit.py @@ -392,6 +392,11 @@ class TestBulkEdit(DirectoriesMixin, TestCase): self.assertFalse(Document.objects.filter(id=self.doc1.id).exists()) self.assertFalse(Document.objects.filter(id=version.id).exists()) + Document.deleted_objects.get(id=self.doc1.id).restore(strict=False) + + self.assertTrue(Document.objects.filter(id=self.doc1.id).exists()) + self.assertTrue(Document.objects.filter(id=version.id).exists()) + def test_delete_version_document_keeps_root(self) -> None: version = Document.objects.create( checksum="A-v1", diff --git a/src/documents/views.py b/src/documents/views.py index 4cf775bd6..9f2b1fba1 100644 --- a/src/documents/views.py +++ b/src/documents/views.py @@ -5431,7 +5431,10 @@ class TrashView(ListModelMixin, PassUserMixin): model = Document - queryset = Document.deleted_objects.all() + # A version is listed separately only when its root is not in the trash. + queryset = Document.deleted_objects.exclude( + root_document_id__in=Document.deleted_objects.values("id"), + ) def get(self, request: Request, format: str | None = None) -> Response: self.serializer_class = DocumentSerializer @@ -5462,7 +5465,15 @@ class TrashView(ListModelMixin, PassUserMixin): return HttpResponseForbidden("Insufficient permissions") action = serializer.validated_data.get("action") if action == "restore": - restored = list(Document.deleted_objects.filter(id__in=doc_ids)) + restored = list(self.get_queryset().filter(id__in=doc_ids)) + if len(restored) != len(doc_ids): + raise ValidationError( + { + "documents": [ + "Restore the root document instead of one of its versions.", + ], + }, + ) for doc in restored: doc.restore(strict=False) if restored: