Opened 8 years ago

Closed 4 years ago

#8968 closed New feature (wontfix)

No way to utilize `next` parameter to redirect after comment deletion

Reported by: Dmitry Dzhus Owned by: Kevin Kubasik
Component: contrib.comments Version: 1.0
Severity: Normal Keywords: comments
Cc: kevin@…, waylan@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

How is default «delete comment» view from new comments system meant to
get next argument (see contrib/comments/views/moderation.py)?

I think good way to do it is to support next GET request parameter, so
that one could use /delete/123/?next=/blog/entry/bla-blah as a link to
comment deletion page in order to redirect to page other than default
«deleted» view after actually removing the comment.

This works with «post comment» because next_redirect in post_comment function can see the next POST parameter in data. This does not work with «delete comment» because confirmation page is rendered first with next="None" in confirmation form.

Attachments (10)

add-redirections-via-next-get.diff (1.1 KB) - added by Dmitry Dzhus 8 years ago.
A simple patch to enable using next GET parameter with «delete comment» view
add-redirections-via-next-get.2.diff (999 bytes) - added by Dmitry Dzhus 8 years ago.
A simple patch to enable using next GET parameter with «delete comment» view
fix-next-redirect.diff (2.5 KB) - added by neithere 8 years ago.
This patch enables correct redirecting from moderation views by passing a next parameter via GET
comments-moderation-next-param.diff (1.2 KB) - added by Dmitri Fedortchenko <d@…> 7 years ago.
I still believe this is a shortcoming…
comments-moderation-next-param.2.diff (1.1 KB) - added by Dmitri Fedortchenko <d@…> 7 years ago.
I still believe this is a shortcoming…
fix_8968.patch (4.8 KB) - added by Kevin Kubasik 7 years ago.
fix_8968_v2.patch (5.6 KB) - added by Kevin Kubasik 7 years ago.
Updated Version of Kevins Patch. Still Needs Docs.
fix_8968_v3.patch (5.3 KB) - added by Kevin Kubasik 7 years ago.
fix_8968_v4.patch (10.7 KB) - added by Kevin Kubasik 7 years ago.
Docs, Tests and Code.
fix_8968_v5.patch (6.7 KB) - added by Kevin Kubasik 7 years ago.

Download all attachments as: .zip

Change History (36)

Changed 8 years ago by Dmitry Dzhus

A simple patch to enable using next GET parameter with «delete comment» view

Changed 8 years ago by Dmitry Dzhus

A simple patch to enable using next GET parameter with «delete comment» view

comment:1 Changed 8 years ago by Dmitry Dzhus

Has patch: set
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:2 Changed 8 years ago by anonymous

Component: Contrib appsdjango.contrib.comments

comment:3 Changed 8 years ago by neithere

The little change introduced in the existing patch should apply to all django.contrib.comments.moderation methods that expect the next argument (which simply can't be in the path as it's a path itself). These methods are approve(), delete() and flag().

I think that next=None should be removed from method signatures, too. In this case the next variable would be undefined in the POST handling code, so we should either get its value from POST or simply pass next=None to next_redirect() because the latter can get the same data itself.

Or did I miss something?

(By the way, a non-existent comment.permalink is used instead of the actual comment.get_absolute_url in source:django/trunk/django/contrib/comments/templates/comments/delete.html and friends. That's a bit off-topic though.)

Changed 8 years ago by neithere

Attachment: fix-next-redirect.diff added

This patch enables correct redirecting from moderation views by passing a next parameter via GET

comment:4 Changed 8 years ago by arne

If a user decides to preview a comment before posting it, the next parameter is also lost. #9268 describes the problem, but wants to solve more than just passing the next parameter. Maybe this patch could be modified to also pass the next parameter for the preview view.

naive approach:

--- contrib/comments/views/comments.py
+++ contrib/comments/views/comments.py
@@ -80,6 +80,7 @@
template_list, {
"comment" : form.data.get("comment", ""),
"form" : form,
+ "next": data.get("next", None)
},
RequestContext(request, {})
)

comment:5 Changed 8 years ago by Jacob

Triage Stage: UnreviewedAccepted

comment:6 Changed 8 years ago by Jacob

milestone: 1.1

comment:7 Changed 8 years ago by Kevin Kubasik

Owner: changed from nobody to Kevin Kubasik

I got this foo.

comment:8 Changed 8 years ago by Kevin Kubasik

Cc: kevin@… added

comment:9 Changed 7 years ago by Jacob

Resolution: wontfix
Status: newclosed

