Merge lp:~allenap/maas/notifications-websocket into lp:~maas-committers/maas/trunk

[Gavin Panella]

Basic WebSocket handler for Notification.

[Lee Trager (ltrager)]

LGTM just one comment below.

review: Approve

[Gavin Panella (allenap)]

Thanks for the review.

=== modified file 'src/maasserver/models/notification.py'
--- src/maasserver/models/notification.py	2017-01-04 15:59:23 +0000
+++ src/maasserver/models/notification.py	2017-01-09 16:30:12 +0000
@@ -8,6 +8,7 @@
 ]
 
 from django.contrib.auth.models import User
+from django.db import connection
 from django.db.models import (
     BooleanField,
     CharField,
@@ -66,17 +67,28 @@
     def find_for_user(self, user):
         """Find notifications for the given user.
 
-        :return: A `RawQuerySet` of `Notification` instances that haven't been
+        :return: A `QuerySet` of `Notification` instances that haven't been
             dismissed by `user`.
         """
         if user.is_superuser:
             where = "notification.users OR notification.admins"
         else:
             where = "notification.users"
-        return self.raw(self._sql_find_for_user % where, [user.id, user.id])
+        # We want to return a QuerySet because things like the WebSocket
+        # handler code wants to use order_by. This seems reasonable. However,
+        # we can't do outer joins with Django so we have to use self.raw().
+        # However #2, that returns a RawQuerySet which doesn't do order_by.
+        # Nor can we do self.filter(id__in=self.raw(...)) to "legitimise" a
+        # raw query. Nope, we have to actually fetch those IDs then issue
+        # another query to get a QuerySet for a set of Notification rows.
+        with connection.cursor() as cursor:
+            find_ids = self._sql_find_ids_for_user % where
+            cursor.execute(find_ids, [user.id, user.id])
+            ids = [nid for (nid, ) in cursor.fetchall()]
+        return self.filter(id__in=ids)
 
-    _sql_find_for_user = """\
-    SELECT notification.* FROM maasserver_notification AS notification
+    _sql_find_ids_for_user = """\
+    SELECT notification.id FROM maasserver_notification AS notification
     LEFT OUTER JOIN maasserver_notificationdismissal AS dismissal ON
       (dismissal.notification_id = notification.id AND dismissal.user_id = %%s)
     WHERE (notification.user_id = %%s OR %s) AND dismissal.id IS NULL

=== modified file 'src/maasserver/models/tests/test_notification.py'
--- src/maasserver/models/tests/test_notification.py	2017-01-05 09:49:36 +0000
+++ src/maasserver/models/tests/test_notification.py	2017-01-09 16:30:12 +0000
@@ -7,6 +7,7 @@
 
 import random
 
+from django.db.models.query import QuerySet
 from maasserver.models.notification import Notification
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
@@ -16,6 +17,8 @@
     Equals,
     HasLength,
     Is,
+    IsInstance,
+    MatchesAll,
     MatchesStructure,
     Not,
 )
@@ -165,7 +168,10 @@
     def assertNotifications(self, user, notifications):
         self.assertThat(
             Notification.objects.find_for_user(user),
-            AfterPreprocessing(list, Equals(notifications)))
+            MatchesAll(
+                IsInstance(QuerySet),  # Not RawQuerySet.
+                AfterPreprocessing(list, Equals(notifications)),
+            ))
 
     def test_find_and_dismiss_notifications_for_user(self):
         user = factory.make_User()

=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py	2017-01-05 18:32:27 +0000
+++ src/maasserver/testing/factory.py	2017-01-09 16:30:12 +0000
@@ -82,6 +82,7 @@
     MDNS,
     Neighbour,
     Node,
+    Notification,
     OwnerData,
     PackageRepository,
     Partition,
@@ -2067,6 +2068,29 @@
             distributions=distributions, disabled_pockets=disabled_pockets,
             components=components, arches=arches, key=key, default=default)
 
+    def make_Notification(
+            self, message=None, *, ident=None, user=None, users=False,
+            admins=False, context=None):
+
+        if context is None:
+            context_name = self.make_name("name")
+            context = {context_name: self.make_name("value")}
+
+        if message is None:
+            context_names = [key for key in context if key.isidentifier()]
+            if len(context_names) == 0:
+                message = self.make_name("message")
+            else:
+                context_name = random.choice(context_names)
+                message = self.make_name("message-{%s}" % context_name)
+
+        notification = Notification(
+            ident=ident, user=user, users=users, admins=admins,
+            message=message, context=context)
+        notification.save()
+
+        return notification
+
 
 # Create factory singleton.
 factory = Factory()

