[Django] #20288: admin popup querystring inconsistency

Django

unread,

Apr 18, 2013, 4:35:21 PM4/18/13

to django-...@googlegroups.com

#20288: admin popup querystring inconsistency
-----------------------------------------+--------------------
Reporter: Keryn Knight <django@…> | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------------+--------------------
* The `ChangeList` objects, as well as `get_actions` both use the variable
`IS_POPUP_VAR` (defined in the same module as `ChangeList`) whose value is
the string `pop`
* The `change_view`, `add_view`, `response_add`, `response_change` instead
use a hard-coded string value `_popup`

This should be refactored because it is fragile and inconsistent, and
means one cannot annotate links with `?popup_variable=1` to get the admin
view without the masthead etc, because using `_popup` on the ChangeList
will force it to redirect to `?e=1` (not a valid filter lookup), and using
`pop` on the other views won't do the desired thing.

--
Ticket URL: <https://code.djangoproject.com/ticket/20288>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

Django

unread,

Apr 18, 2013, 8:57:11 PM4/18/13

to django-...@googlegroups.com

#20288: admin popup querystring inconsistency

-------------------------------------+-------------------------------------
Reporter: Keryn Knight | Owner: nobody
<django@…> | Status: new
Type: | Version: master
Cleanup/optimization | Resolution:
Component: contrib.admin | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Changes (by charettes):

* needs_better_patch: => 0
* needs_tests: => 0
* easy: 0 => 1
* needs_docs: => 0
* type: Uncategorized => Cleanup/optimization
* stage: Unreviewed => Accepted

--
Ticket URL: <https://code.djangoproject.com/ticket/20288#comment:1>

Django

unread,

Apr 19, 2013, 5:17:06 AM4/19/13

to django-...@googlegroups.com

#20288: admin popup querystring inconsistency

Comment (by Keryn Knight <django@…>):

Additionally, the following files would need to be pulled inline to be
consistent:

* updating
`django/contrib/admin/static/admin/js/admin/RelatedObjectLookups.js` -
`showAddAnotherPopup` function.
* updating `django/contrib/admin/templates/admin/change_form.html` -
hidden input name
* updating
`django/contrib/admin/templates/admin/auth/user/change_password.html` -
hidden input name
* updating `django/contrib/admin/templates/admin/change_list.html` -
''object-tools-items'' block (add link).
* updating `django/contrib/admin/templates/admin/search_form.html` - ''N
total'' link.

Note that this would probably also technically be a backwards incompatible
change, because people may be relying on the inconsistency in the Real-
World, though the lack of ticket for it to date indicates little enough
problem.

--
Ticket URL: <https://code.djangoproject.com/ticket/20288#comment:2>

Django

unread,

Apr 27, 2013, 9:18:18 AM4/27/13

to django-...@googlegroups.com

#20288: admin popup querystring inconsistency

Comment (by tomask):

Hi,

I attached a patch for this.

Basically, I changed all occurrences of _popup into IS_POPUP_VAR and all
tests like '_popup in request.REQUEST' into '_popup in request.REQUEST or
IS_POPUP_VAR in request.REQUEST'. So, this should be a backwards
compatible change and a django deprecation policy may be applied. I also
changed the tests that were testing _popup parameter to test both _popup
and IS_POPUP_VAR.

I am only not sure about
django/contrib/admin/static/admin/js/admin/RelatedObjectLookups.js. Since
that is a static js file, I don't know how I can inject content of
IS_POPUP_VAR into it. I changed the _popup parameter to _pop but that is
not really an optimal solution because if the value of IS_POPUP_VAR
changes, this will break. Any ideas?

I successfully ran the tests with '--settings=test_sqlite --selenium'.

My first attempt of a patch :)

Tomas

--
Ticket URL: <https://code.djangoproject.com/ticket/20288#comment:3>

Django

unread,

Apr 27, 2013, 9:19:01 AM4/27/13

to django-...@googlegroups.com

#20288: admin popup querystring inconsistency

Has patch: 1 | UI/UX: 0

Needs tests: 0 |
Easy pickings: 1 |
-------------------------------------+-------------------------------------

Changes (by tomask):

* cc: t.l.krajca@… (added)
* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/20288#comment:4>

Django

unread,

Apr 27, 2013, 9:21:40 AM4/27/13

to django-...@googlegroups.com

#20288: admin popup querystring inconsistency

Comment (by tomask):

I am happy to have a go at creating a pull request if that looks ok.

--
Ticket URL: <https://code.djangoproject.com/ticket/20288#comment:5>

Django

unread,

Jun 18, 2013, 5:29:14 AM6/18/13

to django-...@googlegroups.com

#20288: admin popup querystring inconsistency

Keywords: | Patch needs improvement: 1

Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 1 |
-------------------------------------+-------------------------------------

Changes (by loic84):

* needs_better_patch: 0 => 1

Comment:

This patch is a no-go in my opinion.

