Opened 16 years ago

Last modified 10 months ago

#9739 assigned Bug

Admin does not correctly prefill DataTimeField from URL

Reported by: gilhad Owned by: Fabian Binz
Component: contrib.admin Version: 1.0
Severity: Normal Keywords:
Cc: Sarah Boyce Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

I was not able to format URL for Admin interface to prefill DateTimeField with given value.

It worked in 0.96, but does not work in 1.0 ( I used ../admin/MyApp/MyTable/add/?box=359&datum_date=2008-12-01&datum_time=17:30:27)

After some looking on source code and testing i find a solution:

  • in /django/contrib/admin/options.py before line 520 add
      if isinstance(f, models.DateTimeField):
             initial[k] = initial[k].split(",")
    

For reference - the model of MyTable is such :

class MyTable(models.Model):
        box = models.ForeignKey(Boxes)
        datum =  models.DateTimeField(null=True, blank=True)

(plus some other insignificant fields.
The "datum" field should be prefilled with some date, which is computed by long way (not simple now()) and the use must be able to edit it BEFORE saving it)


The problem arises from DateTimeField be treated by MultiWidget, but not properly broken when got by URL (GET)


Patch:

--- options.py.old      2008-12-01 19:56:34.000000000 +0100
+++ options.py  2008-12-01 19:40:34.000000000 +0100
@@ -517,6 +517,8 @@
                     continue
                 if isinstance(f, models.ManyToManyField):
                     initial[k] = initial[k].split(",")
+                if isinstance(f, models.DateTimeField):
+                    initial[k] = initial[k].split(",")
             form = ModelForm(initial=initial)
             for FormSet in self.get_formsets(request):
                 formset = FormSet(instance=self.model())

Attachments (4)

AdminDateTimePatch.diff (1.2 KB ) - added by gilhad 16 years ago.
9739-r16345.diff (3.4 KB ) - added by Alexander Herrmann 13 years ago.
ticket9737.diff (3.4 KB ) - added by Gandalfar 13 years ago.
patch against r17364
patch9739.diff (3.3 KB ) - added by Ridley Larsen 10 years ago.
patch against a2e56db

Download all attachments as: .zip

Change History (33)

comment:1 by gilhad, 16 years ago

Typo - (Admin does NOT correctly prefill DataTimeField from URL (GET) + patch)

comment:2 by gilhad, 16 years ago

Needs documentation: set

comment:3 by Malcolm Tredinnick, 16 years ago

Has patch: unset
Summary: Admin does correctly prefill DataTimeField from URL (GET) + patchAdmin does not correctly prefill DataTimeField from URL

Edited description for clarity.

Please attach a proper patch to the ticket, rather than mixing the proposed changes into the description of the problem. It makes things a lot easier for reviewing and further improvements (if subsequent patches are made).

by gilhad, 16 years ago

Attachment: AdminDateTimePatch.diff added

comment:4 by gilhad, 16 years ago

Has patch: set
Needs documentation: unset

Added patch for code AND documentation

comment:5 by Jacob, 16 years ago

Triage Stage: UnreviewedAccepted

comment:6 by Chris Beaven, 15 years ago

Needs tests: set

comment:7 by Luke Plant, 14 years ago

Severity: Normal
Type: Bug

comment:8 by Alexander Herrmann, 13 years ago

Easy pickings: unset
Owner: changed from nobody to Alexander Herrmann
Status: newassigned
UI/UX: unset

by Alexander Herrmann, 13 years ago

Attachment: 9739-r16345.diff added

comment:9 by Alexander Herrmann, 13 years ago

Needs tests: unset

attached a new diff for the current source and added test

by Gandalfar, 13 years ago

Attachment: ticket9737.diff added

patch against r17364

comment:10 by Gandalfar, 13 years ago

I've tested that patch works correctly with current trunk and cleaned-up docs syntax.

comment:11 by Aymeric Augustin, 10 years ago

I dislike the idea of blessing hand-crafted URLs as a public API.

comment:12 by Tim Graham, 10 years ago

Patch needs improvement: set

In any case, the patch needs to be updated to apply cleanly.

in reply to:  12 comment:13 by Ridley Larsen, 10 years ago

Replying to timo:

In any case, the patch needs to be updated to apply cleanly.

I have attached a patch that applies cleanly and passes testing.

comment:14 by Ridley Larsen, 10 years ago

Owner: changed from Alexander Herrmann to Ridley Larsen

comment:15 by Claude Paroz, 10 years ago

Ridley, would it be possible to turn the patch into a Github pull request?

comment:16 by Tim Graham, 10 years ago

Patch needs improvement: unset

comment:17 by Markus Holtermann, 10 years ago

It feels to me like using a single white space instead of a comma is more intuitive. 2015-04-05 12:34:56 vs 2015-04-05,12:34:56

comment:18 by Tim Graham, 10 years ago

Patch needs improvement: set

by Ridley Larsen, 10 years ago

Attachment: patch9739.diff added

patch against a2e56db

