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

Already on GitHub? Sign in to your account

Fixes ticket 16319 #922

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

paulcollinsiii commented Mar 18, 2013

This is mainly a cleanup of the patch in 16319. I changed the formatting
of the success_message to use the .format() system instead.

Paul Collins added some commits Mar 18, 2013

Paul Collins Fixes ticket 16319
This is mainly a cleanup of the patch in 16319. I changed the formatting
of the success_message to use the .format() system instead.
cc2d7dd
Paul Collins Merge branch 'master' into ticket_16319 564ee84
Paul Collins fixup comments for SuccessMessageMixin 0aca654
Paul Collins Updated per comments from claudep b0c2ce5

@aaugustin aaugustin and 1 other commented on an outdated diff Mar 19, 2013

django/contrib/messages/views.py
+
+
+class SuccessMessageMixin(FormMixin):
+ """
+ A mixin that adds a success message on successful form submission
+ """
+ success_message = None
+
+ def form_valid(self, form):
+ success_message = self.get_success_message()
+ if success_message:
+ messages.success(self.request, success_message)
+ return super(SuccessMessageMixin, self).form_valid(form)
+
+ def get_success_message(self):
+ if hasattr(self, 'object') and self.object:
@aaugustin

aaugustin Mar 19, 2013

Owner

Is this conditional really necessary?

If it is, it means that in some cases a success message with placeholders could be returned, that sounds bad.

@paulcollinsiii

paulcollinsiii Mar 19, 2013

Contributor

What if the Mixin is being used on a non ModelForm style FormView?
I can change this further to signal what the string should be formatted with, but is there a preference on style for that?

@aaugustin aaugustin commented on an outdated diff Mar 19, 2013

docs/ref/contrib/messages.txt
@@ -286,6 +286,33 @@ example::
use one of the ``add_message`` family of methods. It does not hide failures
that may occur for other reasons.
+Adding messages in Class Based Views
+------------------------------------
+
+If you're writing a form processing view and want to add a success message on
+valid form submission you can use the
+``'django.contrib.messages.views.SuccessMessageMixin'``.
@aaugustin

aaugustin Mar 19, 2013

Owner

The single quotes aren't necessary.

@aaugustin aaugustin commented on the diff Mar 19, 2013

docs/ref/contrib/messages.txt
@@ -286,6 +286,33 @@ example::
use one of the ``add_message`` family of methods. It does not hide failures
that may occur for other reasons.
+Adding messages in Class Based Views
+------------------------------------
+
+If you're writing a form processing view and want to add a success message on
+valid form submission you can use the
+``'django.contrib.messages.views.SuccessMessageMixin'``.
+For example::
+
+ # views.py
+ from django.contrib.messages.views import SuccessMessageMixin
+ from django.views.generic.edit import CreateView
+ from myapp.models import Author
+
+
@aaugustin

aaugustin Mar 19, 2013

Owner

Extra line break -- keep the docs compact.

@aaugustin aaugustin commented on an outdated diff Mar 19, 2013

docs/ref/contrib/messages.txt
+valid form submission you can use the
+``'django.contrib.messages.views.SuccessMessageMixin'``.
+For example::
+
+ # views.py
+ from django.contrib.messages.views import SuccessMessageMixin
+ from django.views.generic.edit import CreateView
+ from myapp.models import Author
+
+
+ class AuthorCreate(CreateView, SuccessMessageMixin):
+ model = Author
+ success_url = '/success/'
+ success_message = "%(name)s was created successfully"
+
+.. note::
@aaugustin

aaugustin Mar 19, 2013

Owner

Notes are pretty heavy in the rendered docs, this could be more readable as regular text.

Same comment for the warning below.

@aaugustin aaugustin commented on an outdated diff Mar 19, 2013

