Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#33321 closed Bug (wontfix)

Django admin doesn't render "add another / modify" icons next to ForeignKey fields that are declared in the ModelForm

Reported by: James Pic Owned by: nobody
Component: contrib.admin Version: 3.2
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: yes

Description

Django admin renders very convenient "add another" and "modify" buttons next to ForeignKey fields when the admin uses a form like this:

class TForm(forms.ModelForm):
    class Meta:
        model = TModel
        fields = ('name', 'test')
        widgets = {
            'test': MyWidget(),
        }

class TAdmin(admin.ModelAdmin):
    form = TForm

BUT, it won't if my field is declared in the ModelForm as such:

class TForm(forms.ModelForm):
    test = forms.ModelChoiceField(
        widget=MyWidget(),
        queryset=Some.objects.all(),
    )
    class Meta:
        model = TModel
        fields = ('name', 'test')
        

class TAdmin(admin.ModelAdmin):
    form = TForm

This is confusing behavior is not documented AFAIK, and users have been opening issues on github about this, I just debugged it out by chance because it was in my way.

This is because Django admin:

  • Does the formfield.widget = RelatedFieldWidgetWrapper(formfield.widget) decoration on generated fields
  • And then overrides generated fields with the declared fields

As you can see in this trace session:

> /home/jpic/.local/lib/python3.9/site-packages/django/forms/models.py(279)__new__()
-> fields.update(new_class.declared_fields)
(Pdb) l
274  	                message = message % (', '.join(missing_fields),
275  	                                     opts.model.__name__)
276  	                raise FieldError(message)
277  	            # Override default model fields with any custom declared ones
278  	            # (plus, include all the other declared fields).
279  ->	            fields.update(new_class.declared_fields)
280  	        else:
281  	            fields = new_class.declared_fields
282
283  	        new_class.base_fields = fields
284
(Pdb) fields
{'name': <django.forms.fields.CharField object at 0x7f0a251cf0d0>, 'test': <django.forms.models.ModelChoiceField object at 0x7f0a251cf9a0>}
(Pdb) fields.declared_fields
*** AttributeError: 'dict' object has no attribute 'declared_fields'
(Pdb) new_class.declared_fields
{'test': <dal_alight.fields.ModelAlight object at 0x7f0a283b6460>}
(Pdb) new_class.declared_fields['test'].widget
<dal_alight.fields.ModelAlightWidget object at 0x7f0a283b6520>
(Pdb) fields['test'].widget
<django.contrib.admin.widgets.RelatedFieldWidgetWrapper object at 0x7f0a251cfdf0>

I believe that Django should always decorate related field widgets with RelatedFieldWidgetWrapper, unless the widget is explicitely incompatible with it. The meta code for this proposal is: if not getattr(formfield.widget, 'incompatible_with_related_field_widget_wrapper', False): formfield.widget = RelatedFieldWidgetWrapper(formfield.widget)

Change History (2)

comment:1 by Carlton Gibson, 3 years ago

Resolution: wontfix
Status: newclosed

Hi James.

BUT, it won't if my field is declared in the ModelForm...

Yes. So, this is the standard (and expected) behaviour for this kind of generation: if you declare the field yourself, then that's what you get.

The same applies to ModelForms, DRF serialisers, django-filter FilterSets, and I would expect everything else similar: it comes up on DRF as Why was my read_only_field not applied to my explicitly defined serialiser field?

The relevant docs are the extended note in the Overriding the default fields section of the ModelForms topic docs. It says more but:

The fields that are automatically generated depend on the content of the Meta class and on which fields have already been defined declaratively. Basically, ModelForm will only generate fields that are missing from the form, or in other words, fields that weren’t defined declaratively.

To me, it would be deeply surprising if we were to change the behaviour for the admin here: You get the field you declare except in this case is going to be more difficult to reason about in aggregate.

If I wanted to address this in my project, I'd probably look overriding ModelAdmin.get_form(), rather than pulling knowledge of RelatedFieldWidgetWrapper into forms/models.py.

comment:2 by James Pic, 3 years ago

Hello Carlton!

Basically, ModelForm will only generate fields that are missing from the form, or in other words, fields that weren’t defined declaratively.

Actually my report shows it's generating all fields and then overriding them with the declared fields ... Not sure if this distinction is important enough to be reflected in the documentation though.

It's fine for me though, in our apps we don't advertise to declare formfields anymore, but rather to monkey patch Django formfield() method which actually lets Django think it's generating the foreign key fields and the decorated it with RelatedFieldWidgetWrapper, which has also been copy/pasted into an external app so it could be reusable outside the admin, by me.

The most proper solution IMHO is to extract RelatedFieldWidgetWrapper outside from django.contrib.admin and put in in django.forms but that's not going to happen neither. We'd have to at least begin integrating ESM, and change jQuery for custom HTMLElements to deal with element lifecycle, if we want to be able provide native fields with solid JS integration.

Last edited 3 years ago by James Pic (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top