#20288 closed Cleanup/optimization (fixed)
admin popup querystring inconsistency
Reported by: | 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 asget_actions
both use the variableIS_POPUP_VAR
(defined in the same module asChangeList
) whose value is the stringpop
- 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)
Change History (13)
comment:1 by , 12 years ago
Easy pickings: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Cleanup/optimization |
comment:2 by , 12 years ago
by , 12 years ago
Attachment: | ticket_20288.diff added |
---|
comment:3 by , 12 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 , 12 years ago
Cc: | added |
---|---|
Has patch: | set |
comment:6 by , 11 years ago
Patch needs improvement: | set |
---|
This patch is a no-go in my opinion.
- 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".
{% 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.
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 changeIS_POPUP_VAR
to_popup
, than to rename every occurences of_popup
toIS_POPUP_VAR
.
comment:7 by , 11 years ago
Cc: | 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 , 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.
comment:9 by , 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 , 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 , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Additionally, the following files would need to be pulled inline to be consistent:
django/contrib/admin/static/admin/js/admin/RelatedObjectLookups.js
-showAddAnotherPopup
function.django/contrib/admin/templates/admin/change_form.html
- hidden input namedjango/contrib/admin/templates/admin/auth/user/change_password.html
- hidden input namedjango/contrib/admin/templates/admin/change_list.html
- object-tools-items block (add link).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.