Fixed #20288 -- Fixed inconsistency in the naming of the popup GET parameter #1285

Closed
wants to merge 1 commit into from

2 participants

@loic
Django member

No description provided.

@bmispelon bmispelon commented on the diff Jun 19, 2013
django/contrib/admin/templatetags/admin_urls.py
@@ -47,7 +46,8 @@ def add_preserved_filters(context, url, popup=False):
merged_qs.update(preserved_filters)
if popup:
- merged_qs['_popup'] = 1
+ from django.contrib.admin.options import IS_POPUP_VAR
@bmispelon
Django member

There's no circular import issue here, is there? The import should go at the top of the file.

@loic
Django member
loic added a note Jun 19, 2013

There is, add_preserved_filters is used in options.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kezabelle kezabelle and 1 other commented on an outdated diff Jun 19, 2013
docs/releases/1.6.txt
@@ -725,6 +725,11 @@ Miscellaneous
returned ``False`` for blank passwords. This has been corrected in this
release: blank passwords are now valid.
+* The changelist view previously accepted a ``pop`` GET parameter to signify it was to

It may be worth clarifying that the changelist view in question is the ModelAdmin one, and link to the modeladmin docs [and perhaps link directly to the changelist method in those docs, if it has a named reference]

@loic
Django member
loic added a note Jun 19, 2013

Good point, making the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kezabelle kezabelle and 1 other commented on an outdated diff Jun 19, 2013
docs/releases/1.6.txt
@@ -725,6 +725,11 @@ Miscellaneous
returned ``False`` for blank passwords. This has been corrected in this
release: blank passwords are now valid.
+* The changelist view previously accepted a ``pop`` GET parameter to signify it was to
+ be displayed in a popup. This parameter has been renamed to ``_popup`` to be consistent
+ with the rest of the admin views. You should update your custom templates if they

Is there any merit to changing "with the rest of the admin views" to something like "with the rest of the admin views, and avoid potential collisions with model field names"? It's evidently not something that's ever come up (as we're > ticket 20,000), so it could go either way as to whether it's worth a mention.

@loic
Django member
loic added a note Jun 19, 2013

The potential collisions would have been an issue if we tried to standardize on "pop", it's not an existing problem, so I don't think we should put it in the release notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bmispelon
Django member

Merged in 7462a78, thanks.

@bmispelon bmispelon closed this Jun 19, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment