From bb775a16c1edeaacf1e07b553b6cce8a7b93d117 Mon Sep 17 00:00:00 2001 From: shamoon <4887959+shamoon@users.noreply.github.com> Date: Sat, 14 Nov 2026 11:01:10 -0800 Subject: [PATCH] Move the merge as version heirarchy checks to the bulk edit method --- src/documents/bulk_edit.py | 8 + src/documents/serialisers.py | 15 +- .../tests/test_merge_documents_as_versions.py | 160 +++++++++++------- 3 files changed, 105 insertions(+), 78 deletions(-) diff --git a/src/documents/bulk_edit.py b/src/documents/bulk_edit.py index ea4bd04d86..72f2633d2f 100644 --- a/src/documents/bulk_edit.py +++ b/src/documents/bulk_edit.py @@ -702,7 +702,15 @@ def merge_as_versions( ) documents_by_id = {document.id: document for document in documents} + if any(document.root_document_id is not None for document in documents): + raise ValueError("Only top-level documents can be merged as versions.") + source_ids = [doc_id for doc_id in doc_ids if doc_id != root_document_id] + if Document.global_objects.filter(root_document_id__in=source_ids).exists(): + raise ValueError( + "Documents with existing versions cannot be merged into another document.", + ) + root_document = documents_by_id[root_document_id] next_version_index = ( Document.global_objects.filter( diff --git a/src/documents/serialisers.py b/src/documents/serialisers.py index 3f3d16bb78..2c907fe209 100644 --- a/src/documents/serialisers.py +++ b/src/documents/serialisers.py @@ -1717,19 +1717,8 @@ class MergeDocumentsAsVersionsSerializer(DocumentListSerializer): "root_document_id must be one of the selected documents.", ) - selected_documents = Document.objects.filter(id__in=documents) - if selected_documents.filter(root_document__isnull=False).exists(): - raise serializers.ValidationError( - "Only top-level documents can be merged as versions.", - ) - - source_document_ids = set(documents) - {attrs["root_document_id"]} - if Document.global_objects.filter( - root_document_id__in=source_document_ids, - ).exists(): - raise serializers.ValidationError( - "Documents with existing versions cannot be merged into another document.", - ) + # Version state checks (e.g., whether a document is already part of a version chain) + # moved to merge_as_versions() under row locks, since it could change between queueing and execution return attrs diff --git a/src/documents/tests/test_merge_documents_as_versions.py b/src/documents/tests/test_merge_documents_as_versions.py index cb0a744316..5eb5789d19 100644 --- a/src/documents/tests/test_merge_documents_as_versions.py +++ b/src/documents/tests/test_merge_documents_as_versions.py @@ -105,67 +105,6 @@ class TestMergeDocumentsAsVersionsSerializer(TestCase): self.assertFalse(serializer.is_valid()) self.assertIn("documents", serializer.errors) - def test_rejects_selected_version(self) -> None: - version = Document.objects.create( - checksum="D", - title="D", - root_document=self.doc1, - version_index=1, - ) - serializer = MergeDocumentsAsVersionsSerializer( - data={ - "documents": [version.id, self.doc2.id], - "root_document_id": self.doc2.id, - }, - ) - - self.assertFalse(serializer.is_valid()) - self.assertEqual( - serializer.errors["non_field_errors"][0], - "Only top-level documents can be merged as versions.", - ) - - def test_rejects_source_document_with_versions(self) -> None: - Document.objects.create( - checksum="D", - title="D", - root_document=self.doc1, - version_index=1, - ) - serializer = MergeDocumentsAsVersionsSerializer( - data={ - "documents": [self.doc1.id, self.doc2.id], - "root_document_id": self.doc2.id, - }, - ) - - self.assertFalse(serializer.is_valid()) - self.assertEqual( - serializer.errors["non_field_errors"][0], - "Documents with existing versions cannot be merged into another document.", - ) - - def test_rejects_source_document_with_trashed_versions(self) -> None: - version = Document.objects.create( - checksum="D", - title="D", - root_document=self.doc1, - version_index=1, - ) - version.delete() # trashed, but still points at doc1 - serializer = MergeDocumentsAsVersionsSerializer( - data={ - "documents": [self.doc1.id, self.doc2.id], - "root_document_id": self.doc2.id, - }, - ) - - self.assertFalse(serializer.is_valid()) - self.assertEqual( - serializer.errors["non_field_errors"][0], - "Documents with existing versions cannot be merged into another document.", - ) - def test_allows_root_document_with_versions(self) -> None: Document.objects.create( checksum="D", @@ -184,6 +123,95 @@ class TestMergeDocumentsAsVersionsSerializer(TestCase): class TestMergeDocumentsAsVersions(TestCase): + def setUp(self) -> None: + self.doc1 = Document.objects.create(checksum="test-A", title="A") + self.doc2 = Document.objects.create(checksum="test-B", title="B") + + def test_rejects_selected_version(self) -> None: + version = Document.objects.create( + checksum="D", + title="D", + root_document=self.doc1, + version_index=1, + ) + with self.assertRaisesMessage( + ValueError, + "Only top-level documents can be merged as versions.", + ): + merge_as_versions( + [version.id, self.doc2.id], + root_document_id=self.doc2.id, + ) + + def test_rejects_source_document_with_versions(self) -> None: + Document.objects.create( + checksum="D", + title="D", + root_document=self.doc1, + version_index=1, + ) + with self.assertRaisesMessage( + ValueError, + "Documents with existing versions cannot be merged into another document.", + ): + merge_as_versions( + [self.doc1.id, self.doc2.id], + root_document_id=self.doc2.id, + ) + + def test_rejects_source_document_with_trashed_versions(self) -> None: + version = Document.objects.create( + checksum="D", + title="D", + root_document=self.doc1, + version_index=1, + ) + version.delete() # trashed, but still points at doc1 + with self.assertRaisesMessage( + ValueError, + "Documents with existing versions cannot be merged into another document.", + ): + merge_as_versions( + [self.doc1.id, self.doc2.id], + root_document_id=self.doc2.id, + ) + + @mock.patch("documents.bulk_edit.DocumentsStatusManager") + @mock.patch("documents.bulk_edit.bulk_update_documents.apply_async") + @mock.patch("documents.search.get_backend") + def test_rejects_source_that_gained_versions_after_validation( + self, + _get_backend_mock, + _bulk_update_mock, + _status_manager_mock, + ) -> None: + destination = Document.objects.create(checksum="destination") + serializer = MergeDocumentsAsVersionsSerializer( + data={ + "documents": [self.doc1.id, destination.id], + "root_document_id": destination.id, + }, + ) + self.assertTrue(serializer.is_valid(), serializer.errors) + + merge_as_versions( + [self.doc1.id, self.doc2.id], + root_document_id=self.doc1.id, + ) + with self.assertRaisesMessage( + ValueError, + "Documents with existing versions cannot be merged into another document.", + ): + merge_as_versions( + serializer.validated_data["documents"], + root_document_id=serializer.validated_data["root_document_id"], + ) + + self.doc1.refresh_from_db() + self.doc2.refresh_from_db() + self.assertIsNone(self.doc1.root_document_id) + self.assertEqual(self.doc2.root_document_id, self.doc1.id) + @mock.patch("documents.bulk_edit.DocumentsStatusManager") @mock.patch("documents.bulk_edit.bulk_update_documents.apply_async") @mock.patch("documents.search.get_backend") @@ -456,8 +484,9 @@ class TestMergeDocumentsAsVersionsAPI(APITestCase): self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) merge_mock.assert_not_called() - @mock.patch("documents.views.bulk_edit.merge_as_versions") - def test_rejects_source_document_with_versions(self, merge_mock) -> None: + @mock.patch("documents.views.execute_bulk_operation.apply_async") + def test_queues_source_document_with_versions(self, enqueue_mock) -> None: + enqueue_mock.return_value.id = "queued-merge" Document.objects.create( checksum="C", title="C", @@ -475,8 +504,9 @@ class TestMergeDocumentsAsVersionsAPI(APITestCase): format="json", ) - self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - merge_mock.assert_not_called() + self.assertEqual(response.status_code, status.HTTP_200_OK) + enqueue_mock.assert_called_once() + self.assertEqual(response.data["task_id"], "queued-merge") self.doc1.refresh_from_db() self.assertIsNone(self.doc1.root_document_id)