Merge pull request #3493 from alphagov/reach-send-181665654

Add boilerplate for sending SMS via Reach

README.md

@@ -29,6 +29,7 @@ export NOTIFY_ENVIRONMENT='development'
 
 export MMG_API_KEY='MMG_API_KEY'
 export FIRETEXT_API_KEY='FIRETEXT_ACTUAL_KEY'
+export REACH_API_KEY='REACH_API_KEY'
 export NOTIFICATION_QUEUE_PREFIX='YOUR_OWN_PREFIX'
 
 export FLASK_APP=application.py
@@ -45,6 +46,7 @@ Things to change:
 ```
 notify-pass credentials/firetext
 notify-pass credentials/mmg
+notify-pass credentials/reach
 ```
 
 ### Postgres

app/__init__.py

@@ -36,6 +36,7 @@ from app.clients.email.aws_ses import AwsSesClient
 from app.clients.email.aws_ses_stub import AwsSesStubClient
 from app.clients.sms.firetext import FiretextClient
 from app.clients.sms.mmg import MMGClient
+from app.clients.sms.reach import ReachClient
 
 
 class SQLAlchemy(_SQLAlchemy):
@@ -56,6 +57,7 @@ ma = Marshmallow()
 notify_celery = NotifyCelery()
 firetext_client = FiretextClient()
 mmg_client = MMGClient()
+reach_client = ReachClient()
 aws_ses_client = AwsSesClient()
 aws_ses_stub_client = AwsSesStubClient()
 encryption = Encryption()
@@ -98,6 +100,7 @@ def create_app(application):
     logging.init_app(application, statsd_client)
     firetext_client.init_app(application, statsd_client=statsd_client)
     mmg_client.init_app(application, statsd_client=statsd_client)
+    reach_client.init_app(application, statsd_client=statsd_client)
 
     aws_ses_client.init_app(application.config['AWS_REGION'], statsd_client=statsd_client)
     aws_ses_stub_client.init_app(
@@ -107,7 +110,10 @@ def create_app(application):
     )
     # If a stub url is provided for SES, then use the stub client rather than the real SES boto client
     email_clients = [aws_ses_stub_client] if application.config['SES_STUB_URL'] else [aws_ses_client]
-    notification_provider_clients.init_app(sms_clients=[firetext_client, mmg_client], email_clients=email_clients)
+    notification_provider_clients.init_app(
+        sms_clients=[firetext_client, mmg_client, reach_client],
+        email_clients=email_clients
+    )
 
     notify_celery.init_app(application)
     encryption.init_app(application)

app/celery/provider_tasks.py