comment:19 by Ridley Larsen, 10 years ago

I've attached a new patch with a different approach. Instead of special-casing the SplitDateTimeWidget, I modified its' decompress method to account for string values. Thanks Tim for the advice on the isinstance call.

comment:20 by Tim Graham, 10 years ago

Patch needs improvement: unset

comment:21 by Tim Graham, 10 years ago

Patch needs improvement: set

comment:22 by Tim Graham, 10 years ago

#19431 is a duplicate with an alternate approach that might be worth looking at to see if any bits could be incorporated.

comment:23 by Gilhad, 8 years ago

I updated to Djago 1.10 on more sites, the problem still remains (cannot prefill admins datetime field) and the original fix still works (just the lines moved after line 1404 now).

Would be possible to somehow resolve it and incorporate it in main stream, so I would not be forced to manually patch it each and every version? Please, prettty, pretty please ...

comment:24 by Baptiste Mispelon, 5 years ago

Just a quick update to note that this issue still happens in the current master (e8fcdaad5c428878d0a5d6ba820d957013f75595) with the following error message:

'str' object has no attribute 'utcoffset'

However there's been a bit of progress as there's now a documented way to populate a form using the data from request.GET with ModelAdmin.get_changeform_initial_data() [1] (introduced in 1.7).

Interestingly, the current implementation of get_changeform_initial() has a special case to handle ManyToManyField [2]but that special case is neither documented nor tested (removing it doesn't trigger any test failure).
I also find it curious that the current implementation doesn't seem to interact at all with the form layer (it only interacts with the model layer, checking that the keys of request.GET correspond to existing fields). Forms (and widgets) tend to be good at converting plain strings to useful python objects so maybe it could help resolve this ticket without hardcoding a bunch of special cases?

[1] https://docs.djangoproject.com/en/2.2/ref/contrib/admin/#django.contrib.admin.ModelAdmin.get_changeform_initial_data
[2] https://github.com/django/django/blob/e8fcdaad5c428878d0a5d6ba820d957013f75595/django/contrib/admin/options.py#L1495

comment:25 by Mariusz Felisiak, 20 months ago

Owner: Ridley Larsen removed
Status: assignednew

comment:26 by Sarah Boyce, 19 months ago

Cc: Sarah Boyce added

comment:27 by AP Jama, 16 months ago

Owner: set to AP Jama
Status: newassigned

comment:28 by Fabian Binz, 10 months ago

Owner: changed from AP Jama to Fabian Binz

comment:29 by Fabian Binz, 10 months ago

I've looked at this ticket for quite some time during and after the recent sprint at the Cologne Django Meetup.

For now, I've come to the conclusion, that - while there are definitely ways to build the functionality that's requested in the ticket - it's actually quite hard to do that in a manner that's general enough to cover all cases.

What follows is a lengthy discussion of my current understanding of the problem, partly to just have a record of this for myself, but maybe we could also discuss a way forward:

  1. Handling of initial data for multi-value fields
  2. Parsing of localized datetime strings
  3. Deviation from the way HTML forms normally work

The source of the "bug" is actually pretty simple: The SplitDateTimeWidget.decompress method expects its input to be a datetime object, which is not the case when the initial data is passed via a query parameter. According to the docs, this is a reasonable behavior, since the decompress method actually should only receive valid inputs (i.e. a datetime object). So, I guess it's not unreasonable to crash if passed a plain string.

So, why is this contract violated in this case? Because the initial data specified via query parameters is passed verbatim - via a bunch of indirections - to the widget's decompress method, without any form of validation or parsing.

Now, how could we actually parse or validate the input properly? In the case of a datetime field, I think we'd either have to settle on some standard way of formatting datetimes (datetime.isofromat seems reasonable as opposed to a comma- or space-separated representation discussed above) or we could take into account the DATETIME_FORMAT setting that the user has set. However, this doesn't really seem feasible.

Fundamentally, I think the issue is that POST and GET data are not handled in a similar manner for MultiValueFields/MultiWidgets:
Let's say we have a datetime field named "created_at". The SplitDateTimeWidget would render HTML similar to the following:

<input name="created_at_0" ...>
<input name="created_at_1" ...>

Normally, we'd use query parameters like "created_at_0=2024-01-02" and "created_at_1=15:31:23" to pre-fill both the date and the time. For multi-value fields however, we expect that we can set both values using a single query parameter.
When data is POSTed, we don't have that problem, since both inputs are simply separate values and can easily be passed to the MultiValueField.compress method.

For my taste, having this difference between POST and GET does not make much sense and it would seem more consistent to just use the individual input names to pre-fill fields. This also enables partially pre-filling inputs, which is much harder when expecting to have a single representation of a value.

But I also see the convenience of having a single URL parameter to set multiple fields. There are even test cases which kind-of test this behavior (though not for the SplitDateTimeWidget). So maybe an alternative would be to make the serialization/deserialization to/from string a part of the interface of the MultiValueField/MutliWidget classes, so that those methods can be used where needed.

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