Merge pull request #686 from alphagov/refactor-send-tasks-into-shared-code

Refactor send tasks into shared code

app/__init__.py

@@ -70,6 +70,7 @@ def create_app(app_name=None):
     from app.provider_details.rest import provider_details as provider_details_blueprint
     from app.spec.rest import spec as spec_blueprint
     from app.organisation.rest import organisation_blueprint
+    from app.delivery.rest import delivery_blueprint
 
     application.register_blueprint(service_blueprint, url_prefix='/service')
     application.register_blueprint(user_blueprint, url_prefix='/user')
@@ -78,6 +79,7 @@ def create_app(app_name=None):
     application.register_blueprint(notifications_blueprint)
     application.register_blueprint(job_blueprint)
     application.register_blueprint(invite_blueprint)
+    application.register_blueprint(delivery_blueprint)
     application.register_blueprint(accept_invite, url_prefix='/invite')
 
     application.register_blueprint(template_statistics_blueprint)

app/celery/provider_tasks.py

@@ -1,31 +1,17 @@
-from datetime import datetime
-
 from flask import current_app
-from notifications_utils.recipients import (
-    validate_and_format_phone_number
-)
-from notifications_utils.template import Template, get_sms_fragment_count
-from notifications_utils.renderers import HTMLEmail, PlainTextEmail, SMSMessage
 
-from app import notify_celery, statsd_client, clients, create_uuid
+from app import notify_celery
-from app.clients.email import EmailClientException
+from app.dao import notifications_dao
-from app.clients.sms import SmsClientException
+from app.dao.notifications_dao import update_notification_status_by_id
-from app.dao.notifications_dao import (
-    get_notification_by_id,
-    dao_update_notification,
-    update_notification_status_by_id
-)
-from app.dao.provider_details_dao import get_provider_details_by_notification_type
-from app.dao.services_dao import dao_fetch_service_by_id
-from app.celery.research_mode_tasks import send_sms_response, send_email_response
-from app.dao.templates_dao import dao_get_template_by_id
-
-from app.models import SMS_TYPE, EMAIL_TYPE, KEY_TYPE_TEST, BRANDING_ORG
 from app.statsd_decorators import statsd
 