docs/ref/contrib/messages.txt
+ from django.views.generic.edit import CreateView
+ from myapp.models import Author
+
+
+ class AuthorCreate(CreateView, SuccessMessageMixin):
+ model = Author
+ success_url = '/success/'
+ success_message = "%(name)s was created successfully"
+
+.. note::
+ In ``success_message`` fields from the ``model`` are available for string
+ interpolation using the ``%(field_name)s`` syntax.
+
+.. warning::
+ The order of the inherited classes matters. ``SuccessMessageMixin`` needs
+ to come after any form processing mixins
@aaugustin

aaugustin Mar 19, 2013

Owner

Missing point.

@aaugustin aaugustin commented on an outdated diff Mar 19, 2013

tests/generic_views/edit.py
@@ -315,3 +315,17 @@ def test_delete_without_redirect(self):
except ImproperlyConfigured:
pass
+class SuccessMessageMixinTests(TestCase):
+ urls = 'generic_views.urls'
+
+ def test_set_messages_success(self):
+ author = {'name': 'John Doe',
+ 'slug': 'success-msg'}
+ req = self.client.post('/edit/authors/create/msg/', author)
+ self.assertIn(views.AuthorCreateViewWithMsg.success_message % author,
+ req.cookies['messages'].value)
+
+ def test_set_message_false(self):
+ req = self.client.post('/edit/authors/create/msg/',
+ {'name': 'John Doe'})
+ self.assertFalse('messages' in req.cookies)
@aaugustin

aaugustin Mar 19, 2013

Owner

Use assertNotIn.

@aaugustin aaugustin and 1 other commented on an outdated diff Mar 19, 2013

tests/generic_views/edit.py
@@ -315,3 +315,17 @@ def test_delete_without_redirect(self):
except ImproperlyConfigured:
pass
+class SuccessMessageMixinTests(TestCase):
+ urls = 'generic_views.urls'
+
+ def test_set_messages_success(self):
+ author = {'name': 'John Doe',
+ 'slug': 'success-msg'}
+ req = self.client.post('/edit/authors/create/msg/', author)
+ self.assertIn(views.AuthorCreateViewWithMsg.success_message % author,
@aaugustin

aaugustin Mar 19, 2013

Owner

Minor: I tend to prefer testing with a literal: "John Doe was created successfully". That's slightly more reliable. But that's your call :)

@paulcollinsiii

paulcollinsiii Mar 19, 2013

Contributor

Actually taking that I think from Carl's talk since I can update the test in one place rather than having to update the constant twice. I might be confusing it with another talk. The PyCon firehose of learning was a bit intense this time around.

@ptone ptone commented on an outdated diff Mar 19, 2013

django/contrib/messages/views.py
@@ -0,0 +1,18 @@
+from django.views.generic.edit import FormMixin, ModelFormMixin
@ptone

ptone Mar 19, 2013

Member

unsed ModelFormMixin

@ptone ptone commented on an outdated diff Mar 19, 2013

docs/ref/contrib/messages.txt
+:class:`~django.contrib.messages.views.SuccessMessageMixin`.
+For example::
+
+ # views.py
+ from django.contrib.messages.views import SuccessMessageMixin
+ from django.views.generic.edit import CreateView
+ from myapp.models import Author
+
+ class AuthorCreate(SuccessMessageMixin, CreateView):
+ model = Author
+ success_url = '/success/'
+ success_message = "%(name)s was created successfully"
+
+The cleaned data from the ``form`` is available for string interpolation using
+the ``%(field_name)s`` syntax. For ModelForms, if you need access to fields
+from the saved ``object`` override the ``get_success_message`` method. For
@ptone

ptone Mar 19, 2013

Member

ideally get_success_message should be a link to that method's docs

Member

ptone commented Mar 19, 2013

Made a couple minor inline comments.

I think the tests should be moved to django/contrib/messages/tests, since contrib can't count on core in tests, but we shouldn't build dependencies for the other way around.

Otherwise, on first review, this looks like it delivers on the feature request

Member

ptone commented Mar 20, 2013

merged in 9a85ad8

@ptone ptone closed this Mar 20, 2013

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