Skip to content

Loading…

Fixed #21936 -- Refactored DeleteView to be compatible with SuccessMessa... #2585

Closed
wants to merge 1 commit into from

5 participants

@cpsimpson

Refactored DeleteView to use the form related base classes and mixins, like the
other class-based generic editing views. Added tests for the handling the
DELETE http method and ensuring the SuccessMessageMixin works. Updated DeleteView
documentation to reflect the new base classes.
The majority of the code changes were provided by Simon Charette.

@charettes charettes commented on an outdated diff
tests/generic_views/test_forms.py
@@ -17,3 +17,10 @@ class Meta:
class ContactForm(forms.Form):
name = forms.CharField()
message = forms.CharField(widget=forms.Textarea)
+
+
+class DeleteForm(forms.ModelForm):
+
+ class Meta:
+ model = Author
+ fields = []
@charettes Django member

Do you think it would make sense to test a real use case form intead?

class ConfirmDeleteForm(forms.Form):
    confirm = models.BooleanField()

And update the tests accordingly to delete/post confirm=1.

As I was updating the tests, I found that data cannot currently be sent with the DELETE method. When doing further research, I wasn't sure whether this should be allowed or not. The test client accepts a data parameter for DELETE, but the HTTP spec suggests that you shouldn't expect data, like you can for a POST: http://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9.7 If we want to I can figure out how to actually get data through the chain. Otherwise I can update the documentation to reflect the changes instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on the diff
docs/ref/class-based-views/generic-editing.txt
@@ -198,9 +201,9 @@ DeleteView
.. class:: django.views.generic.edit.DeleteView
A view that displays a confirmation page and deletes an existing object.
- The given object will only be deleted if the request method is ``POST``. If
@timgraham Django member

need a versionchanged annotation here with a brief description of the change

I haven't updated this documentation yet, while I find out whether we should be using the DELETE method, or if this should revert to the original.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on an outdated diff
docs/releases/1.8.txt
@@ -160,6 +160,14 @@ Validators
* ...
+Views
@timgraham Django member

Consider titling this section "Generic views"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on an outdated diff
docs/releases/1.8.txt
@@ -160,6 +160,14 @@ Validators
* ...
+Views
+^^^^^
+
+* :class:`~django.views.generic.edit.DeleteView` now inherits from
+ :class:`~django.views.generic.edit.ProcessFormView`. This allows compatibility
+ with features like the :class:`~django.contrib.messages.views.SuccessMessageMixin`
+ from the django messages contrib app.
@timgraham Django member

"the django messages contrib app" -> :mod:django.contrib.messages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on an outdated diff
tests/generic_views/test_edit.py
@@ -367,3 +367,20 @@ def test_delete_without_redirect(self):
# get_absolute_url provided
with self.assertRaises(ImproperlyConfigured):
self.client.post('/edit/author/%d/delete/naive/' % a.pk)
+
+ def test_delete_with_form(self):
+ a = Author.objects.create(
+ name='Randall Munroe',
+ slug='randall-munroe',
+ )
@timgraham Django member

deindent the )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham
Django member

Seems like it might be simplest to defer the issue of whether or not 'DELETE' can be used and postpone that to a separate ticket. What do you think?

@cpsimpson

That seems reasonable to me.

@cpsimpson

I've gotten my branch into a state where I can't figure out how to rebase properly to get rid of the merge commits. Anyone have any ideas how I can get this back into a good state?

@cpsimpson

Those were the instructions I was following. I think I managed to get the merge commit back out now. So I think I'm getting closer. Thanks.

@cpsimpson

I'm not sure about the isort failure that I'm getting when it runs the build, either. I've run isort -rc . in the root folder and commit all modified import statements. Is there a way to tell what the error is?

@MarkusH
Django member

Select the "default" build and click "Console Output" in the left menu: http://djangoci.com/job/isort/31/label=trusty-pr/console

ERROR: /home/jenkins/workspace/isort/label/trusty-pr/django/db/backends/sqlite3/schema.py Imports are incorrectly sorted.
@timgraham
Django member

_sqlite3 getting moved might be a Python version difference or something. Maybe we need to add it to "known_third_party" or something...

@cpsimpson

That does seem to be the import statement that was causing the issue. I've reverted my change to that file. Is marking the import statement somehow something I should do in this pull request, or is it best handled separately?