+from app.delivery import send_to_providers
+from sqlalchemy.orm.exc import NoResultFound
+
 
 def retry_iteration_to_delay(retry=0):
     """
+    :param retry times we have performed a retry
     Given current retry calculate some delay before retrying
     0: 10 seconds
     1: 60 seconds (1 minutes)
@@ -47,39 +33,15 @@ def retry_iteration_to_delay(retry=0):
     return delays.get(retry, 10)
 
 
-@notify_celery.task(bind=True, name="send-sms-to-provider", max_retries=5, default_retry_delay=5)
+@notify_celery.task(bind=True, name="deliver_sms", max_retries=5, default_retry_delay=5)
 @statsd(namespace="tasks")
-def send_sms_to_provider(self, service_id, notification_id):
+def deliver_sms(self, notification_id):
-    service = dao_fetch_service_by_id(service_id)
-    provider = provider_to_use(SMS_TYPE, notification_id)
-    notification = get_notification_by_id(notification_id)
-    if notification.status == 'created':
-        template_model = dao_get_template_by_id(notification.template_id, notification.template_version)
-        template = Template(
-            template_model.__dict__,
-            values={} if not notification.personalisation else notification.personalisation,
-            renderer=SMSMessage(prefix=service.name, sender=service.sms_sender)
-        )
     try:
-            if service.research_mode or notification.key_type == KEY_TYPE_TEST:
+        notification = notifications_dao.get_notification_by_id(notification_id)
-                send_sms_response.apply_async(
+        if not notification:
-                    (provider.get_name(), str(notification_id), notification.to), queue='research-mode'
+            raise NoResultFound()
-                )
+        send_to_providers.send_sms_to_provider(notification)
-                notification.billable_units = 0
+    except Exception as e:
-            else:
-                provider.send_sms(
-                    to=validate_and_format_phone_number(notification.to),
-                    content=template.replaced,
-                    reference=str(notification_id),
-                    sender=service.sms_sender
-                )
-                notification.billable_units = get_sms_fragment_count(template.replaced_content_count)
-
-            notification.sent_at = datetime.utcnow()
-            notification.sent_by = provider.get_name()
-            notification.status = 'sending'
-            dao_update_notification(notification)
-        except SmsClientException as e:
         try:
             current_app.logger.error(
                 "RETRY: SMS notification {} failed".format(notification_id)
@@ -88,78 +50,21 @@ def send_sms_to_provider(self, service_id, notification_id):
             self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries))
         except self.MaxRetriesExceededError:
             current_app.logger.error(
-                    "RETRY FAILED: task send_sms_to_provider failed for notification {}".format(notification.id),
+                "RETRY FAILED: task send_sms_to_provider failed for notification {}".format(notification_id),
                 e
             )
-                update_notification_status_by_id(notification.id, 'technical-failure')
+            update_notification_status_by_id(notification_id, 'technical-failure')
-
-        current_app.logger.info(
-            "SMS {} sent to provider at {}".format(notification_id, notification.sent_at)
-        )
-        delta_milliseconds = (datetime.utcnow() - notification.created_at).total_seconds() * 1000
-        statsd_client.timing("sms.total-time", delta_milliseconds)
 
 
-def provider_to_use(notification_type, notification_id):
+@notify_celery.task(bind=True, name="deliver_email", max_retries=5, default_retry_delay=5)
-    active_providers_in_order = [
-        provider for provider in get_provider_details_by_notification_type(notification_type) if provider.active
-    ]
-
-    if not active_providers_in_order:
-        current_app.logger.error(
-            "{} {} failed as no active providers".format(notification_type, notification_id)
-        )
-        raise Exception("No active {} providers".format(notification_type))
-
-    return clients.get_client_by_name_and_type(active_providers_in_order[0].identifier, notification_type)
-
-
-@notify_celery.task(bind=True, name="send-email-to-provider", max_retries=5, default_retry_delay=5)
 @statsd(namespace="tasks")
-def send_email_to_provider(self, service_id, notification_id):
+def deliver_email(self, notification_id):
-    service = dao_fetch_service_by_id(service_id)
-    provider = provider_to_use(EMAIL_TYPE, notification_id)
-    notification = get_notification_by_id(notification_id)
-    if notification.status == 'created':
     try:
-            template_dict = dao_get_template_by_id(notification.template_id, notification.template_version).__dict__
+        notification = notifications_dao.get_notification_by_id(notification_id)
-
+        if not notification:
-            html_email = Template(
+            raise NoResultFound()
-                template_dict,
+        send_to_providers.send_email_to_provider(notification)
-                values=notification.personalisation,
+    except Exception as e:
-                renderer=get_html_email_renderer(service)
-            )
-
-            plain_text_email = Template(
-                template_dict,
-                values=notification.personalisation,
-                renderer=PlainTextEmail()
-            )
-
-            if service.research_mode or notification.key_type == KEY_TYPE_TEST:
-                reference = str(create_uuid())
-                send_email_response.apply_async(
-                    (provider.get_name(), reference, notification.to), queue='research-mode'
-                )
-                notification.billable_units = 0
-            else:
-                from_address = '"{}" <{}@{}>'.format(service.name, service.email_from,
-                                                     current_app.config['NOTIFY_EMAIL_DOMAIN'])
-                reference = provider.send_email(
-                    from_address,
-                    notification.to,
-                    plain_text_email.replaced_subject,
-                    body=plain_text_email.replaced,
-                    html_body=html_email.replaced,
-                    reply_to_address=service.reply_to_email_address,
-                )
-
-            notification.reference = reference
-            notification.sent_at = datetime.utcnow()
-            notification.sent_by = provider.get_name(),
-            notification.status = 'sending'
-            dao_update_notification(notification)
-        except EmailClientException as e:
         try:
             current_app.logger.error(
                 "RETRY: Email notification {} failed".format(notification_id)
@@ -168,32 +73,53 @@ def send_email_to_provider(self, service_id, notification_id):
             self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries))
         except self.MaxRetriesExceededError:
             current_app.logger.error(
-                    "RETRY FAILED: task send_email_to_provider failed for notification {}".format(notification.id),
+                "RETRY FAILED: task send_email_to_provider failed for notification {}".format(notification_id),
                 e
             )
-                update_notification_status_by_id(notification.id, 'technical-failure')
+            update_notification_status_by_id(notification_id, 'technical-failure')
 
-        current_app.logger.info(
+
-            "Email {} sent to provider at {}".format(notification_id, notification.sent_at)
+@notify_celery.task(bind=True, name="send-sms-to-provider", max_retries=5, default_retry_delay=5)
+@statsd(namespace="tasks")
+def send_sms_to_provider(self, service_id, notification_id):
+    try:
+        notification = notifications_dao.get_notification_by_id(notification_id)
+        if not notification:
+            raise NoResultFound()
+        send_to_providers.send_sms_to_provider(notification)
+    except Exception as e:
+        try:
+            current_app.logger.error(
+                "RETRY: SMS notification {} failed".format(notification_id)
             )
-        delta_milliseconds = (datetime.utcnow() - notification.created_at).total_seconds() * 1000
+            current_app.logger.exception(e)
-        statsd_client.timing("email.total-time", delta_milliseconds)
+            self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries))
-
+        except self.MaxRetriesExceededError:
-
+            current_app.logger.error(
-def get_html_email_renderer(service):
+                "RETRY FAILED: task send_sms_to_provider failed for notification {}".format(notification_id),
-    govuk_banner = service.branding != BRANDING_ORG
+                e
-    if service.organisation:
-        logo = '{}{}{}'.format(
-            current_app.config['ADMIN_BASE_URL'],
-            current_app.config['BRANDING_PATH'],
-            service.organisation.logo
             )
-        branding = {
+            update_notification_status_by_id(notification_id, 'technical-failure')
-            'brand_colour': service.organisation.colour,
-            'brand_logo': logo,
-            'brand_name': service.organisation.name,
-        }
-    else:
-        branding = {}
 
-    return HTMLEmail(govuk_banner=govuk_banner, **branding)
+
+@notify_celery.task(bind=True, name="send-email-to-provider", max_retries=5, default_retry_delay=5)
+@statsd(namespace="tasks")
+def send_email_to_provider(self, service_id, notification_id):
+    try:
+        notification = notifications_dao.get_notification_by_id(notification_id)
+        if not notification:
+            raise NoResultFound()
+        send_to_providers.send_email_to_provider(notification)
+    except Exception as e:
+        try:
+            current_app.logger.error(
+                "RETRY: Email notification {} failed".format(notification_id)
+            )
+            current_app.logger.exception(e)
+            self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries))
+        except self.MaxRetriesExceededError:
+            current_app.logger.error(
+                "RETRY FAILED: task send_email_to_provider failed for notification {}".format(notification_id),
+                e
+            )
+            update_notification_status_by_id(notification_id, 'technical-failure')

app/clients/sms/__init__.py

@@ -1,11 +1,16 @@
 from app.clients import (Client, ClientException)
 
 
-class SmsClientException(ClientException):
+class SmsClientResponseException(ClientException):
     '''
-    Base Exception for SmsClients
+    Base Exception for SmsClientsResponses
     '''
-    pass
+
+    def __init__(self, message):
+        self.message = message
+
+    def __str__(self):
+        return "Message {}".format(self.message)
 
 
 class SmsClient(Client):

app/clients/sms/firetext.py

@@ -1,12 +1,10 @@
+import json
 import logging
 
 from monotonic import monotonic
-from requests import request, RequestException, HTTPError
+from requests import request, RequestException
 
-from app.clients.sms import (
+from app.clients.sms import (SmsClient, SmsClientResponseException)
-    SmsClient,
-    SmsClientException
-)
 from app.clients import STATISTICS_DELIVERED, STATISTICS_FAILURE
 
 logger = logging.getLogger(__name__)
@@ -42,13 +40,14 @@ def get_firetext_responses(status):
     return firetext_responses[status]
 
 
-class FiretextClientException(SmsClientException):
+class FiretextClientResponseException(SmsClientResponseException):
-    def __init__(self, response):
+    def __init__(self, response, exception):
-        self.code = response['code']
+        self.status_code = response.status_code
-        self.description = response['description']
+        self.text = response.text
+        self.exception = exception
 
     def __str__(self):
-        return "Code {} description {}".format(self.code, self.description)
+        return "Code {} text {} exception {}".format(self.status_code, self.text, str(self.exception))
 
 
 class FiretextClient(SmsClient):
@@ -62,11 +61,27 @@ class FiretextClient(SmsClient):
         self.api_key = current_app.config.get('FIRETEXT_API_KEY')
         self.from_number = current_app.config.get('FROM_NUMBER')
         self.name = 'firetext'
+        self.url = "https://www.firetext.co.uk/api/sendsms/json"
         self.statsd_client = statsd_client
 
     def get_name(self):
         return self.name
 
+    def record_outcome(self, success, response):
+        log_message = "API {} request {} on {} response status_code {}".format(
+            "POST",
+            "succeeded" if success else "failed",
+            self.url,
+            response.status_code
+        )
+
+        if success:
+            self.current_app.logger.info(log_message)
+            self.statsd_client.incr("clients.firetext.success")
+        else:
+            self.statsd_client.incr("clients.firetext.error")
+            self.current_app.logger.error(log_message)
+
     def send_sms(self, to, content, reference, sender=None):
 
         data = {
@@ -81,36 +96,23 @@ class FiretextClient(SmsClient):
         try:
             response = request(
                 "POST",
-                "https://www.firetext.co.uk/api/sendsms/json",
+                self.url,
                 data=data
             )
-            firetext_response = response.json()
-            if firetext_response['code'] != 0:
-                raise FiretextClientException(firetext_response)
             response.raise_for_status()
-            self.current_app.logger.info(
+            try:
-                "API {} request on {} succeeded with {} '{}'".format(
+                json.loads(response.text)
-                    "POST",
+                if response.json()['code'] != 0:
-                    "https://www.firetext.co.uk/api/sendsms",
+                    raise ValueError()
-                    response.status_code,
+            except (ValueError, AttributeError) as e:
-                    firetext_response.items()
+                self.record_outcome(False, response)
-                )
+                raise FiretextClientResponseException(response=response, exception=e)
-            )
+            self.record_outcome(True, response)
         except RequestException as e:
-            api_error = HTTPError.create(e)
+            self.record_outcome(False, e.response)
-            logger.error(
+            raise FiretextClientResponseException(response=e.response, exception=e)
-                "API {} request on {} failed with {} '{}'".format(
-                    "POST",
-                    "https://www.firetext.co.uk/api/sendsms",
-                    api_error.status_code,
-                    api_error.message
-                )
-            )
-            self.statsd_client.incr("clients.firetext.error")
-            raise api_error
         finally:
             elapsed_time = monotonic() - start_time
             self.current_app.logger.info("Firetext request finished in {}".format(elapsed_time))
-            self.statsd_client.incr("clients.firetext.success")
             self.statsd_client.timing("clients.firetext.request-time", elapsed_time)
         return response

app/clients/sms/loadtesting.py

@@ -1,4 +1,7 @@
 import logging
+
+from flask import current_app
+
 from app.clients.sms.firetext import (
     FiretextClient
 )
@@ -13,7 +16,9 @@ class LoadtestingClient(FiretextClient):
 
     def init_app(self, config, statsd_client, *args, **kwargs):
         super(FiretextClient, self).__init__(*args, **kwargs)
+        self.current_app = current_app
         self.api_key = config.config.get('LOADTESTING_API_KEY')
-        self.from_number = config.config.get('LOADTESTING_NUMBER')
+        self.from_number = config.config.get('FROM_NUMBER')
         self.name = 'loadtesting'
+        self.url = "https://www.firetext.co.uk/api/sendsms/json"
         self.statsd_client = statsd_client

app/clients/sms/mmg.py

@@ -1,8 +1,9 @@
 import json
 from monotonic import monotonic
-from requests import (request, RequestException, HTTPError)
+from requests import (request, RequestException)
+
 from app.clients import (STATISTICS_DELIVERED, STATISTICS_FAILURE)
-from app.clients.sms import (SmsClient, SmsClientException)
+from app.clients.sms import (SmsClient, SmsClientResponseException)
 
 mmg_response_map = {
     '2': {
@@ -42,13 +43,14 @@ def get_mmg_responses(status):
     return mmg_response_map.get(status, mmg_response_map.get('default'))
 
 
-class MMGClientException(SmsClientException):
+class MMGClientResponseException(SmsClientResponseException):
-    def __init__(self, error_response):
+    def __init__(self, response, exception):
-        self.code = error_response['Error']
+        self.status_code = response.status_code
-        self.description = error_response['Description']
+        self.text = response.text
+        self.exception = exception
 
     def __str__(self):
-        return "Code {} description {}".format(self.code, self.description)
+        return "Code {} text {} exception {}".format(self.status_code, self.text, str(self.exception))
 
 
 class MMGClient(SmsClient):
@@ -65,6 +67,21 @@ class MMGClient(SmsClient):
         self.statsd_client = statsd_client
         self.mmg_url = current_app.config.get('MMG_URL')
 
+    def record_outcome(self, success, response):
+        log_message = "API {} request {} on {} response status_code {}".format(
+            "POST",
+            "succeeded" if success else "failed",
+            self.mmg_url,
+            response.status_code
+        )
+
+        if success:
+            self.current_app.logger.info(log_message)
+            self.statsd_client.incr("clients.mmg.success")
+        else:
+            self.statsd_client.incr("clients.mmg.error")
+            self.current_app.logger.error(log_message)
+
     def get_name(self):
         return self.name
 
@@ -79,38 +96,30 @@ class MMGClient(SmsClient):
         }
 
         start_time = monotonic()
-
         try:
-            response = request("POST", self.mmg_url,
+            response = request(
+                "POST",
+                self.mmg_url,
                 data=json.dumps(data),
-                               headers={'Content-Type': 'application/json',
+                headers={
-                                        'Authorization': 'Basic {}'.format(self.api_key)})
+                    'Content-Type': 'application/json',
-            if response.status_code != 200:
+                    'Authorization': 'Basic {}'.format(self.api_key)
-                raise MMGClientException(response.json())
+                }
+            )
+
             response.raise_for_status()
-            self.current_app.logger.info(
+            try:
-                "API {} request on {} succeeded with {} '{}'".format(
+                json.loads(response.text)
-                    "POST",
+            except (ValueError, AttributeError) as e:
-                    self.mmg_url,
+                self.record_outcome(False, response)
-                    response.status_code,
+                raise MMGClientResponseException(response=response, exception=e)
-                    response.json().items()
+            self.record_outcome(True, response)
-                )
-            )
         except RequestException as e:
-            api_error = HTTPError.create(e)
+            self.record_outcome(False, e.response)
-            self.current_app.logger.error(
+            raise MMGClientResponseException(response=e.response, exception=e)
-                "API {} request on {} failed with {} '{}'".format(
-                    "POST",
-                    self.mmg_url,
-                    api_error.status_code,
-                    api_error.message
-                )
-            )
-            self.statsd_client.incr("clients.mmg.error")
-            raise api_error
         finally:
             elapsed_time = monotonic() - start_time
             self.statsd_client.timing("clients.mmg.request-time", elapsed_time)
-            self.statsd_client.incr("clients.mmg.success")
             self.current_app.logger.info("MMG request finished in {}".format(elapsed_time))
+
         return response

app/delivery/rest.py

@@ -0,0 +1,46 @@
+from flask import Blueprint, jsonify
+
+from app.delivery import send_to_providers
+from app.models import EMAIL_TYPE
+from app.celery import provider_tasks
+from app.dao import notifications_dao
+from flask import current_app
+
+delivery_blueprint = Blueprint('delivery', __name__)
+
+from app.errors import register_errors
+
+register_errors(delivery_blueprint)
+
+
+@delivery_blueprint.route('/deliver/notification/<uuid:notification_id>', methods=['POST'])
+def send_notification_to_provider(notification_id):
+    notification = notifications_dao.get_notification_by_id(notification_id)
+    if not notification:
+        return jsonify({"result": "error", "message": "No result found"}), 404
+
+    if notification.notification_type == EMAIL_TYPE:
+        send_response(
+            send_to_providers.send_email_to_provider,
+            provider_tasks.deliver_email,
+            notification,
+            'send-email')
+    else:
+        send_response(
+            send_to_providers.send_sms_to_provider,
+            provider_tasks.deliver_sms,
+            notification,
+            'send-sms')
+    return jsonify({}), 204
+
+
+def send_response(send_call, task_call, notification, queue):
+    try:
+        send_call(notification)
+    except Exception as e:
+        current_app.logger.exception(
+            "Failed to send notification, retrying in celery. ID {} type {}".format(
+                notification.id,
+                notification.notification_type),
+            e)
+        task_call.apply_async((str(notification.id)), queue=queue)

app/delivery/send_to_providers.py

@@ -0,0 +1,135 @@
+from datetime import datetime
+
+from flask import current_app
+from notifications_utils.recipients import (
+    validate_and_format_phone_number
+)
+from notifications_utils.template import Template, get_sms_fragment_count
+from notifications_utils.renderers import HTMLEmail, PlainTextEmail, SMSMessage
+
+from app import clients, statsd_client, create_uuid
+from app.dao.notifications_dao import dao_update_notification
+from app.dao.provider_details_dao import get_provider_details_by_notification_type
+from app.dao.services_dao import dao_fetch_service_by_id
+from app.celery.research_mode_tasks import send_sms_response, send_email_response
+from app.dao.templates_dao import dao_get_template_by_id
+
+from app.models import SMS_TYPE, KEY_TYPE_TEST, BRANDING_ORG, EMAIL_TYPE
+
+
+def send_sms_to_provider(notification):
+    service = dao_fetch_service_by_id(notification.service_id)
+    provider = provider_to_use(SMS_TYPE, notification.id)
+    if notification.status == 'created':
+        template_model = dao_get_template_by_id(notification.template_id, notification.template_version)
+        template = Template(
+            template_model.__dict__,
+            values={} if not notification.personalisation else notification.personalisation,
+            renderer=SMSMessage(prefix=service.name, sender=service.sms_sender)
+        )
+        if service.research_mode or notification.key_type == KEY_TYPE_TEST:
+            send_sms_response.apply_async(
+                (provider.get_name(), str(notification.id), notification.to), queue='research-mode'
+            )
+            notification.billable_units = 0
+        else:
+            provider.send_sms(
+                to=validate_and_format_phone_number(notification.to),
+                content=template.replaced,
+                reference=str(notification.id),
+                sender=service.sms_sender
+            )
+            notification.billable_units = get_sms_fragment_count(template.replaced_content_count)
+
+        notification.sent_at = datetime.utcnow()
+        notification.sent_by = provider.get_name()
+        notification.status = 'sending'
+        dao_update_notification(notification)
+
+        current_app.logger.info(
+            "SMS {} sent to provider at {}".format(notification.id, notification.sent_at)
+        )
+        delta_milliseconds = (datetime.utcnow() - notification.created_at).total_seconds() * 1000
+        statsd_client.timing("sms.total-time", delta_milliseconds)
+
+
+def send_email_to_provider(notification):
+    service = dao_fetch_service_by_id(notification.service_id)
+    provider = provider_to_use(EMAIL_TYPE, notification.id)
+    if notification.status == 'created':
+        template_dict = dao_get_template_by_id(notification.template_id, notification.template_version).__dict__
+
+        html_email = Template(
+            template_dict,
+            values=notification.personalisation,
+            renderer=get_html_email_renderer(service)
+        )
+
+        plain_text_email = Template(
+            template_dict,
+            values=notification.personalisation,
+            renderer=PlainTextEmail()
+        )
+
+        if service.research_mode or notification.key_type == KEY_TYPE_TEST:
+            reference = str(create_uuid())
+            send_email_response.apply_async(
+                (provider.get_name(), reference, notification.to), queue='research-mode'
+            )
+            notification.billable_units = 0
+        else:
+            from_address = '"{}" <{}@{}>'.format(service.name, service.email_from,
+                                                 current_app.config['NOTIFY_EMAIL_DOMAIN'])
+            reference = provider.send_email(
+                from_address,
+                notification.to,
+                plain_text_email.replaced_subject,
+                body=plain_text_email.replaced,
+                html_body=html_email.replaced,
+                reply_to_address=service.reply_to_email_address,
+            )
+
+        notification.reference = reference
+        notification.sent_at = datetime.utcnow()
+        notification.sent_by = provider.get_name(),
+        notification.status = 'sending'
+        dao_update_notification(notification)
+
+        current_app.logger.info(
+            "Email {} sent to provider at {}".format(notification.id, notification.sent_at)
+        )
+        delta_milliseconds = (datetime.utcnow() - notification.created_at).total_seconds() * 1000
+        statsd_client.timing("email.total-time", delta_milliseconds)
+
+
+def provider_to_use(notification_type, notification_id):
+    active_providers_in_order = [
+        provider for provider in get_provider_details_by_notification_type(notification_type) if provider.active
+    ]
+
+    if not active_providers_in_order:
+        current_app.logger.error(
+            "{} {} failed as no active providers".format(notification_type, notification_id)
+        )
+        raise Exception("No active {} providers".format(notification_type))
+
+    return clients.get_client_by_name_and_type(active_providers_in_order[0].identifier, notification_type)
+
+
+def get_html_email_renderer(service):
+    govuk_banner = service.branding != BRANDING_ORG
+    if service.organisation:
+        logo = '{}{}{}'.format(
+            current_app.config['ADMIN_BASE_URL'],
+            current_app.config['BRANDING_PATH'],
+            service.organisation.logo
+        )
+        branding = {
+            'brand_colour': service.organisation.colour,
+            'brand_logo': logo,
+            'brand_name': service.organisation.name,
+        }
+    else:
+        branding = {}
+
+    return HTMLEmail(govuk_banner=govuk_banner, **branding)

tests/app/celery/test_provider_tasks.py

@@ -1,28 +1,10 @@
-import uuid
-from datetime import datetime
-
-import pytest
 from celery.exceptions import MaxRetriesExceededError
-from unittest.mock import ANY
-from notifications_utils.recipients import validate_phone_number, format_phone_number
-
-import app
-from app import mmg_client
 from app.celery import provider_tasks
-from app.celery.provider_tasks import send_sms_to_provider, send_email_to_provider
+from app.celery.provider_tasks import send_sms_to_provider, send_email_to_provider, deliver_sms, deliver_email
-from app.celery.research_mode_tasks import send_sms_response, send_email_response
 from app.clients.email import EmailClientException
-from app.clients.sms import SmsClientException
+from app.models import Notification
-from app.dao import notifications_dao, provider_details_dao
-from app.models import (
-    Notification,
-    Organisation,
-    KEY_TYPE_NORMAL,
-    KEY_TYPE_TEST,
-    BRANDING_ORG,
-    BRANDING_BOTH,
-    KEY_TYPE_TEAM)
 from tests.app.conftest import sample_notification
+import app
 
 
 def test_should_have_decorated_tasks_functions():
@@ -58,250 +40,119 @@ def test_should_by_240_minute_delay_on_retry_two():
     assert provider_tasks.retry_iteration_to_delay(4) == 14400
 
 
-def test_should_return_highest_priority_active_provider(notify_db, notify_db_session):
+def test_should_call_send_sms_to_provider_from_deliver_sms_task(
-    providers = provider_details_dao.get_provider_details_by_notification_type('sms')
-
-    first = providers[0]
-    second = providers[1]
-
-    assert provider_tasks.provider_to_use('sms', '1234').name == first.identifier
-
-    first.priority = 20
-    second.priority = 10
-
-    provider_details_dao.dao_update_provider_details(first)
-    provider_details_dao.dao_update_provider_details(second)
-
-    assert provider_tasks.provider_to_use('sms', '1234').name == second.identifier
-
-    first.priority = 10
-    first.active = False
-    second.priority = 20
-
-    provider_details_dao.dao_update_provider_details(first)
-    provider_details_dao.dao_update_provider_details(second)
-
-    assert provider_tasks.provider_to_use('sms', '1234').name == second.identifier
-
-    first.active = True
-    provider_details_dao.dao_update_provider_details(first)
-
-    assert provider_tasks.provider_to_use('sms', '1234').name == first.identifier
-
-
-def test_should_send_personalised_template_to_correct_sms_provider_and_persist(
         notify_db,
         notify_db_session,
-    sample_template_with_placeholders,
-    mocker
-):
-    db_notification = sample_notification(notify_db, notify_db_session, template=sample_template_with_placeholders,
-                                          to_field="+447234123123", personalisation={"name": "Jo"},
-                                          status='created')
-
-    mocker.patch('app.mmg_client.send_sms')
-    mocker.patch('app.mmg_client.get_name', return_value="mmg")
-
-    send_sms_to_provider(
-        db_notification.service_id,
-        db_notification.id
-    )
-
-    mmg_client.send_sms.assert_called_once_with(
-        to=format_phone_number(validate_phone_number("+447234123123")),
-        content="Sample service: Hello Jo\nYour thing is due soon",
-        reference=str(db_notification.id),
-        sender=None
-    )
-    notification = Notification.query.filter_by(id=db_notification.id).one()
-
-    assert notification.status == 'sending'
-    assert notification.sent_at <= datetime.utcnow()
-    assert notification.sent_by == 'mmg'
-    assert notification.billable_units == 1
-    assert notification.personalisation == {"name": "Jo"}
-
-
-def test_should_send_personalised_template_to_correct_email_provider_and_persist(
-    notify_db,
-    notify_db_session,
-    sample_email_template_with_placeholders,
-    mocker
-):
-    db_notification = sample_notification(
-        notify_db=notify_db, notify_db_session=notify_db_session,
-        template=sample_email_template_with_placeholders,
-        to_field="jo.smith@example.com",
-        personalisation={'name': 'Jo'}
-    )
-
-    mocker.patch('app.aws_ses_client.send_email', return_value='reference')
-    mocker.patch('app.aws_ses_client.get_name', return_value="ses")
-
-    send_email_to_provider(
-        db_notification.service_id,
-        db_notification.id
-    )
-
-    app.aws_ses_client.send_email.assert_called_once_with(
-        '"Sample service" <sample.service@test.notify.com>',
-        'jo.smith@example.com',
-        'Jo',
-        body='Hello Jo\nThis is an email from GOV.\u200bUK',
-        html_body=ANY,
-        reply_to_address=None
-    )
-    assert '<!DOCTYPE html' in app.aws_ses_client.send_email.call_args[1]['html_body']
-
-    notification = Notification.query.filter_by(id=db_notification.id).one()
-
-    assert notification.status == 'sending'
-    assert notification.sent_at <= datetime.utcnow()
-    assert notification.sent_by == 'ses'
-    assert notification.personalisation == {"name": "Jo"}
-
-
-def test_send_sms_should_use_template_version_from_notification_not_latest(
-        notify_db,
-        notify_db_session,
-        sample_template,
-        mocker):
-    db_notification = sample_notification(notify_db, notify_db_session,
-                                          template=sample_template, to_field='+447234123123',
-                                          status='created')
-
-    mocker.patch('app.mmg_client.send_sms')
-    mocker.patch('app.mmg_client.get_name', return_value="mmg")
-    version_on_notification = sample_template.version
-
-    # Change the template
-    from app.dao.templates_dao import dao_update_template, dao_get_template_by_id
-    sample_template.content = sample_template.content + " another version of the template"
-    dao_update_template(sample_template)
-    t = dao_get_template_by_id(sample_template.id)
-    assert t.version > version_on_notification
-
-    send_sms_to_provider(
-        db_notification.service_id,
-        db_notification.id
-    )
-
-    mmg_client.send_sms.assert_called_once_with(
-        to=format_phone_number(validate_phone_number("+447234123123")),
-        content="Sample service: This is a template:\nwith a newline",
-        reference=str(db_notification.id),
-        sender=None
-    )
-
-    persisted_notification = notifications_dao.get_notification_by_id(db_notification.id)
-    assert persisted_notification.to == db_notification.to
-    assert persisted_notification.template_id == sample_template.id
-    assert persisted_notification.template_version == version_on_notification
-    assert persisted_notification.template_version != sample_template.version
-    assert persisted_notification.status == 'sending'
-    assert not persisted_notification.personalisation
-
-
-@pytest.mark.parametrize('research_mode,key_type', [
-    (True, KEY_TYPE_NORMAL),
-    (False, KEY_TYPE_TEST)
-])
-def test_should_call_send_sms_response_task_if_research_mode(notify_db, sample_service, sample_notification, mocker,
-                                                             research_mode, key_type):
-    mocker.patch('app.mmg_client.send_sms')
-    mocker.patch('app.mmg_client.get_name', return_value="mmg")
-    mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async')
-
-    if research_mode:
-        sample_service.research_mode = True
-        notify_db.session.add(sample_service)
-        notify_db.session.commit()
-
-    sample_notification.key_type = key_type
-
-    send_sms_to_provider(
-        sample_notification.service_id,
-        sample_notification.id
-    )
-    assert not mmg_client.send_sms.called
-    send_sms_response.apply_async.assert_called_once_with(
-        ('mmg', str(sample_notification.id), sample_notification.to), queue='research-mode'
-    )
-
-    persisted_notification = notifications_dao.get_notification_by_id(sample_notification.id)
-    assert persisted_notification.to == sample_notification.to
-    assert persisted_notification.template_id == sample_notification.template_id
-    assert persisted_notification.status == 'sending'
-    assert persisted_notification.sent_at <= datetime.utcnow()
-    assert persisted_notification.sent_by == 'mmg'
-    assert not persisted_notification.personalisation
-
-
-@pytest.mark.parametrize('research_mode,key_type, billable_units', [
-    (True, KEY_TYPE_NORMAL, 0),
-    (False, KEY_TYPE_NORMAL, 1),
-    (False, KEY_TYPE_TEST, 0),
-    (True, KEY_TYPE_TEST, 0),
-    (True, KEY_TYPE_TEAM, 0),
-    (False, KEY_TYPE_TEAM, 1)
-])
-def test_should_update_billable_units_according_to_research_mode_and_key_type(notify_db,
-                                                                              sample_service,
         sample_notification,
-                                                                              mocker,
-                                                                              research_mode,
-                                                                              key_type,
-                                                                              billable_units):
-
-    assert Notification.query.count() == 1
-
-    mocker.patch('app.mmg_client.send_sms')
-    mocker.patch('app.mmg_client.get_name', return_value="mmg")
-    mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async')
-    if research_mode:
-        sample_service.research_mode = True
-        notify_db.session.add(sample_service)
-        notify_db.session.commit()
-    sample_notification.key_type = key_type
-
-    send_sms_to_provider(
-        sample_notification.service_id,
-        sample_notification.id
-    )
-
-    assert Notification.query.get(sample_notification.id).billable_units == billable_units, \
-        "Research mode: {0}, key type: {1}, billable_units: {2}".format(research_mode, key_type, billable_units)
-
-
-def test_should_not_send_to_provider_when_status_is_not_created(notify_db, notify_db_session,
-                                                                sample_service,
         mocker):
-    notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session,
+    mocker.patch('app.delivery.send_to_providers.send_sms_to_provider')
-                                       service=sample_service,
-                                       status='sending')
-    mocker.patch('app.mmg_client.send_sms')
-    mocker.patch('app.mmg_client.get_name', return_value="mmg")
-    mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async')
 
-    send_sms_to_provider(
+    deliver_sms(sample_notification.id)
-        notification.service_id,
+    app.delivery.send_to_providers.send_sms_to_provider.assert_called_with(sample_notification)
-        notification.id
-    )
-
-    app.mmg_client.send_sms.assert_not_called()
-    app.celery.research_mode_tasks.send_sms_response.apply_async.assert_not_called()
 
 
-def test_should_go_into_technical_error_if_exceeds_retries(
+def test_should_add_to_retry_queue_if_notification_not_found_in_deliver_sms_task(
+        notify_db,
+        notify_db_session,
+        mocker):
+    mocker.patch('app.delivery.send_to_providers.send_sms_to_provider')
+    mocker.patch('app.celery.provider_tasks.deliver_sms.retry')
+
+    notification_id = app.create_uuid()
+
+    deliver_sms(notification_id)
+    app.delivery.send_to_providers.send_sms_to_provider.assert_not_called()
+    app.celery.provider_tasks.deliver_sms.retry.assert_called_with(queue="retry", countdown=10)
+
+
+def test_should_call_send_sms_to_provider_from_send_sms_to_provider_task(
+        notify_db,
+        notify_db_session,
+        sample_notification,
+        mocker):
+    mocker.patch('app.delivery.send_to_providers.send_sms_to_provider')
+
+    send_sms_to_provider(sample_notification.service_id, sample_notification.id)
+    app.delivery.send_to_providers.send_sms_to_provider.assert_called_with(sample_notification)
+
+
+def test_should_add_to_retry_queue_if_notification_not_found_in_send_sms_to_provider_task(
+        notify_db,
+        notify_db_session,
+        mocker):
+    mocker.patch('app.delivery.send_to_providers.send_sms_to_provider')
+    mocker.patch('app.celery.provider_tasks.send_sms_to_provider.retry')
+
+    notification_id = app.create_uuid()
+    service_id = app.create_uuid()
+
+    send_sms_to_provider(service_id, notification_id)
+    app.delivery.send_to_providers.send_sms_to_provider.assert_not_called()
+    app.celery.provider_tasks.send_sms_to_provider.retry.assert_called_with(queue="retry", countdown=10)
+
+
+def test_should_call_send_email_to_provider_from_deliver_email_task(
+        notify_db,
+        notify_db_session,
+        sample_notification,
+        mocker):
+    mocker.patch('app.delivery.send_to_providers.send_email_to_provider')
+
+    deliver_email(sample_notification.id)
+    app.delivery.send_to_providers.send_email_to_provider.assert_called_with(sample_notification)
+
+
+def test_should_add_to_retry_queue_if_notification_not_found_in_deliver_email_task(
+        notify_db,
+        notify_db_session,
+        mocker):
+    mocker.patch('app.delivery.send_to_providers.send_email_to_provider')
+    mocker.patch('app.celery.provider_tasks.deliver_email.retry')
+
+    notification_id = app.create_uuid()
+
+    deliver_email(notification_id)
+    app.delivery.send_to_providers.send_email_to_provider.assert_not_called()
+    app.celery.provider_tasks.deliver_email.retry.assert_called_with(queue="retry", countdown=10)
+
+
+def test_should_call_send_email_to_provider_from_email_task(
+        notify_db,
+        notify_db_session,
+        sample_notification,
+        mocker):
+    mocker.patch('app.delivery.send_to_providers.send_email_to_provider')
+
+    send_email_to_provider(sample_notification.service_id, sample_notification.id)
+    app.delivery.send_to_providers.send_email_to_provider.assert_called_with(sample_notification)
+
+
+def test_should_add_to_retry_queue_if_notification_not_found_in_send_email_to_provider_task(
+        notify_db,
+        notify_db_session,
+        mocker):
+    mocker.patch('app.delivery.send_to_providers.send_email_to_provider')
+    mocker.patch('app.celery.provider_tasks.send_email_to_provider.retry')
+
+    notification_id = app.create_uuid()
+    service_id = app.create_uuid()
+
+    send_email_to_provider(service_id, notification_id)
+    app.delivery.send_to_providers.send_email_to_provider.assert_not_called()
+    app.celery.provider_tasks.send_email_to_provider.retry.assert_called_with(queue="retry", countdown=10)
+
+
+# DO THESE FOR THE 4 TYPES OF TASK
+
+def test_should_go_into_technical_error_if_exceeds_retries_on_send_sms_to_provider_task(
         notify_db,
         notify_db_session,
         sample_service,
         mocker):
-
     notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session,
                                        service=sample_service, status='created')
 
-    mocker.patch('app.mmg_client.send_sms', side_effect=SmsClientException("EXPECTED"))
+    mocker.patch('app.delivery.send_to_providers.send_sms_to_provider', side_effect=Exception("EXPECTED"))
     mocker.patch('app.celery.provider_tasks.send_sms_to_provider.retry', side_effect=MaxRetriesExceededError())
 
     send_sms_to_provider(
@@ -315,92 +166,33 @@ def test_should_go_into_technical_error_if_exceeds_retries(
     assert db_notification.status == 'technical-failure'
 
 
-def test_should_send_sms_sender_from_service_if_present(
+def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_sms_task(
         notify_db,
         notify_db_session,
         sample_service,
-        sample_template,
         mocker):
-    db_notification = sample_notification(notify_db, notify_db_session, template=sample_template,
-                                          to_field="+447234123123",
-                                          status='created')
-
-    sample_service.sms_sender = 'elevenchars'
-    notify_db.session.add(sample_service)
-    notify_db.session.commit()
-
-    mocker.patch('app.mmg_client.send_sms')
-    mocker.patch('app.mmg_client.get_name', return_value="mmg")
-
-    send_sms_to_provider(
-        db_notification.service_id,
-        db_notification.id
-    )
-
-    mmg_client.send_sms.assert_called_once_with(
-        to=format_phone_number(validate_phone_number("+447234123123")),
-        content="This is a template:\nwith a newline",
-        reference=str(db_notification.id),
-        sender=sample_service.sms_sender
-    )
-
-
-@pytest.mark.parametrize('research_mode,key_type', [
-    (True, KEY_TYPE_NORMAL),
-    (False, KEY_TYPE_TEST)
-])
-def test_send_email_to_provider_should_call_research_mode_task_response_task_if_research_mode(
-        notify_db,
-        notify_db_session,
-        sample_service,
-        sample_email_template,
-        ses_provider,
-        mocker,
-        research_mode,
-        key_type):
     notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session,
-                                       template=sample_email_template,
+                                       service=sample_service, status='created')
-                                       to_field="john@smith.com",
-                                       key_type=key_type
-                                       )
 
-    reference = uuid.uuid4()
+    mocker.patch('app.delivery.send_to_providers.send_sms_to_provider', side_effect=Exception("EXPECTED"))
-    mocker.patch('app.uuid.uuid4', return_value=reference)
+    mocker.patch('app.celery.provider_tasks.deliver_sms.retry', side_effect=MaxRetriesExceededError())
-    mocker.patch('app.aws_ses_client.send_email')
-    mocker.patch('app.aws_ses_client.get_name', return_value="ses")
-    mocker.patch('app.celery.research_mode_tasks.send_email_response.apply_async')
 
-    if research_mode:
+    deliver_sms(
-        sample_service.research_mode = True
-        notify_db.session.add(sample_service)
-        notify_db.session.commit()
-    send_email_to_provider(
-        sample_service.id,
         notification.id
     )
-    assert not app.aws_ses_client.send_email.called
-    send_email_response.apply_async.assert_called_once_with(
-        ('ses', str(reference), 'john@smith.com'), queue="research-mode"
-    )
-    persisted_notification = Notification.query.filter_by(id=notification.id).one()
 
-    assert persisted_notification.to == 'john@smith.com'
+    provider_tasks.deliver_sms.retry.assert_called_with(queue='retry', countdown=10)
-    assert persisted_notification.template_id == sample_email_template.id
+
-    assert persisted_notification.status == 'sending'
+    db_notification = Notification.query.filter_by(id=notification.id).one()
-    assert persisted_notification.sent_at <= datetime.utcnow()
+    assert db_notification.status == 'technical-failure'
-    assert persisted_notification.created_at <= datetime.utcnow()
-    assert persisted_notification.sent_by == 'ses'
-    assert persisted_notification.reference == str(reference)
-    assert persisted_notification.billable_units == 0
 
 
-def test_send_email_to_provider_should_go_into_technical_error_if_exceeds_retries(
+def test_send_email_to_provider_should_go_into_technical_error_if_exceeds_retries_on_send_email_to_provider_task(
         notify_db,
         notify_db_session,
         sample_service,
         sample_email_template,
         mocker):
-
     notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session,
                                        service=sample_service, status='created', template=sample_email_template)
 
@@ -418,86 +210,22 @@ def test_send_email_to_provider_should_go_into_technical_error_if_exceeds_retrie
     assert db_notification.status == 'technical-failure'
 
 
-def test_send_email_to_provider_should_not_send_to_provider_when_status_is_not_created(notify_db, notify_db_session,
+def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_email_task(
+        notify_db,
+        notify_db_session,
         sample_service,
-                                                                                       sample_email_template,
         mocker):
     notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session,
-                                       template=sample_email_template,
+                                       service=sample_service, status='created')
-                                       service=sample_service,
-                                       status='sending')
-    mocker.patch('app.aws_ses_client.send_email')
-    mocker.patch('app.aws_ses_client.get_name', return_value="ses")
-    mocker.patch('app.celery.research_mode_tasks.send_email_response.apply_async')
 
-    send_sms_to_provider(
+    mocker.patch('app.delivery.send_to_providers.send_email_to_provider', side_effect=Exception("EXPECTED"))
-        notification.service_id,
+    mocker.patch('app.celery.provider_tasks.deliver_email.retry', side_effect=MaxRetriesExceededError())
+
+    deliver_email(
         notification.id
     )
 
-    app.aws_ses_client.send_email.assert_not_called()
+    provider_tasks.deliver_email.retry.assert_called_with(queue='retry', countdown=10)
-    app.celery.research_mode_tasks.send_email_response.apply_async.assert_not_called()
 
-
+    db_notification = Notification.query.filter_by(id=notification.id).one()
-def test_send_email_should_use_service_reply_to_email(
+    assert db_notification.status == 'technical-failure'
-        notify_db, notify_db_session,
-        sample_service,
-        sample_email_template,
-        mocker):
-    mocker.patch('app.aws_ses_client.send_email', return_value='reference')
-    mocker.patch('app.aws_ses_client.get_name', return_value="ses")
-
-    db_notification = sample_notification(notify_db, notify_db_session, template=sample_email_template)
-    sample_service.reply_to_email_address = 'foo@bar.com'
-
-    send_email_to_provider(
-        db_notification.service_id,
-        db_notification.id,
-    )
-
-    app.aws_ses_client.send_email.assert_called_once_with(
-        ANY,
-        ANY,
-        ANY,
-        body=ANY,
-        html_body=ANY,
-        reply_to_address=sample_service.reply_to_email_address
-    )
-
-
-def test_get_html_email_renderer_should_return_for_normal_service(sample_service):
-    renderer = provider_tasks.get_html_email_renderer(sample_service)
-    assert renderer.govuk_banner
-    assert renderer.brand_colour is None
-    assert renderer.brand_logo is None
-    assert renderer.brand_name is None
-
-
-@pytest.mark.parametrize('branding_type, govuk_banner', [
-    (BRANDING_ORG, False),
-    (BRANDING_BOTH, True)
-])
-def test_get_html_email_renderer_with_branding_details(branding_type, govuk_banner, notify_db, sample_service):
-    sample_service.branding = branding_type
-    org = Organisation(colour='#000000', logo='justice-league.png', name='Justice League')
-    sample_service.organisation = org
-    notify_db.session.add_all([sample_service, org])
-    notify_db.session.commit()
-
-    renderer = provider_tasks.get_html_email_renderer(sample_service)
-
-    assert renderer.govuk_banner == govuk_banner
-    assert renderer.brand_colour == '000000'
-    assert renderer.brand_name == 'Justice League'
-
-
-def test_get_html_email_renderer_prepends_logo_path(notify_db, sample_service):
-    sample_service.branding = BRANDING_ORG
-    org = Organisation(colour='#000000', logo='justice-league.png', name='Justice League')
-    sample_service.organisation = org
-    notify_db.session.add_all([sample_service, org])
-    notify_db.session.commit()
-
-    renderer = provider_tasks.get_html_email_renderer(sample_service)
-
-    assert renderer.brand_logo == 'http://localhost:6012/static/images/email-template/crests/justice-league.png'

tests/app/clients/test_firetext.py

@@ -1,9 +1,10 @@
+from requests import HTTPError
 from urllib.parse import parse_qs
 
 import pytest
 import requests_mock
 
-from app.clients.sms.firetext import (get_firetext_responses, FiretextClientException)
+from app.clients.sms.firetext import get_firetext_responses, SmsClientResponseException
 
 
 def test_should_return_correct_details_for_delivery():
@@ -88,12 +89,26 @@ def test_send_sms_raises_if_firetext_rejects(mocker, mock_firetext_client):
         'responseData': ''
     }
 
-    with pytest.raises(FiretextClientException) as exc, requests_mock.Mocker() as request_mock:
+    with pytest.raises(SmsClientResponseException) as exc, requests_mock.Mocker() as request_mock:
         request_mock.post('https://www.firetext.co.uk/api/sendsms/json', json=response_dict, status_code=200)
         mock_firetext_client.send_sms(to, content, reference)
 
-    assert exc.value.code == 1
+    assert exc.value.status_code == 200
-    assert exc.value.description == 'Some kind of error'
+    assert '"description": "Some kind of error"' in exc.value.text
+    assert '"code": 1' in exc.value.text
+
+
+def test_send_sms_raises_if_firetext_rejects(mocker, mock_firetext_client):
+    to = content = reference = 'foo'
+    response_dict = {"something": "gone bad"}
+
+    with pytest.raises(SmsClientResponseException) as exc, requests_mock.Mocker() as request_mock:
+        request_mock.post('https://www.firetext.co.uk/api/sendsms/json', json=response_dict, status_code=400)
+        mock_firetext_client.send_sms(to, content, reference)
+
+    assert exc.value.status_code == 400
+    assert exc.value.text == '{"something": "gone bad"}'
+    assert type(exc.value.exception) == HTTPError
 
 
 def test_send_sms_override_configured_shortcode_with_sender(mocker, mock_firetext_client):

tests/app/clients/test_mmg.py

@@ -1,8 +1,13 @@
+import json
+
 import pytest
 import requests_mock
-from app import mmg_client
+from requests import HTTPError
 
-from app.clients.sms.mmg import (get_mmg_responses, MMGClientException)
+from app import mmg_client
+from app.clients.sms import SmsClientResponseException
+
+from app.clients.sms.mmg import get_mmg_responses
 
 
 def test_should_return_correct_details_for_delivery():
@@ -72,12 +77,14 @@ def test_send_sms_raises_if_mmg_rejects(notify_api, mocker):
         'Description': 'Some kind of error'
     }
 
-    with pytest.raises(MMGClientException) as exc, requests_mock.Mocker() as request_mock:
+    with pytest.raises(SmsClientResponseException) as exc, requests_mock.Mocker() as request_mock:
         request_mock.post('https://api.mmg.co.uk/json/api.php', json=response_dict, status_code=400)
         mmg_client.send_sms(to, content, reference)
 
-    assert exc.value.code == 206
+    assert exc.value.status_code == 400
-    assert exc.value.description == 'Some kind of error'
+    assert '"Error": 206' in exc.value.text
+    assert '"Description": "Some kind of error"' in exc.value.text
+    assert type(exc.value.exception) == HTTPError
 
 
 def test_send_sms_override_configured_shortcode_with_sender(notify_api, mocker):
@@ -93,3 +100,16 @@ def test_send_sms_override_configured_shortcode_with_sender(notify_api, mocker):
 
     request_args = request_mock.request_history[0].json()
     assert request_args['sender'] == 'fromservice'
+
+
+def test_send_sms_raises_if_mmg_fails_to_return_json(notify_api, mocker):
+    to = content = reference = 'foo'
+    response_dict = 'NOT AT ALL VALID JSON {"key" : "value"}}'
+
+    with pytest.raises(SmsClientResponseException) as exc, requests_mock.Mocker() as request_mock:
+        request_mock.post('https://api.mmg.co.uk/json/api.php', text=response_dict, status_code=200)
+        mmg_client.send_sms(to, content, reference)
+
+    assert 'app.clients.sms.mmg.MMGClientResponseException: Code 200 text NOT AT ALL VALID JSON {"key" : "value"}} exception Expecting value: line 1 column 1 (char 0)' in str(exc)  # noqa
+    assert exc.value.status_code == 200
+    assert exc.value.text == 'NOT AT ALL VALID JSON {"key" : "value"}}'

tests/app/delivery/test_rest.py

@@ -0,0 +1,105 @@
+from flask import json
+
+import app
+from tests import create_authorization_header
+
+
+def test_should_reject_if_not_authenticated(notify_api):
+    with notify_api.test_request_context():
+        with notify_api.test_client() as client:
+            response = client.post('/deliver/notification/{}'.format(app.create_uuid()))
+            assert response.status_code == 401
+
+
+def test_should_reject_if_invalid_uuid(notify_api):
+    with notify_api.test_request_context():
+        with notify_api.test_client() as client:
+            auth = create_authorization_header()
+            response = client.post(
+                '/deliver/notification/{}',
+                headers=[auth]
+            )
+        body = json.loads(response.get_data(as_text=True))
+        assert response.status_code == 404
+        assert body['message'] == 'The requested URL was not found on the server.  If you entered the URL manually please check your spelling and try again.'  # noqa
+        assert body['result'] == 'error'
+
+
+def test_should_reject_if_notification_id_cannot_be_found(notify_api):
+    with notify_api.test_request_context():
+        with notify_api.test_client() as client:
+            auth = create_authorization_header()
+            response = client.post(
+                '/deliver/notification/{}'.format(app.create_uuid()),
+                headers=[auth]
+            )
+        body = json.loads(response.get_data(as_text=True))
+        assert response.status_code == 404
+        assert body['message'] == 'No result found'
+        assert body['result'] == 'error'
+
+
+def test_should_call_send_sms_to_provider_as_primary(notify_api, sample_notification, mocker):
+    mocker.patch('app.delivery.send_to_providers.send_sms_to_provider')
+    with notify_api.test_request_context():
+        with notify_api.test_client() as client:
+            auth = create_authorization_header()
+            response = client.post(
+                '/deliver/notification/{}'.format(sample_notification.id),
+                headers=[auth]
+            )
+            app.delivery.send_to_providers.send_sms_to_provider.assert_called_with(sample_notification)
+            assert response.status_code == 204
+
+
+def test_should_call_send_email_to_provider_as_primary(notify_api, sample_email_notification, mocker):
+    mocker.patch('app.delivery.send_to_providers.send_email_to_provider')
+    with notify_api.test_request_context():
+        with notify_api.test_client() as client:
+            auth = create_authorization_header()
+            response = client.post(
+                '/deliver/notification/{}'.format(sample_email_notification.id),
+                headers=[auth]
+            )
+        app.delivery.send_to_providers.send_email_to_provider.assert_called_with(sample_email_notification)
+        assert response.status_code == 204
+
+
+def test_should_call_deliver_sms_task_if_send_sms_to_provider_fails(notify_api, sample_notification, mocker):
+    mocker.patch('app.delivery.send_to_providers.send_sms_to_provider', side_effect=Exception())
+    mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async')
+
+    with notify_api.test_request_context():
+        with notify_api.test_client() as client:
+            auth = create_authorization_header()
+            response = client.post(
+                '/deliver/notification/{}'.format(sample_notification.id),
+                headers=[auth]
+            )
+        app.delivery.send_to_providers.send_sms_to_provider.assert_called_with(sample_notification)
+        app.celery.provider_tasks.deliver_sms.apply_async.assert_called_with(
+            (str(sample_notification.id)), queue='send-sms'
+        )
+        assert response.status_code == 204
+
+
+def test_should_call_deliver_email_task_if_send_email_to_provider_fails(
+        notify_api,
+        sample_email_notification,
+        mocker
+):
+    mocker.patch('app.delivery.send_to_providers.send_email_to_provider', side_effect=Exception())
+    mocker.patch('app.celery.provider_tasks.deliver_email.apply_async')
+
+    with notify_api.test_request_context():
+        with notify_api.test_client() as client:
+            auth = create_authorization_header()
+            response = client.post(
+                '/deliver/notification/{}'.format(sample_email_notification.id),
+                headers=[auth]
+            )
+        app.delivery.send_to_providers.send_email_to_provider.assert_called_with(sample_email_notification)
+        app.celery.provider_tasks.deliver_email.apply_async.assert_called_with(
+            (str(sample_email_notification.id)), queue='send-email'
+        )
+        assert response.status_code == 204

tests/app/delivery/test_send_to_providers.py

@@ -0,0 +1,445 @@
+import uuid
+from datetime import datetime
+
+import pytest
+from unittest.mock import ANY
+
+import app
+from app import mmg_client
+from app.dao import (provider_details_dao, notifications_dao)
+from app.delivery import send_to_providers
+from app.models import Notification, KEY_TYPE_NORMAL, KEY_TYPE_TEST, BRANDING_ORG, BRANDING_BOTH, Organisation, \
+    KEY_TYPE_TEAM
+from tests.app.conftest import sample_notification
+
+from notifications_utils.recipients import validate_phone_number, format_phone_number
+
+
+def test_should_return_highest_priority_active_provider(notify_db, notify_db_session):
+    providers = provider_details_dao.get_provider_details_by_notification_type('sms')
+
+    first = providers[0]
+    second = providers[1]
+
+    assert send_to_providers.provider_to_use('sms', '1234').name == first.identifier
+
+    first.priority = 20
+    second.priority = 10
+
+    provider_details_dao.dao_update_provider_details(first)
+    provider_details_dao.dao_update_provider_details(second)
+
+    assert send_to_providers.provider_to_use('sms', '1234').name == second.identifier
+
+    first.priority = 10
+    first.active = False
+    second.priority = 20
+
+    provider_details_dao.dao_update_provider_details(first)
+    provider_details_dao.dao_update_provider_details(second)
+
+    assert send_to_providers.provider_to_use('sms', '1234').name == second.identifier
+
+    first.active = True
+    provider_details_dao.dao_update_provider_details(first)
+
+    assert send_to_providers.provider_to_use('sms', '1234').name == first.identifier
+
+
+def test_should_send_personalised_template_to_correct_sms_provider_and_persist(
+    notify_db,
+    notify_db_session,
+    sample_template_with_placeholders,
+    mocker
+):
+    db_notification = sample_notification(notify_db, notify_db_session, template=sample_template_with_placeholders,
+                                          to_field="+447234123123", personalisation={"name": "Jo"},
+                                          status='created')
+
+    mocker.patch('app.mmg_client.send_sms')
+    mocker.patch('app.mmg_client.get_name', return_value="mmg")
+
+    send_to_providers.send_sms_to_provider(
+        db_notification
+    )
+
+    mmg_client.send_sms.assert_called_once_with(
+        to=format_phone_number(validate_phone_number("+447234123123")),
+        content="Sample service: Hello Jo\nYour thing is due soon",
+        reference=str(db_notification.id),
+        sender=None
+    )
+    notification = Notification.query.filter_by(id=db_notification.id).one()
+
+    assert notification.status == 'sending'
+    assert notification.sent_at <= datetime.utcnow()
+    assert notification.sent_by == 'mmg'
+    assert notification.billable_units == 1
+    assert notification.personalisation == {"name": "Jo"}
+
+
+def test_should_send_personalised_template_to_correct_email_provider_and_persist(
+    notify_db,
+    notify_db_session,
+    sample_email_template_with_placeholders,
+    mocker
+):
+    db_notification = sample_notification(
+        notify_db=notify_db, notify_db_session=notify_db_session,
+        template=sample_email_template_with_placeholders,
+        to_field="jo.smith@example.com",
+        personalisation={'name': 'Jo'}
+    )
+
+    mocker.patch('app.aws_ses_client.send_email', return_value='reference')
+    mocker.patch('app.aws_ses_client.get_name', return_value="ses")
+
+    send_to_providers.send_email_to_provider(
+        db_notification
+    )
+
+    app.aws_ses_client.send_email.assert_called_once_with(
+        '"Sample service" <sample.service@test.notify.com>',
+        'jo.smith@example.com',
+        'Jo',
+        body='Hello Jo\nThis is an email from GOV.\u200bUK',
+        html_body=ANY,
+        reply_to_address=None
+    )
+    assert '<!DOCTYPE html' in app.aws_ses_client.send_email.call_args[1]['html_body']
+
+    notification = Notification.query.filter_by(id=db_notification.id).one()
+
+    assert notification.status == 'sending'
+    assert notification.sent_at <= datetime.utcnow()
+    assert notification.sent_by == 'ses'
+    assert notification.personalisation == {"name": "Jo"}
+
+
+def test_send_sms_should_use_template_version_from_notification_not_latest(
+        notify_db,
+        notify_db_session,
+        sample_template,
+        mocker):
+    db_notification = sample_notification(notify_db, notify_db_session,
+                                          template=sample_template, to_field='+447234123123',
+                                          status='created')
+
+    mocker.patch('app.mmg_client.send_sms')
+    mocker.patch('app.mmg_client.get_name', return_value="mmg")
+    version_on_notification = sample_template.version
+
+    # Change the template
+    from app.dao.templates_dao import dao_update_template, dao_get_template_by_id
+    sample_template.content = sample_template.content + " another version of the template"
+    dao_update_template(sample_template)
+    t = dao_get_template_by_id(sample_template.id)
+    assert t.version > version_on_notification
+
+    send_to_providers.send_sms_to_provider(
+        db_notification
+    )
+
+    mmg_client.send_sms.assert_called_once_with(
+        to=format_phone_number(validate_phone_number("+447234123123")),
+        content="Sample service: This is a template:\nwith a newline",
+        reference=str(db_notification.id),
+        sender=None
+    )
+
+    persisted_notification = notifications_dao.get_notification_by_id(db_notification.id)
+    assert persisted_notification.to == db_notification.to
+    assert persisted_notification.template_id == sample_template.id
+    assert persisted_notification.template_version == version_on_notification
+    assert persisted_notification.template_version != sample_template.version
+    assert persisted_notification.status == 'sending'
+    assert not persisted_notification.personalisation
+
+
+@pytest.mark.parametrize('research_mode,key_type', [
+    (True, KEY_TYPE_NORMAL),
+    (False, KEY_TYPE_TEST)
+])
+def test_should_call_send_sms_response_task_if_research_mode(notify_db, sample_service, sample_notification, mocker,
+                                                             research_mode, key_type):
+    mocker.patch('app.mmg_client.send_sms')
+    mocker.patch('app.mmg_client.get_name', return_value="mmg")
+    mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async')
+
+    if research_mode:
+        sample_service.research_mode = True
+        notify_db.session.add(sample_service)
+        notify_db.session.commit()
+
+    sample_notification.key_type = key_type
+
+    send_to_providers.send_sms_to_provider(
+        sample_notification
+    )
+    assert not mmg_client.send_sms.called
+    send_to_providers.send_sms_response.apply_async.assert_called_once_with(
+        ('mmg', str(sample_notification.id), sample_notification.to), queue='research-mode'
+    )
+
+    persisted_notification = notifications_dao.get_notification_by_id(sample_notification.id)
+    assert persisted_notification.to == sample_notification.to
+    assert persisted_notification.template_id == sample_notification.template_id
+    assert persisted_notification.status == 'sending'
+    assert persisted_notification.sent_at <= datetime.utcnow()
+    assert persisted_notification.sent_by == 'mmg'
+    assert not persisted_notification.personalisation
+
+
+@pytest.mark.parametrize('research_mode,key_type', [
+    (True, KEY_TYPE_NORMAL),
+    (False, KEY_TYPE_TEST)
+])
+def test_should_set_billable_units_to_zero_in_research_mode_or_test_key(
+        notify_db, sample_service, sample_notification, mocker, research_mode, key_type):
+
+    mocker.patch('app.mmg_client.send_sms')
+    mocker.patch('app.mmg_client.get_name', return_value="mmg")
+    mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async')
+    if research_mode:
+        sample_service.research_mode = True
+        notify_db.session.add(sample_service)
+        notify_db.session.commit()
+    sample_notification.key_type = key_type
+
+    send_to_providers.send_sms_to_provider(
+        sample_notification
+    )
+
+    assert notifications_dao.get_notification_by_id(sample_notification.id).billable_units == 0
+
+
+def test_should_not_send_to_provider_when_status_is_not_created(notify_db, notify_db_session,
+                                                                sample_service,
+                                                                mocker):
+    notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session,
+                                       service=sample_service,
+                                       status='sending')
+    mocker.patch('app.mmg_client.send_sms')
+    mocker.patch('app.mmg_client.get_name', return_value="mmg")
+    mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async')
+
+    send_to_providers.send_sms_to_provider(
+        notification
+    )
+
+    app.mmg_client.send_sms.assert_not_called()
+    app.celery.research_mode_tasks.send_sms_response.apply_async.assert_not_called()
+
+
+def test_should_send_sms_sender_from_service_if_present(
+        notify_db,
+        notify_db_session,
+        sample_service,
+        sample_template,
+        mocker):
+    db_notification = sample_notification(notify_db, notify_db_session, template=sample_template,
+                                          to_field="+447234123123",
+                                          status='created')
+
+    sample_service.sms_sender = 'elevenchars'
+    notify_db.session.add(sample_service)
+    notify_db.session.commit()
+
+    mocker.patch('app.mmg_client.send_sms')
+    mocker.patch('app.mmg_client.get_name', return_value="mmg")
+
+    send_to_providers.send_sms_to_provider(
+        db_notification
+    )
+
+    mmg_client.send_sms.assert_called_once_with(
+        to=format_phone_number(validate_phone_number("+447234123123")),
+        content="This is a template:\nwith a newline",
+        reference=str(db_notification.id),
+        sender=sample_service.sms_sender
+    )
+
+
+@pytest.mark.parametrize('research_mode,key_type', [
+    (True, KEY_TYPE_NORMAL),
+    (False, KEY_TYPE_TEST)
+])
+def test_send_email_to_provider_should_call_research_mode_task_response_task_if_research_mode(
+        notify_db,
+        notify_db_session,
+        sample_service,
+        sample_email_template,
+        ses_provider,
+        mocker,
+        research_mode,
+        key_type):
+    notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session,
+                                       template=sample_email_template,
+                                       to_field="john@smith.com",
+                                       key_type=key_type
+                                       )
+
+    reference = uuid.uuid4()
+    mocker.patch('app.uuid.uuid4', return_value=reference)
+    mocker.patch('app.aws_ses_client.send_email')
+    mocker.patch('app.aws_ses_client.get_name', return_value="ses")
+    mocker.patch('app.celery.research_mode_tasks.send_email_response.apply_async')
+
+    if research_mode:
+        sample_service.research_mode = True
+        notify_db.session.add(sample_service)
+        notify_db.session.commit()
+
+    send_to_providers.send_email_to_provider(
+        notification
+    )
+    assert not app.aws_ses_client.send_email.called
+    send_to_providers.send_email_response.apply_async.assert_called_once_with(
+        ('ses', str(reference), 'john@smith.com'), queue="research-mode"
+    )
+    persisted_notification = Notification.query.filter_by(id=notification.id).one()
+
+    assert persisted_notification.to == 'john@smith.com'
+    assert persisted_notification.template_id == sample_email_template.id
+    assert persisted_notification.status == 'sending'
+    assert persisted_notification.sent_at <= datetime.utcnow()
+    assert persisted_notification.created_at <= datetime.utcnow()
+    assert persisted_notification.sent_by == 'ses'
+    assert persisted_notification.reference == str(reference)
+    assert persisted_notification.billable_units == 0
+
+
+def test_send_email_to_provider_should_not_send_to_provider_when_status_is_not_created(notify_db, notify_db_session,
+                                                                                       sample_service,
+                                                                                       sample_email_template,
+                                                                                       mocker):
+    notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session,
+                                       template=sample_email_template,
+                                       service=sample_service,
+                                       status='sending')
+    mocker.patch('app.aws_ses_client.send_email')
+    mocker.patch('app.aws_ses_client.get_name', return_value="ses")
+    mocker.patch('app.celery.research_mode_tasks.send_email_response.apply_async')
+
+    send_to_providers.send_sms_to_provider(
+        notification
+    )
+
+    app.aws_ses_client.send_email.assert_not_called()
+    app.celery.research_mode_tasks.send_email_response.apply_async.assert_not_called()
+
+
+def test_send_email_should_use_service_reply_to_email(
+        notify_db, notify_db_session,
+        sample_service,
+        sample_email_template,
+        mocker):
+    mocker.patch('app.aws_ses_client.send_email', return_value='reference')
+    mocker.patch('app.aws_ses_client.get_name', return_value="ses")
+
+    db_notification = sample_notification(notify_db, notify_db_session, template=sample_email_template)
+    sample_service.reply_to_email_address = 'foo@bar.com'
+
+    send_to_providers.send_email_to_provider(
+        db_notification,
+    )
+
+    app.aws_ses_client.send_email.assert_called_once_with(
+        ANY,
+        ANY,
+        ANY,
+        body=ANY,
+        html_body=ANY,
+        reply_to_address=sample_service.reply_to_email_address
+    )
+
+
+def test_get_html_email_renderer_should_return_for_normal_service(sample_service):
+    renderer = send_to_providers.get_html_email_renderer(sample_service)
+    assert renderer.govuk_banner
+    assert renderer.brand_colour is None
+    assert renderer.brand_logo is None
+    assert renderer.brand_name is None
+
+
+@pytest.mark.parametrize('branding_type, govuk_banner', [
+    (BRANDING_ORG, False),
+    (BRANDING_BOTH, True)
+])
+def test_get_html_email_renderer_with_branding_details(branding_type, govuk_banner, notify_db, sample_service):
+    sample_service.branding = branding_type
+    org = Organisation(colour='#000000', logo='justice-league.png', name='Justice League')
+    sample_service.organisation = org
+    notify_db.session.add_all([sample_service, org])
+    notify_db.session.commit()
+
+    renderer = send_to_providers.get_html_email_renderer(sample_service)
+
+    assert renderer.govuk_banner == govuk_banner
+    assert renderer.brand_colour == '000000'
+    assert renderer.brand_name == 'Justice League'
+
+
+def test_get_html_email_renderer_prepends_logo_path(notify_db, sample_service):
+    sample_service.branding = BRANDING_ORG
+    org = Organisation(colour='#000000', logo='justice-league.png', name='Justice League')
+    sample_service.organisation = org
+    notify_db.session.add_all([sample_service, org])
+    notify_db.session.commit()
+
+    renderer = send_to_providers.get_html_email_renderer(sample_service)
+
+    assert renderer.brand_logo == 'http://localhost:6012/static/images/email-template/crests/justice-league.png'
+
+
+def test_should_not_set_billable_units_if_research_mode(notify_db, sample_service, sample_notification, mocker):
+    mocker.patch('app.mmg_client.send_sms')
+    mocker.patch('app.mmg_client.get_name', return_value="mmg")
+    mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async')
+
+    sample_service.research_mode = True
+    notify_db.session.add(sample_service)
+    notify_db.session.commit()
+
+    send_to_providers.send_sms_to_provider(
+        sample_notification
+    )
+
+    persisted_notification = notifications_dao.get_notification_by_id(sample_notification.id)
+    assert persisted_notification.billable_units == 0
+
+
+@pytest.mark.parametrize('research_mode,key_type, billable_units', [
+    (True, KEY_TYPE_NORMAL, 0),
+    (False, KEY_TYPE_NORMAL, 1),
+    (False, KEY_TYPE_TEST, 0),
+    (True, KEY_TYPE_TEST, 0),
+    (True, KEY_TYPE_TEAM, 0),
+    (False, KEY_TYPE_TEAM, 1)
+])
+def test_should_update_billable_units_according_to_research_mode_and_key_type(notify_db,
+                                                                              sample_service,
+                                                                              sample_notification,
+                                                                              mocker,
+                                                                              research_mode,
+                                                                              key_type,
+                                                                              billable_units):
+
+    assert Notification.query.count() == 1
+
+    mocker.patch('app.mmg_client.send_sms')
+    mocker.patch('app.mmg_client.get_name', return_value="mmg")
+    mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async')
+    if research_mode:
+        sample_service.research_mode = True
+        notify_db.session.add(sample_service)
+        notify_db.session.commit()
+
+    sample_notification.key_type = key_type
+
+    send_to_providers.send_sms_to_provider(
+        sample_notification
+    )
+
+    assert Notification.query.get(sample_notification.id).billable_units == billable_units, \
+        "Research mode: {0}, key type: {1}, billable_units: {2}".format(research_mode, key_type, billable_units)