Code

Opened 6 years ago

Closed 4 years ago

Last modified 4 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: UI/UX:

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 5 years ago.
redirect_to_with_query_string2.diff (5.8 KB) - added by steingrd 5 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 5 years ago.
Patch that applies cleanly to r11688

Download all attachments as: .zip

Change History (13)

comment:1 Changed 6 years ago by anonymous

  • Cc fcorreia@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 5 years ago by jacob

  • milestone set to 1.1
  • Triage Stage changed from Unreviewed to Accepted

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

Changed 5 years ago by steingrd

comment:3 Changed 5 years ago by steingrd

  • Cc steingrd@… added
  • Has patch set
  • Owner changed from nobody to steingrd
  • Status changed from new to assigned

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 5 years ago by mtredinnick

  • Triage Stage changed from Accepted to Design 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 5 years ago by steingrd

adds a query_string optional keyword arg to redirect_to

comment:5 Changed 5 years ago by steingrd

  • Owner changed from steingrd to nobody
  • Status changed from assigned to new

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 5 years ago by jacob

  • milestone changed from 1.1 to 1.2
  • Triage Stage changed from Design decision needed to Accepted

Changed 5 years ago by steingrd

Patch that applies cleanly to r11688

comment:7 Changed 4 years ago by ericholscher

  • Triage Stage changed from Accepted to Ready for checkin

Patch looks good to me.

comment:8 Changed 4 years ago by ubernostrum

  • milestone 1.2 deleted

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

comment:9 Changed 4 years ago by mtredinnick

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

(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 4 years ago by claudep

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

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.