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

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
+@shared_task
-@receiver(models.signals.post_save, sender=CustomField)
+def process_cf_select_update(custom_field: CustomField):
-def check_paths_and_prune_custom_fields(sender, instance: CustomField, **kwargs):
     """
-    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 = {
+        process_cf_select_update.delay(instance)
-            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)
 
 
 @receiver(models.signals.post_delete, sender=CustomField)

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:

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
+        # handler should have been called once via the async task
-        self.assertEqual(m.delay.call_count, 1)
+        self.assertEqual(m.call_count, 1)
 
 
 class TestFileHandlingWithArchive(DirectoriesMixin, FileSystemAssertsMixin, TestCase):