Is this a good class based view?

Using Django Forms & APIs
1

Recently I found out about class based views and started implementing them in my social media Django app.
I was wondering if this view that I have written is a good way to write views and if not what can i improve on this.

# Read post Class Based View
class ReadPostView(DetailView, FormMixin):
    model = Post
    template_name = 'posts/read_post.html'
    context_object_name = 'post'
    # the id that the class will look for in the url
    pk_url_kwarg = 'post_id'

    form_class = CommentForm
    success_url = 'home'

    def get_context_data(self, **kwargs):
        context = super().get_context_data(**kwargs)
        post = self.object
        comments = Comment.objects.filter(post=post).order_by('-comment_at')
        context['comments'] = comments
        # Because we need to pass the form in the get request too
        context['form'] = self.get_form()            
        return context
    
    @method_decorator(login_required)
    def post(self, request, *args, **kwargs):
        self.object = self.get_object()
        form = self.get_form()
        if form.is_valid():
            comment = form.save(commit=False)
            comment.post = self.object
            comment.comment_author = request.user
            comment.save()
            messages.success(request, "Comment Added!")
            return redirect(self.get_success_url())

    def get_success_url(self) -> str:
        return reverse('read-post', kwargs={'post_id': self.object.id})

Also I request you guys to keep the english simple as it is not my first language, Thankyou!

2

Does it perform as intended? If so, the rest can improve over time

Have you considered subclassing LoginRequiredMixin and providing a login_url rather than using a decorator?

Example:

from django.contrib.auth.mixins import LoginRequiredMixin

class ReadPostView(LoginRequiredMixin, FormMixin, DetailView):
    login_url = "/user/login/"

Why was it necessary to override get_success_url() completely? Alternatively, set self.success_url then call super().get_success_url()

2 Likes
3

Welcome @Rachit74 !

In addition to the comments above, any mixin classes should be listed before the base class in the class definition.

Second, for any class showing additional detail with a form, I always recommend that you use the FormView as the base class. The DetailView doesn’t really add any functionality in this situation.
You can then add any additional detail to be displayed with that form as part of your get_context_data method. That way, you don’t need to do anything special with your form handling unless it’s required.

1 Like
4

In short,

FormView can effectively replace FormMixin and DetailView in @Rachit74’s case.
Very cool answer.

5

you can use prefetch queryset instead override get_context_data method.

and you are not need success_url, because you override get_success_url method.

form django.db.models import Prefetch
class ReadPostView(DetailView, FormMixin):
    # model = Post
    queryset = Post.objects.all().prefetch_related(
        Prefetch('{comment reverse related name}', Comment.objects.filter(post=post).order_by('-comment_at'))
    )
6

Hey @onyeibo thanks for the LoginRequiredMixin tip didn’t knew about that, but in my case I want the user to be logged in if they are going to comment, so I think that @method_decorator(login_required) makes more sense, because applying the mixin makes the complete read post function view to require a login.

7

I’m not sure that really make sense from a UI/UX perspective, because someone who isn’t logged in will still see the form. So they might get the impression that they can comment - but only get an error or redirected after typing in their comment and submitting.

8

Hey @KenWhitesell I was just experimenting with your recommendation of FormView and using it does make more sense here is an snippet

class ReadPostView(FormView):
    template_name = 'posts/read_post.html'
    form_class = CommentForm

    def get_context_data(self, **kwargs: Any) -> dict[str, Any]:
        context = super().get_context_data(**kwargs)
        post_id = self.kwargs['post_id']
        context['post'] = get_object_or_404(Post, id=post_id)
        return context
    
    def form_valid(self, form: Any) -> HttpResponse:
        post = self.get_context_data()['post']
        comment = form.save(commit=False)
        comment.comment_author = self.request.user
        comment.post = post
        comment.save()

Also regarding about the form UI/UX I actually use a logic in my template that for a user that is not logged in the template will show login to comment

{% if user.is_authenticated %}
<a href="#" class="show-link" onclick="toggleBox(event, 'comment-box')">Add a Comment</a>
{% else %}
<a href="{% url 'login' %}" class="hi">Login</a> to comment
{% endif %}

Hopefully this makes sense of why i can’t use LoginMixin on the class :slight_smile:

9

Did you truncate your post before the end of the form_valid function? If not, your form_valid method here is not returning an HttpResponse. You typically will return an HttpResponseRedirect after a successful submission.

Yes, this makes sense.

10

Yes I am redirecting after the form has been submitted.

also i am doing this to prevent any unauthenticated user form commenting

 def form_valid(self, form: Any):
        if not self.request.user.is_authenticated:
            return redirect('login')
1 Like