Remove use of auto_id without pattern for dynamically added sockets (#4473)

Replace use of auto_id with a dedicated form_id. Supplying `auto_id`
changes django's behaviour of setting the id on form elements (it
removed the default "id_" prefix) as explained in the [docs
here](https://docs.djangoproject.com/en/5.2/ref/forms/api/#configuring-form-elements-html-id-attributes-and-label-tags).
Because we only use the value to set the form id, we can use our own
context variable for that.

Also clean up some things:
- Rename `DICOMUploadWidgetSuffixes` to `DICOM_UPLOAD_WIDGET_SUFFIXES`
- Only check if socket exists for socket slugs that start with the right
prefix (reduce calls to database when loading form fields for
dynamically added sockets)

Related to DIAGNijmegen/rse-roadmap#435

Author: koopmant

app/grandchallenge/cases/widgets.py

@@ -211,7 +211,7 @@ def compress(self, values):
             return non_empty_values[0]
 
 
-DICOMUploadWidgetSuffixes = ["dicom-image-name", "dicom-user-uploads"]
+DICOM_UPLOAD_WIDGET_SUFFIXES = ["dicom-image-name", "dicom-user-uploads"]
 
 
 class DICOMUploadWithName(NamedTuple):
@@ -230,8 +230,8 @@ class DICOMUploadWidget(MultiWidget):
 
     def __init__(self, attrs=None):
         widgets = {
-            DICOMUploadWidgetSuffixes[0]: DICOMImageSetNameInput(),
-            DICOMUploadWidgetSuffixes[1]: DICOMUserUploadMultipleWidget(),
+            DICOM_UPLOAD_WIDGET_SUFFIXES[0]: DICOMImageSetNameInput(),
+            DICOM_UPLOAD_WIDGET_SUFFIXES[1]: DICOMUserUploadMultipleWidget(),
         }
         super().__init__(widgets, attrs)
 

app/grandchallenge/components/forms.py

@@ -18,7 +18,7 @@
 from django.utils.text import format_lazy
 
 from grandchallenge.algorithms.models import AlgorithmImage
-from grandchallenge.cases.widgets import DICOMUploadWidgetSuffixes
+from grandchallenge.cases.widgets import DICOM_UPLOAD_WIDGET_SUFFIXES
 from grandchallenge.components.backends.exceptions import (
     CIVNotEditableException,
 )
@@ -218,8 +218,11 @@ def __init__(self, *args, instance, base_obj, user, **kwargs):  # noqa C901
             interface_slug = self.parse_slug(slug=slug)
 
             if (
-                ComponentInterface.objects.filter(slug=interface_slug).exists()
+                interface_slug
                 and slug not in self.fields.keys()
+                and ComponentInterface.objects.filter(
+                    slug=interface_slug
+                ).exists()
             ):
                 interface = ComponentInterface.objects.filter(
                     slug=interface_slug
@@ -257,8 +260,12 @@ def __init__(self, *args, instance, base_obj, user, **kwargs):  # noqa C901
 
     @staticmethod
     def parse_slug(*, slug):
+        if not slug.startswith(INTERFACE_FORM_FIELD_PREFIX):
+            return None
+
         interface_slug = slug[len(INTERFACE_FORM_FIELD_PREFIX) :]
-        for known_suffix in DICOMUploadWidgetSuffixes:
+
+        for known_suffix in DICOM_UPLOAD_WIDGET_SUFFIXES:
             if interface_slug.endswith(f"_{known_suffix}"):
                 base_slug = interface_slug[: -len(f"_{known_suffix}")]
                 return base_slug
@@ -332,10 +339,12 @@ def __init__(
         interface,
         base_obj,
         user,
+        form_id,
         htmx_url,
         **kwargs,
     ):
         super().__init__(*args, **kwargs)
+        self.id = form_id
         data = kwargs.get("data")
 
         try:
@@ -365,7 +374,7 @@ def __init__(
             "hx-get": htmx_url,
             "hx-trigger": "interfaceSelected",
             "disabled": selected_interface is not None,
-            "hx-target": f"#form-{kwargs['auto_id']}",
+            "hx-target": f"#form-{form_id}",
             "hx-swap": "outerHTML",
             "hx-include": "this",
         }
@@ -386,7 +395,7 @@ def __init__(
             widget_kwargs["url"] = (
                 "components:component-interface-autocomplete"
             )
-            interface_field_name = f"interface-{kwargs['auto_id']}"
+            interface_field_name = f"interface-{form_id}"
             widget_kwargs["forward"] = [interface_field_name]
         widget_kwargs["attrs"] = attrs
 

app/grandchallenge/components/templates/components/new_interface_create.html

@@ -1,6 +1,7 @@
 {% load crispy_forms_tags %}
-<form class="extra-interface-form border rounded p-2 my-2" id="form-{{ form.auto_id }}">
-  {% csrf_token %}
-  {{ form|crispy }}
+
+<form class="extra-interface-form border rounded p-2 my-2" id="form-{{ form.id }}">
+    {% csrf_token %}
+    {{ form|crispy }}
     <button type="button" class="btn btn-danger remove-form">Remove</button>
 </form>

app/grandchallenge/components/views.py

@@ -195,7 +195,6 @@ def get_form_kwargs(self):
         kwargs.update(
             {
                 "user": self.request.user,
-                "auto_id": f"id-{uuid.uuid4()}",
                 "base_obj": self.base_object,
                 "instance": instance,
             }
@@ -269,7 +268,7 @@ def get_form_kwargs(self):
             "base_obj": self.base_object,
             "interface": self.request.GET.get("interface"),
             "user": self.request.user,
-            "auto_id": f"id-{uuid.uuid4()}",
+            "form_id": uuid.uuid4(),
             "htmx_url": self.get_htmx_url(),
         }
 

app/tests/cases_tests/test_forms.py

@@ -2,7 +2,7 @@
 from django.test import Client
 
 from grandchallenge.cases.models import RawImageUploadSession
-from grandchallenge.cases.widgets import DICOMUploadWidgetSuffixes
+from grandchallenge.cases.widgets import DICOM_UPLOAD_WIDGET_SUFFIXES
 from grandchallenge.components.form_fields import INTERFACE_FORM_FIELD_PREFIX
 from grandchallenge.components.forms import MultipleCIVForm
 from grandchallenge.components.models import ComponentInterface
@@ -82,8 +82,8 @@ def test_upload_some_images(
     "slug",
     [
         f"{INTERFACE_FORM_FIELD_PREFIX}foo-bar",
-        f"{INTERFACE_FORM_FIELD_PREFIX}foo-bar_{DICOMUploadWidgetSuffixes[0]}",
-        f"{INTERFACE_FORM_FIELD_PREFIX}foo-bar_{DICOMUploadWidgetSuffixes[1]}",
+        f"{INTERFACE_FORM_FIELD_PREFIX}foo-bar_{DICOM_UPLOAD_WIDGET_SUFFIXES[0]}",
+        f"{INTERFACE_FORM_FIELD_PREFIX}foo-bar_{DICOM_UPLOAD_WIDGET_SUFFIXES[1]}",
     ],
 )
 @pytest.mark.django_db

app/tests/cases_tests/test_widget.py

@@ -6,8 +6,8 @@
 from guardian.shortcuts import assign_perm
 
 from grandchallenge.cases.widgets import (
+    DICOM_UPLOAD_WIDGET_SUFFIXES,
     DICOMUploadField,
-    DICOMUploadWidgetSuffixes,
     DICOMUploadWithName,
     FlexibleImageField,
 )
@@ -222,10 +222,10 @@ def test_dicom_upload_field_validation():
     )
     parsed_value_for_upload_from_user = field.widget.value_from_datadict(
         data={
-            f"{prefixed_interface_slug}_{DICOMUploadWidgetSuffixes[1]}": [
+            f"{prefixed_interface_slug}_{DICOM_UPLOAD_WIDGET_SUFFIXES[1]}": [
                 str(upload1.pk)
             ],
-            f"{prefixed_interface_slug}_{DICOMUploadWidgetSuffixes[0]}": "test_image",
+            f"{prefixed_interface_slug}_{DICOM_UPLOAD_WIDGET_SUFFIXES[0]}": "test_image",
         },
         name=f"{prefixed_interface_slug}",
         files={},
@@ -246,10 +246,10 @@ def test_dicom_upload_field_validation():
     )
     parsed_value_from_upload_from_other_user = field.widget.value_from_datadict(
         data={
-            f"{prefixed_interface_slug}_{DICOMUploadWidgetSuffixes[1]}": [
+            f"{prefixed_interface_slug}_{DICOM_UPLOAD_WIDGET_SUFFIXES[1]}": [
                 str(upload2.pk)
             ],
-            f"{prefixed_interface_slug}_{DICOMUploadWidgetSuffixes[0]}": "test_image_2",
+            f"{prefixed_interface_slug}_{DICOM_UPLOAD_WIDGET_SUFFIXES[0]}": "test_image_2",
         },
         name=f"{prefixed_interface_slug}",
         files={},

app/tests/conftest.py

@@ -13,7 +13,7 @@
 
 from grandchallenge.algorithms.models import Job
 from grandchallenge.cases.widgets import (
-    DICOMUploadWidgetSuffixes,
+    DICOM_UPLOAD_WIDGET_SUFFIXES,
     ImageWidgetChoices,
 )
 from grandchallenge.components.backends import docker_client
@@ -516,10 +516,10 @@ def get_interface_form_data(
     ci = ComponentInterface.objects.get(slug=interface_slug)
     if ci.is_dicom_image_kind:
         form_data = {
-            f"{INTERFACE_FORM_FIELD_PREFIX}{interface_slug}_{DICOMUploadWidgetSuffixes[0]}": data[
+            f"{INTERFACE_FORM_FIELD_PREFIX}{interface_slug}_{DICOM_UPLOAD_WIDGET_SUFFIXES[0]}": data[
                 0
             ],
-            f"{INTERFACE_FORM_FIELD_PREFIX}{interface_slug}_{DICOMUploadWidgetSuffixes[1]}": data[
+            f"{INTERFACE_FORM_FIELD_PREFIX}{interface_slug}_{DICOM_UPLOAD_WIDGET_SUFFIXES[1]}": data[
                 1
             ],
         }

app/tests/reader_studies_tests/test_forms.py

@@ -1620,7 +1620,7 @@ def test_display_set_add_interface_form():
         interface=None,
         user=user,
         htmx_url="foo",
-        auto_id="1",
+        form_id="1",
     )
     assert sorted(form.fields.keys()) == ["interface-1"]
 
@@ -1630,7 +1630,7 @@ def test_display_set_add_interface_form():
         interface=ci_file.pk,
         user=user,
         htmx_url="foo",
-        auto_id="1",
+        form_id="1",
     )
     assert sorted(form.fields.keys()) == [
         f"{INTERFACE_FORM_FIELD_PREFIX}{ci_file.slug}",
@@ -1647,7 +1647,7 @@ def test_display_set_add_interface_form():
         interface=ci_value.pk,
         user=user,
         htmx_url="foo",
-        auto_id="1",
+        form_id="1",
     )
     assert sorted(form.fields.keys()) == [
         f"{INTERFACE_FORM_FIELD_PREFIX}{ci_value.slug}",
@@ -1664,7 +1664,7 @@ def test_display_set_add_interface_form():
         interface=ci_image.pk,
         user=user,
         htmx_url="foo",
-        auto_id="1",
+        form_id="1",
     )
     assert sorted(form.fields.keys()) == [
         f"{INTERFACE_FORM_FIELD_PREFIX}{ci_image.slug}",