@cpsimpson cpsimpson Fixed #21936 -- Refactored DeleteView to be compatible with SuccessMe…
…ssageMixin

Refactored DeleteView to use the form related base classes and mixins, like the
other class-based generic editting views.  Added tests for the handling the
DELETE http method and ensuring the SuccessMessageMixin works. Updated DeleteView
documentation to reflect the new base classes.
The majority of the code changes were provided by Simon Charette.
ae1586f
@timgraham
Django member

Separate change will be fine. What is your system/Python version? Latest version of isort?
Ubuntu 14.04/Python 2.7/3.4/isort 3.9.6 here and on CI.

@cpsimpson

OS X (10.10.2), Python 2.7.9, isort 3.9.6

@timgraham
Django member

buildbot, add to whitelist.

@timgraham
Django member

Sorry, this has gone stale again as messages tests have been moved out of contrib to tests/messages_tests.

@auvipy

@timgraham opened a new pull #4256

@MarkusH
Django member

Close as per request

@MarkusH MarkusH closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 6, 2015
  1. @cpsimpson

    Fixed #21936 -- Refactored DeleteView to be compatible with SuccessMe…

    cpsimpson committed with cpsimpson
    …ssageMixin
    
    Refactored DeleteView to use the form related base classes and mixins, like the
    other class-based generic editting views.  Added tests for the handling the
    DELETE http method and ensuring the SuccessMessageMixin works. Updated DeleteView
    documentation to reflect the new base classes.
    The majority of the code changes were provided by Simon Charette.
