converted the post_comment view to a class based view building on top of the generic FormView #170
Conversation
We tried to strictly follow the logic of the original code and keep as much of it as possible while just migrating the code into the structure of the generic FormView.
Instead of passing the CommentPostBadRequest object we just initialize it within the BadRequest itself.
thomst
changed the title
| from django.views.generic.edit import FormView | ||
| from django.shortcuts import render, resolve_url | ||
|
|
||
| from ..compat import url_has_allowed_host_and_scheme |
There was a problem hiding this comment.
everywhere else we use absolute imports, I think we should use an absolute import here too.
| """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) |
There was a problem hiding this comment.
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())
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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().
| form = self.get_form() | ||
|
|
||
| # Check security information | ||
| if form.security_errors(): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :-)
| except BadRequest as exc: | ||
| return exc.response | ||
| else: | ||
| return self.form_valid(form) |
There was a problem hiding this comment.
Similar comment. FormView is supposed to be doing this for you and you can simply override form_valid/form_invalid methods.
There was a problem hiding this comment.
It might be a question of taste..
|
@claudep it seems Travis CI has migrated from .org to .com for open source repositories and also uses GitHub actions integration (from what I can tell). Can you re-enable this repository on travis-ci.com b/c I don't seem to have permissions to do so. |
tox run well on my changes: |
|
Tests should be runable again now. |
As discussed in issue #82 a classed based view would ease the task of customize the post_comment view. I took the effort and rebuild the logic of the post_comment view as a class based view. I was anxious to not alter the logic of the routine itself. Just migrated it into the scructure of the generic FormView.