diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 632b4f937..c94120e79 100644 --- a/app/celery/provider_tasks.py +++ b/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 diff --git a/app/clients/sms/__init__.py b/app/clients/sms/__init__.py index 90d63ed0d..e6accdb8a 100644 --- a/app/clients/sms/__init__.py +++ b/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): diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index e9760a4e6..5ffb749eb 100644 --- a/app/clients/sms/firetext.py +++ b/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 ( - SmsClient, - SmsClientException -) +from app.clients.sms import (SmsClient, SmsClientResponseException) 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): - def __init__(self, response): - self.code = response['code'] - self.description = response['description'] +class FiretextClientResponseException(SmsClientResponseException): + def __init__(self, response, exception): + self.status_code = response.status_code + 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( - "API {} request on {} succeeded with {} '{}'".format( - "POST", - "https://www.firetext.co.uk/api/sendsms", - response.status_code, - firetext_response.items() - ) - ) + try: + json.loads(response.text) + if response.json()['code'] != 0: + raise ValueError() + except (ValueError, AttributeError) as e: + 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) - logger.error( - "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 + 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 finished in {}".format(elapsed_time)) - self.statsd_client.incr("clients.firetext.success") self.statsd_client.timing("clients.firetext.request-time", elapsed_time) return response diff --git a/app/clients/sms/loadtesting.py b/app/clients/sms/loadtesting.py index a3e500ba8..53ebca9f9 100644 --- a/app/clients/sms/loadtesting.py +++ b/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 diff --git a/app/clients/sms/mmg.py b/app/clients/sms/mmg.py index 066c1f259..8919b1358 100644 --- a/app/clients/sms/mmg.py +++ b/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): - def __init__(self, error_response): - self.code = error_response['Error'] - self.description = error_response['Description'] +class MMGClientResponseException(SmsClientResponseException): + def __init__(self, response, exception): + self.status_code = response.status_code + 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, - data=json.dumps(data), - headers={'Content-Type': 'application/json', - 'Authorization': 'Basic {}'.format(self.api_key)}) - if response.status_code != 200: - raise MMGClientException(response.json()) + response = request( + "POST", + self.mmg_url, + data=json.dumps(data), + headers={ + 'Content-Type': 'application/json', + 'Authorization': 'Basic {}'.format(self.api_key) + } + ) + response.raise_for_status() - self.current_app.logger.info( - "API {} request on {} succeeded with {} '{}'".format( - "POST", - self.mmg_url, - response.status_code, - response.json().items() - ) - ) + try: + json.loads(response.text) + except (ValueError, AttributeError) as e: + self.record_outcome(False, response) + raise MMGClientResponseException(response=response, exception=e) + self.record_outcome(True, response) except RequestException as e: - api_error = HTTPError.create(e) - self.current_app.logger.error( - "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 + 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.statsd_client.incr("clients.mmg.success") self.current_app.logger.info("MMG request finished in {}".format(elapsed_time)) + return response diff --git a/tests/app/clients/test_firetext.py b/tests/app/clients/test_firetext.py index ea8630913..80737e23d 100644 --- a/tests/app/clients/test_firetext.py +++ b/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.description == 'Some kind of error' + assert exc.value.status_code == 200 + 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): diff --git a/tests/app/clients/test_mmg.py b/tests/app/clients/test_mmg.py index 649764d89..d87f2f914 100644 --- a/tests/app/clients/test_mmg.py +++ b/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.description == 'Some kind of error' + assert exc.value.status_code == 400 + 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"}}'