Code

Opened 15 months ago

Closed 13 months ago

Last modified 4 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 15 months ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 15 months 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 15 months 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 15 months ago by tomask

comment:3 Changed 15 months 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 15 months ago by tomask

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

comment:5 Changed 15 months ago by tomask

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

comment:6 Changed 13 months 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 13 months 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 13 months 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 13 months ago by loic84 (next)

comment:9 Changed 13 months 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 13 months 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 13 months 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 4 months ago by Tim Graham <timograham@…>

In 274048351ae2fc35995645a6dad7c98f74f51eb1:

Removed reading of legacy admin popup GET parameter.

refs #20288.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.