Alter usage of message extends (#2567)

* Alter usage of message extends

Instead of using the messages.add method directly, add message for the message
notification backend using the storage adapter directly. This also removes the
limitation on the storage backend that requires a request.user is authenticated,
as we are likely going to be mostly adding messages through celery or other
request-less mechanisms.

* Fix tests

* Make sure we can't add messages for anonymous users and fix tests

Author: agjohnson

readthedocs/notifications/backends.py

@@ -1,6 +1,7 @@
 """Notification to message backends"""
 
 from django.conf import settings
+from django.http import HttpRequest
 from django.utils.module_loading import import_string
 from messages_extends.constants import INFO_PERSISTENT
 from messages_extends import add_message
@@ -67,13 +68,21 @@ class SiteBackend(Backend):
     name = 'site'
 
     def send(self, notification):
-        add_message(
-            request=self.request,
+        # Instead of calling the standard messages.add method, this instead
+        # manipulates the storage directly. This is because we don't have a
+        # request object and need to mock one out to fool the message storage
+        # into saving a message for a separate user.
+        cls_name = settings.MESSAGE_STORAGE
+        cls = import_string(cls_name)
+        req = HttpRequest()
+        setattr(req, 'session', '')
+        storage = cls(req)
+        storage.add(
+            level=LEVEL_MAPPING.get(notification.level, INFO_PERSISTENT),
             message=notification.render(
                 backend_name=self.name,
                 source_format=HTML
             ),
-            level=LEVEL_MAPPING.get(notification.level, INFO_PERSISTENT),
             extra_tags='',
             user=notification.user,
         )

readthedocs/notifications/storages.py

@@ -45,10 +45,11 @@ def _get(self, *args, **kwargs):
         return safe_messages, all_ret
 
     def add(self, level, message, extra_tags='', *args, **kwargs):
-        if self.request.user.is_authenticated():
+        user = kwargs.get('user') or self.request.user
+        if not user.is_anonymous():
             persist_messages = (PersistentMessage.objects
                                 .filter(message=message,
-                                        user=self.request.user,
+                                        user=user,
                                         read=False))
             if persist_messages.exists():
                 return

readthedocs/rtd_tests/tests/test_notifications.py

@@ -4,10 +4,12 @@
 import django_dynamic_fixture as fixture
 from django.test import TestCase
 from django.test.utils import override_settings
-from django.contrib.auth.models import User
+from django.contrib.auth.models import User, AnonymousUser
+from messages_extends.models import Message as PersistentMessage
 
 from readthedocs.notifications import Notification
 from readthedocs.notifications.backends import EmailBackend, SiteBackend
+from readthedocs.notifications.constants import ERROR
 from readthedocs.builds.models import Build
 
 
@@ -22,6 +24,8 @@
 class NotificationTests(TestCase):
 
     def test_notification_custom(self, send, render_to_string):
+        render_to_string.return_value = 'Test'
+
         class TestNotification(Notification):
             name = 'foo'
             subject = 'This is {{ foo.id }}'
@@ -54,23 +58,36 @@ class NotificationBackendTests(TestCase):
 
     @mock.patch('readthedocs.notifications.backends.send_email')
     def test_email_backend(self, send_email, render_to_string):
+        render_to_string.return_value = 'Test'
+
         class TestNotification(Notification):
             name = 'foo'
             subject = 'This is {{ foo.id }}'
             context_object_name = 'foo'
+            level = ERROR
 
         build = fixture.get(Build)
         req = mock.MagicMock()
-        notify = TestNotification(object=build, request=req)
+        user = fixture.get(User)
+        notify = TestNotification(object=build, request=req, user=user)
         backend = EmailBackend(request=req)
         backend.send(notify)
 
+        self.assertEqual(render_to_string.call_count, 1)
         send_email.assert_has_calls([
-            mock.call(level=21, request=req, message=mock.ANY, extra_tags='')
+            mock.call(
+                request=mock.ANY,
+                template='core/email/common.txt',
+                context={'content': 'Test'},
+                subject=u'This is 1',
+                template_html='core/email/common.html',
+                recipient=user.email,
+            )
         ])
 
-    @mock.patch('readthedocs.notifications.backends.add_message')
-    def test_email_backend(self, add_message, render_to_string):
+    def test_message_backend(self, render_to_string):
+        render_to_string.return_value = 'Test'
+
         class TestNotification(Notification):
             name = 'foo'
             subject = 'This is {{ foo.id }}'
@@ -83,7 +100,33 @@ class TestNotification(Notification):
         backend = SiteBackend(request=req)
         backend.send(notify)
 
-        add_message.assert_has_calls([
-            mock.call(level=21, request=req, message=mock.ANY, extra_tags='',
-                      user=user)
-        ])
+        self.assertEqual(render_to_string.call_count, 1)
+        self.assertEqual(PersistentMessage.objects.count(), 1)
+
+        message = PersistentMessage.objects.first()
+        self.assertEqual(message.user, user)
+
+    def test_message_anonymous_user(self, render_to_string):
+        """Anonymous user still throwns exception on persistent messages"""
+        render_to_string.return_value = 'Test'
+
+        class TestNotification(Notification):
+            name = 'foo'
+            subject = 'This is {{ foo.id }}'
+            context_object_name = 'foo'
+
+        build = fixture.get(Build)
+        user = AnonymousUser()
+        req = mock.MagicMock()
+        notify = TestNotification(object=build, request=req, user=user)
+        backend = SiteBackend(request=req)
+
+        self.assertEqual(PersistentMessage.objects.count(), 0)
+
+        # We should never be adding persistent messages for anonymous users.
+        # Make sure message_extends sitll throws an exception here
+        with self.assertRaises(NotImplementedError):
+            backend.send(notify)
+
+        self.assertEqual(render_to_string.call_count, 1)
+        self.assertEqual(PersistentMessage.objects.count(), 0)