diff --git a/README.md b/README.md index ea3636115..8edd6bd15 100644 --- a/README.md +++ b/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 diff --git a/app/__init__.py b/app/__init__.py index cc21ef59a..15f4f1e48 100644 --- a/app/__init__.py +++ b/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) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 1b1bf6894..a274635ce 100644 --- a/app/celery/provider_tasks.py +++ b/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( diff --git a/app/clients/email/__init__.py b/app/clients/email/__init__.py index e680be23d..004f76b3a 100644 --- a/app/clients/email/__init__.py +++ b/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.') diff --git a/app/clients/email/aws_ses.py b/app/clients/email/aws_ses.py index 4588a53b7..1f8b2f75c 100644 --- a/app/clients/email/aws_ses.py +++ b/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): - return self.name + @property + def name(self): + return 'ses' def send_email(self, source, diff --git a/app/clients/email/aws_ses_stub.py b/app/clients/email/aws_ses_stub.py index 4d78e22b5..bce9d3e9a 100644 --- a/app/clients/email/aws_ses_stub.py +++ b/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): - return self.name + @property + def name(self): + return 'ses' def send_email(self, source, diff --git a/app/clients/sms/__init__.py b/app/clients/sms/__init__.py index e9f90f06a..38f3c55c9 100644 --- a/app/clients/sms/__init__.py +++ b/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.') diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index 0d9b23b44..0d8f5a685 100644 --- a/app/clients/sms/firetext.py +++ b/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): - super(SmsClient, self).__init__(*args, **kwargs) - self.current_app = current_app - self.api_key = current_app.config.get('FIRETEXT_API_KEY') - self.international_api_key = current_app.config.get('FIRETEXT_INTERNATIONAL_API_KEY') - 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 init_app(self, *args, **kwargs): + super().init_app(*args, **kwargs) + self.api_key = self.current_app.config.get('FIRETEXT_API_KEY') + self.international_api_key = self.current_app.config.get('FIRETEXT_INTERNATIONAL_API_KEY') + self.url = self.current_app.config.get('FIRETEXT_URL') - def get_name(self): - return self.name + @property + def name(self): + return 'firetext' - def record_outcome(self, success, response): - 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): + def try_send_sms(self, to, content, reference, international, sender): 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() - 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: - 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) + raise ValueError("Expected 'code' to be '0'") + except (ValueError, AttributeError): + raise SmsClientResponseException("Invalid response JSON") + except RequestException: + raise SmsClientResponseException("Request failed") + return response diff --git a/app/clients/sms/mmg.py b/app/clients/sms/mmg.py index ec32fa0a9..b05c275ea 100644 --- a/app/clients/sms/mmg.py +++ b/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): - super(SmsClient, self).__init__(*args, **kwargs) - self.current_app = current_app - self.api_key = current_app.config.get('MMG_API_KEY') - 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 init_app(self, *args, **kwargs): + super().init_app(*args, **kwargs) + self.api_key = self.current_app.config.get('MMG_API_KEY') + self.mmg_url = self.current_app.config.get('MMG_URL') - def record_outcome(self, success, response): - 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.mmg_url, - status_code - ) + @property + def name(self): + return 'mmg' - 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.warning(log_message) - - def get_name(self): - return self.name - - def send_sms(self, to, content, reference, international, multi=True, sender=None): + def try_send_sms(self, to, content, reference, international, sender): 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: - self.record_outcome(False, response) - raise MMGClientResponseException(response=response, exception=e) - self.record_outcome(True, response) - 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)) + except (ValueError, AttributeError): + raise SmsClientResponseException("Invalid response JSON") + except RequestException: + raise SmsClientResponseException("Request failed") return response diff --git a/app/clients/sms/reach.py b/app/clients/sms/reach.py index d9b016e2a..3541f8b5c 100644 --- a/app/clients/sms/reach.py +++ b/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): - pass # TODO + @property + def name(self): + return 'reach' - def send_sms(self, to, content, reference, international, multi=True, sender=None): - pass # TODO + def try_send_sms(self, to, content, reference, international, sender): + 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 diff --git a/app/config.py b/app/config.py index a9e95f930..f36fdb7c5 100644 --- a/app/config.py +++ b/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'] diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 37994691d..162bb3f3a 100644 --- a/app/delivery/send_to_providers.py +++ b/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) diff --git a/migrations/versions/0367_add_reach.py b/migrations/versions/0367_add_reach.py new file mode 100644 index 000000000..92eef85fd --- /dev/null +++ b/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'") diff --git a/tests/app/clients/test_firetext.py b/tests/app/clients/test_firetext.py index a24fba3ba..75febf605 100644 --- a/tests/app/clients/test_firetext.py +++ b/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 '"description": "Some kind of error"' in exc.value.text - assert '"code": 1' in exc.value.text + assert "Invalid response JSON" in str(exc.value) -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 exc.value.text == '{"something": "gone bad"}' - assert type(exc.value.exception) == HTTPError + assert "Request failed" in str(exc.value) -def test_send_sms_override_configured_shortcode_with_sender(mocker, mock_firetext_client): - to = '+447234567890' - content = 'my message' - reference = 'my reference' - response_dict = { - 'code': 0, - } - sender = 'fromservice' +def test_try_send_sms_raises_if_firetext_fails_to_return_json(notify_api, mock_firetext_client): + to = content = reference = 'foo' + response_dict = 'NOT AT ALL VALID JSON {"key" : "value"}}' - 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, sender=sender) + with pytest.raises(SmsClientResponseException) as exc, requests_mock.Mocker() as request_mock: + request_mock.post('https://example.com/firetext', text=response_dict, status_code=200) + mock_firetext_client.try_send_sms(to, content, reference, False, 'sender') - request_args = parse_qs(request_mock.request_history[0].text) - assert request_args['from'][0] == 'fromservice' + assert "Invalid response JSON" in str(exc.value) -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 exc.value.text == 'Gateway Time-out' + assert "Request failed" in str(exc.value) -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 exc.value.text == 'Gateway Time-out' + assert "Request failed" in str(exc.value) diff --git a/tests/app/clients/test_mmg.py b/tests/app/clients/test_mmg.py index 096c57207..767701ed9 100644 --- a/tests/app/clients/test_mmg.py +++ b/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 MMGClientResponseException, get_mmg_responses +from app.clients.sms.mmg import SmsClientResponseException, 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 '"Error": 206' in exc.value.text - assert '"Description": "Some kind of error"' in exc.value.text - assert type(exc.value.exception) == HTTPError + assert "Request failed" in str(exc.value) -def test_send_sms_override_configured_shortcode_with_sender(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): +def test_try_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 exc.value.status_code == 200 - assert exc.value.text == 'NOT AT ALL VALID JSON {"key" : "value"}}' + assert "Invalid response JSON" in str(exc.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 exc.value.text == 'Gateway Time-out' + assert "Request failed" in str(exc.value) -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 exc.value.text == 'Gateway Time-out' + assert "Request failed" in str(exc.value) diff --git a/tests/app/clients/test_reach.py b/tests/app/clients/test_reach.py index 00cf4f034..b62a5fd0b 100644 --- a/tests/app/clients/test_reach.py +++ b/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) diff --git a/tests/app/clients/test_sms.py b/tests/app/clients/test_sms.py new file mode 100644 index 000000000..b61777a35 --- /dev/null +++ b/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' + ) diff --git a/tests/app/dao/test_provider_details_dao.py b/tests/app/dao/test_provider_details_dao.py index eb7ed5d98..5e68498e9 100644 --- a/tests/app/dao/test_provider_details_dao.py +++ b/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) - - assert providers[0].priority < providers[1].priority + priorities = [provider.priority for provider in providers] + 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' - assert result[0].display_name == 'AWS SES' - assert result[0].created_by_name is None - assert result[0].current_month_billable_sms == 0 + ses = next(result for result in results if result.identifier == 'ses') + firetext = next(result for result in results if result.identifier == 'firetext') + mmg = next(result for result in results if result.identifier == 'mmg') - assert result[1].identifier == 'firetext' - assert result[1].notification_type == 'sms' - assert result[1].supports_international is False - assert result[1].active is True - assert result[1].current_month_billable_sms == 5 + assert ses.display_name == 'AWS SES' + assert ses.created_by_name is None + assert ses.current_month_billable_sms == 0 - assert result[2].identifier == 'mmg' - assert result[2].display_name == 'MMG' - assert result[2].supports_international is True - assert result[2].active is True - assert result[2].current_month_billable_sms == 4 + assert firetext.display_name == 'Firetext' + assert firetext.notification_type == 'sms' + assert firetext.supports_international is False + assert firetext.active is True + assert firetext.current_month_billable_sms == 5 - assert result[3].identifier == 'dvla' - assert result[3].current_month_billable_sms == 0 - assert result[3].supports_international is False + assert mmg.identifier == 'mmg' + assert mmg.display_name == 'MMG' + assert mmg.supports_international is True + assert mmg.active is True + assert mmg.current_month_billable_sms == 4 diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 9fde06690..404d9c513 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/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): diff --git a/tests/app/provider_details/test_rest.py b/tests/app/provider_details/test_rest.py index 79606b7c0..4e973568f 100644 --- a/tests/app/provider_details/test_rest.py +++ b/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 {x['identifier'] for x in json_resp} == {'ses', 'firetext', 'mmg', 'dvla'} + assert len(json_resp) > 0 + 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())