Opened 7 years ago

Closed 6 years ago

Last modified 4 years ago

#9268 closed (fixed)

Pass custom form values from post-comment to preview-comment

Reported by: taojian Owned by: stuartk
Component: contrib.comments Version: master
Severity: Keywords: comment preview
Cc: girzel@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

It's possible to add custom fields to the comment post form (to provide a 'next' value, or other custom values), but if the user opts to preview their comment, those custom values are lost. Currently the only context variables being passed to the preview view are the text of the comment and the values from the cleaned form (which don't include custom values). There may be negative implications I'm not aware of, but couldn't we just pass the 'data' variable (containing all the un-cleaned POST data) into the preview?

Attachments (5)

comment_view.diff (484 bytes) - added by stuartk 6 years ago.
Diff against rev 9844
comments.py (4.5 KB) - added by stuartk 6 years ago.
/django/contrib/comments/views/comments.py
comments_doc.diff (15.6 KB) - added by stuartk 6 years ago.
Changes to comments documentation at docs/ref/contrib/comments/index.txt
comments_doc.2.diff (1.3 KB) - added by kmtracey 6 years ago.
Better docs diff
comments.patch (451 bytes) - added by leanmeandonothingmachine 6 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 6 years ago by anonymous

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to anonymous
  • Patch needs improvement unset
  • Status changed from new to assigned

comment:2 Changed 6 years ago by stuartk

  • Owner changed from anonymous to stuartk
  • Status changed from assigned to new

Changed 6 years ago by stuartk

Diff against rev 9844

Changed 6 years ago by stuartk

/django/contrib/comments/views/comments.py

comment:3 Changed 6 years ago by stuartk

In the patch I've passed through the 'data' variable to the preview/error page. From there, you can access 'next' using 'data.next'. In fact you could access any other arbitrary field value passed through from the original comment form.

comment:4 Changed 6 years ago by stuartk

  • Has patch set

Changed 6 years ago by stuartk

Changes to comments documentation at docs/ref/contrib/comments/index.txt

comment:5 Changed 6 years ago by stuartk

Diff for comments documentation added.

Diff isn't that helpful, as it makes it unclear what has changed. No changes to the original text, only an additional section right before the bottom notes section, detailing the use of the hidden 'next' tag.

Changed 6 years ago by kmtracey

Better docs diff

comment:6 Changed 6 years ago by kmtracey

Looks like the svn:eol-style native property is missing for that file (actually possibly the whole doc tree -- I can't find it set anywhere) so line ending differences caused the original diff to look like the whole file was changed. I posted a diff using consistent line endings so just showing what actually new.

comment:7 Changed 6 years ago by taojian

I reported this originally, but since then I've started thinking that a far cleaner and more robust solution is just making your own custom CommentForm. That way custom values stick with postdata and get validated along with everything else. Right now, the only extraneous value that the post_comment view "listens" for is the "next" value: would it make sense to pass "next" along to the preview/errors form redisplay, and assume everything else is the responsibility of a custom form? It just seems so unwieldy to dump the whole POST dictionary back into the view.

Just a thought...

comment:8 Changed 6 years ago by stuartk

@kmtracey, thanks for sorting the diff out. I tried but couldn't get it to do what you did.

comment:9 Changed 6 years ago by stuartk

Perhaps a commiter could decide which is more appropriate to pass through, all post data or single value for next url?

It would be fixed either way, so I'm not fussed.

comment:10 Changed 6 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:11 Changed 6 years ago by jacob

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

comment:12 Changed 6 years ago by jacob

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

(In [10418]) Fixed #9268: pass the "next" param through in the comment preview/post view. Also updated the docs to make this a bit clearer.

comment:13 Changed 6 years ago by jacob

(In [10419]) [1.0.X] Fixed #9268: pass the "next" param through in the comment preview/post view. Also updated the docs to make this a bit clearer. Backport of r10418 from trunk.

comment:14 Changed 6 years ago by saqimtiaz

  • Resolution fixed deleted
  • Status changed from closed to reopened

I'm afraid the next parameter is still not being passed through to the preview template for me. I'm using the latest svn checkout, and have a custom comment form with the next field. Redirects work as they should if not previewing. When I try to preview, the form does not have the necessary field. Further debugging of the preview template shows that next is None in the template and therefore the field is not created. I believe this is because the above fix is incomplete. It does not read the value of next from the POST data before rendering the preview.

If we change "next": next in line 83 of comments.py to "next": form.data.get("next",next), everything works as it should and next is defined in the template: http://code.djangoproject.com/browser/django/branches/releases/1.0.X/django/contrib/comments/views/comments.py?rev=10419#L83

For the time being I am working around this "bug" by getting the value of next in the template via form.data.next

I'm very new to Django so there's a chance I may have missed something in which case I apologize for possibly muddying the waters on this.

comment:15 Changed 6 years ago by russellm

Duplicate report in #11125

Changed 6 years ago by leanmeandonothingmachine

comment:16 Changed 6 years ago by russellm

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

(In [11019]) Fixed #9268 -- Ensured that the next argument is passed on when previewing comments. Thanks to leanmeandonothingmachine for the patch.

comment:17 Changed 6 years ago by russellm

(In [11020]) [1.0.X] Fixed #9268 -- Ensured that the next argument is passed on when previewing comments. Thanks to leanmeandonothingmachine for the patch.

Merge of r11019 from trunk.

comment:19 Changed 6 years ago by mfeet

  • Needs documentation set

I have recently come across this aswell, and again using {{ data.next }} does not work.

Instead the code is making a variable {{ next }} instead available, which is wrong in the docs. Is this what is wanted? I thought the way it was meant to be accessed was through {{ data.next }}, but this currently is not the case.

comment:20 Changed 4 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 deleted

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