Provide a hook for compound form/formset validation in ModelAdmin

[mrts]

Sometimes it is necessary to validate inline formset data based on the main form data and vice-versa.

See e.g. ​http://groups.google.com/group/django-users/browse_thread/thread/259c1679ddaaceb8/6ad3698e5451d3fe for a use case.

My use case is somewhat similar: an object can be published only if its associated items meet certain criteria. Thus, to validate the "is published" flag in the form, access to formsets is required.

For advanced users a simple change is sufficient: substitute if all_valid(formsets) and form_validated: with if self.formsets_are_valid(formsets, form, form_validated) and form_validated: in admin/options.py, where def formset_are_valid(self, formsets, ...): return all_valid(formsets). The latter can then be overriden for arbitrary further manipulations. Will upload a patch eventually.

However, this means that people have to mess with either Form or FormSet internals directly (_non_form_errors etc) to expose the errors.

This is yet another proof of a conceptual problem, quoting myself from ​http://groups.google.com/group/django-developers/browse_thread/thread/f9aae709a7fda689/fb16a17ab6a31931 :

"Inline formsets usually represent
composition, i.e. the inline objects are inseparable from the
main entity. The relation is stronger than a ForeignKey (from the
main entity to another entity), yet the latter is displayed as a
field of the form and can be easily manipulated -- but not the inlines."

So, this is yet another reason for introducing FormsetFields in the long run -- Form.clean() would have access to formsets both in the admin and ordinary forms.

[mrts]

See ​http://gist.github.com/296411 for example usage.

[Russell Keith-Magee]

Patch needs tests.

Also - regarding the prototype for formsets_are_valid():

• For consistency with other parts of Django (e.g., inlineformset_factory), I'd say the argument order should be request, new_object, form, formset
• Is there a reason that form_validated is required as an argument? I can understand the "provide everything just in case" approach, but given that if form_validated=False, it doesn't matter what you return from formsets_are_valid, it doesn't make much sense to me to pass it in as an argument. Am I missing something?

[mrts]

Replying to russellm:

Patch needs tests.

Will be done.

• For consistency with other parts of Django (e.g., inlineformset_factory), I'd say the argument order should be request, new_object, form, formset

Agreed, will be done.

• Is there a reason that form_validated is required as an argument? I can understand the "provide everything just in case" approach, but given that if form_validated=False, it doesn't matter what you return from formsets_are_valid, it doesn't make much sense to me to pass it in as an argument. Am I missing something?

One may decide to avoid any formset processing if the form is not valid.

The example at ​http://gist.github.com/296411 is not my use case so I can't comment on the actual requirements. but e.g. it may be useful to check if form_is_valid before validating base language/is approved. Feel free to disagree though.

[mrts]

I'll have to abandon driving this further, sorry.

[Shaun Cutts]

I think there is another use case that can be addressed in this: I wanted to show an additional formset based on objects that are not directly related to a given object (they are "grandchildren"). The inline formset interface wasn't flexible enough for this, so I am displaying them in the {% after_related_objects %} block in the template, adding the additional formset to the extra_context by intercepting change_view. This works fine in this case. However, there is nowhere to trigger validation for the additional formset I have added. An overall ModelAdmin hook that allows extra validation would be great. (To address my use case, I wouldn't have called it "formsets_are_valid", but that's ok... :))

Note: a workaround that I have been able to use is to override ModelAdmin.get_form() and tack the additional validation onto the validation for the base form. However, this is obviously a hack.

Thanks!

[Aymeric Augustin]

Change UI/UX from NULL to False.

[Aymeric Augustin]

Change Easy pickings from NULL to False.

[Ramez Issac]

PR ​https://github.com/django/django/pull/16777

[Natalia Bidart]

In order to properly review the PR, I've been investigating solutions for this ticket, and as I mentioned in ​PR #16777, I think there is currently enough flexibility in overriding the inline's formset so that any custom validation involving both the parent form/instance and the formset themselves can be done there. Following the example referenced in the ticket description use case, in order to block Clip instances to be approved without a matching ClipDescription in the same language, the customization would look similar to this:

# models.py
from django.db import models


LANGS = ['de', 'en', 'es', 'fr', 'it']


class Clip(models.Model):
    is_approved = models.BooleanField(default=False)
    language = models.CharField(choices=[(i, i) for i in LANGS], max_length=2)


