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.
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
Fixed #21936 -- Allowed DeleteView to work with custom Forms and SuccessMessageMixin. #14634
Conversation
AFAIK this PR was based on more latest suggestions https://github.com/django/django/pull/5992/files you could check |
@auvipy This PR is based on @demestav's work in #13362, which was originally based on @cpsimpson's work in #2585. I've used the test cases from #13362 directly (with minor edits), but changed the implementation in #5992 wasn't even looked at until you mentioned it. Looking now it seems to be based entirely on #2585, but didn't address the review comments, which is OK, time is limited, but presumably why you never pushed it to conclusion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carltongibson Thanks 👍 I left small comments.
def form_valid(self, form): | ||
success_url = self.get_success_url() | ||
self.object.delete() | ||
return HttpResponseRedirect(success_url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it makes sense to add a hook (with better name) that could be called here and in DeletionMixin
:
class DeletionMixin:
...
def _delete(self):
if not getattr(self, 'object', None):
self.object = self.get_object()
success_url = self.get_success_url()
self.object.delete()
return HttpResponseRedirect(success_url)
def delete(self, request, *args, **kwargs):
return self._delete()
class BaseDeleteView(DeletionMixin, FormMixin, BaseDetailView):
...
def form_valid(self, form):
return self._delete()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... I could go either way here. At what point is it easier just to read it in place than go look for it in another class? (There's no definite answer there I think.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, the current form is more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, super. Thanks — I didn't change it 😄
(My overriding feeling is that we have enough hooks already here, and than this one wouldn't ever be used anywhere...)
But 👍 Thanks.
…essMessageMixin. Thanks to Mariusz Felisiak for review. Co-authored-by: Demetris Stavrou <demestav@gmail.com> Co-authored-by: Caroline Simpson <github@hoojiboo.com>
Alternate to #13362, building on @demestav's PR there.
ticket-21936