Opened 15 years ago

Closed 15 years ago

Last modified 12 years ago

#10677 closed (fixed)

post_save_moderation breaks confirmation view

Reported by: nate-django@… Owned by: nobody
Component: contrib.comments Version: 1.1-beta
Severity: Keywords: django comments moderation
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Jacob)

I tried out the new moderation tools and I found that when a comment is deleted in the post_save signal handler post_save_moderation(), it breaks the comment confirmation view because the comment instance is gone and _get_pk_val() returns None. This causes the following traceback for the redirected request.

File "/var/lib/python-support/python2.5/django/core/handlers/base.py" in get_response
  86.                 response = callback(request, *callback_args, **callback_kwargs)
File "/var/lib/python-support/python2.5/django/contrib/comments/views/utils.py" in confirmed
  41.                 comment = comments.get_model().objects.get(pk=request.GET['c'])
File "/var/lib/python-support/python2.5/django/db/models/manager.py" in get
  93.         return self.get_query_set().get(*args, **kwargs)
File "/var/lib/python-support/python2.5/django/db/models/query.py" in get
  303.         clone = self.filter(*args, **kwargs)
File "/var/lib/python-support/python2.5/django/db/models/query.py" in filter
  489.         return self._filter_or_exclude(False, *args, **kwargs)
File "/var/lib/python-support/python2.5/django/db/models/query.py" in _filter_or_exclude
  507.             clone.query.add_q(Q(*args, **kwargs))
File "/var/lib/python-support/python2.5/django/db/models/sql/query.py" in add_q
  1258.                             can_reuse=used_aliases)
File "/var/lib/python-support/python2.5/django/db/models/sql/query.py" in add_filter
  1201.         self.where.add((alias, col, field, lookup_type, value), connector)
File "/var/lib/python-support/python2.5/django/db/models/sql/where.py" in add
  48.                 params = field.get_db_prep_lookup(lookup_type, value)
File "/var/lib/python-support/python2.5/django/db/models/fields/__init__.py" in get_db_prep_lookup
  202.             return [self.get_db_prep_value(value)]
File "/var/lib/python-support/python2.5/django/db/models/fields/__init__.py" in get_db_prep_value
  353.         return int(value)

Exception Type: ValueError at /comments/posted/
Exception Value: invalid literal for int() with base 10: 'None'

Shouldn't moderation.py connect to the comment_will_be_posted signal so post_comment() can handle the signal response?

Attachments (2)

10677.diff (3.8 KB ) - added by Thejaswi Puthraya 15 years ago.
git-patch against the latest checkout
10677_2.diff (613 bytes ) - added by Thejaswi Puthraya 15 years ago.
fixes the broken tests issue

Download all attachments as: .zip

Change History (10)

comment:1 by Jacob, 15 years ago

Description: modified (diff)
Triage Stage: UnreviewedAccepted

comment:2 by Thejaswi Puthraya, 15 years ago

Has patch: set
Keywords: django comments moderation added

The patch will delete the comment at the comment_was_posted signal handler and redirect to the comment_done or next page.

by Thejaswi Puthraya, 15 years ago

Attachment: 10677.diff added

git-patch against the latest checkout

comment:3 by nate-django@…, 15 years ago

The patch looks a lot like what I did to fix the issue for my site. My only problem with it is that I think the signal handler should return False so the bad comment never touches the database. But that causes post_comment() to return CommentPostBadRequest which does not have a template when DEBUG == False. So if a comment_will_be_posted signal handler returns False to refuse the comment, the user/client/spammer gets a 400 response and a blank page. I think the 400 is right, but I don't really like the blank page idea.

comment:4 by Thejaswi Puthraya, 15 years ago

Actually, my confusion was also similar. Whether to allow the comment to be
posted and user given no indication of deletion or give the user
a warning of some kind.

But then, I decided to stick to the former (default) method that
comment-utils had been following for so long.

I believe the latter is very much possible because now the
comment_was_posted signal also passes the request as a kwarg.

Let's see what the devs think about this.

comment:5 by Jacob, 15 years ago

If you make this change the comment moderators tests start failing. Someone will need to explain why before this can go in.

in reply to:  5 comment:6 by nate-django@…, 15 years ago

Replying to jacob:

If you make this change the comment moderators tests start failing. Someone will need to explain why before this can go in.

The testcases create comments using the models. They do not go through the post_comment view which is where the comment_will_be_posted and comment_was_posted signals are dispatched from.

by Thejaswi Puthraya, 15 years ago

Attachment: 10677_2.diff added

fixes the broken tests issue

comment:7 by Jacob, 15 years ago

Resolution: fixed
Status: newclosed

I believe this was a duplicate of #11113 and fixed in [10784]. Please reopen if I'm wrong.

comment:8 by Jacob, 12 years ago

milestone: 1.1

Milestone 1.1 deleted

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