From 340754d86538381d0bb9926a402d4721486dede0 Mon Sep 17 00:00:00 2001 From: Jan Kleine Date: Wed, 15 Oct 2025 19:10:25 +0200 Subject: [PATCH] Enhancement: use friendly file names when emailing documents (#11055) --- src/documents/mail.py | 51 ++++++++++-- src/documents/signals/handlers.py | 3 +- src/documents/tests/test_api_documents.py | 6 +- src/documents/tests/test_api_email.py | 99 ++++++++++++++++++++--- src/documents/views.py | 12 +-- 5 files changed, 142 insertions(+), 29 deletions(-) diff --git a/src/documents/mail.py b/src/documents/mail.py index fa7d61feb..240b41e18 100644 --- a/src/documents/mail.py +++ b/src/documents/mail.py @@ -1,16 +1,23 @@ +from __future__ import annotations + from email import message_from_bytes -from pathlib import Path +from typing import TYPE_CHECKING from django.conf import settings from django.core.mail import EmailMessage from filelock import FileLock +if TYPE_CHECKING: + from documents.models import Document + def send_email( subject: str, body: str, to: list[str], - attachments: list[tuple[Path, str]], + attachments: list[Document], + *, + use_archive: bool, ) -> int: """ Send an email with attachments. @@ -19,7 +26,8 @@ def send_email( subject: Email subject body: Email body text to: List of recipient email addresses - attachments: List of (path, mime_type) tuples for attachments (the list may be empty) + attachments: List of documents to attach (the list may be empty) + use_archive: Whether to attach archive versions when available Returns: Number of emails sent @@ -32,19 +40,48 @@ def send_email( to=to, ) + used_filenames: set[str] = set() + # Something could be renaming the file concurrently so it can't be attached with FileLock(settings.MEDIA_LOCK): - for attachment_path, mime_type in attachments: + for document in attachments: + attachment_path = ( + document.archive_path + if use_archive and document.has_archive_version + else document.source_path + ) + + friendly_filename = _get_unique_filename( + document, + used_filenames, + archive=use_archive and document.has_archive_version, + ) + used_filenames.add(friendly_filename) + with attachment_path.open("rb") as f: content = f.read() - if mime_type == "message/rfc822": + if document.mime_type == "message/rfc822": # See https://forum.djangoproject.com/t/using-emailmessage-with-an-attached-email-file-crashes-due-to-non-ascii/37981 content = message_from_bytes(content) email.attach( - filename=attachment_path.name, + filename=friendly_filename, content=content, - mimetype=mime_type, + mimetype=document.mime_type, ) return email.send() + + +def _get_unique_filename(doc: Document, used_names: set[str], *, archive: bool) -> str: + """ + Constructs a unique friendly filename for the given document. + + The filename might not be unique enough, so a counter is appended if needed. + """ + counter = 0 + while True: + filename = doc.get_public_filename(archive=archive, counter=counter) + if filename not in used_names: + return filename + counter += 1 diff --git a/src/documents/signals/handlers.py b/src/documents/signals/handlers.py index 12eedb49b..8862b064b 100644 --- a/src/documents/signals/handlers.py +++ b/src/documents/signals/handlers.py @@ -1164,12 +1164,13 @@ def run_workflows( try: attachments = [] if action.email.include_document and original_file: - attachments = [(original_file, document.mime_type)] + attachments = [document] n_messages = send_email( subject=subject, body=body, to=action.email.to.split(","), attachments=attachments, + use_archive=False, ) logger.debug( f"Sent {n_messages} notification email(s) to {action.email.to}", diff --git a/src/documents/tests/test_api_documents.py b/src/documents/tests/test_api_documents.py index 906cfa351..3f7b2c385 100644 --- a/src/documents/tests/test_api_documents.py +++ b/src/documents/tests/test_api_documents.py @@ -3022,7 +3022,8 @@ class TestDocumentApi(DirectoriesMixin, DocumentConsumeDelayMixin, APITestCase): ) self.assertEqual(len(mail.outbox), 1) - self.assertEqual(mail.outbox[0].attachments[0][0], "archive.pdf") + expected_filename = f"{doc.created} test.pdf" + self.assertEqual(mail.outbox[0].attachments[0][0], expected_filename) self.client.post( f"/api/documents/{doc2.pk}/email/", @@ -3035,7 +3036,8 @@ class TestDocumentApi(DirectoriesMixin, DocumentConsumeDelayMixin, APITestCase): ) self.assertEqual(len(mail.outbox), 2) - self.assertEqual(mail.outbox[1].attachments[0][0], "test2.pdf") + expected_filename2 = f"{doc2.created} test2.pdf" + self.assertEqual(mail.outbox[1].attachments[0][0], expected_filename2) @mock.patch("django.core.mail.message.EmailMessage.send", side_effect=Exception) def test_email_document_errors(self, mocked_send): diff --git a/src/documents/tests/test_api_email.py b/src/documents/tests/test_api_email.py index ed2fed750..0f9bd9695 100644 --- a/src/documents/tests/test_api_email.py +++ b/src/documents/tests/test_api_email.py @@ -40,10 +40,19 @@ class TestEmail(DirectoriesMixin, SampleDirMixin, APITestCase): filename="test2.pdf", ) - # Copy sample files to document paths - shutil.copy(self.SAMPLE_DIR / "simple.pdf", self.doc1.archive_path) - shutil.copy(self.SAMPLE_DIR / "simple.pdf", self.doc1.source_path) - shutil.copy(self.SAMPLE_DIR / "simple.pdf", self.doc2.source_path) + # Copy sample files to document paths (using different files to distinguish versions) + shutil.copy( + self.SAMPLE_DIR / "documents" / "originals" / "0000001.pdf", + self.doc1.archive_path, + ) + shutil.copy( + self.SAMPLE_DIR / "documents" / "originals" / "0000002.pdf", + self.doc1.source_path, + ) + shutil.copy( + self.SAMPLE_DIR / "documents" / "originals" / "0000003.pdf", + self.doc2.source_path, + ) @override_settings( EMAIL_ENABLED=True, @@ -52,11 +61,13 @@ class TestEmail(DirectoriesMixin, SampleDirMixin, APITestCase): def test_email_success(self): """ GIVEN: - - Multiple existing documents + - Multiple existing documents (doc1 with archive, doc2 without) WHEN: - API request is made to bulk email documents THEN: - Email is sent with all documents attached + - Archive version used by default for doc1 + - Original version used for doc2 (no archive available) """ response = self.client.post( self.ENDPOINT, @@ -81,10 +92,22 @@ class TestEmail(DirectoriesMixin, SampleDirMixin, APITestCase): self.assertEqual(email.body, "Here are your documents") self.assertEqual(len(email.attachments), 2) - # Check attachment names (should default to archive version for doc1, original for doc2) attachment_names = [att[0] for att in email.attachments] - self.assertIn("archive1.pdf", attachment_names) - self.assertIn("test2.pdf", attachment_names) + self.assertEqual(len(attachment_names), 2) + self.assertIn(f"{self.doc1!s}.pdf", attachment_names) + self.assertIn(f"{self.doc2!s}.pdf", attachment_names) + + doc1_attachment = next( + att for att in email.attachments if att[0] == f"{self.doc1!s}.pdf" + ) + archive_size = self.doc1.archive_path.stat().st_size + self.assertEqual(len(doc1_attachment[1]), archive_size) + + doc2_attachment = next( + att for att in email.attachments if att[0] == f"{self.doc2!s}.pdf" + ) + original_size = self.doc2.source_path.stat().st_size + self.assertEqual(len(doc2_attachment[1]), original_size) @override_settings( EMAIL_ENABLED=True, @@ -115,7 +138,12 @@ class TestEmail(DirectoriesMixin, SampleDirMixin, APITestCase): self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(len(mail.outbox), 1) - self.assertEqual(mail.outbox[0].attachments[0][0], "test1.pdf") + + attachment = mail.outbox[0].attachments[0] + self.assertEqual(attachment[0], f"{self.doc1!s}.pdf") + + original_size = self.doc1.source_path.stat().st_size + self.assertEqual(len(attachment[1]), original_size) def test_email_missing_required_fields(self): """ @@ -301,6 +329,59 @@ class TestEmail(DirectoriesMixin, SampleDirMixin, APITestCase): ) self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + @override_settings( + EMAIL_ENABLED=True, + EMAIL_BACKEND="django.core.mail.backends.locmem.EmailBackend", + ) + def test_email_duplicate_filenames(self): + """ + GIVEN: + - Multiple documents with the same title + WHEN: + - API request is made to bulk email documents + THEN: + - Filenames are made unique with counters + """ + doc3 = Document.objects.create( + title="test1", + mime_type="application/pdf", + content="this is document 3", + checksum="3", + filename="test3.pdf", + ) + shutil.copy(self.SAMPLE_DIR / "simple.pdf", doc3.source_path) + + doc4 = Document.objects.create( + title="test1", + mime_type="application/pdf", + content="this is document 4", + checksum="4", + filename="test4.pdf", + ) + shutil.copy(self.SAMPLE_DIR / "simple.pdf", doc4.source_path) + + response = self.client.post( + self.ENDPOINT, + json.dumps( + { + "documents": [self.doc1.pk, doc3.pk, doc4.pk], + "addresses": "test@example.com", + "subject": "Test", + "message": "Test message", + }, + ), + content_type="application/json", + ) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(len(mail.outbox), 1) + + attachment_names = [att[0] for att in mail.outbox[0].attachments] + self.assertEqual(len(attachment_names), 3) + self.assertIn(f"{self.doc1!s}.pdf", attachment_names) + self.assertIn(f"{doc3!s}_01.pdf", attachment_names) + self.assertIn(f"{doc3!s}_02.pdf", attachment_names) + @mock.patch( "django.core.mail.message.EmailMessage.send", side_effect=Exception("Email error"), diff --git a/src/documents/views.py b/src/documents/views.py index eec24d13d..4168eba38 100644 --- a/src/documents/views.py +++ b/src/documents/views.py @@ -1207,21 +1207,13 @@ class DocumentViewSet( ): return HttpResponseForbidden("Insufficient permissions") - attachments = [] - for doc in documents: - attachment_path = ( - doc.archive_path - if use_archive_version and doc.has_archive_version - else doc.source_path - ) - attachments.append((attachment_path, doc.mime_type)) - try: send_email( subject=subject, body=message, to=addresses, - attachments=attachments, + attachments=documents, + use_archive=use_archive_version, ) logger.debug(