Fix: change async handling of select custom field updates (#11490)

diff --git a/src/documents/signals/handlers.py b/src/documents/signals/handlers.py
index 0e53c93576cb881085d67d52a20a7866520d2418..e410b54e200e61e8b105952b1df4fe3941386342 100644 (file)
--- a/src/documents/signals/handlers.py
+++ b/src/documents/signals/handlers.py
@@ -396,7 +396,6 @@ class CannotMoveFilesException(Exception):
 @receiver(models.signals.post_save, sender=CustomFieldInstance, weak=False)
 @receiver(models.signals.m2m_changed, sender=Document.tags.through, weak=False)
 @receiver(models.signals.post_save, sender=Document, weak=False)
-@shared_task
 def update_filename_and_move_files(
     sender,
     instance: Document | CustomFieldInstance,
@@ -533,34 +532,43 @@ def update_filename_and_move_files(
             )
 
 
-# should be disabled in /src/documents/management/commands/document_importer.py handle
-@receiver(models.signals.post_save, sender=CustomField)
-def check_paths_and_prune_custom_fields(sender, instance: CustomField, **kwargs):
+@shared_task
+def process_cf_select_update(custom_field: CustomField):
     """
-    When a custom field is updated:
+    Update documents tied to a select custom field:
+
     1. 'Select' custom field instances get their end-user value (e.g. in file names) from the select_options in extra_data,
     which is contained in the custom field itself. So when the field is changed, we (may) need to update the file names
     of all documents that have this custom field.
     2. If a 'Select' field option was removed, we need to nullify the custom field instances that have the option.
     """
+    select_options = {
+        option["id"]: option["label"]
+        for option in custom_field.extra_data.get("select_options", [])
+    }
+
+    # Clear select values that no longer exist
+    custom_field.fields.exclude(
+        value_select__in=select_options.keys(),
+    ).update(value_select=None)
+
+    for cf_instance in custom_field.fields.select_related("document").iterator():
+        # Update the filename and move files if necessary
+        update_filename_and_move_files(CustomFieldInstance, cf_instance)
+
+
+# should be disabled in /src/documents/management/commands/document_importer.py handle
+@receiver(models.signals.post_save, sender=CustomField)
+def check_paths_and_prune_custom_fields(sender, instance: CustomField, **kwargs):
+    """
+    When a custom field is updated, check if we need to update any documents. Done async to avoid slowing down the save operation.
+    """
     if (
         instance.data_type == CustomField.FieldDataType.SELECT
         and instance.fields.count() > 0
         and instance.extra_data
     ):  # Only select fields, for now
-        select_options = {
-            option["id"]: option["label"]
-            for option in instance.extra_data.get("select_options", [])
-        }
-
-        for cf_instance in instance.fields.all():
-            # Check if the current value is still a valid option
-            if cf_instance.value not in select_options:
-                cf_instance.value_select = None
-                cf_instance.save(update_fields=["value_select"])
-
-            # Update the filename and move files if necessary
-            update_filename_and_move_files.delay(sender, cf_instance)
+        process_cf_select_update.delay(instance)
 
 
 @receiver(models.signals.post_delete, sender=CustomField)

diff --git a/src/documents/tests/test_api_custom_fields.py b/src/documents/tests/test_api_custom_fields.py
index b6e6c134238c7a07da7097f8f8c1fce4102b82bf..8cc8f2cb2acf4a845461ba89101a7fab6c1ca33b 100644 (file)
--- a/src/documents/tests/test_api_custom_fields.py
+++ b/src/documents/tests/test_api_custom_fields.py
@@ -1,5 +1,6 @@
 import json
 from datetime import date
+from unittest import mock
 from unittest.mock import ANY
 
 from django.contrib.auth.models import Permission
@@ -276,6 +277,52 @@ class TestCustomFieldsAPI(DirectoriesMixin, APITestCase):
         doc.refresh_from_db()
         self.assertEqual(doc.custom_fields.first().value, None)
 
+    @mock.patch("documents.signals.handlers.process_cf_select_update.delay")
+    def test_custom_field_update_offloaded_once(self, mock_delay):
+        """
+        GIVEN:
+            - A select custom field attached to multiple documents
+        WHEN:
+            - The select options are updated
+        THEN:
+            - The async update task is enqueued once
+        """
+        cf_select = CustomField.objects.create(
+            name="Select Field",
+            data_type=CustomField.FieldDataType.SELECT,
+            extra_data={
+                "select_options": [
+                    {"label": "Option 1", "id": "abc-123"},
+                    {"label": "Option 2", "id": "def-456"},
+                ],
+            },
+        )
+
+        documents = [
+            Document.objects.create(
+                title="WOW",
+                content="the content",
+                checksum=f"{i}",
+                mime_type="application/pdf",
+            )
+            for i in range(3)
+        ]
+        for document in documents:
+            CustomFieldInstance.objects.create(
+                document=document,
+                field=cf_select,
+                value_select="def-456",
+            )
+
+        cf_select.extra_data = {
+            "select_options": [
+                {"label": "Option 1", "id": "abc-123"},
+            ],
+        }
+        cf_select.save()
+
+        mock_delay.assert_called_once_with(cf_select)
+
     def test_custom_field_select_old_version(self):
         """
         GIVEN:

diff --git a/src/documents/tests/test_file_handling.py b/src/documents/tests/test_file_handling.py
index 117a964baa072f6ea7315687fd26defdfb7974eb..a1fd3d67441eed132200e9149653976f8ace566a 100644 (file)
--- a/src/documents/tests/test_file_handling.py
+++ b/src/documents/tests/test_file_handling.py
@@ -530,6 +530,7 @@ class TestFileHandling(DirectoriesMixin, FileSystemAssertsMixin, TestCase):
 
     @override_settings(
         FILENAME_FORMAT="{{title}}_{{custom_fields|get_cf_value('test')}}",
+        CELERY_TASK_ALWAYS_EAGER=True,
     )
     @mock.patch("documents.signals.handlers.update_filename_and_move_files")
     def test_select_cf_updated(self, m):
@@ -569,7 +570,7 @@ class TestFileHandling(DirectoriesMixin, FileSystemAssertsMixin, TestCase):
         self.assertEqual(generate_filename(doc), Path("document_apple.pdf"))
 
         # handler should not have been called
-        self.assertEqual(m.delay.call_count, 0)
+        self.assertEqual(m.call_count, 0)
         cf.extra_data = {
             "select_options": [
                 {"label": "aubergine", "id": "abc123"},
@@ -579,8 +580,8 @@ class TestFileHandling(DirectoriesMixin, FileSystemAssertsMixin, TestCase):
         }
         cf.save()
         self.assertEqual(generate_filename(doc), Path("document_aubergine.pdf"))
-        # handler should have been called via delay
-        self.assertEqual(m.delay.call_count, 1)
+        # handler should have been called once via the async task
+        self.assertEqual(m.call_count, 1)
 
 
 class TestFileHandlingWithArchive(DirectoriesMixin, FileSystemAssertsMixin, TestCase):