Opened 8 years ago

Closed 8 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 Iacopo Spalletti)

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 Iacopo Spalletti, 8 years ago

Description: modified (diff)
Triage Stage: UnreviewedAccepted

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

comment:2 by Iacopo Spalletti, 8 years ago

Owner: changed from nobody to Iacopo Spalletti
Status: newassigned

comment:3 by Iacopo Spalletti, 8 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 Iacopo Spalletti, 8 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 Iacopo Spalletti, 8 years ago

Type: UncategorizedCleanup/optimization

comment:6 by Iacopo Spalletti, 8 years ago

Has patch: set

comment:7 by Tim Graham, 8 years ago

Component: contrib.adminDocumentation
Has patch: unset
Summary: formfield_overrides doesn't work for models.DateTimeField with SplitDateTimeWidgetDocument 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 Iacopo Spalletti, 8 years ago

timgraham should I also leave the note in AdminSplitDateTime, or should I remove it?

comment:9 by Tim Graham, 8 years ago

I don't think it's needed.

comment:10 by Marysia Lowas-Rzechonek, 8 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 Tim Graham, 8 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 Iacopo Spalletti, 8 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 Marysia Lowas-Rzechonek, 8 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 Tim Graham, 8 years ago

Has patch: set

comment:15 by Tim Graham, 8 years ago

Component: Documentationcontrib.admin
Summary: Document that SplitDateTimeWidget requires SplitDateTimeFieldMerge admin's FORMFIELD_FOR_DBFIELD_DEFAULTS with formfield_overrides
Triage Stage: AcceptedReady for checkin

Makes sense. For future reference, don't forget to check "Has patch" so the ticket appears in the review queue.

comment:16 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: assignedclosed

In b9290b1d:

Fixed #26449 -- Merged admin's FORMFIELD_FOR_DBFIELD_DEFAULTS with formfield_overrides.

Useful for overriding the DateTimeField widget.

Note: See TracTickets for help on using tickets.
Back to Top