From 6d5f4e92cc420085e206f3b8facf30144da79854 Mon Sep 17 00:00:00 2001 From: shamoon <4887959+shamoon@users.noreply.github.com> Date: Wed, 10 Jan 2024 10:18:55 -0800 Subject: [PATCH] Enhancement: title assignment placeholder error handling, fallback (#5282) --- .../workflow-edit-dialog.component.html | 2 +- src/documents/consumer.py | 15 ++++--- src/documents/serialisers.py | 38 +++++++++++++--- src/documents/signals/handlers.py | 34 ++++++++------ src/documents/tests/test_api_workflows.py | 39 ++++++++++++++++ src/documents/tests/test_consumer.py | 10 +++++ src/documents/tests/test_workflows.py | 44 +++++++++++++++++++ 7 files changed, 157 insertions(+), 25 deletions(-) diff --git a/src-ui/src/app/components/common/edit-dialog/workflow-edit-dialog/workflow-edit-dialog.component.html b/src-ui/src/app/components/common/edit-dialog/workflow-edit-dialog/workflow-edit-dialog.component.html index b4d4b02ca..76d788e59 100644 --- a/src-ui/src/app/components/common/edit-dialog/workflow-edit-dialog/workflow-edit-dialog.component.html +++ b/src-ui/src/app/components/common/edit-dialog/workflow-edit-dialog/workflow-edit-dialog.component.html @@ -99,7 +99,7 @@
- + diff --git a/src/documents/consumer.py b/src/documents/consumer.py index b7a559575..06e9f68fc 100644 --- a/src/documents/consumer.py +++ b/src/documents/consumer.py @@ -726,12 +726,17 @@ class Consumer(LoggingMixin): storage_type = Document.STORAGE_TYPE_UNENCRYPTED + title = file_info.title[:127] + if self.override_title is not None: + try: + title = self._parse_title_placeholders(self.override_title) + except Exception as e: + self.log.error( + f"Error occurred parsing title override '{self.override_title}', falling back to original. Exception: {e}", + ) + document = Document.objects.create( - title=( - self._parse_title_placeholders(self.override_title) - if self.override_title is not None - else file_info.title - )[:127], + title=title, content=text, mime_type=mime_type, checksum=hashlib.md5(self.working_copy.read_bytes()).hexdigest(), diff --git a/src/documents/serialisers.py b/src/documents/serialisers.py index 233c56367..eb0280fbd 100644 --- a/src/documents/serialisers.py +++ b/src/documents/serialisers.py @@ -1386,12 +1386,38 @@ class WorkflowActionSerializer(serializers.ModelSerializer): def validate(self, attrs): # Empty strings treated as None to avoid unexpected behavior - if ( - "assign_title" in attrs - and attrs["assign_title"] is not None - and len(attrs["assign_title"]) == 0 - ): - attrs["assign_title"] = None + if "assign_title" in attrs: + if attrs["assign_title"] is not None and len(attrs["assign_title"]) == 0: + attrs["assign_title"] = None + else: + try: + # test against all placeholders, see consumer.py `parse_doc_title_w_placeholders` + attrs["assign_title"].format( + correspondent="", + document_type="", + added="", + added_year="", + added_year_short="", + added_month="", + added_month_name="", + added_month_name_short="", + added_day="", + added_time="", + owner_username="", + original_filename="", + created="", + created_year="", + created_year_short="", + created_month="", + created_month_name="", + created_month_name_short="", + created_day="", + created_time="", + ) + except (ValueError, KeyError) as e: + raise serializers.ValidationError( + {"assign_title": f'Invalid f-string detected: "{e.args[0]}"'}, + ) return attrs diff --git a/src/documents/signals/handlers.py b/src/documents/signals/handlers.py index 01c62f079..eee06bb6e 100644 --- a/src/documents/signals/handlers.py +++ b/src/documents/signals/handlers.py @@ -570,19 +570,27 @@ def run_workflow( document.owner = action.assign_owner if action.assign_title is not None: - document.title = parse_doc_title_w_placeholders( - action.assign_title, - document.correspondent.name - if document.correspondent is not None - else "", - document.document_type.name - if document.document_type is not None - else "", - document.owner.username if document.owner is not None else "", - timezone.localtime(document.added), - document.original_filename, - timezone.localtime(document.created), - ) + try: + document.title = parse_doc_title_w_placeholders( + action.assign_title, + document.correspondent.name + if document.correspondent is not None + else "", + document.document_type.name + if document.document_type is not None + else "", + document.owner.username + if document.owner is not None + else "", + document.added, + document.original_filename, + document.created, + ) + except Exception: + logger.exception( + f"Error occurred parsing title assignment '{action.assign_title}', falling back to original", + extra={"group": logging_group}, + ) if ( action.assign_view_users is not None diff --git a/src/documents/tests/test_api_workflows.py b/src/documents/tests/test_api_workflows.py index d7a7ad6ff..21e887c24 100644 --- a/src/documents/tests/test_api_workflows.py +++ b/src/documents/tests/test_api_workflows.py @@ -248,6 +248,45 @@ class TestApiWorkflows(DirectoriesMixin, APITestCase): self.assertEqual(WorkflowTrigger.objects.count(), 1) + def test_api_create_invalid_assign_title(self): + """ + GIVEN: + - API request to create a workflow + - Invalid f-string for assign_title + WHEN: + - API is called + THEN: + - Correct HTTP 400 response + - No objects are created + """ + response = self.client.post( + self.ENDPOINT, + json.dumps( + { + "name": "Workflow 1", + "order": 1, + "triggers": [ + { + "type": WorkflowTrigger.WorkflowTriggerType.DOCUMENT_UPDATED, + }, + ], + "actions": [ + { + "assign_title": "{created_year]", + }, + ], + }, + ), + content_type="application/json", + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertIn( + "Invalid f-string detected", + response.data["actions"][0]["assign_title"][0], + ) + + self.assertEqual(Workflow.objects.count(), 1) + def test_api_create_workflow_trigger_action_empty_fields(self): """ GIVEN: diff --git a/src/documents/tests/test_consumer.py b/src/documents/tests/test_consumer.py index 748e49e10..f77265970 100644 --- a/src/documents/tests/test_consumer.py +++ b/src/documents/tests/test_consumer.py @@ -423,6 +423,16 @@ class TestConsumer(DirectoriesMixin, FileSystemAssertsMixin, TestCase): self.assertEqual(document.title, "Override Title") self._assert_first_last_send_progress() + def testOverrideTitleInvalidPlaceholders(self): + with self.assertLogs("paperless.consumer", level="ERROR") as cm: + document = self.consumer.try_consume_file( + self.get_test_file(), + override_title="Override {correspondent]", + ) + self.assertEqual(document.title, "sample") + expected_str = "Error occurred parsing title override 'Override {correspondent]', falling back to original" + self.assertIn(expected_str, cm.output[0]) + def testOverrideCorrespondent(self): c = Correspondent.objects.create(name="test") diff --git a/src/documents/tests/test_workflows.py b/src/documents/tests/test_workflows.py index b4ad4aa57..b688eecc9 100644 --- a/src/documents/tests/test_workflows.py +++ b/src/documents/tests/test_workflows.py @@ -966,6 +966,50 @@ class TestWorkflows(DirectoriesMixin, FileSystemAssertsMixin, APITestCase): expected_str = f"Document correspondent {doc.correspondent} does not match {trigger.filter_has_correspondent}" self.assertIn(expected_str, cm.output[1]) + def test_document_added_invalid_title_placeholders(self): + """ + GIVEN: + - Existing workflow with added trigger type + - Assign title field has an error + WHEN: + - File that matches is added + THEN: + - Title is not updated, error is output + """ + trigger = WorkflowTrigger.objects.create( + type=WorkflowTrigger.WorkflowTriggerType.DOCUMENT_ADDED, + filter_filename="*sample*", + ) + action = WorkflowAction.objects.create( + assign_title="Doc {created_year]", + ) + w = Workflow.objects.create( + name="Workflow 1", + order=0, + ) + w.triggers.add(trigger) + w.actions.add(action) + w.save() + + now = timezone.localtime(timezone.now()) + created = now - timedelta(weeks=520) + doc = Document.objects.create( + original_filename="sample.pdf", + title="sample test", + content="Hello world bar", + created=created, + ) + + with self.assertLogs("paperless.handlers", level="ERROR") as cm: + document_consumption_finished.send( + sender=self.__class__, + document=doc, + ) + expected_str = f"Error occurred parsing title assignment '{action.assign_title}', falling back to original" + self.assertIn(expected_str, cm.output[0]) + + self.assertEqual(doc.title, "sample test") + def test_document_updated_workflow(self): trigger = WorkflowTrigger.objects.create( type=WorkflowTrigger.WorkflowTriggerType.DOCUMENT_UPDATED,