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

[Gavin Panella]

WebSocket listener behaviour for Notification.

[Mike Pontillo (mpontillo)]

Looks good to me. Just one thing below to consider changing.

review: Approve

[Gavin Panella (allenap)]

Thanks!

[MAAS Lander (maas-lander)]

The attempt to merge lp:~allenap/maas/notifications-websocket-listener into lp:maas failed. Below is the output from the failed tests.

Get:1 http://security.ubuntu.com/ubuntu xenial-security InRelease [102 kB]
Hit:2 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial InRelease
Get:3 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates InRelease [102 kB]
Hit:4 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-backports InRelease
Get:5 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates/main amd64 Packages [457 kB]
Get:6 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates/universe amd64 Packages [383 kB]
Fetched 1,044 kB in 0s (1,728 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
    --no-install-recommends install apache2 archdetect-deb authbind avahi-utils bash bind9 bind9utils build-essential bzr bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport dh-systemd distro-info dnsutils firefox freeipmi-tools git gjs ipython isc-dhcp-common isc-dhcp-server libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libnss-wrapper libpq-dev make nodejs-legacy npm postgresql pxelinux python3-all python3-apt python3-attr python3-bson python3-convoy python3-crochet python3-cssselect python3-curtin python3-dev python3-distro-info python3-django python3-django-nose python3-django-piston3 python3-dnspython python3-docutils python3-formencode python3-hivex python3-httplib2 python3-jinja2 python3-jsonschema python3-lxml python3-netaddr python3-netifaces python3-novaclient python3-oauth python3-oauthlib python3-openssl python3-paramiko python3-petname python3-pexpect python3-psycopg2 python3-pyinotify python3-pyparsing python3-pyvmomi python3-requests python3-seamicroclient python3-setuptools python3-simplestreams python3-sphinx python3-tempita python3-twisted python3-txtftp python3-tz python3-yaml python3-zope.interface python-bson python-crochet python-django python-django-piston python-djorm-ext-pgarray python-formencode python-lxml python-netaddr python-netifaces python-pocket-lint python-psycopg2 python-simplejson python-tempita python-twisted python-yaml socat syslinux-common tgt ubuntu-cloudimage-keyring wget xvfb
Reading package lists...
Building dependency tree...
Reading state information...
authbind is already the newest version (2.1.1+nmu1).
avahi-utils is already the newest version (0.6.32~rc+dfsg-1ubuntu2).
build-essential is already the newest version (12.1ubuntu2).
debhelper is already the newest version (9.20160115ubuntu3).
distro-info is already the newest version (0.14build1).
git is already the newest version (1:2.7.4-0ubuntu1).
libjs-angularjs is already the newest version (1.2.28-1ubuntu2).
libjs-jquery is already the newest version (1.11.3+dfsg-4).
libjs-yui3-full is already the newest version (3.5.1-1ubuntu3).
libjs-yui3-min is already the newest version (3.5.1-1ubuntu3).
make is already the newest version (4.1-6).
postgresql is already the newest version (9.5+173).
pxelinux is already the newest version (3:6.03+dfsg-11ubuntu1).
python-formencode is already the ...

=== modified file 'src/maasserver/models/notification.py'
--- src/maasserver/models/notification.py	2017-01-09 15:44:55 +0000
+++ src/maasserver/models/notification.py	2017-01-24 08:41:46 +0000
@@ -134,6 +134,14 @@
         """Render this notification's message using its context."""
         return self.message.format(**self.context)
 
+    def is_relevant_to(self, user):
+        """Is this notification relevant to the given user?"""
+        return user is not None and (
+            (self.user_id is not None and self.user_id == user.id) or
+            (self.users and not user.is_superuser) or
+            (self.admins and user.is_superuser)
+        )
+
     def dismiss(self, user):
         """Dismiss this notification.
 

=== modified file 'src/maasserver/models/tests/test_notification.py'
--- src/maasserver/models/tests/test_notification.py	2017-01-09 15:44:55 +0000
+++ src/maasserver/models/tests/test_notification.py	2017-01-24 08:41:46 +0000
@@ -228,6 +228,37 @@
         self.assertThat(notification.id, Is(None))
         self.assertThat(Notification.objects.all(), HasLength(0))
 
+    def test_is_relevant_to_user(self):
+        user = factory.make_User()
+        user2 = factory.make_User()
+        admin = factory.make_admin()
+
+        Yes, No = Is(True), Is(False)
+
+        notification_to_user = Notification(user=user)
+        self.assertThat(notification_to_user.is_relevant_to(None), No)
+        self.assertThat(notification_to_user.is_relevant_to(user), Yes)
+        self.assertThat(notification_to_user.is_relevant_to(user2), No)
+        self.assertThat(notification_to_user.is_relevant_to(admin), No)
+
+        notification_to_users = Notification(users=True)
+        self.assertThat(notification_to_users.is_relevant_to(None), No)
+        self.assertThat(notification_to_users.is_relevant_to(user), Yes)
+        self.assertThat(notification_to_users.is_relevant_to(user2), Yes)
+        self.assertThat(notification_to_users.is_relevant_to(admin), No)
+
+        notification_to_admins = Notification(admins=True)
+        self.assertThat(notification_to_admins.is_relevant_to(None), No)
+        self.assertThat(notification_to_admins.is_relevant_to(user), No)
+        self.assertThat(notification_to_admins.is_relevant_to(user2), No)
+        self.assertThat(notification_to_admins.is_relevant_to(admin), Yes)
+
+        notification_to_all = Notification(users=True, admins=True)
+        self.assertThat(notification_to_all.is_relevant_to(None), No)
+        self.assertThat(notification_to_all.is_relevant_to(user), Yes)
+        self.assertThat(notification_to_all.is_relevant_to(user2), Yes)
+        self.assertThat(notification_to_all.is_relevant_to(admin), Yes)
+
 
 class TestNotificationRepresentation(MAASServerTestCase):
     """Tests for the `Notification` representation."""

=== modified file 'src/maasserver/websockets/handlers/__init__.py'
--- src/maasserver/websockets/handlers/__init__.py	2017-01-23 11:24:56 +0000
+++ src/maasserver/websockets/handlers/__init__.py	2017-01-24 08:41:46 +0000
@@ -19,6 +19,7 @@
     "IPRangeHandler",
     "IPRangeHandler",
     "MachineHandler",
+    "NotificationHandler",
     "PackageRepositoryHandler",
     "ServiceHandler",
     "SpaceHandler",
@@ -43,6 +44,7 @@
 from maasserver.websockets.handlers.general import GeneralHandler
 from maasserver.websockets.handlers.iprange import IPRangeHandler
 from maasserver.websockets.handlers.machine import MachineHandler
+from maasserver.websockets.handlers.notification import NotificationHandler
 from maasserver.websockets.handlers.packagerepository import (
     PackageRepositoryHandler,
 )

=== modified file 'src/maasserver/websockets/handlers/notification.py'
--- src/maasserver/websockets/handlers/notification.py	2017-01-09 16:19:02 +0000
+++ src/maasserver/websockets/handlers/notification.py	2017-01-24 08:41:46 +0000
@@ -8,6 +8,7 @@
 ]
 
 from maasserver.models.notification import Notification
+from maasserver.utils.orm import reload_object
 from maasserver.websockets.handlers.timestampedmodel import (
     TimestampedModelHandler,
 )
@@ -19,6 +20,7 @@
         object_class = Notification
         allowed_methods = {'list', 'get', 'dismiss'}
         exclude = list_exclude = {"context"}
+        listen_channels = {'notification', 'notificationdismissal'}
 
     def get_queryset(self):
         """Return `Notifications` for the current user."""
@@ -39,3 +41,33 @@
         notifications = self.get_queryset().filter(id__in=ids)
         for notification in notifications:
             notification.dismiss(self.user)
+
+    def on_listen(self, channel, action, pk):
+        """Intercept `on_listen` because dismissals must be handled specially.
+
+        The docstring for `on_listen` suggests that handlers should override
+        `listen` instead of `on_listen`. However, `Handler.on_listen` makes an
+        assumption about notifications from the database that does not hold
+        here: dismissals are notified as "$notification_id:$user_id" strings,
+        not as simply "$notification_id".
+        """
+        if channel == "notification":
+            return super().on_listen(channel, action, pk)
+        elif channel == "notificationdismissal":
+            pk, user_id = map(int, pk.split(":"))
+            if self.user.id == user_id:
+                # Send a dismissal as a delete of the notification.
+                return super().on_listen("notification", "delete", pk)
+            else:
+                return None  # Not relevant to this user.
+        else:
+            return None  # Channel not recognised.
+
+    def listen(self, channel, action, pk):
+        """Only deal with notifications that are relevant to the user."""
+        notification = super().listen(channel, action, pk)
+        user = reload_object(self.user)  # Check for up-to-date privs.
+        if notification.is_relevant_to(user):
+            return notification
+        else:
+            return None

=== modified file 'src/maasserver/websockets/handlers/tests/test_notification.py'
--- src/maasserver/websockets/handlers/tests/test_notification.py	2017-01-09 16:19:02 +0000
+++ src/maasserver/websockets/handlers/tests/test_notification.py	2017-01-24 08:41:46 +0000
@@ -5,16 +5,31 @@
 
 __all__ = []
 
+from itertools import product
+import random
+from unittest.mock import sentinel
+
+from maasserver.models import User
 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.base import (
+    dehydrate_datetime,
+    Handler,
+)
 from maasserver.websockets.handlers.notification import NotificationHandler
+from maastesting.matchers import (
+    MockCalledOnceWith,
+    MockNotCalled,
+)
+from testscenarios import multiply_scenarios
 from testtools.matchers import (
     AfterPreprocessing,
     AllMatch,
     Equals,
     Is,
+    IsInstance,
+    MatchesAll,
     MatchesDict,
     MatchesListwise,
     Not,
@@ -113,3 +128,125 @@
         self.assertThat(notifications, AllMatch(Not(HasBeenDismissedBy(user))))
         handler.dismiss({"id": [str(ntfn.id) for ntfn in notifications]})
         self.assertThat(notifications, AllMatch(HasBeenDismissedBy(user)))
+
+
+class TestNotificationHandlerListening(MAASServerTestCase):
+    """Tests for `NotificationHandler` listening to database messages."""
+
+    def test_on_listen_for_notification_up_calls(self):
+        super_on_listen = self.patch(Handler, "on_listen")
+        super_on_listen.return_value = sentinel.on_listen
+
+        user = factory.make_User()
+        handler = NotificationHandler(user, {})
+
+        self.assertThat(
+            handler.on_listen("notification", sentinel.action, sentinel.pk),
+            Is(sentinel.on_listen))
+        self.assertThat(
+            super_on_listen, MockCalledOnceWith(
+                "notification", sentinel.action, sentinel.pk))
+
+    def test_on_listen_for_dismissal_up_calls_with_delete(self):
+        super_on_listen = self.patch(Handler, "on_listen")
+        super_on_listen.return_value = sentinel.on_listen
+
+        user = factory.make_User()
+        handler = NotificationHandler(user, {})
+        notification = factory.make_Notification(user=user)
+
+        # A dismissal notification from the database.
+        dismissal = "%d:%d" % (notification.id, user.id)
+
+        self.assertThat(
+            handler.on_listen(
+                "notificationdismissal", sentinel.action, dismissal),
+            Is(sentinel.on_listen))
+        self.assertThat(
+            super_on_listen, MockCalledOnceWith(
+                "notification", "delete", notification.id))
+
+    def test_on_listen_for_dismissal_for_other_user_does_nothing(self):
+        super_on_listen = self.patch(Handler, "on_listen")
+        super_on_listen.return_value = sentinel.on_listen
+
+        user = factory.make_User()
+        handler = NotificationHandler(user, {})
+        notification = factory.make_Notification(user=user)
+
+        # A dismissal notification from the database FOR ANOTHER USER.
+        dismissal = "%d:%d" % (notification.id, random.randrange(1, 99999))
+
+        self.assertThat(handler.on_listen(
+            "notificationdismissal", sentinel.action, dismissal), Is(None))
+        self.assertThat(super_on_listen, MockNotCalled())
+
+    def test_listen_reloads_user(self):
+        admin = factory.make_admin()
+        handler = NotificationHandler(admin, {})
+        notification = factory.make_Notification(admins=True)
+
+        # The notification is currently relevant to `admin`.
+        self.assertThat(handler.listen(
+            "notification", "create", notification.id), Equals(notification))
+
+        # Change `admin` to a regular user in the database.
+        admin_alt = User.objects.get(id=admin.id)
+        admin_alt.is_superuser = False
+        admin_alt.save()
+
+        # The change is noticed because `handler.listen` reloads the user,
+        # meaning the notification is no longer relevant.
+        self.assertThat(handler.listen(
+            "notification", "create", notification.id), Is(None))
+
+
+class TestNotificationHandlerListeningScenarios(MAASServerTestCase):
+    """Tests for `NotificationHandler` listening to database messages."""
+
+    scenarios_users = (
+        ("user", dict(make_user=factory.make_User)),
+        ("admin", dict(make_user=factory.make_admin)),
+    )
+
+    scenarios_notifications = (
+        ("to-user=%s;to-users=%s;to-admins=%s" % scenario,
+         dict(zip(("to_user", "to_users", "to_admins"), scenario)))
+        for scenario in product(
+            (False, True, "Other"),  # To specific user.
+            (False, True),           # To all users.
+            (False, True),           # To all admins.
+        )
+    )
+
+    scenarios = multiply_scenarios(
+        scenarios_users, scenarios_notifications)
+
+    def test_on_listen(self):
+        user = self.make_user()
+
+        if self.to_user is False:
+            to_user = None
+        elif self.to_user is True:
+            to_user = user
+        else:
+            to_user = factory.make_User()
+
+        notification = factory.make_Notification(
+            user=to_user, users=self.to_users, admins=self.to_admins)
+
+        if notification.is_relevant_to(user):
+            expected = MatchesAll(
+                IsInstance(tuple),
+                MatchesListwise((
+                    Equals("notification"), Equals("create"),
+                    MatchesRenderedNotification(notification),
+                )),
+                first_only=True,
+            )
+        else:
+            expected = Is(None)
+
+        handler = NotificationHandler(user, {})
+        self.assertThat(handler.on_listen(
+            "notification", "create", notification.id), expected)

=== modified file 'src/maasserver/websockets/tests/test_protocol.py'
--- src/maasserver/websockets/tests/test_protocol.py	2016-10-26 15:38:28 +0000
+++ src/maasserver/websockets/tests/test_protocol.py	2017-01-24 08:41:46 +0000
@@ -697,6 +697,8 @@
     "fabric",
     "iprange",
     "machine",
+    "notification",
+    "notificationdismissal",
     "packagerepository",
     "service",
     "space",
@@ -710,18 +712,19 @@
 )
 
 ALL_HANDLERS = (
-    "config",
     "bootresource",
+    "config",
     "controller",
     "device",
+    "dhcpsnippet",
     "discovery",
-    "dhcpsnippet",
     "domain",
     "event",
     "fabric",
     "general",
     "iprange",
     "machine",
+    "notification",
     "packagerepository",
     "service",
     "space",