More refactors

- single base method to send notifications
- knows about service id (present if job based)
- knows about jobs - if needed
- knows about type

Does the right thing

Was lots of shared code around error checking now in one place.

app/celery/tasks.py

@@ -45,6 +45,7 @@ def send_email(service_id, notification_id, subject, from_address, encrypted_not
             template_id=notification['template'],
             to=notification['to'],
             service_id=service_id,
+            job_id=notification.get('job', None),
             status='sent'
         )
         save_notification(notification_db_object)

app/notifications/rest.py

@@ -18,7 +18,8 @@ from app.schemas import (
     email_notification_schema,
     sms_template_notification_schema,
     notification_status_schema,
-    job_sms_template_notification_schema
+    job_sms_template_notification_schema,
+    job_email_template_notification_schema
 )
 from app.celery.tasks import send_sms, send_email
 from sqlalchemy.orm.exc import NoResultFound
@@ -29,6 +30,9 @@ from app.errors import register_errors
 
 register_errors(notifications)
 
+SMS_NOTIFICATION = 'sms'
+EMAIL_NOTIFICATION = 'email'
+
 
 def create_notification_id():
     return str(uuid.uuid4())
@@ -45,22 +49,36 @@ def get_notifications(notification_id):
 
 @notifications.route('/sms', methods=['POST'])
 def create_sms_notification():
-    return base_create_sms_notification(expects_job=False)
+    return send_notification(notification_type=SMS_NOTIFICATION, expects_job=False)
 
 
 @notifications.route('/sms/service/<service_id>', methods=['POST'])
-def create_sms_for_service(service_id):
+def create_sms_for_job(service_id):
-    return base_create_sms_notification(service_id, expects_job=True)
+    return send_notification(service_id=service_id, notification_type=SMS_NOTIFICATION, expects_job=True)
 
 
-def base_create_sms_notification(service_id=None, expects_job=False):
+@notifications.route('/email', methods=['POST'])
+def create_email_notification():
+    return send_notification(notification_type=EMAIL_NOTIFICATION, expects_job=False)
+
+
+@notifications.route('/email/service/<service_id>', methods=['POST'])
+def create_email_notification_for_job(service_id):
+    return send_notification(service_id=service_id, notification_type=EMAIL_NOTIFICATION, expects_job=True)
+
+
+def send_notification(notification_type, service_id=None, expects_job=False):
+    assert notification_type
+
     if not service_id:
         service_id = api_user['client']
 
     if expects_job:
-        schema = job_sms_template_notification_schema
+        schema = job_sms_template_notification_schema if notification_type is SMS_NOTIFICATION else \
+            job_email_template_notification_schema
     else:
-        schema = sms_template_notification_schema
+        schema = sms_template_notification_schema if notification_type is SMS_NOTIFICATION else \
+            email_notification_schema
 
     notification, errors = schema.load(request.get_json())
     if errors:
@@ -85,49 +103,32 @@ def base_create_sms_notification(service_id=None, expects_job=False):
         if not job:
             return jsonify(result="error", message={'job': ['Job {} not found'.format(notification['job'])]}), 400
 
-    service = services_dao.dao_fetch_service_by_id(service_id)
-
-    if service.restricted:
-        if notification['to'] not in [user.email_address for user in service.users]:
-            return jsonify(result="error", message={'to': ['Invalid phone number for restricted service']}), 400
-
-    notification_id = create_notification_id()
-
-    send_sms.apply_async((
-        api_user['client'],
-        notification_id,
-        encryption.encrypt(notification)),
-        queue='sms')
-    return jsonify({'notification_id': notification_id}), 201
-
-
-@notifications.route('/email', methods=['POST'])
-def create_email_notification():
-    notification, errors = email_notification_schema.load(request.get_json())
-    if errors:
-        return jsonify(result="error", message=errors), 400
-
-    template = templates_dao.dao_get_template_by_id_and_service_id(
-        template_id=notification['template'],
-        service_id=api_user['client']
-    )
-
-    if not template:
-        return jsonify(result="error", message={'template': ['Template not found']}), 400
-
     service = services_dao.dao_fetch_service_by_id(api_user['client'])
 
     if service.restricted:
