Re: [Django] #9739: Admin does not correctly prefill DataTimeField from URL
Django
------------------------------------+-------------------------------
Reporter: gilhad | Owner: dArignac
Type: Bug | Status: assigned
Milestone: | Component: contrib.admin
Version: 1.0 | Severity: Normal
Resolution: | Keywords:
Triage Stage: Accepted | Has patch: 1
Needs documentation: 0 | Needs tests: 1
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------+-------------------------------
Changes (by dArignac):
* owner: nobody => dArignac
* status: new => assigned
* ui_ux: => 0
* easy: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/9739#comment:8>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
------------------------------------+-------------------------------
Reporter: gilhad | Owner: dArignac
Type: Bug | Status: assigned
Milestone: | Component: contrib.admin
Version: 1.0 | Severity: Normal
Resolution: | Keywords:
Triage Stage: Accepted | Has patch: 1
UI/UX: 0 |
------------------------------------+-------------------------------
Changes (by dArignac):
* needs_tests: 1 => 0
Comment:
attached a new diff for the current source and added test
--
Ticket URL: <https://code.djangoproject.com/ticket/9739#comment:9>
Django
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Gandalfar):
I've tested that patch works correctly with current trunk and cleaned-up
docs syntax.
--
Ticket URL: <https://code.djangoproject.com/ticket/9739#comment:10>
Django
-------------------------------+------------------------------------
Reporter: gilhad | Owner: dArignac
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------
Comment (by aaugustin):
I dislike the idea of blessing hand-crafted URLs as a public API.
--
Ticket URL: <https://code.djangoproject.com/ticket/9739#comment:11>
Django
-------------------------------+------------------------------------
Reporter: gilhad | Owner: dArignac
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------
* needs_better_patch: 0 => 1
Comment:
In any case, the patch needs to be updated to apply cleanly.
--
Ticket URL: <https://code.djangoproject.com/ticket/9739#comment:12>
Django
-------------------------------+------------------------------------
Reporter: gilhad | Owner: dArignac
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------
* Attachment "patch9739.diff" added.
patch against a2e56db
--
Ticket URL: <https://code.djangoproject.com/ticket/9739>
Django
-------------------------------+------------------------------------
Reporter: gilhad | Owner: dArignac
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------
Comment (by RidleyLarsen):
Replying to [comment:12 timo]:
> In any case, the patch needs to be updated to apply cleanly.
I have attached a patch that applies cleanly and passes testing.
--
Ticket URL: <https://code.djangoproject.com/ticket/9739#comment:13>
Django
Reporter: gilhad | Owner: RidleyLarsen
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by RidleyLarsen):
* owner: dArignac => RidleyLarsen
--
Ticket URL: <https://code.djangoproject.com/ticket/9739#comment:14>
Django
-------------------------------+----------------------------------------
Reporter: gilhad | Owner: RidleyLarsen
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+----------------------------------------
Comment (by claudep):
Ridley, would it be possible to turn the patch into a Github pull request?
--
Ticket URL: <https://code.djangoproject.com/ticket/9739#comment:15>
Django
-------------------------------+----------------------------------------
Reporter: gilhad | Owner: RidleyLarsen
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+----------------------------------------
* needs_better_patch: 1 => 0
Comment:
[https://github.com/django/django/pull/4433 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/9739#comment:16>
Django
-------------------------------+----------------------------------------
Reporter: gilhad | Owner: RidleyLarsen
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+----------------------------------------
Comment (by MarkusH):
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`
--
Ticket URL: <https://code.djangoproject.com/ticket/9739#comment:17>
Django
-------------------------------+----------------------------------------
Reporter: gilhad | Owner: RidleyLarsen
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+----------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/9739#comment:18>
Django
-------------------------------+----------------------------------------
Reporter: gilhad | Owner: RidleyLarsen
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+----------------------------------------
* Attachment "patch9739.diff" added.
patch against a2e56db
--
Ticket URL: <https://code.djangoproject.com/ticket/9739>
Django
-------------------------------+----------------------------------------
Reporter: gilhad | Owner: RidleyLarsen
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+----------------------------------------
Comment (by RidleyLarsen):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/9739#comment:19>
Django
-------------------------------+----------------------------------------
Reporter: gilhad | Owner: RidleyLarsen
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+----------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/9739#comment:20>
Django
-------------------------------+----------------------------------------
Reporter: gilhad | Owner: RidleyLarsen
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+----------------------------------------
Changes (by timgraham):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/9739#comment:21>
Django
-------------------------------+----------------------------------------
Reporter: gilhad | Owner: RidleyLarsen
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+----------------------------------------
Comment (by timgraham):
#19431 is a duplicate with an alternate approach that might be worth
looking at to see if any bits could be incorporated.
--
Ticket URL: <https://code.djangoproject.com/ticket/9739#comment:22>
Django
Reporter: gilhad | Owner: Ridley Larsen
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Gilhad):
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 ...
--
Ticket URL: <https://code.djangoproject.com/ticket/9739#comment:23>
Django
-------------------------------+-----------------------------------------
Reporter: gilhad | Owner: Ridley Larsen
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+-----------------------------------------
Comment (by Baptiste Mispelon):
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
--
Ticket URL: <https://code.djangoproject.com/ticket/9739#comment:24>
Django
-------------------------------+------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: 1.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------
* owner: Ridley Larsen => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/9739#comment:25>
Django
-------------------------------+------------------------------------
Reporter: gilhad | Owner: (none)
Type: Bug | Status: new
Component: contrib.admin | Version: 1.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------
* cc: Sarah Boyce (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/9739#comment:26>
Django
-------------------------------+------------------------------------
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------
* owner: (none) => AP Jama
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/9739#comment:27>
Django
-------------------------------+------------------------------------
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------
* owner: AP Jama => fbinz
--
Ticket URL: <https://code.djangoproject.com/ticket/9739#comment:28>
Django
Reporter: gilhad | Owner: Fabian Binz
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Fabian Binz):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/9739#comment:29>