Opened 14 years ago

Closed 13 years ago

Last modified 13 years ago

#9966 closed (fixed)

Generic view "redirect_to" to carry existing query string

Reported by: FilipeCorreia Owned by: nobody
Component: Generic views Version: 1.0
Severity: Keywords:
Cc: fcorreia@…, steingrd@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The "redirect_to" generic view currently does not pass along any query string that may exist in the original request.

An extra parameter could be added to "redirect_to", specifying if the original query string should be appended or not. See the following thread:
http://groups.google.com/group/django-users/browse_frm/thread/4566f4040cef518a/8bfcd6ddd4ebb9f5

Attachments (3)

redirect_to_with_query_string.diff (6.5 KB) - added by steingrd 14 years ago.
redirect_to_with_query_string2.diff (5.8 KB) - added by steingrd 14 years ago.
adds a query_string optional keyword arg to redirect_to
redirect_to_with_query_string_r11688.diff (6.0 KB) - added by steingrd 14 years ago.
Patch that applies cleanly to r11688

Download all attachments as: .zip

Change History (13)

comment:1 Changed 14 years ago by anonymous

Cc: fcorreia@… added

comment:2 Changed 14 years ago by Jacob

milestone: 1.1
Triage Stage: UnreviewedAccepted

I'd expect the query string to *always* be passed through, actually; no need for an option.

Changed 14 years ago by steingrd

comment:3 Changed 14 years ago by steingrd

Cc: steingrd@… added
Has patch: set
Owner: changed from nobody to steingrd
Status: newassigned

I added a patch that passes through query strings on redirect in django.views.generic.simple.redirect_to.

I've also written a test case for the redirect_to view, unfortunately it shows up as a file named /dev/null in my patch, the filename is tests/regressiontests/views/tests/generic/simple.py. The feature broke a few tests in regressiontests/test_client_regress/models.py, these have been updated as well.

I haven't updated the documentation as English is not my first language.

comment:4 Changed 14 years ago by Malcolm Tredinnick

Triage Stage: AcceptedDesign decision needed

Have to say, I disagree with Jacob's comment:2. Inthe general case, it's not going to be correct to pass through the query string. That string is specific to the particular resource requested, not all resources that the site serves and redirect is redirecting to a different resource. It's appropriate to pass on the query string when, say, redirecting to a canonical URL form (e.g. appending a slash to the URL), but, in general, no, it's not.

I'd be -1 on making this behave this way always. As an option, though, it's absolutely a good idea.

Changed 14 years ago by steingrd

adds a query_string optional keyword arg to redirect_to

comment:5 Changed 14 years ago by steingrd

Owner: changed from steingrd to nobody
Status: assignednew

I actually agree with Malcolm, the query string should only follow when configured to so. I have created a new patch that adds a query_string keyword argument to the redirect_to function.

I've added the option to the generic views reference documentation and updated all tests.

comment:6 Changed 14 years ago by Jacob

milestone: 1.11.2
Triage Stage: Design decision neededAccepted

Changed 14 years ago by steingrd

Patch that applies cleanly to r11688

comment:7 Changed 13 years ago by Eric Holscher

Triage Stage: AcceptedReady for checkin

Patch looks good to me.

comment:8 Changed 13 years ago by James Bennett

milestone: 1.2

1.2 is feature-frozen, moving this feature request off the milestone.

comment:9 Changed 13 years ago by Malcolm Tredinnick

Resolution: fixed
Status: newclosed

(In [13746]) Add option to redirect_to view to allow passing along the query string
from the original request. Default is current behaviour, which is not to
pass the query string (it often won't be appropriate to do so).

Thanks to steingrd@… for the patch and tests. Fixed #9966.

comment:10 Changed 13 years ago by Claude Paroz

Shouldn't the doc mention that this was added in dev version?

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