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

[Gavin Panella]

New form for creating and updating notifications.

Required some alterations to the Notification model because nothing quite works right with Django.

[Lee Trager (ltrager)]

Forms never works the way you want them to, but this looks good!

review: Approve

[Gavin Panella (allenap)]

Thanks!

[MAAS Lander (maas-lander)]

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

Hit:1 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial InRelease
Get:2 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates InRelease [102 kB]
Get:3 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-backports InRelease [102 kB]
Get:4 http://security.ubuntu.com/ubuntu xenial-security InRelease [102 kB]
Fetched 306 kB in 0s (567 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 newest version (1.3.0-0ubuntu5).
python-lxml is already the newest version (3.5.0-1build1).
python-netaddr is already the newest version (0.7.18-1).
python-netifaces is already the newest version (0.10.4-0.1build2).
python-psyc...

=== added file 'src/maasserver/forms/notification.py'
--- src/maasserver/forms/notification.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/forms/notification.py	2017-02-14 13:46:55 +0000
@@ -0,0 +1,36 @@
+# Copyright 2017 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Notification form."""
+
+__all__ = [
+    "NotificationForm",
+]
+
+import json
+
+from maasserver.forms import MAASModelForm
+from maasserver.models.notification import Notification
+
+
+class NotificationForm(MAASModelForm):
+    """Notification creation/edit form."""
+
+    class Meta:
+        model = Notification
+        fields = (
+            'ident',
+            'user',
+            'users',
+            'admins',
+            'message',
+            'context',
+            'category',
+        )
+
+    def clean_context(self):
+        data = self.cleaned_data.get("context")
+        if data is None or len(data) == 0 or data.isspace():
+            return {}  # Default to an empty dict when in doubt.
+        else:
+            return json.loads(data)

=== added file 'src/maasserver/forms/tests/test_notification.py'
--- src/maasserver/forms/tests/test_notification.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/forms/tests/test_notification.py	2017-02-14 13:46:55 +0000
@@ -0,0 +1,90 @@
+# Copyright 2017 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for Notification forms."""
+
+__all__ = []
+
+import json
+import random
+
+from maasserver.forms.notification import NotificationForm
+from maasserver.testing.factory import factory
+from maasserver.testing.testcase import MAASServerTestCase
+from testtools.matchers import (
+    Equals,
+    MatchesStructure,
+)
+
+
+categories = "info", "success", "warning", "error"
+
+
+class TestNotificationForm(MAASServerTestCase):
+
+    def test__notification_can_be_created_with_just_message(self):
+        notification_message = factory.make_name("message")
+        form = NotificationForm({
+            "message": notification_message,
+        })
+        self.assertTrue(form.is_valid(), form.errors)
+        notification = form.save()
+        self.assertThat(notification, MatchesStructure.byEquality(
+            ident=None, message=notification_message, user=None, users=False,
+            admins=False, category="info", context={},
+        ))
+
+    def test__notification_can_be_created_with_all_fields(self):
+        user = factory.make_User()
+        data = {
+            "ident": factory.make_name("ident"),
+            "user": str(user.id),
+            "users": random.choice(["true", "false"]),
+            "admins": random.choice(["true", "false"]),
+            "message": factory.make_name("message"),
+            "context": json.dumps({
+                factory.make_name("key"): factory.make_name("value"),
+            }),
+            "category": random.choice(categories),
+        }
+        form = NotificationForm(data)
+        self.assertTrue(form.is_valid(), form.errors)
+        notification = form.save()
+        expected = dict(
+            data, user=user, users=(data["users"] == "true"),
+            admins=(data["admins"] == "true"),
+            context=json.loads(data["context"]),
+        )
+        self.assertThat(
+            notification,
+            MatchesStructure.byEquality(**expected),
+        )
+
+    def test__notification_can_be_updated(self):
+        notification = factory.make_Notification()
+        user = factory.make_User()
+        data = {
+            "ident": factory.make_name("ident"),
+            "user": str(user.id),
+            "users": "false" if notification.users else "true",
+            "admins": "false" if notification.admins else "true",
+            "message": factory.make_name("message"),
+            "context": json.dumps({
+                factory.make_name("key"): factory.make_name("value"),
+            }),
+            "category": random.choice(
+                [c for c in categories if c != notification.category]),
+        }
+        form = NotificationForm(instance=notification, data=data)
+        self.assertTrue(form.is_valid(), form.errors)
+        notification_saved = form.save()
+        self.assertThat(notification_saved, Equals(notification))
+        expected = dict(
+            data, user=user, users=(data["users"] == "true"),
+            admins=(data["admins"] == "true"),
+            context=json.loads(data["context"]),
+        )
+        self.assertThat(
+            notification_saved,
+            MatchesStructure.byEquality(**expected),
+        )

=== added file 'src/maasserver/migrations/builtin/maasserver/0112_update_notification.py'
--- src/maasserver/migrations/builtin/maasserver/0112_update_notification.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/migrations/builtin/maasserver/0112_update_notification.py	2017-02-14 13:46:55 +0000
@@ -0,0 +1,38 @@
+from django.conf import settings
+from django.db import (
+    migrations,
+    models,
+)
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('maasserver', '0111_remove_component_error'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='notification', name='category',
+            field=models.CharField(
+                default='info', choices=[
+                    ('error', 'Error'), ('warning', 'Warning'),
+                    ('success', 'Success'), ('info', 'Informational'),
+                ], max_length=10, blank=True),
+        ),
+        migrations.AlterField(
+            model_name='notification', name='ident',
+            field=models.CharField(
+                null=True, default=None, max_length=40, blank=True),
+        ),
+        migrations.AlterField(
+            model_name='notification', name='message',
+            field=models.TextField(),
+        ),
+        migrations.AlterField(
+            model_name='notification', name='user',
+            field=models.ForeignKey(
+                null=True, default=None, blank=True,
+                to=settings.AUTH_USER_MODEL),
+        ),
+    ]

