diff --git a/django/contrib/admin/options.py b/django/contrib/admin/options.py index e67f081557..2f39c6a896 100644 --- a/django/contrib/admin/options.py +++ b/django/contrib/admin/options.py @@ -1261,7 +1261,7 @@ class ModelAdmin(BaseModelAdmin): form_validated = True else: form_validated = False - new_object = self.model() + new_object = form.instance formsets, inline_instances = self._create_formsets(request, new_object) if all_valid(formsets) and form_validated: self.save_model(request, new_object, form, False) @@ -1342,7 +1342,7 @@ class ModelAdmin(BaseModelAdmin): new_object = self.save_form(request, form, change=True) else: form_validated = False - new_object = obj + new_object = form.instance formsets, inline_instances = self._create_formsets(request, new_object) if all_valid(formsets) and form_validated: self.save_model(request, new_object, form, True) diff --git a/docs/ref/contrib/admin/index.txt b/docs/ref/contrib/admin/index.txt index 3313f99250..efd72f58db 100644 --- a/docs/ref/contrib/admin/index.txt +++ b/docs/ref/contrib/admin/index.txt @@ -1870,6 +1870,12 @@ The ``InlineModelAdmin`` class adds: through to :func:`~django.forms.models.inlineformset_factory` when creating the formset for this inline. +.. warning:: + When writing custom validation for ``InlineModelAdmin`` forms, be cautious + of writing validation that relies on features of the parent model. If the + parent model fails to validate, it may be left in an inconsistent state as + described in the warning in :ref:`validation-on-modelform`. + .. attribute:: InlineModelAdmin.extra This controls the number of extra forms the formset will display in diff --git a/tests/admin_views/admin.py b/tests/admin_views/admin.py index 039383a7dc..5a895958b0 100644 --- a/tests/admin_views/admin.py +++ b/tests/admin_views/admin.py @@ -7,6 +7,7 @@ import os from django import forms from django.contrib import admin from django.contrib.admin.views.main import ChangeList +from django.core.exceptions import ValidationError from django.core.files.storage import FileSystemStorage from django.core.mail import EmailMessage from django.core.servers.basehttp import FileWrapper @@ -31,7 +32,8 @@ from .models import (Article, Chapter, Account, Media, Child, Parent, Picture, AdminOrderedCallable, Report, Color2, UnorderedObject, MainPrepopulated, RelatedPrepopulated, UndeletableObject, UnchangeableObject, UserMessenger, Simple, Choice, ShortMessage, Telegram, FilteredManager, EmptyModelHidden, - EmptyModelVisible, EmptyModelMixin, State, City, Restaurant, Worker) + EmptyModelVisible, EmptyModelMixin, State, City, Restaurant, Worker, + ParentWithDependentChildren, DependentChild) def callable_year(dt_value): @@ -716,6 +718,28 @@ class ChoiceList(admin.ModelAdmin): fields = ['choice'] +class DependentChildAdminForm(forms.ModelForm): + """ + Issue #20522 + Form to test child dependency on parent object's validation + """ + def clean(self): + parent = self.cleaned_data.get('parent') + if parent.family_name and parent.family_name != self.cleaned_data.get('family_name'): + raise ValidationError("Children must share a family name with their parents " + + "in this contrived test case") + return super(DependentChildAdminForm, self).clean() + + +class DependentChildInline(admin.TabularInline): + model = DependentChild + form = DependentChildAdminForm + + +class ParentWithDependentChildrenAdmin(admin.ModelAdmin): + inlines = [DependentChildInline] + + # Tests for ticket 11277 ---------------------------------- class FormWithoutHiddenField(forms.ModelForm): @@ -872,6 +896,7 @@ site.register(Color2, CustomTemplateFilterColorAdmin) site.register(Simple, AttributeErrorRaisingAdmin) site.register(UserMessenger, MessageTestingAdmin) site.register(Choice, ChoiceList) +site.register(ParentWithDependentChildren, ParentWithDependentChildrenAdmin) site.register(EmptyModelHidden, EmptyModelHiddenAdmin) site.register(EmptyModelVisible, EmptyModelVisibleAdmin) site.register(EmptyModelMixin, EmptyModelMixinAdmin) diff --git a/tests/admin_views/models.py b/tests/admin_views/models.py index e0e2a6fc08..b517530952 100644 --- a/tests/admin_views/models.py +++ b/tests/admin_views/models.py @@ -712,6 +712,26 @@ class Choice(models.Model): choices=((1, 'Yes'), (0, 'No'), (None, 'No opinion'))) +class ParentWithDependentChildren(models.Model): + """ + Issue #20522 + Model where the validation of child foreign-key relationships depends + on validation of the parent + """ + some_required_info = models.PositiveIntegerField() + family_name = models.CharField(max_length=255, blank=False) + + +class DependentChild(models.Model): + """ + Issue #20522 + Model that depends on validation of the parent class for one of its + fields to validate during clean + """ + parent = models.ForeignKey(ParentWithDependentChildren) + family_name = models.CharField(max_length=255) + + class _Manager(models.Manager): def get_queryset(self): return super(_Manager, self).get_queryset().filter(pk__gt=1) diff --git a/tests/admin_views/tests.py b/tests/admin_views/tests.py index 3906224e33..2cc0618cc3 100644 --- a/tests/admin_views/tests.py +++ b/tests/admin_views/tests.py @@ -51,7 +51,8 @@ from .models import (Article, BarAccount, CustomArticle, EmptyModel, FooAccount, AdminOrderedModelMethod, AdminOrderedAdminMethod, AdminOrderedCallable, Report, MainPrepopulated, RelatedPrepopulated, UnorderedObject, Simple, UndeletableObject, UnchangeableObject, Choice, ShortMessage, - Telegram, Pizza, Topping, FilteredManager, City, Restaurant, Worker) + Telegram, Pizza, Topping, FilteredManager, City, Restaurant, Worker, + ParentWithDependentChildren) from .admin import site, site2, CityAdmin @@ -4597,7 +4598,7 @@ class TestLabelVisibility(TestCase): @override_settings(PASSWORD_HASHERS=('django.contrib.auth.hashers.SHA1PasswordHasher',)) -class AdminViewOnSiteTest(TestCase): +class AdminViewOnSiteTests(TestCase): urls = "admin_views.urls" fixtures = ['admin-views-users.xml', 'admin-views-restaurants.xml'] @@ -4607,6 +4608,64 @@ class AdminViewOnSiteTest(TestCase): def tearDown(self): self.client.logout() + def test_add_view_form_and_formsets_run_validation(self): + """ + Issue #20522 + Verifying that if the parent form fails validation, the inlines also + run validation even if validation is contingent on parent form data + """ + # The form validation should fail because 'some_required_info' is + # not included on the parent form, and the family_name of the parent + # does not match that of the child + post_data = {"family_name": "Test1", + "dependentchild_set-TOTAL_FORMS": "1", + "dependentchild_set-INITIAL_FORMS": "0", + "dependentchild_set-MAX_NUM_FORMS": "1", + "dependentchild_set-0-id": "", + "dependentchild_set-0-parent": "", + "dependentchild_set-0-family_name": "Test2"} + response = self.client.post('/test_admin/admin/admin_views/parentwithdependentchildren/add/', + post_data) + + # just verifying the parent form failed validation, as expected -- + # this isn't the regression test + self.assertTrue('some_required_info' in response.context['adminform'].form.errors) + + # actual regression test + for error_set in response.context['inline_admin_formset'].formset.errors: + self.assertEqual(['Children must share a family name with their parents in this contrived test case'], + error_set.get('__all__')) + + def test_change_view_form_and_formsets_run_validation(self): + """ + Issue #20522 + Verifying that if the parent form fails validation, the inlines also + run validation even if validation is contingent on parent form data + """ + pwdc = ParentWithDependentChildren.objects.create(some_required_info=6, + family_name="Test1") + # The form validation should fail because 'some_required_info' is + # not included on the parent form, and the family_name of the parent + # does not match that of the child + post_data = {"family_name": "Test2", + "dependentchild_set-TOTAL_FORMS": "1", + "dependentchild_set-INITIAL_FORMS": "0", + "dependentchild_set-MAX_NUM_FORMS": "1", + "dependentchild_set-0-id": "", + "dependentchild_set-0-parent": str(pwdc.id), + "dependentchild_set-0-family_name": "Test1"} + response = self.client.post('/test_admin/admin/admin_views/parentwithdependentchildren/%d/' + % pwdc.id, post_data) + + # just verifying the parent form failed validation, as expected -- + # this isn't the regression test + self.assertTrue('some_required_info' in response.context['adminform'].form.errors) + + # actual regression test + for error_set in response.context['inline_admin_formset'].formset.errors: + self.assertEqual(['Children must share a family name with their parents in this contrived test case'], + error_set.get('__all__')) + def test_validate(self): "Ensure that the view_on_site value is either a boolean or a callable" CityAdmin.view_on_site = True