class ClipDescription(models.Model):
    clip = models.ForeignKey(Clip, on_delete=models.CASCADE)
    text = models.TextField()
    language = models.CharField(choices=[(i, i) for i in LANGS], max_length=2)

# admin.py
from django import forms
from django.contrib import admin
from django.core.exceptions import ValidationError

from .models import Clip, ClipDescription


class InlineFormset(forms.models.BaseInlineFormSet):
    def clean(self):
        languages = {
            f.cleaned_data['language']
            for f in self.forms
            if 'language' in f.cleaned_data
            # This next line filters out inline objects that did exist
            # but will be deleted if we let this form validate --
            # obviously we don't want to count those if our goal is to
            # enforce a condition of related objects.
            and not f.cleaned_data.get('DELETE', False)
        }
        if (
            self.instance.is_approved
            and self.instance.language not in languages
        ):
            raise ValidationError(
                'A Clip cannot be approved without a description in the same language'
            )


class ClipDescriptionInline(admin.TabularInline):
    model = ClipDescription
    formset = InlineFormset


class ClipAdmin(admin.ModelAdmin):
    inlines = [
        ClipDescriptionInline,
    ]


admin.site.register(Clip, ClipAdmin)

In summary, I'm proposing to close this issue as invalid/wontfix, but I'd live to know what others think first.

[Carlton Gibson]

Hi Natalia -- good example.

The inline has access to the parent object, so that direction at least is covered.

For any remaining cases not covered, I'd be inclined to put them in "the admin is not your app" land — just write a custom view: the admin doesn't have to do everything, no matter how niche.

My suspicion is that a hook here would get very little use. (It's been inactive for 11 years after all.) Nonetheless every reader of the admin site docs would have one more method to read about and understand, before coming to the conclusion it wasn't relevant to them. As such, I'd lean (as often) towards thinking it probably wouldn't pay its way.

Thus I'd agree that wontfix is the right conclusion.
+1

[Mariusz Felisiak]

"wontfix" from me.

[Ramez Issac]

Hello
Thank you Natalia for the example, now I can see the reference way via the instance, didn't know about it .
Now I also agree with the won't fix.

[ldeluigi]

Replying to Natalia Bidart:

In order to properly review the PR, I've been investigating solutions for this ticket, and as I mentioned in ​PR #16777, I think there is currently enough flexibility in overriding the inline's formset so that any custom validation involving both the parent form/instance and the formset themselves can be done there. Following the example referenced in the ticket description use case, in order to block Clip instances to be approved without a matching ClipDescription in the same language, the customization would look similar to this:

# models.py
from django.db import models


LANGS = ['de', 'en', 'es', 'fr', 'it']


class Clip(models.Model):
    is_approved = models.BooleanField(default=False)
    language = models.CharField(choices=[(i, i) for i in LANGS], max_length=2)


class ClipDescription(models.Model):
    clip = models.ForeignKey(Clip, on_delete=models.CASCADE)
    text = models.TextField()
    language = models.CharField(choices=[(i, i) for i in LANGS], max_length=2)

# admin.py
from django import forms
from django.contrib import admin
from django.core.exceptions import ValidationError

from .models import Clip, ClipDescription


class InlineFormset(forms.models.BaseInlineFormSet):
    def clean(self):
        languages = {
            f.cleaned_data['language']
            for f in self.forms
            if 'language' in f.cleaned_data
            # This next line filters out inline objects that did exist
            # but will be deleted if we let this form validate --
            # obviously we don't want to count those if our goal is to
            # enforce a condition of related objects.
            and not f.cleaned_data.get('DELETE', False)
        }
        if (
            self.instance.is_approved
            and self.instance.language not in languages
        ):
            raise ValidationError(
                'A Clip cannot be approved without a description in the same language'
            )


class ClipDescriptionInline(admin.TabularInline):
    model = ClipDescription
    formset = InlineFormset


class ClipAdmin(admin.ModelAdmin):
    inlines = [
        ClipDescriptionInline,
    ]


admin.site.register(Clip, ClipAdmin)

In summary, I'm proposing to close this issue as invalid/wontfix, but I'd live to know what others think first.

What if you need to cross-validate two or more different inline formsets together?

[Natalia Bidart]

Replying to ldeluigi:

What if you need to cross-validate two or more different inline formsets together?

Could you please provide a django test project with an example of your scenario?