=== modified file 'src/maasserver/models/notification.py'
--- src/maasserver/models/notification.py	2017-02-08 10:46:18 +0000
+++ src/maasserver/models/notification.py	2017-02-14 13:46:55 +0000
@@ -10,6 +10,7 @@
 from functools import wraps
 
 from django.contrib.auth.models import User
+from django.core.exceptions import ValidationError
 from django.db import connection
 from django.db.models import (
     BooleanField,
@@ -100,10 +101,12 @@
         :return: A `QuerySet` of `Notification` instances that haven't been
             dismissed by `user`.
         """
-        if user.is_superuser:
-            where = "notification.users OR notification.admins"
+        if user is None:
+            return Notification.objects.none()
+        elif user.is_superuser:
+            query = self._sql_find_ids_for_admins
         else:
-            where = "notification.users"
+            query = self._sql_find_ids_for_users
         # 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().
@@ -112,12 +115,11 @@
         # 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])
+            cursor.execute(query, [user.id, user.id])
             ids = [nid for (nid, ) in cursor.fetchall()]
         return self.filter(id__in=ids)
 
-    _sql_find_ids_for_user = """\
+    _sql_find_ids_for_xxx = """\
     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)
@@ -125,6 +127,9 @@
     ORDER BY notification.updated, notification.id
     """
 
+    _sql_find_ids_for_users = _sql_find_ids_for_xxx % "notification.users"
+    _sql_find_ids_for_admins = _sql_find_ids_for_xxx % "notification.admins"
+
 
 class Notification(CleanSave, TimestampedModel):
     """A notification message.
@@ -156,16 +161,16 @@
     # The ident column *is* unique, but uniqueness will be ensured using a
     # partial index in PostgreSQL. These cannot be expressed using Django. See
     # migrations for the SQL used to create this index.
-    ident = CharField(max_length=40, null=True, blank=True)
-
-    user = ForeignKey(User, null=True, blank=True)
-    users = BooleanField(default=False)
-    admins = BooleanField(default=False)
-
-    message = TextField(null=False, blank=True)
+    ident = CharField(max_length=40, null=True, blank=True, default=None)
+
+    user = ForeignKey(User, null=True, blank=True, default=None)
+    users = BooleanField(null=False, blank=True, default=False)
+    admins = BooleanField(null=False, blank=True, default=False)
+
+    message = TextField(null=False, blank=False)
     context = JSONObjectField(null=False, blank=True, default=dict)
     category = CharField(
-        null=False, blank=False, default="info", max_length=10,
+        null=False, blank=True, default="info", max_length=10,
         choices=[
             ("error", "Error"), ("warning", "Warning"),
             ("success", "Success"), ("info", "Informational"),
@@ -200,7 +205,25 @@
 
     def clean(self):
         super(Notification, self).clean()
-        self.render()  # Just check it works.
+        # Elementary cleaning that Django can't seem to do for us, mainly
+        # because setting blank=False causes any number of problems.
+        if self.ident == "":
+            self.ident = None
+        if self.category == "":
+            self.category = "info"
+        # The context must be a a dict (well, mapping, but we check for dict
+        # because it will be converted to JSON later and a dict-like object
+        # won't do). This could be done as a validator but, meh, I'm sick of
+        # jumping through Django-hoops like a circus animal.
+        if not isinstance(self.context, dict):
+            raise ValidationError(
+                {"context": "Context is not a mapping."})
+        # Finally, check that the notification can be rendered. No point in
+        # doing any of this if we cannot relate the message.
+        try:
+            self.render()
+        except Exception as error:
+            raise ValidationError("Notification cannot be rendered.")
 
     def __repr__(self):
         username = "None" if self.user is None else repr(self.user.username)

=== modified file 'src/maasserver/models/tests/test_notification.py'
--- src/maasserver/models/tests/test_notification.py	2017-02-08 10:46:18 +0000
+++ src/maasserver/models/tests/test_notification.py	2017-02-14 13:46:55 +0000
@@ -8,6 +8,7 @@
 import itertools
 import random
 
+from django.core.exceptions import ValidationError
 from django.db.models.query import QuerySet
 from maasserver.models.notification import Notification
 from maasserver.testing.factory import factory
@@ -177,41 +178,55 @@
     def test_save_checks_that_rendering_works(self):
         message = "Dude, where's my {thing}?"
         notification = Notification(message=message)
-        error = self.assertRaises(KeyError, notification.save)
-        self.assertThat(str(error), Equals(repr("thing")))
+        error = self.assertRaises(ValidationError, notification.save)
+        self.assertThat(error.message_dict, Equals({
+            "__all__": ["Notification cannot be rendered."],
+        }))
         self.assertThat(notification.id, Is(None))
         self.assertThat(Notification.objects.all(), HasLength(0))
 
     def test_is_relevant_to_user(self):
+        make_Notification = factory.make_Notification
+
         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)
+        def assertRelevance(notification, user, yes_or_no):
+            # Ensure that is_relevant_to and find_for_user agree, i.e. if
+            # is_relevant_to returns True, the notification is in the set
+            # returned by find_for_user. Likewise, if is_relevant_to returns
+            # False, the notification is not in the find_for_user set.
+            self.assertThat(notification.is_relevant_to(user), yes_or_no)
+            self.assertThat(
+                Notification.objects.find_for_user(user).filter(
+                    id=notification.id).exists(), yes_or_no)
+
+        notification_to_user = make_Notification(user=user)
+        assertRelevance(notification_to_user, None, No)
+        assertRelevance(notification_to_user, user, Yes)
+        assertRelevance(notification_to_user, user2, No)
+        assertRelevance(notification_to_user, admin, No)
+
+        notification_to_users = make_Notification(users=True)
+        assertRelevance(notification_to_users, None, No)
+        assertRelevance(notification_to_users, user, Yes)
+        assertRelevance(notification_to_users, user2, Yes)
+        assertRelevance(notification_to_users, admin, No)
+
+        notification_to_admins = make_Notification(admins=True)
+        assertRelevance(notification_to_admins, None, No)
+        assertRelevance(notification_to_admins, user, No)
+        assertRelevance(notification_to_admins, user2, No)
+        assertRelevance(notification_to_admins, admin, Yes)
+
+        notification_to_all = make_Notification(users=True, admins=True)
+        assertRelevance(notification_to_all, None, No)
+        assertRelevance(notification_to_all, user, Yes)
+        assertRelevance(notification_to_all, user2, Yes)
+        assertRelevance(notification_to_all, admin, Yes)
 
 
 class TestNotificationRepresentation(MAASServerTestCase):

=== modified file 'src/maasserver/websockets/handlers/tests/test_notification.py'
--- src/maasserver/websockets/handlers/tests/test_notification.py	2017-02-02 17:12:11 +0000
+++ src/maasserver/websockets/handlers/tests/test_notification.py	2017-02-14 13:46:55 +0000
@@ -98,14 +98,13 @@
         notifications = [
             factory.make_Notification(user=admin),  # Will match.
             factory.make_Notification(user=admin2),
-            factory.make_Notification(users=True),  # Will match.
+            factory.make_Notification(users=True),
             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(