Skip to content

converted the post_comment view to a class based view building on top of the generic FormView #170

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

converted the post_comment view to a class based view building on top of the generic FormView #170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6fcedcf
rebuild the comment post view using the generic FormView class
thomst Jul 12, 2021
eb9ca70
made the use of the BadRequest exception more straight foreward
thomst Jul 13, 2021
9ca91bb
use the TemplateResponseMixin logic for rendering a the comment form
thomst Jul 13, 2021
dd5724d
just smooth the code a bit
thomst Jul 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions django_comments/urls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
from django.contrib.contenttypes.views import shortcut
from django.urls import path, re_path

from .views.comments import post_comment, comment_done
from .views.comments import CommentPostView, comment_done
from .views.moderation import (
flag, flag_done, delete, delete_done, approve, approve_done,
)


urlpatterns = [
path('post/', post_comment, name='comments-post-comment'),
path('post/', CommentPostView.as_view(), name='comments-post-comment'),
path('posted/', comment_done, name='comments-comment-done'),
path('flag/<int:comment_id>/', flag, name='comments-flag'),
path('flagged/', flag_done, name='comments-flag-done'),
Expand Down
250 changes: 154 additions & 96 deletions django_comments/views/comments.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from urllib.parse import urlencode

from django import http
from django.apps import apps
from django.conf import settings
Expand All @@ -6,12 +8,16 @@
from django.shortcuts import render
from django.template.loader import render_to_string
from django.utils.html import escape
from django.utils.decorators import method_decorator
from django.views.decorators.csrf import csrf_protect
from django.views.decorators.http import require_POST
from django.views.generic.edit import FormView
from django.shortcuts import render, resolve_url

from ..compat import url_has_allowed_host_and_scheme
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

everywhere else we use absolute imports, I think we should use an absolute import here too.


import django_comments
from django_comments import signals
from django_comments.views.utils import next_redirect, confirmation_view
from django_comments.views.utils import confirmation_view


class CommentPostBadRequest(http.HttpResponseBadRequest):
Expand All @@ -27,105 +33,157 @@ def __init__(self, why):
self.content = render_to_string("comments/400-debug.html", {"why": why})


@csrf_protect
@require_POST
def post_comment(request, next=None, using=None):
class BadRequest(Exception):
"""
Post a comment.

HTTP POST is required. If ``POST['submit'] == "preview"`` or if there are
errors a preview template, ``comments/preview.html``, will be rendered.
Exception raised for a bad post request holding the CommentPostBadRequest
object.
"""
# Fill out some initial data fields from an authenticated user, if present
data = request.POST.copy()
if request.user.is_authenticated:
if not data.get('name', ''):
data["name"] = request.user.get_full_name() or request.user.get_username()
if not data.get('email', ''):
data["email"] = request.user.email

# Look up the object we're trying to comment about
ctype = data.get("content_type")
object_pk = data.get("object_pk")
if ctype is None or object_pk is None:
return CommentPostBadRequest("Missing content_type or object_pk field.")
try:
model = apps.get_model(*ctype.split(".", 1))
target = model._default_manager.using(using).get(pk=object_pk)
except TypeError:
return CommentPostBadRequest(
"Invalid content_type value: %r" % escape(ctype))
except AttributeError:
return CommentPostBadRequest(
"The given content-type %r does not resolve to a valid model." % escape(ctype))
except ObjectDoesNotExist:
return CommentPostBadRequest(
"No object matching content-type %r and object PK %r exists." % (
escape(ctype), escape(object_pk)))
except (ValueError, ValidationError) as e:
return CommentPostBadRequest(
"Attempting to get content-type %r and object PK %r raised %s" % (
escape(ctype), escape(object_pk), e.__class__.__name__))

# Do we want to preview the comment?
preview = "preview" in data

# Construct the comment form
form = django_comments.get_form()(target, data=data)

# Check security information
if form.security_errors():
return CommentPostBadRequest(
"The comment form failed security verification: %s" % escape(str(form.security_errors())))

# If there are errors or if we requested a preview show the comment
if form.errors or preview:
template_list = [
# These first two exist for purely historical reasons.
# Django v1.0 and v1.1 allowed the underscore format for
# preview templates, so we have to preserve that format.
"comments/%s_%s_preview.html" % (model._meta.app_label, model._meta.model_name),
"comments/%s_preview.html" % model._meta.app_label,
# Now the usual directory based template hierarchy.
"comments/%s/%s/preview.html" % (model._meta.app_label, model._meta.model_name),
"comments/%s/preview.html" % model._meta.app_label,
"comments/preview.html",
]
return render(request, template_list, {
"comment": form.data.get("comment", ""),
"form": form,
"next": data.get("next", next),
},
def __init__(self, why):
self.response = CommentPostBadRequest(why)


class CommentPostView(FormView):
http_method_names = ['post']

def get_target_object(self, data):
# Look up the object we're trying to comment about
ctype = data.get("content_type")
object_pk = data.get("object_pk")
if ctype is None or object_pk is None:
raise BadRequest("Missing content_type or object_pk field.")
try:
model = apps.get_model(*ctype.split(".", 1))
return model._default_manager.using(self.kwargs.get('using')).get(pk=object_pk)
except TypeError:
raise BadRequest("Invalid content_type value: %r" % escape(ctype))
except AttributeError:
raise BadRequest("The given content-type %r does not resolve to a valid model." % escape(ctype))
except ObjectDoesNotExist:
raise BadRequest("No object matching content-type %r and object PK %r exists." % (
escape(ctype), escape(object_pk)))
except (ValueError, ValidationError) as e:
raise BadRequest("Attempting to get content-type %r and object PK %r raised %s" % (
escape(ctype), escape(object_pk), e.__class__.__name__))

def get_form_kwargs(self):
data = self.request.POST.copy()
if self.request.user.is_authenticated:
if not data.get('name', ''):
data["name"] = self.request.user.get_full_name() or self.request.user.get_username()
if not data.get('email', ''):
data["email"] = self.request.user.email
return data

def get_form_class(self):
"""Return the form class to use."""
return django_comments.get_form()

def get_form(self, form_class=None):
"""Return an instance of the form to be used in this view."""
if form_class is None:
form_class = self.get_form_class()
return form_class(self.target_object, data=self.data)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this invocation be modeled after the inherited get_form() implementation and move self.target_object and self.data as part of get_form_kwargs ? The parent implementation is

    def get_form(self, form_class=None):
        """Return an instance of the form to be used in this view."""
        if form_class is None:
            form_class = self.get_form_class()
        return form_class(**self.get_form_kwargs())

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we need the data coming from self.get_form_kwargs() and the target-object in various places in the code I decided to assign them to self.data and self.target_object in the post-method.

In my opinion this is the most comprehensible solution.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing is, that the comment form class takes the target_object as positional argument, which cannot be passed by unpacking the dict returned by self.get_form_kwargs().


def get_success_url(self):
"""Return the URL to redirect to after processing a valid form."""
next = self.data.get('next')
fallback = self.kwargs.get('next') or 'comments-comment-done'
get_kwargs = dict(c=self.object._get_pk_val())

if not url_has_allowed_host_and_scheme(url=next, allowed_hosts={self.request.get_host()}):
next = resolve_url(fallback)

if '#' in next:
tmp = next.rsplit('#', 1)
next = tmp[0]
anchor = '#' + tmp[1]
else:
anchor = ''

joiner = ('?' in next) and '&' or '?'
next += joiner + urlencode(get_kwargs) + anchor

return next

def create_comment(self, form):
comment = form.get_comment_object(site_id=get_current_site(self.request).id)
comment.ip_address = self.request.META.get("REMOTE_ADDR", None) or None
if self.request.user.is_authenticated:
comment.user = self.request.user

# Signal that the comment is about to be saved
responses = signals.comment_will_be_posted.send(
sender=comment.__class__,
comment=comment,
request=self.request
)

# Otherwise create the comment
comment = form.get_comment_object(site_id=get_current_site(request).id)
comment.ip_address = request.META.get("REMOTE_ADDR", None) or None
if request.user.is_authenticated:
comment.user = request.user

# Signal that the comment is about to be saved
responses = signals.comment_will_be_posted.send(
sender=comment.__class__,
comment=comment,
request=request
)

for (receiver, response) in responses:
if response is False:
for (receiver, response) in responses:
if response is False:
raise BadRequest("comment_will_be_posted receiver %r killed the comment" % receiver.__name__)

# Save the comment and signal that it was saved
comment.save()
signals.comment_was_posted.send(
sender=comment.__class__,
comment=comment,
request=self.request
)
return comment

def get_template_names(self):
if self.template_name is None:
model = type(self.target_object)
return [
# These first two exist for purely historical reasons.
# Django v1.0 and v1.1 allowed the underscore format for
# preview templates, so we have to preserve that format.
"comments/%s_%s_preview.html" % (model._meta.app_label, model._meta.model_name),
"comments/%s_preview.html" % model._meta.app_label,
# Now the usual directory based template hierarchy.
"comments/%s/%s/preview.html" % (model._meta.app_label, model._meta.model_name),
"comments/%s/preview.html" % model._meta.app_label,
"comments/preview.html",
]
else:
return [self.template_name]

def get_context_data(self, form):
return dict(
form=form,
comment=form.data.get("comment", ""),
next=self.data.get("next", self.kwargs.get('next')),
)

@method_decorator(csrf_protect)
def dispatch(self, *args, **kwargs):
return super().dispatch(*args, **kwargs)

def post(self, request, **kwargs):
self.object = None
self.target_object = None
self.data = self.get_form_kwargs()
try:
self.target_object = self.get_target_object(self.data)
except BadRequest as exc:
return exc.response

form = self.get_form()

# Check security information
if form.security_errors():
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please explain what is your intention here. From what I can see you shouldn't need to override the post method and not call get_form_kwargs() or do form validation manually. All of that is supposed to be handled by FormView under the hood.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I expained above the post method is the first one called. It's the best place to set object attributes that are needed throught out the code, like self.data and self.target_object.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the inherited post method only two cases are differentiated: form-valid and form-invalid. In the logic of the original post_comment view there is a third one: form.security_errors(). Another reason to customize the post-method.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the post method here as the original post_comment method. The original content of the post_comment is split in other methods inherited from the FormView and what is not in other methods remain here in the post method. I think it's a good approach.

The reason why I believe the call to form.security_errors is here, is because the is_valid logic of the CommentForm does not do it. Putting it somewhere else and leaving the class without this post method would leave this new implementation a bit less readable. It could be in the CommentForm, but it would represent more side effects to look after.

What do you think? What do you suggest to do to continue moving forward with this PR? I mean I really like what @thomst did here and it would be great if it landed in the repository.
Thanks for your input :-)

return CommentPostBadRequest(
"comment_will_be_posted receiver %r killed the comment" % receiver.__name__)

# Save the comment and signal that it was saved
comment.save()
signals.comment_was_posted.send(
sender=comment.__class__,
comment=comment,
request=request
)

return next_redirect(request, fallback=next or 'comments-comment-done',
c=comment._get_pk_val())
"The comment form failed security verification: %s" % escape(str(form.security_errors())))

if not form.is_valid() or "preview" in self.data:
return self.form_invalid(form)
else:
try:
self.object = self.create_comment(form)
except BadRequest as exc:
return exc.response
else:
return self.form_valid(form)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar comment. FormView is supposed to be doing this for you and you can simply override form_valid/form_invalid methods.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a question of taste..



comment_done = confirmation_view(
Expand Down