1. We can't standardize to just "pop", it works for the changelist, but
nowhere else, `add_view` accepts arbitrary querystring parameters for
prefilling purpose, (i.e. /admin/auth/user/add/?username=hello). "pop"
would clash with any model field out there named "pop".

2. `{% get_popup_var as popup_var %}`. We shouldn't be manipulating the
querystring in the templates, we do it, it's bad, but let's not add
complexity. `?_popup=1` is a hack, let's not make it a bigger hack.

3. `IS_POPUP_VAR` is only used to disable some feature of ChangeList when
it's in a popup (raw ID widget), it would be much more compatible to just
change `IS_POPUP_VAR` to `_popup`, than to rename every occurences of
`_popup` to `IS_POPUP_VAR`.

--
Ticket URL: <https://code.djangoproject.com/ticket/20288#comment:6>

Django

unread,

Jun 18, 2013, 6:31:19 AM6/18/13

to django-...@googlegroups.com

#20288: admin popup querystring inconsistency

Changes (by bmispelon):

* cc: bmispelon@… (added)

Comment:

Hi,

Regarding 1., I agree that the potential for name collision could is a
problem.
Prefixing the variable name with an underscore would alleviate this issue.

Consequently, if we're going to change the value of `IS_POPUP_VAR`, it
makes sense to re-use `_popup` I think (point 3.).

I don't completely agree with point 2, but I still think a custom tag
might be overkill.
How about passing the `IS_POPUP_VAR` to the context of the templates
instead?

The patch also needs to introduce explicit deprecation warnings for the
old `IS_POPUP_VAR`, as per our deprecation policy [1].

Finally, this change should be mentionned in the release notes.

Thanks.

[1] https://docs.djangoproject.com/en/dev/internals/release-process
/#internal-release-deprecation-policy

--
Ticket URL: <https://code.djangoproject.com/ticket/20288#comment:7>

Django

unread,

Jun 18, 2013, 6:52:31 AM6/18/13

to django-...@googlegroups.com

#20288: admin popup querystring inconsistency

Comment (by loic84):

I can live with `IS_POPUP_VAR` or `is_popup_var` which would go well
alongside the currently existing `is_popup` context variable.

Either way, this ticket overlaps with changes required for #6903. I don't
want to delay any further that 5 years old ticket, so I offer to take care
of this ticket as soon #6903 gets in.

--
Ticket URL: <https://code.djangoproject.com/ticket/20288#comment:8>

Django

unread,

Jun 19, 2013, 1:31:58 PM6/19/13

to django-...@googlegroups.com

#20288: admin popup querystring inconsistency

Comment (by loic84):

PR https://github.com/django/django/pull/1285

I didn't add `is_popup_var`, I think it adds noise to those already
humongous {% url %} tag in the admin templates. We would still have to
hardcode that value in the JavaScript anyway.

For the full deprecation cycle I also have my doubts, seems overkill
considering the small reach of this issue now that we only modify the
`ChangeList`.

--
Ticket URL: <https://code.djangoproject.com/ticket/20288#comment:9>

Django

unread,

Jun 19, 2013, 1:55:33 PM6/19/13

to django-...@googlegroups.com

#20288: admin popup querystring inconsistency

Comment (by Keryn Knight <django@…>):

The consensus on only changing the changelist QS value is the same idea I
had, and loic's PR hits all the targets I knew about. I should've done the
work, so thanks to loic for stepping in.

As it's technically an internal API AFAIK, and not documented (again,
AFAIK), I'm not sure there's any requirement beyond the changelog
documentation re: deprecation?

--
Ticket URL: <https://code.djangoproject.com/ticket/20288#comment:10>

Django

unread,

Jun 19, 2013, 4:19:16 PM6/19/13

to django-...@googlegroups.com

#20288: admin popup querystring inconsistency

-------------------------------------+-------------------------------------
Reporter: Keryn Knight | Owner: nobody

<django@…> | Status: closed
Type: | Version: master
Cleanup/optimization | Resolution: fixed

Changes (by Baptiste Mispelon <bmispelon@…>):

* status: new => closed
* resolution: => fixed

Comment:

In [changeset:"7462a78c1bdef2f37ea9aae5ad05170dbd14b34a"]:
{{{
#!CommitTicketReference repository=""
revision="7462a78c1bdef2f37ea9aae5ad05170dbd14b34a"
Fixed #20288 -- Fixed inconsistency in the naming of the popup GET
parameter.

Thanks to Keryn Knight for the initial report and reviews,
and to tomask for the original patch.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/20288#comment:11>

Django

unread,

Mar 21, 2014, 11:39:19 AM3/21/14

to django-...@googlegroups.com

#20288: admin popup querystring inconsistency

Comment (by Tim Graham <timograham@…>):

In [changeset:"274048351ae2fc35995645a6dad7c98f74f51eb1"]:
{{{
#!CommitTicketReference repository=""
revision="274048351ae2fc35995645a6dad7c98f74f51eb1"
Removed reading of legacy admin popup GET parameter.