[Django] #19303: ModelAdmin.formfield_overrides is ignored for fields with choices
Django
-------------------------------+--------------------
Reporter: bendavis78 | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.4
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
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:
{{{#!python
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/19303>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: bendavis78 | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.4
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by bendavis78):
* needs_better_patch: => 0
* has_patch: 0 => 1
* needs_tests: => 1
* needs_docs: => 0
Comment:
Pull request here: https://github.com/django/django/pull/519
--
Ticket URL: <https://code.djangoproject.com/ticket/19303#comment:1>
Django
Reporter: bendavis78 | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.4
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* needs_better_patch: 0 => 1
* stage: Unreviewed => Accepted
Comment:
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,
because`formfield_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.
--
Ticket URL: <https://code.djangoproject.com/ticket/19303#comment:2>
Django
Reporter: bendavis78 | Owner: LukaszBalcerzak
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.4
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Changes (by LukaszBalcerzak):
* owner: nobody => LukaszBalcerzak
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/19303#comment:3>
Django
Reporter: bendavis78 | Owner: LukaszBalcerzak
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (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`
--
Ticket URL: <https://code.djangoproject.com/ticket/19303#comment:4>
Django
Reporter: bendavis78 | Owner: LukaszBalcerzak
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Changes (by LukaszBalcerzak):
* version: 1.4 => 1.5
--
Ticket URL: <https://code.djangoproject.com/ticket/19303#comment:5>
Django
Reporter: bendavis78 | Owner: LukaszBalcerzak
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.5
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by LukaszBalcerzak):
I have created pull request: https://github.com/django/django/pull/1163
--
Ticket URL: <https://code.djangoproject.com/ticket/19303#comment:6>
Django
Reporter: bendavis78 | Owner:
Type: Bug | LukaszBalcerzak
Component: contrib.admin | Status: assigned
Severity: Normal | Version: 1.5
Keywords: | Resolution:
Has patch: 1 | Triage Stage: Ready for
Needs tests: 0 | checkin
Easy pickings: 0 | Needs documentation: 0
| Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by zuber):
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
* stage: Accepted => Ready for checkin
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/19303#comment:7>
Django
Reporter: bendavis78 | Owner:
Type: Bug | LukaszBalcerzak
Severity: Normal | Version: 1.5
Keywords: | Resolution: fixed
Has patch: 1 | Triage Stage: Ready for
Needs tests: 0 | checkin
Easy pickings: 0 | Needs documentation: 0
| Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"9d1987d7679165ad3a7c2b713a8a488cc1421905"]:
{{{
#!CommitTicketReference repository=""
revision="9d1987d7679165ad3a7c2b713a8a488cc1421905"
Fixed #19303 -- Fixed ModelAdmin.formfield_overrides on fields with
choices
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/19303#comment:8>
Django
Reporter: bendavis78 | Owner:
Type: Bug | LukaszBalcerzak
Component: contrib.admin | Status: closed
Severity: Normal | Version: 1.5
Keywords: | Resolution: fixed
Has patch: 1 | Triage Stage: Ready for
Needs tests: 0 | checkin
Easy pickings: 0 | Needs documentation: 0
| Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by anonymous):
This bug still occurs in 1.6
--
Ticket URL: <https://code.djangoproject.com/ticket/19303#comment:9>
Django
Reporter: bendavis78 | Owner:
Type: Bug | LukaszBalcerzak
Component: contrib.admin | Status: closed
Severity: Normal | Version: 1.5
Keywords: | Resolution: fixed
Has patch: 1 | Triage Stage: Ready for
Needs tests: 0 | checkin
Easy pickings: 0 | Needs documentation: 0
| Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by claudep):
This will be fixed in Django 1.7
--
Ticket URL: <https://code.djangoproject.com/ticket/19303#comment:10>
Django
Reporter: bendavis78 | Owner:
Type: Bug | LukaszBalcerzak
Severity: Release blocker | Version:
Keywords: | 1.7-alpha-1
Has patch: 0 | Resolution:
Needs tests: 0 | Triage Stage:
Easy pickings: 0 | Unreviewed
| Needs documentation: 0
| Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* severity: Normal => Release blocker
* version: 1.5 => 1.7-alpha-1
* has_patch: 1 => 0
* resolution: fixed =>
* stage: Ready for checkin => Unreviewed
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/19303#comment:11>
Django
Reporter: bendavis78 | Owner:
Type: Bug | LukaszBalcerzak
Severity: Release blocker | Version:
Keywords: | 1.7-alpha-1
Needs tests: 0 | Triage Stage:
Easy pickings: 0 | Unreviewed
| Needs documentation: 0
| Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"5046c110cfbf5e867fec47c8c68677a76c9e1b68"]:
{{{
#!CommitTicketReference repository=""
revision="5046c110cfbf5e867fec47c8c68677a76c9e1b68"
Revert "Fixed #19303 -- Fixed ModelAdmin.formfield_overrides on fields
with choices"
This reverts commit 9d1987d7679165ad3a7c2b713a8a488cc1421905.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/19303#comment:12>
Django
Reporter: bendavis78 | Owner:
Type: Bug | LukaszBalcerzak
Component: contrib.admin | Status: closed
Severity: Release blocker | Version:
Keywords: | 1.7-alpha-1
Has patch: 0 | Resolution: fixed
Needs tests: 0 | Triage Stage:
Easy pickings: 0 | Unreviewed
| Needs documentation: 0
| Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"f8dd382a48d7dea8cda04a6674f305651e88c654"]:
{{{
#!CommitTicketReference repository=""
revision="f8dd382a48d7dea8cda04a6674f305651e88c654"
[1.7.x] Revert "Fixed #19303 -- Fixed ModelAdmin.formfield_overrides on
fields with choices"
This reverts commit 9d1987d7679165ad3a7c2b713a8a488cc1421905.
Backport of 5046c110cf from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/19303#comment:13>
Django
Reporter: bendavis78 | Owner: LukaszBalcerzak
Component: contrib.admin | Version: 1.7-alpha-1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* status: closed => new
* resolution: fixed =>
* severity: Release blocker => Normal
* stage: Unreviewed => Accepted
Comment:
I've reverted the original fix.
--
Ticket URL: <https://code.djangoproject.com/ticket/19303#comment:14>
Django
Reporter: bendavis78 | Owner: LukaszBalcerzak
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* version: 1.7-alpha-1 => master
--
Ticket URL: <https://code.djangoproject.com/ticket/19303#comment:15>
Django
Reporter: Ben Davis | Owner: (none)
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* owner: Łukasz Balcerzak => (none)
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/19303#comment:16>
Django
Reporter: Ben Davis | Owner: (none)
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/19303#comment:17>