From feff951dd9823cbaaa129c79147157e14e0896c9 Mon Sep 17 00:00:00 2001 From: Adrian Turjak Date: Thu, 8 Feb 2018 16:07:35 +1300 Subject: [PATCH] Fix notifications not being auto acknowledged Add tests to confirm this is the case, and fix and edge case that when notifications engines are incorrectly configured for a task, to skip them. Change-Id: Ib715924b4e068e3d0c9a43b55183fe86eb27e38f --- adjutant/api/v1/tests/test_api_admin.py | 12 +++ adjutant/api/v1/utils.py | 21 ++--- adjutant/notifications/models.py | 2 +- adjutant/notifications/tests/__init__.py | 0 .../notifications/tests/test_notifications.py | 81 +++++++++++++++++++ 5 files changed, 106 insertions(+), 10 deletions(-) create mode 100644 adjutant/notifications/tests/__init__.py create mode 100644 adjutant/notifications/tests/test_notifications.py diff --git a/adjutant/api/v1/tests/test_api_admin.py b/adjutant/api/v1/tests/test_api_admin.py index 1556bfb..eeafcc3 100644 --- a/adjutant/api/v1/tests/test_api_admin.py +++ b/adjutant/api/v1/tests/test_api_admin.py @@ -450,6 +450,10 @@ class AdminAPITests(APITestCase): self.assertEqual(response.json(), {"errors": ["No notification with this id."]}) + @modify_dict_settings(TASK_SETTINGS={ + 'key_list': ['create_project', 'notifications'], + 'operation': 'delete', + }) def test_notification_acknowledge(self): """ Test that you can acknowledge a notification. @@ -519,6 +523,10 @@ class AdminAPITests(APITestCase): {'errors': ['No notification with this id.']}) + @modify_dict_settings(TASK_SETTINGS={ + 'key_list': ['create_project', 'notifications'], + 'operation': 'delete', + }) def test_notification_re_acknowledge(self): """ Test that you cant reacknowledge a notification. @@ -552,6 +560,10 @@ class AdminAPITests(APITestCase): self.assertEqual(response.json(), {'notes': ['Notification already acknowledged.']}) + @modify_dict_settings(TASK_SETTINGS={ + 'key_list': ['create_project', 'notifications'], + 'operation': 'delete', + }) def test_notification_acknowledge_no_data(self): """ Test that you have to include 'acknowledged': True to the request. diff --git a/adjutant/api/v1/utils.py b/adjutant/api/v1/utils.py index e95ba26..47bb20a 100644 --- a/adjutant/api/v1/utils.py +++ b/adjutant/api/v1/utils.py @@ -170,15 +170,18 @@ def create_notification(task, notes, error=False, engines=True): class_conf = settings.TASK_SETTINGS.get( task.task_type, settings.DEFAULT_TASK_SETTINGS) - for note_engine, conf in (class_conf.get('notifications', {})).items(): - if error: - conf = conf.get('error', {}) - else: - conf = conf.get('standard', {}) - if not conf: - continue - engine = settings.NOTIFICATION_ENGINES[note_engine](conf) - engine.notify(task, notification) + notification_conf = class_conf.get('notifications', {}) + + if notification_conf: + for note_engine, conf in notification_conf.items(): + if error: + conf = conf.get('error', {}) + else: + conf = conf.get('standard', {}) + if not conf: + continue + engine = settings.NOTIFICATION_ENGINES[note_engine](conf) + engine.notify(task, notification) return notification diff --git a/adjutant/notifications/models.py b/adjutant/notifications/models.py index a86c74d..e10e89d 100644 --- a/adjutant/notifications/models.py +++ b/adjutant/notifications/models.py @@ -115,7 +115,7 @@ class EmailNotification(NotificationEngine): email.send(fail_silently=False) if not notification.error: - notification.acknowledge = True + notification.acknowledged = True notification.save() except SMTPException as e: notes = { diff --git a/adjutant/notifications/tests/__init__.py b/adjutant/notifications/tests/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/adjutant/notifications/tests/test_notifications.py b/adjutant/notifications/tests/test_notifications.py new file mode 100644 index 0000000..f4ae644 --- /dev/null +++ b/adjutant/notifications/tests/test_notifications.py @@ -0,0 +1,81 @@ +# Copyright (C) 2015 Catalyst IT Ltd +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import mock + +from django.core import mail + +from rest_framework import status + +from adjutant.api.models import Task, Notification +from adjutant.common.tests.fake_clients import ( + FakeManager, setup_identity_cache) +from adjutant.common.tests.utils import ( + AdjutantAPITestCase, modify_dict_settings) + + +@mock.patch('adjutant.common.user_store.IdentityManager', + FakeManager) +class NotificationTests(AdjutantAPITestCase): + + @modify_dict_settings(TASK_SETTINGS={ + 'key_list': ['create_project', 'notifications'], + 'operation': 'override', + 'value': { + 'EmailNotification': { + 'standard': { + 'emails': ['example@example.com'], + 'reply': 'no-reply@example.com', + 'template': 'notification.txt' + }, + 'error': { + 'emails': ['example@example.com'], + 'reply': 'no-reply@example.com', + 'template': 'notification.txt' + } + } + } + }) + def test_new_project_sends_notification(self): + """ + Confirm that the email notification engine correctly acknowledges + notifications it sends out. + """ + + setup_identity_cache() + + url = "/v1/actions/CreateProject" + data = {'project_name': "test_project", 'email': "test@example.com"} + response = self.client.post(url, data, format='json') + self.assertEqual(response.status_code, status.HTTP_200_OK) + + headers = { + 'project_name': "test_project", + 'project_id': "test_project_id", + 'roles': "admin,_member_", + 'username': "test@example.com", + 'user_id': "test_user_id", + 'authenticated': True + } + new_task = Task.objects.all()[0] + url = "/v1/tasks/" + new_task.uuid + response = self.client.post(url, {'approved': True}, format='json', + headers=headers) + + self.assertEqual(Notification.objects.count(), 1) + self.assertEqual(len(mail.outbox), 3) + + notif = Notification.objects.all()[0] + self.assertEqual(notif.task.uuid, new_task.uuid) + self.assertTrue(notif.acknowledged)