Opened 9 years ago
Closed 9 years ago
#26449 closed Cleanup/optimization (fixed)
Merge admin's FORMFIELD_FOR_DBFIELD_DEFAULTS with formfield_overrides
Reported by: | Marysia Lowas-Rzechonek | Owned by: | Iacopo Spalletti |
---|---|---|---|
Component: | contrib.admin | Version: | 1.9 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
When you override the default widget used in Django Admin for models.DateTimeField using formfield_overrides with any widget that derives from SplitDateTimeWidget, the form will not pass the validation with 'Enter a valid date/time.' error message.
It seems that when updating the default formfield_overrides dict, it overrides the default values instead of merging them. At present, it seems to affect only the DateTimeField.
So, this code will trigger the error: 'Enter a valid date/time.'
formfield_overrides = {models.DateTimeField: {'widgets': widgets.AdminSplitDateTime},}
And this code will be fine:
formfield_overrides = {models.DateTimeField: {'form_class': forms.SplitDateTimeField,'widget': widgets.AdminSplitDateTime},}
The problem in django/contrib/admin/option.py around line 117.
Change History (16)
comment:1 by , 9 years ago
Description: | modified (diff) |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 9 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 9 years ago
After a bit of digging, this is actually a legitimate behavior due to this deprecation https://github.com/django/django/blob/stable/1.8.x/django/forms/fields.py#L507 which went into effect in 1.9 and it's documented in 1.9 release notes.
That said the resulting error is kind of misleading, but being AdminSplitDateTime
undocumented there is actually little way to overcome this.
How about adding a documentation on how to use the AdminSplitDateTime
widget in its docstring?
comment:4 by , 9 years ago
After discussing this with Markus Holtermann we can either provide a bit more detailed information in the release notes with a simple sentence how eventually adapt the code, or do nothing at all
comment:5 by , 9 years ago
Type: | Uncategorized → Cleanup/optimization |
---|
comment:6 by , 9 years ago
Has patch: | set |
---|
comment:7 by , 9 years ago
Component: | contrib.admin → Documentation |
---|---|
Has patch: | unset |
Summary: | formfield_overrides doesn't work for models.DateTimeField with SplitDateTimeWidget → Document that SplitDateTimeWidget requires SplitDateTimeField |
On further reflection, I don't think there's any mention in the documentation (I'm looking in docs/ref/forms/widgets.txt
) that SplitDateTimeWidget
must be used with SplitDateTimeField
. A note there seems like a better course of action rather than repeating requirements of the superclass in a subclass's docstring.
comment:8 by , 9 years ago
timgraham should I also leave the note in AdminSplitDateTime, or should I remove it?
comment:10 by , 9 years ago
I would like to clarify one thing as I'm not sure I understand the solution.
Django Admin, by default uses the AdminSplitDateTime
widget for model DateTimeField
with forms.SplitDateTimeField
. However, when I try to change the default widget with formfield_overrides
to another widget derived from SplitDateTimeWidget
, it doesn't work -> validation error.
This is especially striking, when I try to override the default AdminSplitDateTime
widget with the very same AdminSplitDateTime
widget. (That was just for tests, in fact I wanted to use Django Suit suit.widgets.SuitSplitDateTimeWidget
)
From my investigation, it turned out that Django Admin treats model DateTimeField
a bit specially. It defines not only the default widget but also the form_class
.
FORMFIELD_FOR_DBFIELD_DEFAULTS = { models.DateTimeField: { 'form_class': forms.SplitDateTimeField, 'widget': widgets.AdminSplitDateTime }, models.DateField: {'widget': widgets.AdminDateWidget}, models.TimeField: {'widget': widgets.AdminTimeWidget}, #... }
It works fine, but when I want to override it with:
formfield_overrides = { models.DateTimeField: {'widget': suit.widgets.SuitSplitDateTimeWidget}, }
The resulting dictionary looks something like that:
overrides = { models.DateTimeField: { 'widget': suit.widgets.SuitSplitDateTimeWidget}, models.DateField: {'widget': widgets.AdminDateWidget}, models.TimeField: {'widget': widgets.AdminTimeWidget}, #... }
The form_class
is gone as so the forms.DateTimeField
is used instead of forms.SplitDateTimeField
and that causes the validation error.
Is it a desired behaviour?
If yes, maybe it's good to update also the formfield_overrides
documentation, not just the docstring for AdminSplitDateTime
widget?
Or maybe it's better to keep the form_class
value and override just widget
?
BTW, I planned to look into this but I run out of time during Sprints. I'm still quite happy to carry on if needed.
comment:11 by , 9 years ago
I didn't look at the code much, but your description of the issue reminds me of #12915. Could be related.
comment:12 by , 9 years ago
The behavior described in this ticket is due to this deprecation https://github.com/django/django/blob/stable/1.8.x/django/forms/fields.py#L507 , thus it's an intended behavior
The only possible fix here is to document better how to use formfield_overrides
for AdminSplitDateTime
AdminSplitDateTime
per se it's not documented as is considered part of the private API
We can document SplitDateTimeWidget
but this won't solve this ticket anyway
comment:13 by , 9 years ago
@timgraham That ticket you mentioned is about inheriting and this issue is about merging dictionaries.
@yakky It's not so much about AdminSplitDateTime
as about formfield_overrides
for DateTimeField
. I wanted to use formfield_overrides
to override the default widget for DateTimeField
, and I was doing it according to the published documentation (not the inline docs in the code), and I got an error.
Please see the PR I created which should solve this problem PR for #26449
This is my first contribution to Django, so please forgive me if I happened not to follow the procedures or custom at any point. That was unconsciously done.
comment:14 by , 9 years ago
Has patch: | set |
---|
comment:15 by , 9 years ago
Component: | Documentation → contrib.admin |
---|---|
Summary: | Document that SplitDateTimeWidget requires SplitDateTimeField → Merge admin's FORMFIELD_FOR_DBFIELD_DEFAULTS with formfield_overrides |
Triage Stage: | Accepted → Ready for checkin |
Makes sense. For future reference, don't forget to check "Has patch" so the ticket appears in the review queue.
Tested according to the OP description (minor edit made to the description). I'm able to reproduce this on 1.9 and master. 1.8 is not affeced