View
2 django/contrib/messages/models.py
@@ -0,0 +1,2 @@
+# This file is required to pretend messages has models.
+# Otherwise test models cannot be registered.
View
13 django/contrib/messages/tests/models.py
@@ -0,0 +1,13 @@
+from django.db import models
+from django.utils.encoding import python_2_unicode_compatible
+
+
+@python_2_unicode_compatible
+class SomeObject(models.Model):
+ name = models.CharField(max_length=255)
+
+ class Meta:
+ app_label = "messages"
+
+ def __str__(self):
+ return self.name
View
13 django/contrib/messages/tests/test_mixins.py
@@ -1,4 +1,7 @@
-from django.contrib.messages.tests.urls import ContactFormViewWithMsg
+from django.contrib.messages.tests.models import SomeObject
+from django.contrib.messages.tests.urls import (
+ ContactFormViewWithMsg, DeleteFormViewWithMsg,
+)
from django.core.urlresolvers import reverse
from django.test import TestCase, override_settings
@@ -13,3 +16,11 @@ def test_set_messages_success(self):
req = self.client.post(add_url, author)
self.assertIn(ContactFormViewWithMsg.success_message % author,
req.cookies['messages'].value)
+
+ def test_set_messages_success_on_delete(self):
+ object_to_delete = SomeObject.objects.create(name="MyObject")
+
+ delete_url = reverse('success_msg_on_delete', args=[object_to_delete.pk])
+ req = self.client.delete(delete_url)
+ self.assertIn(DeleteFormViewWithMsg.success_message,
+ req.cookies['messages'].value)
View
12 django/contrib/messages/tests/urls.py
@@ -1,13 +1,14 @@
from django import forms
from django.conf.urls import url
from django.contrib import messages
+from django.contrib.messages.tests.models import SomeObject
from django.contrib.messages.views import SuccessMessageMixin
from django.core.urlresolvers import reverse
from django.http import HttpResponse, HttpResponseRedirect
from django.template import engines
from django.template.response import TemplateResponse
from django.views.decorators.cache import never_cache
-from django.views.generic.edit import FormView
+from django.views.generic.edit import DeleteView, FormView
TEMPLATE = """{% if messages %}
@@ -66,13 +67,20 @@ class ContactForm(forms.Form):
class ContactFormViewWithMsg(SuccessMessageMixin, FormView):
form_class = ContactForm
- success_url = show
+ success_url = "/show/"
success_message = "%(name)s was created successfully"
+class DeleteFormViewWithMsg(SuccessMessageMixin, DeleteView):
+ model = SomeObject
+ success_url = "/show/"
+ success_message = "Object was deleted successfully"
+
+
urlpatterns = [
url('^add/(debug|info|success|warning|error)/$', add, name='add_message'),
url('^add/msg/$', ContactFormViewWithMsg.as_view(), name='add_success_msg'),
+ url('^delete/msg/(?P<pk>\d+)$', DeleteFormViewWithMsg.as_view(), name='success_msg_on_delete'),
url('^show/$', show, name='show_message'),
url('^template_response/add/(debug|info|success|warning|error)/$',
add_template_response, name='add_template_response'),
View
33 django/views/generic/edit.py
@@ -3,14 +3,14 @@
import warnings
from django.core.exceptions import ImproperlyConfigured
-from django.forms import models as model_forms
+from django.forms import Form, models as model_forms
from django.http import HttpResponseRedirect
from django.utils import six
from django.utils.deprecation import RemovedInDjango20Warning
from django.utils.encoding import force_text
from django.views.generic.base import ContextMixin, TemplateResponseMixin, View
from django.views.generic.detail import (
- BaseDetailView, SingleObjectMixin, SingleObjectTemplateResponseMixin,
+ SingleObjectMixin, SingleObjectTemplateResponseMixin,
)
PERCENT_PLACEHOLDER_REGEX = re.compile(r'%\([^\)]+\)') # RemovedInDjango20Warning
@@ -82,7 +82,7 @@ def get_form_kwargs(self):
'prefix': self.get_prefix(),
}
- if self.request.method in ('POST', 'PUT'):
+ if self.request.method in ('POST', 'PUT', 'DELETE'):
kwargs.update({
'data': self.request.POST,
'files': self.request.FILES,
@@ -286,21 +286,27 @@ class DeletionMixin(object):
"""
success_url = None
+ def delete_object(self):
+ success_url = self.get_success_url()
+ self.object.delete()
+ return HttpResponseRedirect(success_url)
+
def delete(self, request, *args, **kwargs):
"""
Calls the delete() method on the fetched object and then
redirects to the success URL.
"""
self.object = self.get_object()
- success_url = self.get_success_url()
- self.object.delete()
- return HttpResponseRedirect(success_url)
+ return self.delete_object()
# Add support for browsers which only accept GET and POST for now.
def post(self, request, *args, **kwargs):
return self.delete(request, *args, **kwargs)
def get_success_url(self):
+ """
+ Returns the supplied success URL.
+ """
if self.success_url:
# force_text can be removed with deprecation warning
self.success_url = force_text(self.success_url)
@@ -318,12 +324,25 @@ def get_success_url(self):
"No URL to redirect to. Provide a success_url.")
-class BaseDeleteView(DeletionMixin, BaseDetailView):
+class BaseDeleteView(DeletionMixin, FormMixin, SingleObjectMixin, ProcessFormView):
"""
Base view for deleting an object.
Using this base class requires subclassing to provide a response mixin.
"""
+ form_class = Form
+
+ def get(self, request, *args, **kwargs):
+ self.object = self.get_object()
+ return super(BaseDeleteView, self).get(request, *args, **kwargs)
+
+ def delete(self, request, *args, **kwargs):
+ self.object = self.get_object()
+ # Process form instead of directly deleting the object.
+ return super(DeletionMixin, self).post(request, *args, **kwargs)
+
+ def form_valid(self, form):
+ return self.delete_object()
class DeleteView(SingleObjectTemplateResponseMixin, BaseDeleteView):
View
14 docs/ref/class-based-views/generic-editing.txt
@@ -39,6 +39,7 @@ FormView
* :class:`django.views.generic.base.TemplateResponseMixin`
* ``django.views.generic.edit.BaseFormView``
* :class:`django.views.generic.edit.FormMixin`
+ * :class:`django.views.generic.base.ContextMixin`
* :class:`django.views.generic.edit.ProcessFormView`
* :class:`django.views.generic.base.View`
@@ -97,6 +98,7 @@ CreateView
* ``django.views.generic.edit.BaseCreateView``
* :class:`django.views.generic.edit.ModelFormMixin`
* :class:`django.views.generic.edit.FormMixin`
+ * :class:`django.views.generic.base.ContextMixin`
* :class:`django.views.generic.detail.SingleObjectMixin`
* :class:`django.views.generic.edit.ProcessFormView`
* :class:`django.views.generic.base.View`
@@ -154,6 +156,7 @@ UpdateView
* ``django.views.generic.edit.BaseUpdateView``
* :class:`django.views.generic.edit.ModelFormMixin`
* :class:`django.views.generic.edit.FormMixin`
+ * :class:`django.views.generic.base.ContextMixin`
* :class:`django.views.generic.detail.SingleObjectMixin`
* :class:`django.views.generic.edit.ProcessFormView`
* :class:`django.views.generic.base.View`
@@ -198,9 +201,9 @@ DeleteView
.. class:: django.views.generic.edit.DeleteView
A view that displays a confirmation page and deletes an existing object.
- The given object will only be deleted if the request method is ``POST``. If
@timgraham Django member