There isn't a bug here. The next param is meant for people who want to wrap one of these views either with a custom URLconf like

    (r'^flag/(\d+)/$', flag, {'next': '/I/am/done/'})

or with a function that wraps the view. If it's not given, the next param will be pulled out of GET or POST (depending), so you can use an <input type=hidden name=next> in your HTML.

Changed 7 years ago by Dmitri Fedortchenko <d@…>

I still believe this is a shortcoming...

Changed 7 years ago by Dmitri Fedortchenko <d@…>

I still believe this is a shortcoming...

comment:10 Changed 7 years ago by Dmitri Fedortchenko <d@…>

It's all good if you happen to stick a next param in a post form, but what if you want to post a link using javascript and include a next parameter as a get request? Then you're out of luck. It seems rather annoying to have to create a custom URLConf or application just to be able to redirect to a page of your choice...

I've tested it, and looked through the code, and the simple fact is that the next parameter is just not included unless you create your own POST form with the next hidden field.

If you try to use a standard get link with next in the query string, it will be lost, as well as if you use a "post link" with a little help of javascript.

I think the next parameter should always be fished out from both GET or POST. The patch I submitted does this, it also keeps the view argument as the dominant next param, and if there is a next in POST, it will be prioritized.

comment:11 Changed 7 years ago by Dmitri Fedortchenko <d@…>

Resolution: wontfix
Status: closedreopened

PS: I can't really see where next_redirect picks up the "next" parameter from GET, if I missed it, please point me in the right direction so I can hide in a corner.

comment:12 Changed 7 years ago by Kevin Kubasik

Ok, I think I have enough tests proving this in the cases where it seems sane to me, but I understand that realistically I probably missed a few. This branch has the solution in it:
http://github.com/kkubasik/django/tree/comment_redir

I have attached a patch as well.

Changed 7 years ago by Kevin Kubasik

Attachment: fix_8968.patch added

comment:13 Changed 7 years ago by George Song

Needs documentation: set
Needs tests: set
Patch needs improvement: set

The latest patch breaks the use case that Jacob cited above. I think we need to decided on the order of precedence here, as there are multiple places where next could come from: view arg, GET param, POST param. The following feels right to me:

  • POST: POST param > GET param > view arg
  • GET: GET param > view arg

If this is the case, we need to test and document accordingly.

comment:14 Changed 7 years ago by Kevin Kubasik

Sounds good to me, if we want to utilize this inheritance order, then I'll implement it this week!

comment:15 Changed 7 years ago by Adrian Holovaty

milestone: 1.11.2

comment:16 Changed 7 years ago by Kevin Kubasik

For the documentation needed, should it be in the main docs? Or just docstrings in the code?

comment:17 Changed 7 years ago by Alex Gaynor

Docstrings are implementation. This is a public facing API which means real docs.

Changed 7 years ago by Kevin Kubasik

Attachment: fix_8968_v2.patch added

Updated Version of Kevins Patch. Still Needs Docs.

Changed 7 years ago by Kevin Kubasik

Attachment: fix_8968_v3.patch added

Changed 7 years ago by Kevin Kubasik

Attachment: fix_8968_v4.patch added

Docs, Tests and Code.

Changed 7 years ago by Kevin Kubasik

Attachment: fix_8968_v5.patch added

comment:18 Changed 7 years ago by Kevin Kubasik

I should note that I chose to explicitly check GET then POST for readability over request.REQUEST. If there is a strong feeling towards changing this to REQUEST, that's not a problem, but I wanted to make sure it was known that the POST preference was explicit.

comment:19 Changed 7 years ago by Waylan Limberg

Cc: waylan@… added

comment:20 Changed 7 years ago by James Bennett

milestone: 1.2

This isn't critical enough to get developer time before the 1.2 release; with luck we'll hit it in a 1.2.x bugfix release, or if not then in 1.3.

comment:21 Changed 5 years ago by Luke Plant

Severity: Normal
Type: New feature

comment:22 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:23 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:24 Changed 4 years ago by Aymeric Augustin

Status: reopenednew

comment:25 Changed 4 years ago by Aymeric Augustin

django.contrib.comments has been deprecated and is no longer supported, so I'm closing this ticket. We're encouraging users to transition to a custom solution, or to a hosted solution like Disqus.

The code itself has moved to https://github.com/django/django-contrib-comments; if you want to keep using it, you could move this bug over there.

comment:26 Changed 4 years ago by Aymeric Augustin

Resolution: wontfix
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top