Opened 2 years ago

Closed 2 years ago

Last modified 16 months ago

#20288 closed Cleanup/optimization (fixed)

admin popup querystring inconsistency

Reported by: Keryn Knight <django@…> Owned by: nobody
Component: contrib.admin Version: master
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 tomask 2 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 2 years ago by charettes

  • Easy pickings set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Cleanup/optimization

comment:2 Changed 2 years ago 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.

Changed 2 years ago by tomask

comment:3 Changed 2 years ago 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

comment:4 Changed 2 years ago by tomask

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

comment:5 Changed 2 years ago by tomask

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

comment:6 Changed 2 years ago by loic84

  • 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 Changed 2 years ago by bmispelon

  • 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 Changed 2 years ago 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.

Version 0, edited 2 years ago by loic84 (next)

comment:9 Changed 2 years ago 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.

comment:10 Changed 2 years ago 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?

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

  • Resolution set to fixed
  • Status changed from new to closed

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 Changed 16 months ago by Tim Graham <timograham@…>

In 274048351ae2fc35995645a6dad7c98f74f51eb1:

Removed reading of legacy admin popup GET parameter.

refs #20288.

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