admin popup querystring inconsistency

[Keryn Knight <django@…>]

• 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.

[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.

[Tomas Krajca]

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

[Tomas Krajca]

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

[loic84]

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.

[Baptiste Mispelon]

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

[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 as #6903 gets in.

[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.

[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?

[Baptiste Mispelon <bmispelon@…>]

In 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.

[Tim Graham <timograham@…>]

In 274048351ae2fc35995645a6dad7c98f74f51eb1:

Removed reading of legacy admin popup GET parameter.

refs #20288.