Opened 11 years ago

Closed 11 years ago

Last modified 10 years ago

#20288 closed Cleanup/optimization (fixed)

admin popup querystring inconsistency

Reported by: Keryn Knight <django@…> Owned by: nobody
Component: contrib.admin Version: dev
Severity: Normal Keywords:
Cc: t.l.krajca@…, bmispelon@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: yes UI/UX: no

Description

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

Attachments (1)

ticket_20288.diff (15.5 KB ) - added by Tomas Krajca 11 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 by Simon Charette, 11 years ago

Easy pickings: set
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

comment:2 by Keryn Knight <django@…>, 11 years ago

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.

by Tomas Krajca, 11 years ago

Attachment: ticket_20288.diff added

comment:3 by Tomas Krajca, 11 years ago

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

comment:4 by Tomas Krajca, 11 years ago

Cc: t.l.krajca@… added
Has patch: set

comment:5 by Tomas Krajca, 11 years ago

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

comment:6 by loic84, 11 years ago

Patch needs improvement: set

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".
  1. {% 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.
  1. 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.

comment:7 by Baptiste Mispelon, 11 years ago

Cc: bmispelon@… added

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

comment:8 by loic84, 11 years ago

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.

Last edited 11 years ago by loic84 (previous) (diff)

comment:9 by loic84, 11 years ago

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.

comment:10 by Keryn Knight <django@…>, 11 years ago

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?

comment:11 by Baptiste Mispelon <bmispelon@…>, 11 years ago

Resolution: fixed
Status: newclosed

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.

comment:12 by Tim Graham <timograham@…>, 10 years ago

In 274048351ae2fc35995645a6dad7c98f74f51eb1:

Removed reading of legacy admin popup GET parameter.

refs #20288.

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