From dbe87bdc5d76afc967e5bb1bd245676044833473 Mon Sep 17 00:00:00 2001 From: Dan Bentley Date: Fri, 23 Sep 2022 14:43:40 +0100 Subject: [PATCH] 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 | 1 + docs/releases/4.1.md | 1 + wagtail/contrib/forms/forms.py | 50 +++++++++++++---------- wagtail/contrib/forms/tests/test_views.py | 41 +++++++++++++++++++ 4 files changed, 71 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.txt b/CHANGELOG.txt index 7951ec3fb8..4f37cacc30 100644 --- a/CHANGELOG.txt +++ b/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 ~~~~~~~~~~~~~~~~~~ diff --git a/docs/releases/4.1.md b/docs/releases/4.1.md index b436ff9505..f0653edb25 100644 --- a/docs/releases/4.1.md +++ b/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 diff --git a/wagtail/contrib/forms/forms.py b/wagtail/contrib/forms/forms.py index 3d1786fd67..25effb0809 100644 --- a/wagtail/contrib/forms/forms.py +++ b/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 + ) + ), + ) diff --git a/wagtail/contrib/forms/tests/test_views.py b/wagtail/contrib/forms/tests/test_views.py index 0e1faca5f9..ad336f7767 100644 --- a/wagtail/contrib/forms/tests/test_views.py +++ b/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):