need a versionchanged annotation here with a brief description of the change

I haven't updated this documentation yet, while I find out whether we should be using the DELETE method, or if this should revert to the original.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
- this view is fetched via ``GET``, it will display a confirmation page that
- should contain a form that POSTs to the same URL.
+ The given object will only be deleted if the request method is ``POST`` or
+ ``DELETE``. If this view is fetched via ``GET``, it will display a
+ confirmation page that should contain a form that POSTs to the same URL.
**Ancestors (MRO)**
@@ -210,8 +213,11 @@ DeleteView
* :class:`django.views.generic.base.TemplateResponseMixin`
* ``django.views.generic.edit.BaseDeleteView``
* :class:`django.views.generic.edit.DeletionMixin`
- * ``django.views.generic.detail.BaseDetailView``
+ * :class:`django.views.generic.edit.ModelFormMixin`
+ * :class:`django.views.generic.edit.FormMixin`
+ * :class:`django.views.generic.base.ContextMixin`
* :class:`django.views.generic.detail.SingleObjectMixin`
+ * :class:`django.views.generic.edit.ProcessFormView`
* :class:`django.views.generic.base.View`
**Attributes**
View
4 docs/ref/contrib/messages.txt
@@ -352,7 +352,9 @@ Adding messages in Class Based Views
.. class:: views.SuccessMessageMixin
Adds a success message attribute to
- :class:`~django.views.generic.edit.FormView` based classes
+ :class:`~django.views.generic.edit.FormView` based classes (for example
+ :class:`~django.views.generic.edit.CreateView`, :class:`~django.views.generic.edit.UpdateView`,
+ :class:`~django.views.generic.edit.DeleteView`)
.. method:: get_success_message(cleaned_data)
View
5 docs/releases/1.9.txt
@@ -114,6 +114,11 @@ Generic Views
* Class based views generated using ``as_view()`` now have ``view_class``
and ``view_initkwargs`` attributes.
+* :class:`~django.views.generic.edit.DeleteView` now inherits from
+ :class:`~django.views.generic.edit.ProcessFormView`. This allows compatibility
+ with features like the :class:`~django.contrib.messages.views.SuccessMessageMixin`
+ from :mod:`~django.contrib.messages`.
+
Internationalization
^^^^^^^^^^^^^^^^^^^^
View
80 tests/generic_views/test_edit.py
@@ -426,3 +426,83 @@ def test_delete_without_redirect(self):
# get_absolute_url provided
with self.assertRaises(ImproperlyConfigured):
self.client.post('/edit/author/%d/delete/naive/' % a.pk)
+
+ def test_delete_with_form_as_post(self):
+ a = Author.objects.create(
+ name='Randall Munroe',
+ slug='randall-munroe',
+ )
+
+ res = self.client.get('/edit/author/%d/delete/form/' % a.pk)
+ self.assertEqual(res.status_code, 200)
+ self.assertEqual(res.context['object'], Author.objects.get(pk=a.pk))
+ self.assertEqual(res.context['author'], Author.objects.get(pk=a.pk))
+ self.assertTemplateUsed(res, 'generic_views/author_confirm_delete.html')
+
+ res = self.client.post('/edit/author/%d/delete/form/' % a.pk,
+ data={"confirm": True})
+ self.assertEqual(res.status_code, 302)
+ self.assertRedirects(res, 'http://testserver/list/authors/')
+ self.assertQuerysetEqual(Author.objects.all(), [])
+
+ def test_delete_with_form_as_post_with_validation_error(self):
+ a = Author.objects.create(
+ name='Randall Munroe',
+ slug='randall-munroe',
+ )
+
+ res = self.client.get('/edit/author/%d/delete/form/' % a.pk)
+ self.assertEqual(res.status_code, 200)
+ self.assertEqual(res.context['object'], Author.objects.get(pk=a.pk))
+ self.assertEqual(res.context['author'], Author.objects.get(pk=a.pk))
+ self.assertTemplateUsed(res, 'generic_views/author_confirm_delete.html')
+
+ res = self.client.post('/edit/author/%d/delete/form/' % a.pk)
+ self.assertEqual(res.status_code, 200)
+ self.assertEqual(len(res.context_data["form"].errors), 2)
+ self.assertEqual(res.context_data["form"].errors["__all__"],
+ [u'You must confirm the delete.'])
+ self.assertEqual(res.context_data["form"].errors["confirm"],
+ [u'This field is required.'])
+
+ def test_delete_with_form_as_delete(self):
+ a = Author.objects.create(
+ name='Randall Munroe',
+ slug='randall-munroe',
+ )
+
+ res = self.client.get('/edit/author/%d/delete/form/' % a.pk)
+ self.assertEqual(res.status_code, 200)
+ self.assertEqual(res.context['object'], Author.objects.get(pk=a.pk))
+ self.assertEqual(res.context['author'], Author.objects.get(pk=a.pk))
+ self.assertTemplateUsed(res, 'generic_views/author_confirm_delete.html')
+
+ res = self.client.delete('/edit/author/%d/delete/form/' % a.pk,
+ data={"confirm": True})
+ # data cannot be sent with a delete method.
+ self.assertEqual(res.status_code, 200)
+ self.assertEqual(len(res.context_data["form"].errors), 2)
+ self.assertEqual(res.context_data["form"].errors["__all__"],
+ [u'You must confirm the delete.'])
+ self.assertEqual(res.context_data["form"].errors["confirm"],
+ [u'This field is required.'])
+
+ def test_delete_with_form_as_delete_with_validation_error(self):
+ a = Author.objects.create(
+ name='Randall Munroe',
+ slug='randall-munroe',
+ )
+
+ res = self.client.get('/edit/author/%d/delete/form/' % a.pk)
+ self.assertEqual(res.status_code, 200)
+ self.assertEqual(res.context['object'], Author.objects.get(pk=a.pk))
+ self.assertEqual(res.context['author'], Author.objects.get(pk=a.pk))
+ self.assertTemplateUsed(res, 'generic_views/author_confirm_delete.html')
+
+ res = self.client.delete('/edit/author/%d/delete/form/' % a.pk)
+ self.assertEqual(res.status_code, 200)
+ self.assertEqual(len(res.context_data["form"].errors), 2)
+ self.assertEqual(res.context_data["form"].errors["__all__"],
+ [u'You must confirm the delete.'])
+ self.assertEqual(res.context_data["form"].errors["confirm"],
+ [u'This field is required.'])
View
12 tests/generic_views/test_forms.py
@@ -17,3 +17,15 @@ class Meta:
class ContactForm(forms.Form):
name = forms.CharField()
message = forms.CharField(widget=forms.Textarea)
+
+
+class ConfirmDeleteForm(forms.Form):
+ confirm = forms.BooleanField()
+
+ def clean(self):
+ cleaned_data = super(ConfirmDeleteForm, self).clean()
+ delete_confirmed = cleaned_data.get("confirm")
+
+ # import pdb; pdb.set_trace()
+ if not delete_confirmed:
+ raise forms.ValidationError("You must confirm the delete.")
View
2 tests/generic_views/urls.py
@@ -113,6 +113,8 @@
views.AuthorDelete.as_view()),
url(r'^edit/author/(?P<pk>[0-9]+)/delete/special/$',
views.SpecializedAuthorDelete.as_view()),
+ url(r'^edit/author/(?P<pk>\d+)/delete/form/$',
+ views.AuthorDeleteFormView.as_view()),
# ArchiveIndexView
url(r'^dates/books/$',
View
10 tests/generic_views/views.py
@@ -7,7 +7,7 @@
from django.views import generic
from .models import Artist, Author, Book, BookSigning, Page
-from .test_forms import AuthorForm, ContactForm
+from .test_forms import AuthorForm, ConfirmDeleteForm, ContactForm
class CustomTemplateView(generic.TemplateView):
@@ -175,6 +175,14 @@ class SpecializedAuthorDelete(generic.DeleteView):
success_url = reverse_lazy('authors_list')
+class AuthorDeleteFormView(generic.DeleteView):
+ model = Author
+ form_class = ConfirmDeleteForm
+
+ def get_success_url(self):
+ return reverse('authors_list')
+
+
class BookConfig(object):
queryset = Book.objects.all()
date_field = 'pubdate'
Something went wrong with that request. Please try again.