Updated clients to have a more robust error handling

- fire text and omg much more similar. Ready to be combined.
- Error handling now for JSON valid responses

app/celery/provider_tasks.py

@@ -8,6 +8,7 @@ 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

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,35 @@ 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 {} response text'{}'".format(
+            "POST",
+            "succeeded" if success else "failed",
+            self.url,
+            response.status_code,
+            response.text
+        )
+
+        if not success:
+            self.statsd_client.incr("clients.firetext.error")
+            self.current_app.logger.error(log_message)
+        else:
+            self.current_app.logger.info(log_message)
+            self.statsd_client.incr("clients.firetext.success")
+
+    @staticmethod
+    def check_response(response):
+        response.raise_for_status()
+        json.loads(response.text)
+        if response.json()['code'] != 0:
+            raise ValueError()
+
     def send_sms(self, to, content, reference, sender=None):
 
         data = {
@@ -81,36 +104,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.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,22 @@ 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 {} response text'{}'".format(
+            "POST",
+            "succeeded" if success else "failed",
+            self.mmg_url,
+            response.status_code,
+            response.text
+        )
+
+        if not success:
+            self.statsd_client.incr("clients.mmg.error")
+            self.current_app.logger.error(log_message)
+        else:
+            self.current_app.logger.info(log_message)
+            self.statsd_client.incr("clients.mmg.success")
+
     def get_name(self):
         return self.name
 
@@ -79,38 +97,30 @@ class MMGClient(SmsClient):
         }
 
         start_time = monotonic()
-
         try:
-            response = request("POST", self.mmg_url,
+            response = request(
-                               data=json.dumps(data),
+                "POST",
-                               headers={'Content-Type': 'application/json',
+                self.mmg_url,
-                                        'Authorization': 'Basic {}'.format(self.api_key)})
+                data=json.dumps(data),
-            if response.status_code != 200:
+                headers={
-                raise MMGClientException(response.json())
+                    'Content-Type': 'application/json',
+                    'Authorization': 'Basic {}'.format(self.api_key)
+                }
+            )
+
             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

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"}}'