- Added logging to indicate that we created the notification and that we sent the notification to a delivery queue.

- Small updates as recommended by review comments.

app/notifications/process_notifications.py

@@ -43,6 +43,7 @@ def persist_notification(template_id,
                          reference=None,
                          notification_id=None,
                          simulated=False):
+    # if simulated create a Notification model to return but do not persist the Notification to the dB
     notification = Notification(
         id=notification_id,
         template_id=template_id,
@@ -62,6 +63,9 @@ def persist_notification(template_id,
     if not simulated:
         dao_create_notification(notification)
         redis_store.incr(redis.daily_limit_cache_key(service.id))
+        current_app.logger.info(
+            "{} {} created at {}".format(notification.notification_type, notification.id, notification.created_at)
+        )
     return notification
 
 
@@ -87,8 +91,9 @@ def send_notification_to_queue(notification, research_mode, queue=None):
         raise SendNotificationToQueueError()
 
     current_app.logger.info(
-        "{} {} created at {}".format(notification.notification_type, notification.id, notification.created_at)
+        "{} {} sent to the {} queue for delivery".format(notification.notification_type,
-    )
+                                                         notification.id,
+                                                         queue))
 
 
 def simulated_recipient(to_address, notification_type):

app/notifications/rest.py

@@ -217,7 +217,7 @@ def send_notification(notification_type):
 
     service = services_dao.dao_fetch_service_by_id(api_user.service_id)
 
-    notification, errors = (
+    notification_form, errors = (
         sms_template_notification_schema if notification_type == SMS_TYPE else email_notification_schema
     ).load(request.get_json())
     if errors:
@@ -225,40 +225,40 @@ def send_notification(notification_type):
 
     check_service_message_limit(api_user.key_type, service)
 
-    template = templates_dao.dao_get_template_by_id_and_service_id(template_id=notification['template'],
+    template = templates_dao.dao_get_template_by_id_and_service_id(template_id=notification_form['template'],
                                                                    service_id=service.id)
 
     check_template_is_for_notification_type(notification_type, template.template_type)
     check_template_is_active(template)
 
-    template_object = create_template_object_for_notification(template, notification.get('personalisation', {}))
+    template_object = create_template_object_for_notification(template, notification_form.get('personalisation', {}))
 
-    _service_allowed_to_send_to(notification, service)
+    _service_allowed_to_send_to(notification_form, service)
 
     # Do not persist or send notification to the queue if it is a simulated recipient
-    simulated = simulated_recipient(notification['to'], notification_type)
+    simulated = simulated_recipient(notification_form['to'], notification_type)
-    saved_notification = persist_notification(template_id=template.id,
+    notification_model = persist_notification(template_id=template.id,
                                               template_version=template.version,
-                                              recipient=notification['to'],
+                                              recipient=notification_form['to'],
                                               service=service,
-                                              personalisation=notification.get('personalisation', None),
+                                              personalisation=notification_form.get('personalisation', None),
                                               notification_type=notification_type,
                                               api_key_id=api_user.id,
                                               key_type=api_user.key_type,
                                               simulated=simulated)
     if not simulated:
         queue_name = 'notify' if template.process_type == PRIORITY else None
-        send_notification_to_queue(notification=saved_notification,
+        send_notification_to_queue(notification=notification_model,
                                    research_mode=service.research_mode,
                                    queue=queue_name)
     else:
-        current_app.logger.info("POST simulated notification for id: {}".format(saved_notification.id))
+        current_app.logger.info("POST simulated notification for id: {}".format(notification_model.id))
-    notification.update({"template_version": template.version})
+    notification_form.update({"template_version": template.version})
 
     return jsonify(
         data=get_notification_return_data(
-            saved_notification.id,
+            notification_model.id,
-            notification,
+            notification_form,
             template_object)
     ), 201
 

app/v2/notifications/get_notifications.py

@@ -14,7 +14,7 @@ from app.v2.notifications.notification_schemas import get_notifications_request
 def get_notification_by_id(id):
     try:
         casted_id = uuid.UUID(id)
-    except ValueError:
+    except ValueError or AttributeError:
         abort(404)
     notification = notifications_dao.get_notification_with_personalisation(
         api_user.service_id, casted_id, key_type=None

tests/app/notifications/test_rest.py

@@ -35,11 +35,12 @@ def test_get_sms_notification_by_id(client, sample_notification):
         assert not notification.get('subject')
 
 
-def test_get_sms_notification_by_invalid_id(client, sample_notification):
+@pytest.mark.parametrize("id", ["1234-badly-formatted-id-7890", "0"])
+def test_get_sms_notification_by_invalid_id(client, sample_notification, id):
         auth_header = create_authorization_header(service_id=sample_notification.service_id)
 
         response = client.get(
-            '/notifications/{}'.format("not_a_valid_id"),
+            '/notifications/{}'.format(id),
             headers=[auth_header])
 
         assert response.status_code == 405

tests/app/v2/notifications/test_get_notifications.py

@@ -164,10 +164,11 @@ def test_get_notification_by_id_nonexistent_id(client, sample_notification):
     }
 
 
-def test_get_notification_by_id_invalid_id(client, sample_notification):
+@pytest.mark.parametrize("id", ["1234-badly-formatted-id-7890", "0"])
+def test_get_notification_by_id_invalid_id(client, sample_notification, id):
     auth_header = create_authorization_header(service_id=sample_notification.service_id)
     response = client.get(
-        path='/v2/notifications/1234-badly-formatted-id-7890',
+        path='/v2/notifications/{}'.format(id),
         headers=[('Content-Type', 'application/json'), auth_header])
 
     assert response.status_code == 404

tests/app/v2/notifications/test_post_notifications.py

@@ -159,7 +159,7 @@ def test_post_email_notification_returns_201(client, sample_email_template_with_
     ('07700 900111', 'sms'),
     ('07700 900222', 'sms')
 ])
-def test_should_not_persist_notification_or_send_notification_if_simulated_recipient(
+def test_should_not_persist_or_send_notification_if_simulated_recipient(
         client,
         recipient,
         notification_type,