From f44922c79516b9caf0e09fb060f20c896668b90f Mon Sep 17 00:00:00 2001 From: Claude Paroz Date: Fri, 8 Feb 2013 21:30:06 +0100 Subject: [PATCH] Fixed #18906 -- Ignored to-be-deleted forms in formset validate_unique Thanks c.pollock at bangor.ac.uk for the report. --- django/forms/formsets.py | 5 ++--- django/forms/models.py | 19 ++++++------------- tests/modeltests/model_formsets/tests.py | 22 +++++++++++++++------- 3 files changed, 23 insertions(+), 23 deletions(-) diff --git a/django/forms/formsets.py b/django/forms/formsets.py index ee9a2b5f63..1addbc617b 100644 --- a/django/forms/formsets.py +++ b/django/forms/formsets.py @@ -179,11 +179,10 @@ class BaseFormSet(object): @property def deleted_forms(self): """ - Returns a list of forms that have been marked for deletion. Raises an - AttributeError if deletion is not allowed. + Returns a list of forms that have been marked for deletion. """ if not self.is_valid() or not self.can_delete: - raise AttributeError("'%s' object has no attribute 'deleted_forms'" % self.__class__.__name__) + return [] # construct _deleted_form_indexes which is just a list of form indexes # that have had their deletion widget set to True if not hasattr(self, '_deleted_form_indexes'): diff --git a/django/forms/models.py b/django/forms/models.py index e2d739fd1a..c4e0301614 100644 --- a/django/forms/models.py +++ b/django/forms/models.py @@ -520,9 +520,9 @@ class BaseModelFormSet(BaseFormSet): # Collect unique_checks and date_checks to run from all the forms. all_unique_checks = set() all_date_checks = set() - for form in self.forms: - if not form.is_valid(): - continue + forms_to_delete = self.deleted_forms + valid_forms = [form for form in self.forms if form.is_valid() and form not in forms_to_delete] + for form in valid_forms: exclude = form._get_validation_exclusions() unique_checks, date_checks = form.instance._get_unique_checks(exclude=exclude) all_unique_checks = all_unique_checks.union(set(unique_checks)) @@ -532,9 +532,7 @@ class BaseModelFormSet(BaseFormSet): # Do each of the unique checks (unique and unique_together) for uclass, unique_check in all_unique_checks: seen_data = set() - for form in self.forms: - if not form.is_valid(): - continue + for form in valid_forms: # get data for each field of each of unique_check row_data = tuple([form.cleaned_data[field] for field in unique_check if field in form.cleaned_data]) if row_data and not None in row_data: @@ -554,9 +552,7 @@ class BaseModelFormSet(BaseFormSet): for date_check in all_date_checks: seen_data = set() uclass, lookup, field, unique_for = date_check - for form in self.forms: - if not form.is_valid(): - continue + for form in valid_forms: # see if we have data for both fields if (form.cleaned_data and form.cleaned_data[field] is not None and form.cleaned_data[unique_for] is not None): @@ -611,10 +607,7 @@ class BaseModelFormSet(BaseFormSet): return [] saved_instances = [] - try: - forms_to_delete = self.deleted_forms - except AttributeError: - forms_to_delete = [] + forms_to_delete = self.deleted_forms for form in self.initial_forms: pk_name = self._pk_field.name raw_pk_value = form._raw_value(pk_name) diff --git a/tests/modeltests/model_formsets/tests.py b/tests/modeltests/model_formsets/tests.py index a028e65143..50ee3c73fb 100644 --- a/tests/modeltests/model_formsets/tests.py +++ b/tests/modeltests/model_formsets/tests.py @@ -42,21 +42,29 @@ class DeletionTests(TestCase): doesn't cause validation errors. """ PoetFormSet = modelformset_factory(Poet, can_delete=True) + poet = Poet.objects.create(name='test') + # One existing untouched and two new unvalid forms data = { - 'form-TOTAL_FORMS': '1', - 'form-INITIAL_FORMS': '0', + 'form-TOTAL_FORMS': '3', + 'form-INITIAL_FORMS': '1', 'form-MAX_NUM_FORMS': '0', - 'form-0-id': '', - 'form-0-name': 'x' * 1000, + 'form-0-id': six.text_type(poet.id), + 'form-0-name': 'test', + 'form-1-id': '', + 'form-1-name': 'x' * 1000, # Too long + 'form-1-id': six.text_type(poet.id), # Violate unique constraint + 'form-1-name': 'test2', } formset = PoetFormSet(data, queryset=Poet.objects.all()) # Make sure this form doesn't pass validation. self.assertEqual(formset.is_valid(), False) - self.assertEqual(Poet.objects.count(), 0) + self.assertEqual(Poet.objects.count(), 1) # Then make sure that it *does* pass validation and delete the object, - # even though the data isn't actually valid. + # even though the data in new forms aren't actually valid. data['form-0-DELETE'] = 'on' + data['form-1-DELETE'] = 'on' + data['form-2-DELETE'] = 'on' formset = PoetFormSet(data, queryset=Poet.objects.all()) self.assertEqual(formset.is_valid(), True) formset.save() @@ -64,7 +72,7 @@ class DeletionTests(TestCase): def test_change_form_deletion_when_invalid(self): """ - Make sure that an add form that is filled out, but marked for deletion + Make sure that a change form that is filled out, but marked for deletion doesn't cause validation errors. """ PoetFormSet = modelformset_factory(Poet, can_delete=True)