Code

Opened 5 years ago

Closed 5 years ago

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

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 5 years ago.
git-patch against the latest checkout
10677_2.diff (613 bytes) - added by thejaswi_puthraya 5 years ago.
fixes the broken tests issue

Download all attachments as: .zip

Change History (10)

comment:1 Changed 5 years ago by jacob

  • Description modified (diff)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 5 years ago by thejaswi_puthraya

  • 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.

Changed 5 years ago by thejaswi_puthraya

git-patch against the latest checkout

comment:3 Changed 5 years ago by nate-django@…

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

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 follow-up: Changed 5 years ago by jacob

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

comment:6 in reply to: ↑ 5 Changed 5 years ago by nate-django@…

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.

Changed 5 years ago by thejaswi_puthraya

fixes the broken tests issue

comment:7 Changed 5 years ago by jacob

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

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

comment:8 Changed 3 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 deleted

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.