Fix check for duplicate FormField.clean_names

WagtailAdminFormPageForm.clean checks for duplicate fields by generating
a clean_name from a FormField's current label, catching duplicate labels.

However, once saved, a FormField's clean_name cannot change. It's
possible for clean_names to clash if a a field is renamed and add a new
field with the same name.

Updated duplicate check to take existing clean_name into account.

Refactored logic to avoid nested loops.

CHANGELOG.txt

@@ -51,6 +51,7 @@ Changelog
  * Fix: Ensure `for_user` argument is passed to the form class when previewing pages (Matt Westcott)
  * Fix: Ensure the capitalisation of the `timesince_simple` tag is consistently added in the template based on usage in context (Stefan Hammer)
  * Fix: Add missing translation usage for the `timesince_last_update` and ensure the translated labels can be easier to work with in Transifex (Stefan Hammer)
+ * Fix: Add additional checks for duplicate form field `clean_name` values in the Form Builder validation and increase performance of checks (Dan Bentley)
 
 4.0.3 (xx.xx.xxxx) - IN DEVELOPMENT
 ~~~~~~~~~~~~~~~~~~

docs/releases/4.1.md

@@ -76,6 +76,7 @@ This feature was developed by Karl Hobley and Matt Westcott.
  * Ensure `for_user` argument is passed to the form class when previewing pages (Matt Westcott)
  * Ensure the capitalisation of the `timesince_simple` tag is consistently added in the template based on usage in context (Stefan Hammer)
  * Add missing translation usage for the `timesince_last_update` and ensure the translated labels can be easier to work with in Transifex (Stefan Hammer)
+ * Add additional checks for duplicate form field `clean_name` values in the Form Builder validation and increase performance of checks (Dan Bentley)
 
 ## Upgrade considerations
 

wagtail/contrib/forms/forms.py

@@ -174,29 +174,35 @@ class SelectDateForm(django.forms.Form):
 
 class WagtailAdminFormPageForm(WagtailAdminPageForm):
     def clean(self):
-
         super().clean()
 
-        # Check for dupe form field labels - fixes #585
+        # Check for duplicate form fields by comparing their internal clean_names
         if "form_fields" in self.formsets:
-            _forms = self.formsets["form_fields"].forms
-            for f in _forms:
-                f.is_valid()
+            forms = self.formsets["form_fields"].forms
+            for form in forms:
+                form.is_valid()
 
-            for i, form in enumerate(_forms):
-                if "label" in form.changed_data:
-                    label = form.cleaned_data.get("label")
-                    clean_name = form.instance.get_field_clean_name()
-                    for idx, ff in enumerate(_forms):
-                        # Exclude self
-                        ff_clean_name = ff.instance.get_field_clean_name()
-                        if idx != i and clean_name == ff_clean_name:
-                            form.add_error(
-                                "label",
-                                django.forms.ValidationError(
-                                    _(
-                                        "There is another field with the label %s, please change one of them."
-                                    )
-                                    % label
-                                ),
-                            )
+            # Use existing clean_name or generate for new fields.
+            # clean_name is set in FormField.save
+            clean_names = [
+                f.instance.clean_name or f.instance.get_field_clean_name()
+                for f in forms
+            ]
+            duplicate_clean_name = next(
+                (n for n in clean_names if clean_names.count(n) > 1), None
+            )
+            if duplicate_clean_name:
+                duplicate_form_field = next(
+                    f
+                    for f in self.formsets["form_fields"].forms
+                    if f.instance.get_field_clean_name() == duplicate_clean_name
+                )
+                duplicate_form_field.add_error(
+                    "label",
+                    django.forms.ValidationError(
+                        _(
+                            "There is another field with the label %s, please change one of them."
+                            % duplicate_form_field.instance.label
+                        )
+                    ),
+                )

wagtail/contrib/forms/tests/test_views.py

@@ -29,6 +29,7 @@ from wagtail.test.testapp.models import (
     FormPageWithCustomSubmissionListView,
 )
 from wagtail.test.utils import WagtailTestUtils
+from wagtail.test.utils.form_data import inline_formset, nested_form_data
 
 
 class TestFormResponsesPanel(TestCase):
@@ -1735,6 +1736,46 @@ class TestDuplicateFormFieldLabels(TestCase, WagtailTestUtils):
             text="There is another field with the label duplicate 1, please change one of them.",
         )
 
+    def test_rename_existing_field_and_add_new_field_with_clashing_clean_name(
+        self,
+    ):
+        """Ensure duplicate field names are checked against existing field clean_names."""
+
+        form_page = FormPage(
+            title="Form page!",
+            form_fields=[FormField(label="Test field", field_type="singleline")],
+        )
+        self.root_page.add_child(instance=form_page)
+
+        # Rename existing field and add new field with label matching original field
+        post_data = nested_form_data(
+            {
+                "title": "Form page!",
+                "slug": "form-page",
+                "form_fields": inline_formset(
+                    [
+                        {
+                            "id": form_page.form_fields.first().pk,
+                            "label": "Other field",
+                            "field_type": "singleline",
+                        },
+                        {"id": "", "label": "Test field", "field_type": "singleline"},
+                    ],
+                    initial=1,
+                ),
+                "action-publish": "action-publish",
+            }
+        )
+        response = self.client.post(
+            reverse("wagtailadmin_pages:edit", args=[form_page.pk]), post_data
+        )
+
+        self.assertEqual(response.status_code, 200)
+        self.assertContains(
+            response,
+            text="There is another field with the label Test field, please change one of them.",
+        )
+
 
 class TestPreview(TestCase, WagtailTestUtils):