Opened 3 years ago

Last modified 5 months ago

#19303 new Bug

ModelAdmin.formfield_overrides is ignored for fields with choices

Reported by: bendavis78 Owned by: LukaszBalcerzak
Component: contrib.admin Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Let's say we have custom field that extends CharField, which holds comma-separated values. Then, let's say we've made a custom widget that overrides CheckboxMultipleSelect, and takes the field's choices and combines them into comma-separated values, and we want to use that in the admin:

    class MyModel(models.Model):
        foo = models.MyCustomField(choices=(('FOO', 'Foo'),('BAR', 'Bar')))

    class MyAdmin(admin.ModelAdmin):
         formfield_overrides = {
             MyCustomField: MyCustomWidget
         }

In ModelAdmin, the formfield_for_dbfield will normally check formfield_overrides to see if there are any overrides. That is, unless, the field has choices. If the field has choices, formfield_for_choice_field is called, and formfield_overrides is ignored completely.

I know that there are other ways of overriding form fields, but it seems that ModelAdmin.formfield_overrides does not work as advertised.

Change History (15)

comment:1 Changed 3 years ago by bendavis78

  • Has patch set
  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset

comment:2 Changed 3 years ago by aaugustin

  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

I assume you meant:

         formfield_overrides = {
             MyCustomField: {'widget': MyCustomWidget}
         }

The patch isn't acceptable because it breaks a test:

(django-dev)myk@mYk tests % PYTHONPATH=.. ./runtests.py --settings=test_sqlite admin_widgets                                                                                   ~/Documents/dev/django/tests
Creating test database for alias 'default'...
Creating test database for alias 'other'...
..........F....................sss.......sss...
======================================================================
FAIL: testFieldWithChoices (regressiontests.admin_widgets.tests.AdminFormfieldForDBFieldTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/myk/Documents/dev/django/tests/regressiontests/admin_widgets/tests.py", line 116, in testFieldWithChoices
    self.assertFormfield(models.Member, 'gender', forms.Select)
  File "/Users/myk/Documents/dev/django/tests/regressiontests/admin_widgets/tests.py", line 58, in assertFormfield
    (model.__class__.__name__, fieldname, widgetclass, type(widget))
AssertionError: Wrong widget for ModelBase.gender: expected <class 'django.forms.widgets.Select'>, got <class 'django.contrib.admin.widgets.AdminTextInputWidget'>

----------------------------------------------------------------------
Ran 41 tests in 0.430s

FAILED (failures=1, skipped=6)
Destroying test database for alias 'default'...
Destroying test database for alias 'other'...

For consistency between formfield_for_choice_field, formfield_for_foreignkey and formfield_for_manytomany, the fix should move the handling of formfield_overrides either up, at the beginning of formfield_for_dbfield, or down, in a common method that would wrap db_field.formfield and be called by each formfield_to_*. Actually it would have to do both, see below.

The expected order of precendence is:

  • formfield_overrides
  • values set by formfield_for_*
  • FORMFIELD_FOR_DBFIELD_DEFAULTS

This can't be achieved without some refactoring, becauseformfield_overrides is currently merged with FORMFIELD_FOR_DBFIELD_DEFAULTS.

This code dates back to f212b24b6469b66424354bf970f3051df180b88d.

Here's how I would fix the problem:

  • merge formfield_overrides in kwargs at the very beginning of formfield_for_dbfield
  • add a wrapper around that merges FORMFIELD_FOR_DBFIELD_DEFAULTS in kwargs and then calls db_field.formfield
  • write tests.

comment:3 Changed 2 years ago by LukaszBalcerzak

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

comment:4 Changed 2 years ago by LukaszBalcerzak

Well, actually - from reading the code - I guess the expected order of precedence should be:

  • values set by formfied_for_* (**kwargs)
  • formfield_overrides
  • FORMFIELD_FOR_DBFIELD_DEFAULTS

comment:5 Changed 2 years ago by LukaszBalcerzak

  • Version changed from 1.4 to 1.5

comment:6 Changed 2 years ago by LukaszBalcerzak

I have created pull request: https://github.com/django/django/pull/1163

comment:7 Changed 2 years ago by zuber

  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

The code is pretty ugly, but I haven't found any other way to solve the issue without breaking backwards compatibility. I'd merge the code to master right now and think of a better solution later.

Ideally we should introduce another dimension to formfield_overrides dict, and choose the widget for formfield based not only on the formfield class, but also on its attributes (choices attribute in particular). See #20465.

comment:8 Changed 21 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 9d1987d7679165ad3a7c2b713a8a488cc1421905:

Fixed #19303 -- Fixed ModelAdmin.formfield_overrides on fields with choices

comment:9 Changed 15 months ago by anonymous

This bug still occurs in 1.6

comment:10 Changed 15 months ago by claudep

This will be fixed in Django 1.7

comment:11 Changed 12 months ago by carljm

  • Has patch unset
  • Resolution fixed deleted
  • Severity changed from Normal to Release blocker
  • Status changed from closed to new
  • Triage Stage changed from Ready for checkin to Unreviewed
  • Version changed from 1.5 to 1.7-alpha-1

This is backwards-incompatible and breaks existing code that is working as desired.

That might be reasonable if it were a clear-cut bugfix, but it's not; I think the new behavior is less correct (and less consistent with the rest of Django) than the old.

The new behavior perhaps makes sense when formfield_overrides is being used on a single "leaf" ModelAdmin subclass. In that case, it is known in advance which fields have choices set and which do not, so the appropriate widget etc overrides can be provided with that knowledge in mind. (Although formfield_overrides operates by field class, not field name, so even in this scenario it's not really conceptually correct.)

But the new behavior is _really_ incorrect when formfield_overrides is used on a reusable ModelAdmin subclass, because in that case you may have no idea whether a given field class will be used with or without choices.

And it is rare that you actually want the same widget with or without choices (which is why in every other area of the forms library Django automatically switches to a select widget when a field has choices set).

Because of this, precedent elsewhere is to provide separate options for with-choices and without-choices (e.g. formfield methods take both form_class and choices_form_class arguments).

I think the correct fix here would be one of the following:

a) Simply document that formfield_overrides does not impact form fields with choices set; if you want to change things when choices are set, override formfield_for_dbfield (or formfield_for_choice_field).

b) Add a formfield_choice_overrides to mirror formfield_overrides for when choices are set.

c) Add a way to distinguish with-choices from without-choices within formfield_overrides.

I listed those choices in my order of preference, but I am open to suggestions and willing to implement any of them.

Marking release-blocker: I think the backwards-incompatibility means that the current fix here should be rolled back before 1.7 is released.

comment:12 Changed 12 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 5046c110cfbf5e867fec47c8c68677a76c9e1b68:

Revert "Fixed #19303 -- Fixed ModelAdmin.formfield_overrides on fields with choices"

This reverts commit 9d1987d7679165ad3a7c2b713a8a488cc1421905.

comment:13 Changed 12 months ago by Tim Graham <timograham@…>

In f8dd382a48d7dea8cda04a6674f305651e88c654:

[1.7.x] Revert "Fixed #19303 -- Fixed ModelAdmin.formfield_overrides on fields with choices"

This reverts commit 9d1987d7679165ad3a7c2b713a8a488cc1421905.

Backport of 5046c110cf from master

comment:14 Changed 12 months ago by timo

  • Resolution fixed deleted
  • Severity changed from Release blocker to Normal
  • Status changed from closed to new
  • Triage Stage changed from Unreviewed to Accepted

I've reverted the original fix.

comment:15 Changed 5 months ago by timgraham

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