[Django] #27559: Admin changelist turns QueryDict into dict
Django
-----------------------------------------+------------------------
Reporter: Jonas | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
We implemented a custom SimpleListFilter for the admin which presents the
choices as a list of checkboxes. Pressing "submit" sends the list to the
backend for filtering.
Unfortunately, this doesn't really work well and it took us a while to
find out why: the `ChangeList` view turns `request.GET` (a `QueryDict`)
[https://github.com/django/django/blob/master/django/contrib/admin/views/main.py#L67
into a dict].
This means, a query string in the form of `?q=123&state=1&state=2` simply
loses all state values except the last one. It still works in principle
but as soon as you e.g. click on a column heading to re-sort, the link
only shows the last value instead of all of them.
I checked the git log and traced this re-casting back to the
"[https://github.com/django/django/commit/9dda4abee1225db7a7b195b84c915fdd141a7260
#diff-ea7d8c684e252f3dad6aa458df7d3070R109 NEW ADMIN MERGE]" in 2005. It
was probably done to be able to manipulate the parameters but seems to be
a very lossy operation for this purpose.
Shouldn't `self.params` rather be kept as a `QueryDict`? We'll get started
on a pull request if there's interest.
([https://groups.google.com/forum/#!topic/django-developers/dxEAq8_DSeM
Asked on django-developers])
--
Ticket URL: <https://code.djangoproject.com/ticket/27559>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
--------------------------------------+------------------------------------
Reporter: Jonas von Poser | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* type: Uncategorized => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
Sounds reasonable, if it can be done in a backwards-compatible way.
--
Ticket URL: <https://code.djangoproject.com/ticket/27559#comment:1>
Django
Reporter: Jonas von Poser | Owner: Ling-Xiao
Type: | Yang
Cleanup/optimization | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Ling-Xiao Yang):
* cc: Ling-Xiao Yang (added)
* owner: nobody => Ling-Xiao Yang
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/27559#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Jonas von Poser | Owner: Ling-Xiao
Type: | Yang
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Ling-Xiao Yang):
This may also require a change in
`django.contrib.admin.filters.ListFilter.__init__()`. This init method now
accepts the argument `params` that has to be a `dict` object and is passed
from `ChangeList.get_filter()`, and eventually from `ChangeList.params`.
Even if we replace `ChangeList.params` as a `QueryDict`, it is still
difficult to pass the multiple GET values to the admin filter classes
while having their init `params` as a `dict` object.
It should require a change in the admin filter `__init__()` definition,
unless it would be safe to iterate through all these GET values that have
the same key, pass one value to the filter every time (while not exposing
other values), and use `Q() | Q()` to link them in
`ChangeList.get_filter()` or `ChangeList.get_queryset()`, in order to keep
the filter part unchanged. I think this would be the minimal drawback to
fix this issue right now, as developers are more supposed to access GET
values in filter's argument `request` in documented methods `lookups(self,
request, model_admin)` and `queryset(self, request, queryset)` but not
`__init__()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/27559#comment:3>
Django
Reporter: Jonas von Poser | Owner: (none)
Type: Cleanup/optimization | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Ling-Xiao Yang):
* status: assigned => new
* owner: Ling-Xiao Yang => (none)
Comment:
I didn't find a good way of ensuring the backward compatibility, as this
turned out as a conceptual problem that has existed for a long time:
1. The admin filters are single-valued by definition
([https://github.com/django/django/blob/f0ef0c49e9284f262fbc63e8a497699ca4a248fe/django/contrib/admin/filters.py#L74-L75
example]) and this is semantically shown as `self.value()` in the code
example of
[https://docs.djangoproject.com/en/1.10/ref/contrib/admin/#django.contrib.admin.ModelAdmin.list_filter
related doc].
2. The `dict`-typed GET parameters are further
[https://github.com/django/django/blob/f0ef0c49e9284f262fbc63e8a497699ca4a248fe/django/contrib/admin/views/main.py#L114
passed from `ChangeList` view to filter classes], which would require a
change in both sides.
3. There is a `ChangeList.get_query_string` method that accepts an
additional `dict`-typed parameter (`new_params`) to allow modification of
GET parameters, see
[https://github.com/django/django/blob/f0ef0c49e9284f262fbc63e8a497699ca4a248fe/django/contrib/admin/filters.py#L74-L75
code]. And this method has been used in many places like
[https://github.com/django/django/blob/f0ef0c49e9284f262fbc63e8a497699ca4a248fe/django/contrib/admin/filters.py#L106
1],
[https://github.com/django/django/blob/f0ef0c49e9284f262fbc63e8a497699ca4a248fe/django/contrib/admin/templatetags/admin_list.py#L44
2],
[https://github.com/django/django/blob/f0ef0c49e9284f262fbc63e8a497699ca4a248fe/django/contrib/admin/templatetags/admin_list.py#L175
3] and so on. If we change every `dict` to `QueryDict`, it would be more
of a systematic change.
4. I considered an implicit OR connection of SQL queries in my last
comment, in order to restrict changes in `ChangeList` only, but this may
not be a good idea. First,
[https://tools.ietf.org/html/rfc3986#section-3.4 RFC 3986] has no
definition on the meaning of multi-valued GET parameter. Also, in the
other extension package django-filters, both OR and AND logic are allowed
in its `MultipleChoiceFilter`, see [http://django-
filter.readthedocs.io/en/develop/ref/filters.html#multiplechoicefilter its
doc]. So a multi-valued GET parameter such as `state=1&state=2` does not
necessarily mean an OR relationship in the filtering process.
I'm deassigning myself here. More people are welcomed to look at this
issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/27559#comment:4>
Django
Reporter: Jonas von Poser | Owner: (none)
Cleanup/optimization |
Component: contrib.admin | Version: dev
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* status: new => closed
* resolution: => duplicate
* stage: Accepted => Unreviewed
Comment:
Duplicate of #1873.
--
Ticket URL: <https://code.djangoproject.com/ticket/27559#comment:5>