@@ -24,7 +24,8 @@ def deliver_sms(self, notification_id):
     except Exception as e:
         if isinstance(e, SmsClientResponseException):
             current_app.logger.warning(
-                "SMS notification delivery for id: {} failed".format(notification_id)
+                "SMS notification delivery for id: {} failed".format(notification_id),
+                exc_info=True
             )
         else:
             current_app.logger.exception(

app/clients/email/__init__.py

@@ -28,5 +28,6 @@ class EmailClient(Client):
     def send_email(self, *args, **kwargs):
         raise NotImplementedError('TODO Need to implement.')
 
-    def get_name(self):
+    @property
+    def name(self):
         raise NotImplementedError('TODO Need to implement.')

app/clients/email/aws_ses.py

@@ -59,11 +59,11 @@ class AwsSesClient(EmailClient):
     def init_app(self, region, statsd_client, *args, **kwargs):
         self._client = boto3.client('ses', region_name=region)
         super(AwsSesClient, self).__init__(*args, **kwargs)
-        self.name = 'ses'
         self.statsd_client = statsd_client
 
-    def get_name(self):
+    @property
-        return self.name
+    def name(self):
+        return 'ses'
 
     def send_email(self,
                    source,

app/clients/email/aws_ses_stub.py

@@ -13,12 +13,12 @@ class AwsSesStubClientException(EmailClientException):
 
 class AwsSesStubClient(EmailClient):
     def init_app(self, region, statsd_client, stub_url):
-        self.name = 'ses'
         self.statsd_client = statsd_client
         self.url = stub_url
 
-    def get_name(self):
+    @property
-        return self.name
+    def name(self):
+        return 'ses'
 
     def send_email(self,
                    source,

app/clients/sms/__init__.py

@@ -1,3 +1,5 @@
+from time import monotonic
+
 from app.clients import Client, ClientException
 
 
@@ -10,7 +12,7 @@ class SmsClientResponseException(ClientException):
         self.message = message
 
     def __str__(self):
-        return "Message {}".format(self.message)
+        return f"SMS client error ({self.message})"
 
 
 class SmsClient(Client):
@@ -18,8 +20,52 @@ class SmsClient(Client):
     Base Sms client for sending smss.
     '''
 
-    def send_sms(self, *args, **kwargs):
+    def init_app(self, current_app, statsd_client):
+        self.current_app = current_app
+        self.statsd_client = statsd_client
+        self.from_number = self.current_app.config.get('FROM_NUMBER')
+
+    def record_outcome(self, success):
+        log_message = "Provider request for {} {}".format(
+            self.name,
+            "succeeded" if success else "failed",
+        )
+
+        if success:
+            self.current_app.logger.info(log_message)
+            self.statsd_client.incr(f"clients.{self.name}.success")
+        else:
+            self.statsd_client.incr(f"clients.{self.name}.error")
+            self.current_app.logger.warning(log_message)
+
+    def send_sms(self, to, content, reference, international, sender):
+        start_time = monotonic()
+
+        if sender is None:
+            # temporary log to see if the following ternary is necessary
+            # or if it's safe to remove it - keep for 1-2 weeks
+            self.current_app.logger.warning(
+                f"send_sms called with 'sender' of 'None' for {reference}"
+            )
+
+        sender = self.from_number if sender is None else sender
+
+        try:
+            response = self.try_send_sms(to, content, reference, international, sender)
+            self.record_outcome(True)
+        except SmsClientResponseException as e:
+            self.record_outcome(False)
+            raise e
+        finally:
+            elapsed_time = monotonic() - start_time
+            self.statsd_client.timing(f"clients.{self.name}.request-time", elapsed_time)
+            self.current_app.logger.info("Reach request for {} finished in {}".format(reference, elapsed_time))
+
+        return response
+
+    def try_send_sms(self, *args, **kwargs):
         raise NotImplementedError('TODO Need to implement.')
 
-    def get_name(self):
+    @property
+    def name(self):
         raise NotImplementedError('TODO Need to implement.')

app/clients/sms/firetext.py

@@ -1,6 +1,5 @@
 import json
 import logging
-from time import monotonic
 
 from requests import RequestException, request
 
@@ -45,63 +44,30 @@ def get_message_status_and_reason_from_firetext_code(detailed_status_code):
     return firetext_codes[detailed_status_code]['status'], firetext_codes[detailed_status_code]['reason']
 
 
-class FiretextClientResponseException(SmsClientResponseException):
-    def __init__(self, response, exception):
-        status_code = response.status_code if response is not None else 504
-        text = response.text if response is not None else "Gateway Time-out"
-        self.status_code = status_code
-        self.text = text
-        self.exception = exception
-
-    def __str__(self):
-        return "Code {} text {} exception {}".format(self.status_code, self.text, str(self.exception))
-
-
 class FiretextClient(SmsClient):
     '''
     FireText sms client.
     '''
 
-    def init_app(self, current_app, statsd_client, *args, **kwargs):
+    def init_app(self, *args, **kwargs):
-        super(SmsClient, self).__init__(*args, **kwargs)
+        super().init_app(*args, **kwargs)
-        self.current_app = current_app
+        self.api_key = self.current_app.config.get('FIRETEXT_API_KEY')
-        self.api_key = current_app.config.get('FIRETEXT_API_KEY')
+        self.international_api_key = self.current_app.config.get('FIRETEXT_INTERNATIONAL_API_KEY')
-        self.international_api_key = current_app.config.get('FIRETEXT_INTERNATIONAL_API_KEY')
+        self.url = self.current_app.config.get('FIRETEXT_URL')
-        self.from_number = current_app.config.get('FROM_NUMBER')
-        self.name = 'firetext'
-        self.url = current_app.config.get('FIRETEXT_URL')
-        self.statsd_client = statsd_client
 
-    def get_name(self):
+    @property
-        return self.name
+    def name(self):
+        return 'firetext'
 
-    def record_outcome(self, success, response):
+    def try_send_sms(self, to, content, reference, international, sender):
-        status_code = response.status_code if response else 503
-
-        log_message = "API {} request {} on {} response status_code {}".format(
-            "POST",
-            "succeeded" if success else "failed",
-            self.url,
-            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.warning(log_message)
-
-    def send_sms(self, to, content, reference, international, sender=None):
         data = {
             "apiKey": self.international_api_key if international else self.api_key,
-            "from": self.from_number if sender is None else sender,
+            "from": sender,
             "to": to.replace('+', ''),
             "message": content,
             "reference": reference
         }
 
-        start_time = monotonic()
         try:
             response = request(
                 "POST",
@@ -113,16 +79,10 @@ class FiretextClient(SmsClient):
             try:
                 json.loads(response.text)
                 if response.json()['code'] != 0:
-                    raise ValueError()
+                    raise ValueError("Expected 'code' to be '0'")
-            except (ValueError, AttributeError) as e:
+            except (ValueError, AttributeError):
-                self.record_outcome(False, response)
+                raise SmsClientResponseException("Invalid response JSON")
-                raise FiretextClientResponseException(response=response, exception=e)
+        except RequestException:
-            self.record_outcome(True, response)
+            raise SmsClientResponseException("Request failed")
-        except RequestException as e:
+
-            self.record_outcome(False, e.response)
-            raise FiretextClientResponseException(response=e.response, exception=e)
-        finally:
-            elapsed_time = monotonic() - start_time
-            self.current_app.logger.info("Firetext request for {} finished in {}".format(reference, elapsed_time))
-            self.statsd_client.timing("clients.firetext.request-time", elapsed_time)
         return response

app/clients/sms/mmg.py

@@ -1,5 +1,4 @@
 import json
-from time import monotonic
 
 from requests import RequestException, request
 
@@ -69,45 +68,25 @@ class MMGClient(SmsClient):
     MMG sms client
     '''
 
-    def init_app(self, current_app, statsd_client, *args, **kwargs):
+    def init_app(self, *args, **kwargs):
-        super(SmsClient, self).__init__(*args, **kwargs)
+        super().init_app(*args, **kwargs)
-        self.current_app = current_app
+        self.api_key = self.current_app.config.get('MMG_API_KEY')
-        self.api_key = current_app.config.get('MMG_API_KEY')
+        self.mmg_url = self.current_app.config.get('MMG_URL')
-        self.from_number = current_app.config.get('FROM_NUMBER')
-        self.name = 'mmg'
-        self.statsd_client = statsd_client
-        self.mmg_url = current_app.config.get('MMG_URL')
 
-    def record_outcome(self, success, response):
+    @property
-        status_code = response.status_code if response else 503
+    def name(self):
-        log_message = "API {} request {} on {} response status_code {}".format(
+        return 'mmg'
-            "POST",
-            "succeeded" if success else "failed",
-            self.mmg_url,
-            status_code
-        )
 
-        if success:
+    def try_send_sms(self, to, content, reference, international, sender):
-            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.warning(log_message)
-
-    def get_name(self):
-        return self.name
-
-    def send_sms(self, to, content, reference, international, multi=True, sender=None):
         data = {
             "reqType": "BULK",
             "MSISDN": to,
             "msg": content,
-            "sender": self.from_number if sender is None else sender,
+            "sender": sender,
             "cid": reference,
-            "multi": multi
+            "multi": True
         }
 
-        start_time = monotonic()
         try:
             response = request(
                 "POST",
@@ -123,16 +102,9 @@ class MMGClient(SmsClient):
             response.raise_for_status()
             try:
                 json.loads(response.text)
-            except (ValueError, AttributeError) as e:
+            except (ValueError, AttributeError):
-                self.record_outcome(False, response)
+                raise SmsClientResponseException("Invalid response JSON")
-                raise MMGClientResponseException(response=response, exception=e)
+        except RequestException:
-            self.record_outcome(True, response)
+            raise SmsClientResponseException("Request failed")
-        except RequestException as e:
-            self.record_outcome(False, e.response)
-            raise MMGClientResponseException(response=e.response, exception=e)
-        finally:
-            elapsed_time = monotonic() - start_time
-            self.statsd_client.timing("clients.mmg.request-time", elapsed_time)
-            self.current_app.logger.info("MMG request for {} finished in {}".format(reference, elapsed_time))
 
         return response

app/clients/sms/reach.py

@@ -1,3 +1,7 @@
+import json
+
+from requests import RequestException, request
+
 from app.clients.sms import SmsClient, SmsClientResponseException
 
 
@@ -12,14 +16,37 @@ def get_reach_responses(status, detailed_status_code=None):
         raise KeyError
 
 
-class ReachClientResponseException(SmsClientResponseException):
-    pass  # TODO (custom exception for errors)
-
-
 class ReachClient(SmsClient):
+    def init_app(self, *args, **kwargs):
+        super().init_app(*args, **kwargs)
+        self.url = self.current_app.config.get('REACH_URL')
 
-    def get_name(self):
+    @property
-        pass  # TODO
+    def name(self):
+        return 'reach'
 
-    def send_sms(self, to, content, reference, international, multi=True, sender=None):
+    def try_send_sms(self, to, content, reference, international, sender):
-        pass  # TODO
+        data = {
+            # TODO
+        }
+
+        try:
+            response = request(
+                "POST",
+                self.url,
+                data=json.dumps(data),
+                headers={
+                    'Content-Type': 'application/json',
+                },
+                timeout=60
+            )
+
+            response.raise_for_status()
+            try:
+                json.loads(response.text)
+            except (ValueError, AttributeError):
+                raise SmsClientResponseException("Invalid response JSON")
+        except RequestException:
+            raise SmsClientResponseException("Request failed")
+
+        return response

app/config.py

@@ -380,6 +380,7 @@ class Config(object):
     # these environment vars aren't defined in the manifest so to set them on paas use `cf set-env`
     MMG_URL = os.environ.get("MMG_URL", "https://api.mmg.co.uk/jsonv2a/api.php")
     FIRETEXT_URL = os.environ.get("FIRETEXT_URL", "https://www.firetext.co.uk/api/sendsms/json")
+    REACH_URL = os.environ.get("REACH_URL", "TODO")
     SES_STUB_URL = os.environ.get("SES_STUB_URL")
 
     AWS_REGION = 'eu-west-1'
@@ -481,6 +482,7 @@ class Test(Development):
 
     MMG_URL = 'https://example.com/mmg'
     FIRETEXT_URL = 'https://example.com/firetext'
+    REACH_URL = 'https://example.com/reach'
 
     CBC_PROXY_ENABLED = True
     DVLA_EMAIL_ADDRESSES = ['success@simulator.amazonses.com', 'success+2@simulator.amazonses.com']

app/delivery/send_to_providers.py

@@ -60,7 +60,7 @@ def send_sms_to_provider(notification):
         key_type = notification.key_type
         if service.research_mode or notification.key_type == KEY_TYPE_TEST:
             update_notification_to_sending(notification, provider)
-            send_sms_response(provider.get_name(), str(notification.id), notification.to)
+            send_sms_response(provider.name, str(notification.id), notification.to)
 
         else:
             try:
@@ -82,7 +82,7 @@ def send_sms_to_provider(notification):
             except Exception as e:
                 notification.billable_units = template.fragment_count
                 dao_update_notification(notification)
-                dao_reduce_sms_provider_priority(provider.get_name(), time_threshold=timedelta(minutes=1))
+                dao_reduce_sms_provider_priority(provider.name, time_threshold=timedelta(minutes=1))
                 raise e
             else:
                 notification.billable_units = template.fragment_count
@@ -158,7 +158,7 @@ def send_email_to_provider(notification):
 
 def update_notification_to_sending(notification, provider):
     notification.sent_at = datetime.utcnow()
-    notification.sent_by = provider.get_name()
+    notification.sent_by = provider.name
     if notification.status not in NOTIFICATION_STATUS_TYPES_COMPLETED:
         notification.status = NOTIFICATION_SENT if notification.international else NOTIFICATION_SENDING
     dao_update_notification(notification)

migrations/versions/0367_add_reach.py

@@ -0,0 +1,54 @@
+"""
+
+Revision ID: 0367_add_reach
+Revises: 0366_letter_rates_2022
+Create Date: 2022-03-24 16:00:00
+
+"""
+import itertools
+import uuid
+from datetime import datetime
+
+from alembic import op
+from sqlalchemy.sql import text
+
+from app.models import LetterRate
+
+
+revision = '0367_add_reach'
+down_revision = '0366_letter_rates_2022'
+
+
+def upgrade():
+    conn = op.get_bind()
+    conn.execute(
+        """
+        INSERT INTO provider_details (
+            id,
+            display_name,
+            identifier,
+            priority,
+            notification_type,
+            active,
+            version,
+            created_by_id
+        )
+        VALUES (
+            '{}',
+            'Reach',
+            'reach',
+            0,
+            'sms',
+            false,
+            1,
+            null
+        )
+        """.format(
+            str(uuid.uuid4()),
+        )
+    )
+
+
+def downgrade():
+    conn = op.get_bind()
+    conn.execute("DELETE FROM provider_details WHERE identifier = 'reach'")

tests/app/clients/test_firetext.py

@@ -2,11 +2,9 @@ from urllib.parse import parse_qs
 
 import pytest
 import requests_mock
-from requests import HTTPError
 from requests.exceptions import ConnectTimeout, ReadTimeout
 
 from app.clients.sms.firetext import (
-    FiretextClientResponseException,
     SmsClientResponseException,
     get_firetext_responses,
 )
@@ -36,7 +34,7 @@ def test_get_firetext_responses_raises_KeyError_if_unrecognised_status_code():
     assert '99' in str(e.value)
 
 
-def test_send_sms_successful_returns_firetext_response(mocker, mock_firetext_client):
+def test_try_send_sms_successful_returns_firetext_response(mocker, mock_firetext_client):
     to = content = reference = 'foo'
     response_dict = {
         'data': [],
@@ -47,7 +45,7 @@ def test_send_sms_successful_returns_firetext_response(mocker, mock_firetext_cli
 
     with requests_mock.Mocker() as request_mock:
         request_mock.post('https://example.com/firetext', json=response_dict, status_code=200)
-        response = mock_firetext_client.send_sms(to, content, reference, False)
+        response = mock_firetext_client.try_send_sms(to, content, reference, False, 'sender')
 
     response_json = response.json()
     assert response.status_code == 200
@@ -55,7 +53,7 @@ def test_send_sms_successful_returns_firetext_response(mocker, mock_firetext_cli
     assert response_json['description'] == 'SMS successfully queued'
 
 
-def test_send_sms_calls_firetext_correctly(mocker, mock_firetext_client):
+def test_try_send_sms_calls_firetext_correctly(mocker, mock_firetext_client):
     to = '+447234567890'
     content = 'my message'
     reference = 'my reference'
@@ -65,7 +63,7 @@ def test_send_sms_calls_firetext_correctly(mocker, mock_firetext_client):
 
     with requests_mock.Mocker() as request_mock:
         request_mock.post('https://example.com/firetext', json=response_dict, status_code=200)
-        mock_firetext_client.send_sms(to, content, reference, False)
+        mock_firetext_client.try_send_sms(to, content, reference, False, 'bar')
 
     assert request_mock.call_count == 1
     assert request_mock.request_history[0].url == 'https://example.com/firetext'
@@ -79,7 +77,7 @@ def test_send_sms_calls_firetext_correctly(mocker, mock_firetext_client):
     assert request_args['reference'][0] == reference
 
 
-def test_send_sms_calls_firetext_correctly_for_international(mocker, mock_firetext_client):
+def test_try_send_sms_calls_firetext_correctly_for_international(mocker, mock_firetext_client):
     to = '+607234567890'
     content = 'my message'
     reference = 'my reference'
@@ -89,7 +87,7 @@ def test_send_sms_calls_firetext_correctly_for_international(mocker, mock_firete
 
     with requests_mock.Mocker() as request_mock:
         request_mock.post('https://example.com/firetext', json=response_dict, status_code=200)
-        mock_firetext_client.send_sms(to, content, reference, True)
+        mock_firetext_client.try_send_sms(to, content, reference, True, 'bar')
 
     assert request_mock.call_count == 1
     assert request_mock.request_history[0].url == 'https://example.com/firetext'
@@ -103,7 +101,7 @@ def test_send_sms_calls_firetext_correctly_for_international(mocker, mock_firete
     assert request_args['reference'][0] == reference
 
 
-def test_send_sms_raises_if_firetext_rejects(mocker, mock_firetext_client):
+def test_try_send_sms_raises_if_firetext_rejects(mocker, mock_firetext_client):
     to = content = reference = 'foo'
     response_dict = {
         'data': [],
@@ -114,60 +112,48 @@ def test_send_sms_raises_if_firetext_rejects(mocker, mock_firetext_client):
 
     with pytest.raises(SmsClientResponseException) as exc, requests_mock.Mocker() as request_mock:
         request_mock.post('https://example.com/firetext', json=response_dict, status_code=200)
-        mock_firetext_client.send_sms(to, content, reference, False)
+        mock_firetext_client.try_send_sms(to, content, reference, False, 'sender')
 
-    assert exc.value.status_code == 200
+    assert "Invalid response JSON" in str(exc.value)
-    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_with_unexpected_data(mocker, mock_firetext_client):
+def test_try_send_sms_raises_if_firetext_rejects_with_unexpected_data(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://example.com/firetext', json=response_dict, status_code=400)
-        mock_firetext_client.send_sms(to, content, reference, False)
+        mock_firetext_client.try_send_sms(to, content, reference, False, 'sender')
 
-    assert exc.value.status_code == 400
+    assert "Request failed" in str(exc.value)
-    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):
+def test_try_send_sms_raises_if_firetext_fails_to_return_json(notify_api, mock_firetext_client):
-    to = '+447234567890'
+    to = content = reference = 'foo'
-    content = 'my message'
+    response_dict = 'NOT AT ALL VALID JSON {"key" : "value"}}'
-    reference = 'my reference'
-    response_dict = {
-        'code': 0,
-    }
-    sender = 'fromservice'
 
-    with requests_mock.Mocker() as request_mock:
+    with pytest.raises(SmsClientResponseException) as exc, requests_mock.Mocker() as request_mock:
-        request_mock.post('https://example.com/firetext', json=response_dict, status_code=200)
+        request_mock.post('https://example.com/firetext', text=response_dict, status_code=200)
-        mock_firetext_client.send_sms(to, content, reference, False, sender=sender)
+        mock_firetext_client.try_send_sms(to, content, reference, False, 'sender')
 
-    request_args = parse_qs(request_mock.request_history[0].text)
+    assert "Invalid response JSON" in str(exc.value)
-    assert request_args['from'][0] == 'fromservice'
 
 
-def test_send_sms_raises_if_firetext_rejects_with_connect_timeout(rmock, mock_firetext_client):
+def test_try_send_sms_raises_if_firetext_rejects_with_connect_timeout(rmock, mock_firetext_client):
     to = content = reference = 'foo'
 
-    with pytest.raises(FiretextClientResponseException) as exc:
+    with pytest.raises(SmsClientResponseException) as exc:
         rmock.register_uri('POST', 'https://example.com/firetext', exc=ConnectTimeout)
-        mock_firetext_client.send_sms(to, content, reference, False)
+        mock_firetext_client.try_send_sms(to, content, reference, False, 'sender')
 
-    assert exc.value.status_code == 504
+    assert "Request failed" in str(exc.value)
-    assert exc.value.text == 'Gateway Time-out'
 
 
-def test_send_sms_raises_if_firetext_rejects_with_read_timeout(rmock, mock_firetext_client):
+def test_try_send_sms_raises_if_firetext_rejects_with_read_timeout(rmock, mock_firetext_client):
     to = content = reference = 'foo'
 
-    with pytest.raises(FiretextClientResponseException) as exc:
+    with pytest.raises(SmsClientResponseException) as exc:
         rmock.register_uri('POST', 'https://example.com/firetext', exc=ReadTimeout)
-        mock_firetext_client.send_sms(to, content, reference, False)
+        mock_firetext_client.try_send_sms(to, content, reference, False, 'sender')
 
-    assert exc.value.status_code == 504
+    assert "Request failed" in str(exc.value)
-    assert exc.value.text == 'Gateway Time-out'

tests/app/clients/test_mmg.py

@@ -1,11 +1,9 @@
 import pytest
 import requests_mock
-from requests import HTTPError
 from requests.exceptions import ConnectTimeout, ReadTimeout
 
 from app import mmg_client
-from app.clients.sms import SmsClientResponseException
+from app.clients.sms.mmg import SmsClientResponseException, get_mmg_responses
-from app.clients.sms.mmg import MMGClientResponseException, get_mmg_responses
 
 
 @pytest.mark.parametrize('detailed_status_code, result', [
@@ -38,20 +36,20 @@ def test_get_mmg_responses_raises_KeyError_if_unrecognised_status_code():
     assert '99' in str(e.value)
 
 
-def test_send_sms_successful_returns_mmg_response(notify_api, mocker):
+def test_try_send_sms_successful_returns_mmg_response(notify_api, mocker):
     to = content = reference = 'foo'
     response_dict = {'Reference': 12345678}
 
     with requests_mock.Mocker() as request_mock:
         request_mock.post('https://example.com/mmg', json=response_dict, status_code=200)
-        response = mmg_client.send_sms(to, content, reference, False)
+        response = mmg_client.try_send_sms(to, content, reference, False, 'sender')
 
     response_json = response.json()
     assert response.status_code == 200
     assert response_json['Reference'] == 12345678
 
 
-def test_send_sms_calls_mmg_correctly(notify_api, mocker):
+def test_try_send_sms_calls_mmg_correctly(notify_api, mocker):
     to = '+447234567890'
     content = 'my message'
     reference = 'my reference'
@@ -59,7 +57,7 @@ def test_send_sms_calls_mmg_correctly(notify_api, mocker):
 
     with requests_mock.Mocker() as request_mock:
         request_mock.post('https://example.com/mmg', json=response_dict, status_code=200)
-        mmg_client.send_sms(to, content, reference, False)
+        mmg_client.try_send_sms(to, content, reference, False, 'testing')
 
     assert request_mock.call_count == 1
     assert request_mock.request_history[0].url == 'https://example.com/mmg'
@@ -74,7 +72,7 @@ def test_send_sms_calls_mmg_correctly(notify_api, mocker):
     assert request_args['multi'] is True
 
 
-def test_send_sms_raises_if_mmg_rejects(notify_api, mocker):
+def test_try_send_sms_raises_if_mmg_rejects(notify_api, mocker):
     to = content = reference = 'foo'
     response_dict = {
         'Error': 206,
@@ -83,59 +81,37 @@ def test_send_sms_raises_if_mmg_rejects(notify_api, mocker):
 
     with pytest.raises(SmsClientResponseException) as exc, requests_mock.Mocker() as request_mock:
         request_mock.post('https://example.com/mmg', json=response_dict, status_code=400)
-        mmg_client.send_sms(to, content, reference, False)
+        mmg_client.try_send_sms(to, content, reference, False, 'sender')
 
-    assert exc.value.status_code == 400
+    assert "Request failed" in str(exc.value)
-    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):
+def test_try_send_sms_raises_if_mmg_fails_to_return_json(notify_api, mocker):
-    to = '+447234567890'
-    content = 'my message'
-    reference = 'my reference'
-    response_dict = {'Reference': 12345678}
-    sender = 'fromservice'
-
-    with requests_mock.Mocker() as request_mock:
-        request_mock.post('https://example.com/mmg', json=response_dict, status_code=200)
-        mmg_client.send_sms(to, content, reference, False, sender=sender)
-
-    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://example.com/mmg', text=response_dict, status_code=200)
-        mmg_client.send_sms(to, content, reference, False)
+        mmg_client.try_send_sms(to, content, reference, False, 'sender')
 
-    assert 'Code 200 text NOT AT ALL VALID JSON {"key" : "value"}} exception Expecting value: line 1 column 1 (char 0)' in str(exc.value)  # noqa
+    assert "Invalid response JSON" in str(exc.value)
-    assert exc.value.status_code == 200
-    assert exc.value.text == 'NOT AT ALL VALID JSON {"key" : "value"}}'
 
 
-def test_send_sms_raises_if_mmg_rejects_with_connect_timeout(rmock):
+def test_try_send_sms_raises_if_mmg_rejects_with_connect_timeout(rmock):
     to = content = reference = 'foo'
 
-    with pytest.raises(MMGClientResponseException) as exc:
+    with pytest.raises(SmsClientResponseException) as exc:
         rmock.register_uri('POST', 'https://example.com/mmg', exc=ConnectTimeout)
-        mmg_client.send_sms(to, content, reference, False)
+        mmg_client.try_send_sms(to, content, reference, False, 'sender')
 
-    assert exc.value.status_code == 504
+    assert "Request failed" in str(exc.value)
-    assert exc.value.text == 'Gateway Time-out'
 
 
-def test_send_sms_raises_if_mmg_rejects_with_read_timeout(rmock):
+def test_try_send_sms_raises_if_mmg_rejects_with_read_timeout(rmock):
     to = content = reference = 'foo'
 
-    with pytest.raises(MMGClientResponseException) as exc:
+    with pytest.raises(SmsClientResponseException) as exc:
         rmock.register_uri('POST', 'https://example.com/mmg', exc=ReadTimeout)
-        mmg_client.send_sms(to, content, reference, False)
+        mmg_client.try_send_sms(to, content, reference, False, 'sender')
 
-    assert exc.value.status_code == 504
+    assert "Request failed" in str(exc.value)
-    assert exc.value.text == 'Gateway Time-out'

tests/app/clients/test_reach.py

@@ -1 +1,84 @@
-# TODO: all of the tests
+import pytest
+import requests_mock
+from requests.exceptions import ConnectTimeout, ReadTimeout
+
+from app import reach_client
+from app.clients.sms import SmsClientResponseException
+
+# TODO: tests for get_reach_responses
+
+
+def test_try_send_sms_successful_returns_reach_response(notify_api, mocker):
+    to = content = reference = 'foo'
+    response_dict = {}  # TODO
+
+    with requests_mock.Mocker() as request_mock:
+        request_mock.post('https://example.com/reach', json=response_dict, status_code=200)
+        response = reach_client.try_send_sms(to, content, reference, False, 'sender')
+
+    # response_json = response.json()
+    assert response.status_code == 200
+    # TODO: assertions
+
+
+def test_try_send_sms_calls_reach_correctly(notify_api, mocker):
+    to = '+447234567890'
+    content = 'my message'
+    reference = 'my reference'
+    response_dict = {}  # TODO
+
+    with requests_mock.Mocker() as request_mock:
+        request_mock.post('https://example.com/reach', json=response_dict, status_code=200)
+        reach_client.try_send_sms(to, content, reference, False, 'sender')
+
+    assert request_mock.call_count == 1
+    assert request_mock.request_history[0].url == 'https://example.com/reach'
+    assert request_mock.request_history[0].method == 'POST'
+
+    # request_args = request_mock.request_history[0].json()
+    # TODO: assertions
+
+
+def test_try_send_sms_raises_if_reach_rejects(notify_api, mocker):
+    to = content = reference = 'foo'
+    response_dict = {
+        'Error': 206,
+        'Description': 'Some kind of error'
+    }
+
+    with pytest.raises(SmsClientResponseException) as exc, requests_mock.Mocker() as request_mock:
+        request_mock.post('https://example.com/reach', json=response_dict, status_code=400)
+        reach_client.try_send_sms(to, content, reference, False, 'sender')
+
+    assert "Request failed" in str(exc)
+
+
+def test_try_send_sms_raises_if_reach_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://example.com/reach', text=response_dict, status_code=200)
+        reach_client.try_send_sms(to, content, reference, False, 'sender')
+
+    assert 'Invalid response JSON' in str(exc.value)
+
+
+def test_try_send_sms_raises_if_reach_rejects_with_connect_timeout(rmock):
+    to = content = reference = 'foo'
+
+    with pytest.raises(SmsClientResponseException) as exc:
+        rmock.register_uri('POST', 'https://example.com/reach', exc=ConnectTimeout)
+        reach_client.try_send_sms(to, content, reference, False, 'sender')
+
+    assert 'Request failed' in str(exc.value)
+
+
+def test_try_send_sms_raises_if_reach_rejects_with_read_timeout(rmock):
+    to = content = reference = 'foo'
+
+    with pytest.raises(SmsClientResponseException) as exc:
+        rmock.register_uri('POST', 'https://example.com/reach', exc=ReadTimeout)
+        reach_client.try_send_sms(to, content, reference, False, 'sender')
+
+    assert 'Request failed' in str(exc.value)

tests/app/clients/test_sms.py

@@ -0,0 +1,66 @@
+import pytest
+
+from app import statsd_client
+from app.clients.sms import SmsClient, SmsClientResponseException
+
+
+@pytest.fixture
+def fake_client(notify_api):
+    class FakeSmsClient(SmsClient):
+        @property
+        def name(self):
+            return 'fake'
+
+    fake_client = FakeSmsClient()
+    fake_client.init_app(notify_api, statsd_client)
+    return fake_client
+
+
+def test_send_sms(fake_client, mocker):
+    mock_send = mocker.patch.object(fake_client, 'try_send_sms')
+
+    fake_client.send_sms(
+        to='to',
+        content='content',
+        reference='reference',
+        international=False,
+        sender=None,
+    )
+
+    mock_send.assert_called_with(
+        'to', 'content', 'reference', False, 'testing'
+    )
+
+
+def test_send_sms_error(fake_client, mocker):
+    mocker.patch.object(
+        fake_client, 'try_send_sms', side_effect=SmsClientResponseException('error')
+    )
+
+    with pytest.raises(SmsClientResponseException):
+        fake_client.send_sms(
+            to='to',
+            content='content',
+            reference='reference',
+            international=False,
+            sender=None,
+        )
+
+
+def test_send_sms_override_configured_shortcode_with_sender(
+    fake_client,
+    mocker
+):
+    mock_send = mocker.patch.object(fake_client, 'try_send_sms')
+
+    fake_client.send_sms(
+        to='to',
+        content='content',
+        reference='reference',
+        international=False,
+        sender='sender'
+    )
+
+    mock_send.assert_called_with(
+        'to', 'content', 'reference', False, 'sender'
+    )

tests/app/dao/test_provider_details_dao.py

@@ -40,7 +40,7 @@ def set_primary_sms_provider(identifier):
 
 def test_can_get_sms_non_international_providers(notify_db_session):
     sms_providers = get_provider_details_by_notification_type('sms')
-    assert len(sms_providers) == 2
+    assert len(sms_providers) > 0
     assert all('sms' == prov.notification_type for prov in sms_providers)
 
 
@@ -53,8 +53,8 @@ def test_can_get_sms_international_providers(notify_db_session):
 
 def test_can_get_sms_providers_in_order_of_priority(notify_db_session):
     providers = get_provider_details_by_notification_type('sms', False)
-
+    priorities = [provider.priority for provider in providers]
-    assert providers[0].priority < providers[1].priority
+    assert priorities == sorted(priorities)
 
 
 def test_can_get_email_providers_in_order_of_priority(notify_db_session):
@@ -332,27 +332,26 @@ def test_dao_get_provider_stats(notify_db_session):
     create_ft_billing('2018-06-15', sms_template_1, provider='firetext', billable_unit=1)
     create_ft_billing('2018-06-28', sms_template_2, provider='mmg', billable_unit=2)
 
-    result = dao_get_provider_stats()
+    results = dao_get_provider_stats()
 
-    assert len(result) == 4
+    assert len(results) > 0
 
-    assert result[0].identifier == 'ses'
+    ses = next(result for result in results if result.identifier == 'ses')
-    assert result[0].display_name == 'AWS SES'
+    firetext = next(result for result in results if result.identifier == 'firetext')
-    assert result[0].created_by_name is None
+    mmg = next(result for result in results if result.identifier == 'mmg')
-    assert result[0].current_month_billable_sms == 0
 
-    assert result[1].identifier == 'firetext'
+    assert ses.display_name == 'AWS SES'
-    assert result[1].notification_type == 'sms'
+    assert ses.created_by_name is None
-    assert result[1].supports_international is False
+    assert ses.current_month_billable_sms == 0
-    assert result[1].active is True
-    assert result[1].current_month_billable_sms == 5
 
-    assert result[2].identifier == 'mmg'
+    assert firetext.display_name == 'Firetext'
-    assert result[2].display_name == 'MMG'
+    assert firetext.notification_type == 'sms'
-    assert result[2].supports_international is True
+    assert firetext.supports_international is False
-    assert result[2].active is True
+    assert firetext.active is True
-    assert result[2].current_month_billable_sms == 4
+    assert firetext.current_month_billable_sms == 5
 
-    assert result[3].identifier == 'dvla'
+    assert mmg.identifier == 'mmg'
-    assert result[3].current_month_billable_sms == 0
+    assert mmg.display_name == 'MMG'
-    assert result[3].supports_international is False
+    assert mmg.supports_international is True
+    assert mmg.active is True
+    assert mmg.current_month_billable_sms == 4

tests/app/delivery/test_send_to_providers.py

@@ -54,7 +54,7 @@ def test_provider_to_use_should_return_random_provider(mocker, notify_db_session
     ret = send_to_providers.provider_to_use('sms', international=False)
 
     mock_choices.assert_called_once_with([mmg, firetext], weights=[25, 75])
-    assert ret.get_name() == 'mmg'
+    assert ret.name == 'mmg'
 
 
 def test_provider_to_use_should_cache_repeated_calls(mocker, notify_db_session):
@@ -89,7 +89,7 @@ def test_provider_to_use_should_only_return_mmg_for_international(
     ret = send_to_providers.provider_to_use('sms', international=True)
 
     mock_choices.assert_called_once_with([mmg], weights=[100])
-    assert ret.get_name() == 'mmg'
+    assert ret.name == 'mmg'
 
 
 def test_provider_to_use_should_only_return_active_providers(mocker, restore_provider_details):
@@ -101,7 +101,7 @@ def test_provider_to_use_should_only_return_active_providers(mocker, restore_pro
     ret = send_to_providers.provider_to_use('sms')
 
     mock_choices.assert_called_once_with([firetext], weights=[100])
-    assert ret.get_name() == 'firetext'
+    assert ret.name == 'firetext'
 
 
 def test_provider_to_use_raises_if_no_active_providers(mocker, restore_provider_details):

tests/app/provider_details/test_rest.py

@@ -10,8 +10,8 @@ from tests.app.db import create_ft_billing
 def test_get_provider_details_returns_all_providers(admin_request, notify_db_session):
     json_resp = admin_request.get('provider_details.get_providers')['provider_details']
 
-    assert len(json_resp) == 4
+    assert len(json_resp) > 0
-    assert {x['identifier'] for x in json_resp} == {'ses', 'firetext', 'mmg', 'dvla'}
+    assert {'ses', 'firetext', 'mmg', 'dvla'} < {x['identifier'] for x in json_resp}
 
 
 def test_get_provider_details_by_id(client, notify_db):
@@ -45,7 +45,7 @@ def test_get_provider_contains_correct_fields(client, sample_template):
         "active", "updated_at", "supports_international",
         "current_month_billable_sms"
     }
-    assert len(json_resp) == 4
+    assert len(json_resp) > 0
     assert allowed_keys == set(json_resp[0].keys())