diff --git a/src/documents/management/commands/document_renamer.py b/src/documents/management/commands/document_renamer.py index 0e16f5cce9..b91c8f8f79 100644 --- a/src/documents/management/commands/document_renamer.py +++ b/src/documents/management/commands/document_renamer.py @@ -1,5 +1,4 @@ -from django.db.models.signals import post_save - +from documents.file_moves import move_files_for_document from documents.management.commands.base import PaperlessCommand from documents.models import Document @@ -11,5 +10,8 @@ class Command(PaperlessCommand): supports_multiprocessing = False def handle(self, *args, **options): - for document in self.track(Document.objects.all(), description="Renaming..."): - post_save.send(Document, instance=document, created=False) + for document in self.track( + Document.objects.only("pk"), + description="Renaming...", + ): + move_files_for_document(document.pk) diff --git a/src/documents/management/commands/document_retagger.py b/src/documents/management/commands/document_retagger.py index fc2e52e86c..06effdf5ef 100644 --- a/src/documents/management/commands/document_retagger.py +++ b/src/documents/management/commands/document_retagger.py @@ -9,6 +9,7 @@ from rich.table import Table from rich.text import Text from documents.classifier import load_classifier +from documents.file_moves import defer_file_moves from documents.management.commands.base import PaperlessCommand from documents.models import Document from documents.signals.handlers import set_correspondent @@ -273,61 +274,63 @@ class Command(PaperlessCommand): description="Retagging...", stats_renderer=render_stats, ): - suggestion = DocumentSuggestion(document=document) + # One file move per document, after all of its assignments. + with defer_file_moves(): + suggestion = DocumentSuggestion(document=document) - if do_correspondent: - correspondent = set_correspondent( - None, - document, - classifier=classifier, - replace=overwrite, - use_first=use_first, - dry_run=suggest, - ) - if correspondent is not None: - stats.correspondents += 1 - suggestion.correspondent = correspondent + if do_correspondent: + correspondent = set_correspondent( + None, + document, + classifier=classifier, + replace=overwrite, + use_first=use_first, + dry_run=suggest, + ) + if correspondent is not None: + stats.correspondents += 1 + suggestion.correspondent = correspondent - if do_document_type: - document_type = set_document_type( - None, - document, - classifier=classifier, - replace=overwrite, - use_first=use_first, - dry_run=suggest, - ) - if document_type is not None: - stats.document_types += 1 - suggestion.document_type = document_type + if do_document_type: + document_type = set_document_type( + None, + document, + classifier=classifier, + replace=overwrite, + use_first=use_first, + dry_run=suggest, + ) + if document_type is not None: + stats.document_types += 1 + suggestion.document_type = document_type - if do_tags: - tags_to_add, tags_to_remove = set_tags( - None, - document, - classifier=classifier, - replace=overwrite, - dry_run=suggest, - ) - stats.tags_added += len(tags_to_add) - stats.tags_removed += len(tags_to_remove) - suggestion.tags_to_add = frozenset(tags_to_add) - suggestion.tags_to_remove = frozenset(tags_to_remove) + if do_tags: + tags_to_add, tags_to_remove = set_tags( + None, + document, + classifier=classifier, + replace=overwrite, + dry_run=suggest, + ) + stats.tags_added += len(tags_to_add) + stats.tags_removed += len(tags_to_remove) + suggestion.tags_to_add = frozenset(tags_to_add) + suggestion.tags_to_remove = frozenset(tags_to_remove) - if do_storage_path: - storage_path = set_storage_path( - None, - document, - classifier=classifier, - replace=overwrite, - use_first=use_first, - dry_run=suggest, - ) - if storage_path is not None: - stats.storage_paths += 1 - suggestion.storage_path = storage_path + if do_storage_path: + storage_path = set_storage_path( + None, + document, + classifier=classifier, + replace=overwrite, + use_first=use_first, + dry_run=suggest, + ) + if storage_path is not None: + stats.storage_paths += 1 + suggestion.storage_path = storage_path - stats.documents_processed += 1 + stats.documents_processed += 1 if suggest: suggestions.append(suggestion) diff --git a/src/documents/tests/test_management.py b/src/documents/tests/test_management.py index 5f0bb3664c..aea36a4f66 100644 --- a/src/documents/tests/test_management.py +++ b/src/documents/tests/test_management.py @@ -18,6 +18,7 @@ if TYPE_CHECKING: from pytest_mock import MockerFixture from documents.file_handling import generate_filename +from documents.file_moves import move_files_for_document from documents.models import Document from documents.tasks import update_document_content_maybe_archive_file from paperless_testing.assertions import FileSystemAssertsMixin @@ -276,6 +277,43 @@ class TestRenamer(DirectoriesMixin, FileSystemAssertsMixin, TestCase): self.assertIsFile(doc2.source_path) self.assertIsFile(doc2.archive_path) + @override_settings(FILENAME_FORMAT="") + def test_rename_moves_each_document_by_direct_call(self) -> None: + """ + GIVEN: + - Two documents whose filename format changed + WHEN: + - document_renamer runs + THEN: + - Each document is moved by a direct call, one per document, + not by a fake post_save signal + - Both files are at their new paths + """ + docs = [] + for title in ("first", "second"): + doc = Document.objects.create(title=title, mime_type="image/jpeg") + doc.filename = generate_filename(doc) + doc.save(write_file_paths=True) + Path(doc.source_path).touch() + docs.append(doc) + + with ( + override_settings(FILENAME_FORMAT="{title}"), + mock.patch( + "documents.management.commands.document_renamer.move_files_for_document", + wraps=move_files_for_document, + ) as mover, + ): + call_command("document_renamer", skip_checks=True) + + self.assertCountEqual( + [c.args[0] for c in mover.call_args_list], + [d.pk for d in docs], + ) + for doc in docs: + doc.refresh_from_db() + self.assertIsFile(doc.source_path) + @pytest.mark.management class TestCreateClassifier: diff --git a/src/documents/tests/test_management_retagger.py b/src/documents/tests/test_management_retagger.py index d909946a84..311438963a 100644 --- a/src/documents/tests/test_management_retagger.py +++ b/src/documents/tests/test_management_retagger.py @@ -4,10 +4,13 @@ Tests for the document_retagger management command. from __future__ import annotations +from typing import TYPE_CHECKING + import pytest from django.core.management import call_command from django.core.management.base import CommandError +from documents.file_moves import _active_collector from documents.models import Correspondent from documents.models import Document from documents.models import DocumentType @@ -21,6 +24,12 @@ from paperless_testing.factories import DocumentTypeFactory from paperless_testing.factories import StoragePathFactory from paperless_testing.factories import TagFactory +if TYPE_CHECKING: + from pytest_django.fixtures import Settings + from pytest_mock import MockerFixture + + from paperless_testing.dirs import PaperlessDirs + # --------------------------------------------------------------------------- # Module-level type aliases # --------------------------------------------------------------------------- @@ -484,3 +493,69 @@ class TestRetaggerEdgeCases(DirectoriesMixin): assert d_first.tags.count() == 0 assert d_unrelated.tags.count() == 2 + + +@pytest.mark.management +@pytest.mark.django_db +class TestRetaggerFileMoves: + def test_each_document_is_processed_in_its_own_scope( + self, + paperless_dirs: PaperlessDirs, + settings: Settings, + mocker: MockerFixture, + ) -> None: + """ + GIVEN: + - Two documents and a filename format using the correspondent + - A classifier suggestion assigning a correspondent to each + WHEN: + - document_retagger runs with --correspondent + THEN: + - Each document is processed inside an open file move scope + - When the second document is processed, the first one's file has + already moved + - Both files end at their final paths + """ + settings.FILENAME_FORMAT = "{correspondent}/{title}" + (paperless_dirs.originals_dir / "none").mkdir(exist_ok=True) + for title in ("first", "second"): + (paperless_dirs.originals_dir / "none" / f"{title}.pdf").write_bytes(b"x") + DocumentFactory( + title=title, + filename=f"none/{title}.pdf", + mime_type="application/pdf", + ) + correspondent = CorrespondentFactory(name="acme") + processed: list[Document] = [] + scope_open: list[bool] = [] + seen_first_moved: list[bool] = [] + + def fake_set_correspondent(sender, document, **kwargs): + scope_open.append(_active_collector.get() is not None) + # Order-agnostic: on the second call, check the first one's file. + if processed: + first = processed[0] + seen_first_moved.append( + ( + paperless_dirs.originals_dir / "acme" / f"{first.title}.pdf" + ).is_file(), + ) + processed.append(document) + document.correspondent = correspondent + document.save(update_fields=("correspondent",)) + return correspondent + + mocker.patch( + "documents.management.commands.document_retagger.load_classifier", + ) + mocker.patch( + "documents.management.commands.document_retagger.set_correspondent", + side_effect=fake_set_correspondent, + ) + + call_command("document_retagger", "--correspondent", skip_checks=True) + + assert scope_open == [True, True] + assert seen_first_moved == [True] + for title in ("first", "second"): + assert (paperless_dirs.originals_dir / "acme" / f"{title}.pdf").is_file()