-        if notification['to'] not in [user.email_address for user in service.users]:
+        if notification_type is SMS_NOTIFICATION:
-            return jsonify(result="error", message={'to': ['Email address not permitted for restricted service']}), 400
+            if notification['to'] not in [user.mobile_number for user in service.users]:
+                return jsonify(
+                    result="error", message={'to': ['Invalid phone number for restricted service']}), 400
+        else:
+            if notification['to'] not in [user.email_address for user in service.users]:
+                return jsonify(
+                    result="error", message={'to': ['Email address not permitted for restricted service']}), 400
 
     notification_id = create_notification_id()
 
-    send_email.apply_async((
+    if notification_type is SMS_NOTIFICATION:
-        api_user['client'],
+        send_sms.apply_async((
-        notification_id,
+            api_user['client'],
-        template.subject,
+            notification_id,
-        "{}@{}".format(service.email_from, current_app.config['NOTIFY_EMAIL_DOMAIN']),
+            encryption.encrypt(notification)),
-        encryption.encrypt(notification)),
+            queue='sms')
-        queue='email')
+    else:
+        send_email.apply_async((
+            api_user['client'],
+            notification_id,
+            template.subject,
+            "{}@{}".format(service.email_from, current_app.config['NOTIFY_EMAIL_DOMAIN']),
+            encryption.encrypt(notification)),
+            queue='email')
     return jsonify({'notification_id': notification_id}), 201

app/schemas.py

@@ -113,6 +113,11 @@ class JobSmsTemplateNotificationSchema(SmsNotificationSchema):
     job = fields.String(required=True)
 
 
+class JobEmailTemplateNotificationSchema(EmailNotificationSchema):
+    template = fields.Int(required=True)
+    job = fields.String(required=True)
+
+
 class SmsAdminNotificationSchema(SmsNotificationSchema):
     content = fields.Str(required=True)
 
@@ -145,6 +150,7 @@ sms_admin_notification_schema = SmsAdminNotificationSchema()
 sms_template_notification_schema = SmsTemplateNotificationSchema()
 job_sms_template_notification_schema = JobSmsTemplateNotificationSchema()
 email_notification_schema = EmailNotificationSchema()
+job_email_template_notification_schema = JobEmailTemplateNotificationSchema()
 notification_status_schema = NotificationStatusSchema()
 notifications_status_schema = NotificationStatusSchema(many=True)
 notification_status_schema_load_json = NotificationStatusSchema(load_json=True)

tests/app/notifications/test_rest.py

@@ -572,7 +572,11 @@ def test_should_reject_email_notification_with_template_id_that_cant_be_found(
             app.celery.tasks.send_email.apply_async.assert_not_called()
             assert response.status_code == 400
             assert data['result'] == 'error'
-            assert data['message']['template'][0] == 'Template not found'
+            test_string = 'Template {} not found for service {}'.format(
+                1234,
+                sample_email_template.service.id
+            )
+            assert test_string in data['message']['template']
 
 
 def test_should_not_allow_email_template_from_another_service(notify_api, service_factory, sample_user, mocker):
@@ -605,7 +609,8 @@ def test_should_not_allow_email_template_from_another_service(notify_api, servic
             app.celery.tasks.send_email.apply_async.assert_not_called()
 
             assert response.status_code == 400
-            assert 'Template not found' in json_resp['message']['template']
+            test_string = 'Template {} not found for service {}'.format(service_2_templates[0].id, service_1.id)
+            assert test_string in json_resp['message']['template']
 
 
 def test_should_not_send_email_if_restricted_and_not_a_service_user(notify_api, sample_email_template, mocker):