Opened 7 years ago

Last modified 8 months ago

#9373 assigned Bug

"This field is required" error even on empty inlines formsets in the admin model page, when hiding a choice field of a custom form.

Reported by: tyrion.mx@… Owned by: schacki
Component: contrib.admin Version: 1.7
Severity: Normal Keywords: admin, inlines
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I have a custom Form for one of my models (in the example SecondModel) that adds one choice field with an initial value.
If I use that model/form as an inline formset and I exclude the extra field using the "fields" attribute of the ModelAdmin, I got "This Field is required" on all the forms of the formset, including those who where left blank. Here a simple example:

# models.py
from django.db import models

class FirstModel(models.Model):
    a = models.CharField(max_length=10)
    b = models.CharField(max_length=10)

class SecondModel(models.Model):
    link = models.ForeignKey(FirstModel)
    c = models.CharField(max_length=10)

# admin.py
from django.contrib import admin
from bug.bugged import models, forms

class SecondAdmin(admin.TabularInline):
    model = models.SecondModel
    form = forms.SecondForm
    fields = ['c']
    extra = 3

class FirstAdmin(admin.ModelAdmin):
    inlines = [SecondAdmin]
 
admin.site.register(models.FirstModel, FirstAdmin)

# forms.py
from django import forms
from bug.bugged import models

DUMMY_CHOICES = (
    (0, 'a'),
    (1, 'b'),
    (2, 'c')
)

class SecondForm(forms.ModelForm):
    d = forms.IntegerField(label='d', initial=0, widget=forms.Select(choices=DUMMY_CHOICES))
    class Meta:
        model = models.SecondModel

The problem is fields = ['c'], commenting out that line I get no errors.
I guess, this is beacuse of the has_changed check, called by the full_clean method of forms.Form. It happens that changed_data compares the initial "d" value that had been set to 0 with the result of "field.widget.value_from_datadict(self.data,..." that is None because we have not a key for "d" in self.data. So changed_data will contain "d" even if the form has not been changed.

From django/forms/forms.py

                prefixed_name = self.add_prefix(name)
                data_value = field.widget.value_from_datadict(self.data, self.files, prefixed_name)
                if not field.show_hidden_initial:
                    initial_value = self.initial.get(name, field.initial)

Change History (10)

comment:1 Changed 7 years ago by jacob

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 4 years ago by lukeplant

  • Severity set to Normal
  • Type set to Bug

comment:3 Changed 4 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:4 Changed 4 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:5 Changed 19 months ago by Siecje

  • Keywords admin inlines added
  • Version changed from 1.0 to 1.7-alpha-1

comment:6 Changed 16 months ago by schacki

  • Owner changed from nobody to schacki
  • Status changed from new to assigned

comment:7 Changed 16 months ago by anonymous

I could verify this issue for 1.7-beta. However, the root cause is not related to Inlines. So let me set up a new example to explain in more detail:

# models.py
from django.db import models

class MyModel(models.Model):
    a = models.CharField(max_length=10)



# forms.py
from django import forms
from . import models

DUMMY_CHOICES = (
    (0, 'x'),
    (1, 'y'),
    (2, 'z')
)

class MyForm(forms.ModelForm):
    b = forms.IntegerField(label='d', initial=0, widget=forms.Select(choices=DUMMY_CHOICES))
    class Meta:
        model = models.MyModel

# admin.py
from django.contrib import admin
from . import models, forms

class MyAdmin(admin.ModelAdmin):
    model = models.MyModel
    form = forms.MyForm
    fields = ['a']

admin.site.register(models.MyModel, MyAdmin)

1. MyForm
The resulting form MyForm would have the form fields 'a' and 'b'. This behavior is documented and supposed to be like that.

2. MyAdmin Form
MyAdmin has the form attribute. This form contains the fields 'a' and 'b'. At the same time it has the fields attribute, which only refers to 'a'. This seemingly contradictory information is resolved as follows: the AdminModel calls the modelform_factory with form=MyForm and fields=a?. The modelform_ factory works like a normal ModelForm described in 1. I.e. the resulting form contains the the fields 'a' and 'b'

3. MyAdmin rendering
However, for rendering, the ModelAdmin refers either to its field or fieldsets attribute. Thus, the field 'b' will not be rendered, also it is available in the form.

4. MyAdmin Form Validation
For any kind of validation, the generated form with fields 'a' and 'b' is used, also only field 'a' was available in the rendered form. This conflict has generated the issue, that has been described in the ticket.

Bottom line: the behavior is as implemented (possibly as expected by the experts) and there are numerous situations, where it is required, e.g. UserAdmin. Nevertheless, this behavior is not expected and very hard to anticipate, especially by beginners. And debugging is very, very difficult, since it reveals itself only very late in the form handling process.

Possible resolutions:

A. Won't Fix

A "won't fix" is therefore not appropriate.

B. Raise Warning
It could be checked during the ModelAdmin instantiation, if we have less fields for display than we we have in the form and raise a warning. However, this would annoy people, who set it up like that on purpose.

C. Raise Error
Like B, but raise an error; wich backwards incompatible and probably not worth the effort

D. Implement a Check
A check (as part of the 1.7 application check), could identify such situations and raise warnings. I am not sure if and how this would be possible (never done it).

E. Amending fields/fieldsets/exclude
If a check identifies the mismatch between admin fields and formfields, we could:
a) add the missing field(s) to the admin fields list
b) add the missing field(s) to the admin formfields; however this could only be a plug, since it is not clear, where to add it in the fieldsets
c) add the missing field(s) to the admin exclude list, since exclude takes precedence.
All 3 options might be backwards incompatible

F. Amending form generation
If a check identifies the mismatch between admin fields and formfields, we could adapt the provided form such that the critical field(s) is removed from the form. This might be backwards incompatible.

comment:8 Changed 16 months ago by schacki

  • Component changed from Forms to contrib.admin

comment:9 Changed 16 months ago by schacki

I have just reviewed the new check framework https://docs.djangoproject.com/en/1.7/topics/checks/. I think we should add a check in the admin for this situation. The advantage is that this check is only done with DEBUG=False. I.e. log files are not populated in production systems.

I am happy to implement this, if this approach is considered the appropriate solution.

Last edited 16 months ago by schacki (previous) (diff)

comment:10 Changed 8 months ago by timgraham

  • Version changed from 1.7-alpha-1 to 1.7
Note: See TracTickets for help on using tickets.
Back to Top