=== added file 'src/maasserver/websockets/handlers/notification.py'
--- src/maasserver/websockets/handlers/notification.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/websockets/handlers/notification.py	2017-01-09 16:30:12 +0000
@@ -0,0 +1,41 @@
+# Copyright 2017 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""The notification handler for the WebSocket connection."""
+
+__all__ = [
+    "NotificationHandler",
+]
+
+from maasserver.models.notification import Notification
+from maasserver.websockets.handlers.timestampedmodel import (
+    TimestampedModelHandler,
+)
+
+
+class NotificationHandler(TimestampedModelHandler):
+
+    class Meta:
+        object_class = Notification
+        allowed_methods = {'list', 'get', 'dismiss'}
+        exclude = list_exclude = {"context"}
+
+    def get_queryset(self):
+        """Return `Notifications` for the current user."""
+        return Notification.objects.find_for_user(self.user)
+
+    def dehydrate(self, obj, data, for_list=False):
+        data["message"] = obj.render()
+        return data
+
+    def dismiss(self, params):
+        """Dismiss the given notification(s).
+
+        :param id: One or more notification IDs to dismiss for the currently
+            logged-in user.
+        """
+        data = self.preprocess_form("dismiss", params)
+        ids = map(int, data.getlist("id"))
+        notifications = self.get_queryset().filter(id__in=ids)
+        for notification in notifications:
+            notification.dismiss(self.user)

=== added file 'src/maasserver/websockets/handlers/tests/test_notification.py'
--- src/maasserver/websockets/handlers/tests/test_notification.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/websockets/handlers/tests/test_notification.py	2017-01-09 16:30:12 +0000
@@ -0,0 +1,115 @@
+# Copyright 2017 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for `maasserver.websockets.handlers.notification`."""
+
+__all__ = []
+
+from maasserver.models.notification import NotificationDismissal
+from maasserver.testing.factory import factory
+from maasserver.testing.testcase import MAASServerTestCase
+from maasserver.websockets.base import dehydrate_datetime
+from maasserver.websockets.handlers.notification import NotificationHandler
+from testtools.matchers import (
+    AfterPreprocessing,
+    AllMatch,
+    Equals,
+    Is,
+    MatchesDict,
+    MatchesListwise,
+    Not,
+)
+
+
+def MatchesRenderedNotification(ntfn):
+    """Return a matcher for a rendered notification."""
+    return MatchesDict({
+        "id": Equals(ntfn.id),
+        "ident": Is(None) if ntfn.ident is None else Equals(ntfn.ident),
+        "user": Is(None) if ntfn.user_id is None else Equals(ntfn.user_id),
+        "users": Is(ntfn.users),
+        "admins": Is(ntfn.admins),
+        "updated": Equals(dehydrate_datetime(ntfn.updated)),
+        "created": Equals(dehydrate_datetime(ntfn.created)),
+        "message": Equals(ntfn.render()),
+    })
+
+
+def HasBeenDismissedBy(user):
+    """Has `user` dismissed the notification we're matching against?"""
+
+    def dismissed_by_user(notification):
+        return NotificationDismissal.objects.filter(
+            notification=notification, user=user).exists()
+
+    return AfterPreprocessing(dismissed_by_user, Is(True))
+
+
+class TestNotificationHandler(MAASServerTestCase):
+    """Tests for `NotificationHandler`."""
+
+    def test_get(self):
+        user = factory.make_User()
+        handler = NotificationHandler(user, {})
+        notification = factory.make_Notification(user=user)
+        self.assertThat(
+            handler.get({"id": notification.id}),
+            MatchesRenderedNotification(notification))
+
+    def test_list(self):
+        user = factory.make_User()
+        user2 = factory.make_User()
+        handler = NotificationHandler(user, {})
+        notifications = [
+            factory.make_Notification(user=user),  # Will match.
+            factory.make_Notification(user=user2),
+            factory.make_Notification(users=True),  # Will match.
+            factory.make_Notification(users=False),
+            factory.make_Notification(admins=True),
+            factory.make_Notification(admins=False),
+        ]
+        expected = [
+            MatchesRenderedNotification(notifications[0]),
+            MatchesRenderedNotification(notifications[2]),
+        ]
+        self.assertThat(
+            handler.list({}), MatchesListwise(expected))
+
+    def test_list_for_admin(self):
+        admin = factory.make_admin()
+        admin2 = factory.make_admin()
+        handler = NotificationHandler(admin, {})
+        notifications = [
+            factory.make_Notification(user=admin),  # Will match.
+            factory.make_Notification(user=admin2),
+            factory.make_Notification(users=True),  # Will match.
+            factory.make_Notification(users=False),
+            factory.make_Notification(admins=True),  # Will match.
+            factory.make_Notification(admins=False),
+        ]
+        expected = [
+            MatchesRenderedNotification(notifications[0]),
+            MatchesRenderedNotification(notifications[2]),
+            MatchesRenderedNotification(notifications[4]),
+        ]
+        self.assertThat(
+            handler.list({}), MatchesListwise(expected))
+
+    def test_dismiss(self):
+        user = factory.make_User()
+        handler = NotificationHandler(user, {})
+        notification = factory.make_Notification(user=user)
+        self.assertThat(notification, Not(HasBeenDismissedBy(user)))
+        handler.dismiss({"id": str(notification.id)})
+        self.assertThat(notification, HasBeenDismissedBy(user))
+
+    def test_dismiss_multiple(self):
+        user = factory.make_User()
+        handler = NotificationHandler(user, {})
+        notifications = [
+            factory.make_Notification(user=user),
+            factory.make_Notification(users=True),
+        ]
+        self.assertThat(notifications, AllMatch(Not(HasBeenDismissedBy(user))))
+        handler.dismiss({"id": [str(ntfn.id) for ntfn in notifications]})
+        self.assertThat(notifications, AllMatch(HasBeenDismissedBy(user)))