From e6e16a81d03f9a9f8213fa4bf3239a19550b8341 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 24 Mar 2022 17:31:53 +0000 Subject: [PATCH 01/12] Simplify getting name of email / sms providers Previously we used a combination of "provider.name" and "get_name()" which was confusing. Using a non-property function also gave me the impression that the name was more dynamic than it actually is. --- app/clients/email/__init__.py | 3 ++- app/clients/email/aws_ses.py | 6 +++--- app/clients/email/aws_ses_stub.py | 6 +++--- app/clients/sms/__init__.py | 3 ++- app/clients/sms/firetext.py | 6 +++--- app/clients/sms/mmg.py | 6 +++--- app/delivery/send_to_providers.py | 6 +++--- tests/app/delivery/test_send_to_providers.py | 6 +++--- 8 files changed, 22 insertions(+), 20 deletions(-) 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..0ca4ff7ae 100644 --- a/app/clients/sms/__init__.py +++ b/app/clients/sms/__init__.py @@ -21,5 +21,6 @@ class SmsClient(Client): def 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..3bece886e 100644 --- a/app/clients/sms/firetext.py +++ b/app/clients/sms/firetext.py @@ -68,12 +68,12 @@ class FiretextClient(SmsClient): 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 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 diff --git a/app/clients/sms/mmg.py b/app/clients/sms/mmg.py index ec32fa0a9..1013424de 100644 --- a/app/clients/sms/mmg.py +++ b/app/clients/sms/mmg.py @@ -74,7 +74,6 @@ class MMGClient(SmsClient): 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') @@ -94,8 +93,9 @@ class MMGClient(SmsClient): self.statsd_client.incr("clients.mmg.error") self.current_app.logger.warning(log_message) - def get_name(self): - return self.name + @property + def name(self): + return 'mmg' def send_sms(self, to, content, reference, international, multi=True, sender=None): data = { 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/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): From 3988a6cd073bdcb1b1b9672c695470696095ae20 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 25 Mar 2022 12:35:22 +0000 Subject: [PATCH 02/12] Include exception info in SMS warning log This makes it easier to debug failures when adding a new provider. --- app/celery/provider_tasks.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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( From 35f710bdf308d3bfe244cf0c784e9ed520255ca1 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 25 Mar 2022 15:41:37 +0000 Subject: [PATCH 03/12] Remove redundant "multi" parameter for MMG client This is never overridden and can't be used in practie because all SMS clients have to use the same interface. Removing it will make it possible to DRY-up some of the code in this method. --- app/clients/sms/mmg.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/clients/sms/mmg.py b/app/clients/sms/mmg.py index 1013424de..b805e11b0 100644 --- a/app/clients/sms/mmg.py +++ b/app/clients/sms/mmg.py @@ -97,14 +97,14 @@ class MMGClient(SmsClient): def name(self): return 'mmg' - def send_sms(self, to, content, reference, international, multi=True, sender=None): + def send_sms(self, to, content, reference, international, sender=None): data = { "reqType": "BULK", "MSISDN": to, "msg": content, "sender": self.from_number if sender is None else sender, "cid": reference, - "multi": multi + "multi": True } start_time = monotonic() From 22e055f4d11f4e396cb8b34e4bb2911112a213bb Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 24 Mar 2022 17:33:45 +0000 Subject: [PATCH 04/12] DRY-up recording the outcome of SMS sending This reduces the code to copy when we add a new provider. I don't think we need to log the URL or status code each time: - The URL is always the same. - A "200" status code is implicit in "success". - Other status codes will be reported as exceptions. Removing these specific elements means "record_outcome" is generic and can be de-duplicated in the base class. --- app/clients/sms/__init__.py | 13 +++++++++++++ app/clients/sms/firetext.py | 23 +++-------------------- app/clients/sms/mmg.py | 22 +++------------------- 3 files changed, 19 insertions(+), 39 deletions(-) diff --git a/app/clients/sms/__init__.py b/app/clients/sms/__init__.py index 0ca4ff7ae..20b7d3fda 100644 --- a/app/clients/sms/__init__.py +++ b/app/clients/sms/__init__.py @@ -18,6 +18,19 @@ class SmsClient(Client): Base Sms client for sending smss. ''' + 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, *args, **kwargs): raise NotImplementedError('TODO Need to implement.') diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index 3bece886e..54b9af0a3 100644 --- a/app/clients/sms/firetext.py +++ b/app/clients/sms/firetext.py @@ -75,23 +75,6 @@ class FiretextClient(SmsClient): 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): data = { "apiKey": self.international_api_key if international else self.api_key, @@ -115,11 +98,11 @@ class FiretextClient(SmsClient): if response.json()['code'] != 0: raise ValueError() except (ValueError, AttributeError) as e: - self.record_outcome(False, response) + self.record_outcome(False) raise FiretextClientResponseException(response=response, exception=e) - self.record_outcome(True, response) + self.record_outcome(True) except RequestException as e: - self.record_outcome(False, e.response) + self.record_outcome(False) raise FiretextClientResponseException(response=e.response, exception=e) finally: elapsed_time = monotonic() - start_time diff --git a/app/clients/sms/mmg.py b/app/clients/sms/mmg.py index b805e11b0..26b035557 100644 --- a/app/clients/sms/mmg.py +++ b/app/clients/sms/mmg.py @@ -77,22 +77,6 @@ class MMGClient(SmsClient): self.statsd_client = statsd_client self.mmg_url = 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 - ) - - 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) - @property def name(self): return 'mmg' @@ -124,11 +108,11 @@ class MMGClient(SmsClient): try: json.loads(response.text) except (ValueError, AttributeError) as e: - self.record_outcome(False, response) + self.record_outcome(False) raise MMGClientResponseException(response=response, exception=e) - self.record_outcome(True, response) + self.record_outcome(True) except RequestException as e: - self.record_outcome(False, e.response) + self.record_outcome(False) raise MMGClientResponseException(response=e.response, exception=e) finally: elapsed_time = monotonic() - start_time From 84578e8a1d87581f24f472844d79ae5d5b204eec Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 25 Mar 2022 11:46:01 +0000 Subject: [PATCH 05/12] Make provider tests agnostic to actual data The provider tests are coupled to actual data in the DB, but we shouldn't have to overhaul the tests when this changes. Assuming we don't delete old providers, just testing a subset of the fixture data should give us enough confidence in the code. --- tests/app/dao/test_provider_details_dao.py | 43 +++++++++++----------- tests/app/provider_details/test_rest.py | 6 +-- 2 files changed, 24 insertions(+), 25 deletions(-) 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/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()) From 3b082477f05588d638a21ce0874356678fd429df Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 25 Mar 2022 13:05:23 +0000 Subject: [PATCH 06/12] DRY-up logging and metrics for sending SMS This avoids duplicating it as we add a new provider and means we can test it all in one place (although it wasn't tested before). I'm not sure why the previous code did "super(..)__init__" in a non-init function - it's a bit late! - so I've just replaced it with a call to the new "init_app" function in the parent class. --- app/clients/sms/__init__.py | 24 +++++++++++++++- app/clients/sms/firetext.py | 26 ++++++----------- app/clients/sms/mmg.py | 23 ++++----------- tests/app/clients/test_firetext.py | 32 ++++++++++----------- tests/app/clients/test_mmg.py | 28 +++++++++---------- tests/app/clients/test_sms.py | 45 ++++++++++++++++++++++++++++++ 6 files changed, 112 insertions(+), 66 deletions(-) create mode 100644 tests/app/clients/test_sms.py diff --git a/app/clients/sms/__init__.py b/app/clients/sms/__init__.py index 20b7d3fda..c7a87812d 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 @@ -18,6 +20,10 @@ class SmsClient(Client): Base Sms client for sending smss. ''' + def init_app(self, current_app, statsd_client): + self.current_app = current_app + self.statsd_client = statsd_client + def record_outcome(self, success): log_message = "Provider request for {} {}".format( self.name, @@ -31,7 +37,23 @@ class SmsClient(Client): self.statsd_client.incr(f"clients.{self.name}.error") self.current_app.logger.warning(log_message) - def send_sms(self, *args, **kwargs): + def send_sms(self, to, content, reference, international, sender=None): + start_time = monotonic() + + 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.') @property diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index 54b9af0a3..c1e529995 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 @@ -62,20 +61,18 @@ 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.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.from_number = self.current_app.config.get('FROM_NUMBER') + self.url = self.current_app.config.get('FIRETEXT_URL') @property def name(self): return 'firetext' - def send_sms(self, to, content, reference, international, sender=None): + def try_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, @@ -84,7 +81,6 @@ class FiretextClient(SmsClient): "reference": reference } - start_time = monotonic() try: response = request( "POST", @@ -98,14 +94,8 @@ class FiretextClient(SmsClient): if response.json()['code'] != 0: raise ValueError() except (ValueError, AttributeError) as e: - self.record_outcome(False) raise FiretextClientResponseException(response=response, exception=e) - self.record_outcome(True) except RequestException as e: - self.record_outcome(False) 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 diff --git a/app/clients/sms/mmg.py b/app/clients/sms/mmg.py index 26b035557..84de84b1e 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,19 +68,17 @@ 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.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.from_number = self.current_app.config.get('FROM_NUMBER') + self.mmg_url = self.current_app.config.get('MMG_URL') @property def name(self): return 'mmg' - def send_sms(self, to, content, reference, international, sender=None): + def try_send_sms(self, to, content, reference, international, sender=None): data = { "reqType": "BULK", "MSISDN": to, @@ -91,7 +88,6 @@ class MMGClient(SmsClient): "multi": True } - start_time = monotonic() try: response = request( "POST", @@ -108,15 +104,8 @@ class MMGClient(SmsClient): try: json.loads(response.text) except (ValueError, AttributeError) as e: - self.record_outcome(False) raise MMGClientResponseException(response=response, exception=e) - self.record_outcome(True) except RequestException as e: - self.record_outcome(False) 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 diff --git a/tests/app/clients/test_firetext.py b/tests/app/clients/test_firetext.py index a24fba3ba..4fae6226e 100644 --- a/tests/app/clients/test_firetext.py +++ b/tests/app/clients/test_firetext.py @@ -36,7 +36,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 +47,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) response_json = response.json() assert response.status_code == 200 @@ -55,7 +55,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 +65,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) assert request_mock.call_count == 1 assert request_mock.request_history[0].url == 'https://example.com/firetext' @@ -79,7 +79,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 +89,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) assert request_mock.call_count == 1 assert request_mock.request_history[0].url == 'https://example.com/firetext' @@ -103,7 +103,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,27 +114,27 @@ 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) 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_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) 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): +def test_try_send_sms_override_configured_shortcode_with_sender(mocker, mock_firetext_client): to = '+447234567890' content = 'my message' reference = 'my reference' @@ -145,29 +145,29 @@ def test_send_sms_override_configured_shortcode_with_sender(mocker, mock_firetex 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) + mock_firetext_client.try_send_sms(to, content, reference, False, sender=sender) request_args = parse_qs(request_mock.request_history[0].text) 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: 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) assert exc.value.status_code == 504 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: 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) assert exc.value.status_code == 504 assert exc.value.text == 'Gateway Time-out' diff --git a/tests/app/clients/test_mmg.py b/tests/app/clients/test_mmg.py index 096c57207..34718c459 100644 --- a/tests/app/clients/test_mmg.py +++ b/tests/app/clients/test_mmg.py @@ -38,20 +38,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) 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 +59,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) assert request_mock.call_count == 1 assert request_mock.request_history[0].url == 'https://example.com/mmg' @@ -74,7 +74,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,7 +83,7 @@ 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) assert exc.value.status_code == 400 assert '"Error": 206' in exc.value.text @@ -91,7 +91,7 @@ def test_send_sms_raises_if_mmg_rejects(notify_api, mocker): assert type(exc.value.exception) == HTTPError -def test_send_sms_override_configured_shortcode_with_sender(notify_api, mocker): +def test_try_send_sms_override_configured_shortcode_with_sender(notify_api, mocker): to = '+447234567890' content = 'my message' reference = 'my reference' @@ -100,42 +100,42 @@ def test_send_sms_override_configured_shortcode_with_sender(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, sender=sender) + mmg_client.try_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) 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"}}' -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: 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) assert exc.value.status_code == 504 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: 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) assert exc.value.status_code == 504 assert exc.value.text == 'Gateway Time-out' diff --git a/tests/app/clients/test_sms.py b/tests/app/clients/test_sms.py new file mode 100644 index 000000000..6741b4f13 --- /dev/null +++ b/tests/app/clients/test_sms.py @@ -0,0 +1,45 @@ +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, + ) + + mock_send.assert_called_with( + 'to', 'content', 'reference', False, None + ) + + +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, + ) From 27ddc4501e3a83120c0823d064570d7e158f6825 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 25 Mar 2022 13:26:16 +0000 Subject: [PATCH 07/12] DRY-up overriding shortcode with sender This avoids duplicating the logic when we add a new provider. --- app/clients/sms/__init__.py | 2 ++ app/clients/sms/firetext.py | 5 ++--- app/clients/sms/mmg.py | 5 ++--- tests/app/clients/test_firetext.py | 31 +++++++----------------------- tests/app/clients/test_mmg.py | 27 ++++++-------------------- tests/app/clients/test_sms.py | 21 +++++++++++++++++++- 6 files changed, 39 insertions(+), 52 deletions(-) diff --git a/app/clients/sms/__init__.py b/app/clients/sms/__init__.py index c7a87812d..6a10374f2 100644 --- a/app/clients/sms/__init__.py +++ b/app/clients/sms/__init__.py @@ -23,6 +23,7 @@ class SmsClient(Client): 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( @@ -39,6 +40,7 @@ class SmsClient(Client): def send_sms(self, to, content, reference, international, sender=None): start_time = monotonic() + sender = self.from_number if sender is None else sender try: response = self.try_send_sms(to, content, reference, international, sender) diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index c1e529995..f2336f2aa 100644 --- a/app/clients/sms/firetext.py +++ b/app/clients/sms/firetext.py @@ -65,17 +65,16 @@ class FiretextClient(SmsClient): 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.from_number = self.current_app.config.get('FROM_NUMBER') self.url = self.current_app.config.get('FIRETEXT_URL') @property def name(self): return 'firetext' - def try_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 diff --git a/app/clients/sms/mmg.py b/app/clients/sms/mmg.py index 84de84b1e..a1408a16e 100644 --- a/app/clients/sms/mmg.py +++ b/app/clients/sms/mmg.py @@ -71,19 +71,18 @@ class MMGClient(SmsClient): def init_app(self, *args, **kwargs): super().init_app(*args, **kwargs) self.api_key = self.current_app.config.get('MMG_API_KEY') - self.from_number = self.current_app.config.get('FROM_NUMBER') self.mmg_url = self.current_app.config.get('MMG_URL') @property def name(self): return 'mmg' - def try_send_sms(self, to, content, reference, international, 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": True } diff --git a/tests/app/clients/test_firetext.py b/tests/app/clients/test_firetext.py index 4fae6226e..74cba31c8 100644 --- a/tests/app/clients/test_firetext.py +++ b/tests/app/clients/test_firetext.py @@ -47,7 +47,7 @@ def test_try_send_sms_successful_returns_firetext_response(mocker, mock_firetext with requests_mock.Mocker() as request_mock: request_mock.post('https://example.com/firetext', json=response_dict, status_code=200) - response = mock_firetext_client.try_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 @@ -65,7 +65,7 @@ def test_try_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.try_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' @@ -89,7 +89,7 @@ def test_try_send_sms_calls_firetext_correctly_for_international(mocker, mock_fi with requests_mock.Mocker() as request_mock: request_mock.post('https://example.com/firetext', json=response_dict, status_code=200) - mock_firetext_client.try_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' @@ -114,7 +114,7 @@ def test_try_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.try_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 @@ -127,36 +127,19 @@ def test_try_send_sms_raises_if_firetext_rejects_with_unexpected_data(mocker, mo 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.try_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 -def test_try_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' - - with requests_mock.Mocker() as request_mock: - request_mock.post('https://example.com/firetext', json=response_dict, status_code=200) - mock_firetext_client.try_send_sms(to, content, reference, False, sender=sender) - - request_args = parse_qs(request_mock.request_history[0].text) - assert request_args['from'][0] == 'fromservice' - - 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: rmock.register_uri('POST', 'https://example.com/firetext', exc=ConnectTimeout) - mock_firetext_client.try_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' @@ -167,7 +150,7 @@ def test_try_send_sms_raises_if_firetext_rejects_with_read_timeout(rmock, mock_f with pytest.raises(FiretextClientResponseException) as exc: rmock.register_uri('POST', 'https://example.com/firetext', exc=ReadTimeout) - mock_firetext_client.try_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' diff --git a/tests/app/clients/test_mmg.py b/tests/app/clients/test_mmg.py index 34718c459..0164af452 100644 --- a/tests/app/clients/test_mmg.py +++ b/tests/app/clients/test_mmg.py @@ -44,7 +44,7 @@ def test_try_send_sms_successful_returns_mmg_response(notify_api, mocker): with requests_mock.Mocker() as request_mock: request_mock.post('https://example.com/mmg', json=response_dict, status_code=200) - response = mmg_client.try_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 @@ -59,7 +59,7 @@ def test_try_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.try_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' @@ -83,7 +83,7 @@ def test_try_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.try_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 @@ -91,28 +91,13 @@ def test_try_send_sms_raises_if_mmg_rejects(notify_api, mocker): assert type(exc.value.exception) == HTTPError -def test_try_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.try_send_sms(to, content, reference, False, sender=sender) - - request_args = request_mock.request_history[0].json() - assert request_args['sender'] == 'fromservice' - - 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.try_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 @@ -124,7 +109,7 @@ def test_try_send_sms_raises_if_mmg_rejects_with_connect_timeout(rmock): with pytest.raises(MMGClientResponseException) as exc: rmock.register_uri('POST', 'https://example.com/mmg', exc=ConnectTimeout) - mmg_client.try_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' @@ -135,7 +120,7 @@ def test_try_send_sms_raises_if_mmg_rejects_with_read_timeout(rmock): with pytest.raises(MMGClientResponseException) as exc: rmock.register_uri('POST', 'https://example.com/mmg', exc=ReadTimeout) - mmg_client.try_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' diff --git a/tests/app/clients/test_sms.py b/tests/app/clients/test_sms.py index 6741b4f13..de0b06441 100644 --- a/tests/app/clients/test_sms.py +++ b/tests/app/clients/test_sms.py @@ -27,7 +27,7 @@ def test_send_sms(fake_client, mocker): ) mock_send.assert_called_with( - 'to', 'content', 'reference', False, None + 'to', 'content', 'reference', False, 'testing' ) @@ -43,3 +43,22 @@ def test_send_sms_error(fake_client, mocker): reference='reference', international=False, ) + + +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' + ) From 015152bab2f6edb4d865116f5d96713d4e329c6d Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 25 Mar 2022 15:03:52 +0000 Subject: [PATCH 08/12] Add boilerplate for sending SMS via Reach This works in conjunction with the new SMS provider stub [^1]. Local testing: - Run the migrations to add Reach as an inactive provider. - Activate the Reach provider locally and deactivate the others. update provider_details set priority = 100, active = false where notification_type = 'sms'; update provider_details set active = true where identifier = 'reach'; - Tweak your local environment to point at the SMS stub. export REACH_URL="http://host.docker.internal:6300/reach" - Start / restart Celery to pick up the config change. - Send a SMS via the Admin app and see the stub log it. - Reset your environment so you can send normal SMS. update provider_details set active = true where notification_type = 'sms'; update provider_details set active = false where identifier = 'reach'; [^1]: https://github.com/alphagov/notifications-sms-provider-stub/pull/10 --- README.md | 2 + app/__init__.py | 8 ++- app/clients/sms/reach.py | 50 ++++++++++++-- app/config.py | 2 + migrations/versions/0367_add_reach.py | 54 +++++++++++++++ tests/app/clients/test_reach.py | 94 ++++++++++++++++++++++++++- 6 files changed, 203 insertions(+), 7 deletions(-) create mode 100644 migrations/versions/0367_add_reach.py 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/clients/sms/reach.py b/app/clients/sms/reach.py index d9b016e2a..ad4eee17f 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 @@ -13,13 +17,49 @@ def get_reach_responses(status, detailed_status_code=None): class ReachClientResponseException(SmsClientResponseException): - pass # TODO (custom exception for errors) + 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 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) as e: + raise ReachClientResponseException(response=response, exception=e) + except RequestException as e: + raise ReachClientResponseException(response=e.response, exception=e) + + 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/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_reach.py b/tests/app/clients/test_reach.py index 00cf4f034..7cc8b2bc3 100644 --- a/tests/app/clients/test_reach.py +++ b/tests/app/clients/test_reach.py @@ -1 +1,93 @@ -# TODO: all of the tests +import pytest +import requests_mock +from requests import HTTPError +from requests.exceptions import ConnectTimeout, ReadTimeout + +from app import reach_client +from app.clients.sms import SmsClientResponseException +from app.clients.sms.reach import ReachClientResponseException + +# 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 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_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 '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"}}' + + +def test_try_send_sms_raises_if_reach_rejects_with_connect_timeout(rmock): + to = content = reference = 'foo' + + with pytest.raises(ReachClientResponseException) as exc: + rmock.register_uri('POST', 'https://example.com/reach', exc=ConnectTimeout) + reach_client.try_send_sms(to, content, reference, False, 'sender') + + assert exc.value.status_code == 504 + assert exc.value.text == 'Gateway Time-out' + + +def test_try_send_sms_raises_if_reach_rejects_with_read_timeout(rmock): + to = content = reference = 'foo' + + with pytest.raises(ReachClientResponseException) as exc: + rmock.register_uri('POST', 'https://example.com/reach', exc=ReadTimeout) + reach_client.try_send_sms(to, content, reference, False, 'sender') + + assert exc.value.status_code == 504 + assert exc.value.text == 'Gateway Time-out' From a2e1d030095bcb7bb5829a37424e1e5fb8937dc3 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 29 Mar 2022 12:37:42 +0100 Subject: [PATCH 09/12] Require "sender" argument to send_sms method In response to [^1]. [^1]: https://github.com/alphagov/notifications-api/pull/3493#discussion_r836616675 --- app/clients/sms/__init__.py | 2 +- tests/app/clients/test_sms.py | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/app/clients/sms/__init__.py b/app/clients/sms/__init__.py index 6a10374f2..3a2ba1367 100644 --- a/app/clients/sms/__init__.py +++ b/app/clients/sms/__init__.py @@ -38,7 +38,7 @@ class SmsClient(Client): 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=None): + def send_sms(self, to, content, reference, international, sender): start_time = monotonic() sender = self.from_number if sender is None else sender diff --git a/tests/app/clients/test_sms.py b/tests/app/clients/test_sms.py index de0b06441..b61777a35 100644 --- a/tests/app/clients/test_sms.py +++ b/tests/app/clients/test_sms.py @@ -24,6 +24,7 @@ def test_send_sms(fake_client, mocker): content='content', reference='reference', international=False, + sender=None, ) mock_send.assert_called_with( @@ -42,6 +43,7 @@ def test_send_sms_error(fake_client, mocker): content='content', reference='reference', international=False, + sender=None, ) From 8432be4fc1e2a3e14136b4b50277e333cf6752e7 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 29 Mar 2022 14:12:19 +0100 Subject: [PATCH 10/12] Add missing test for invalid Firetext JSON This is already tested for MMG (and Reach). --- tests/app/clients/test_firetext.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/app/clients/test_firetext.py b/tests/app/clients/test_firetext.py index 74cba31c8..089a61fc0 100644 --- a/tests/app/clients/test_firetext.py +++ b/tests/app/clients/test_firetext.py @@ -134,6 +134,19 @@ def test_try_send_sms_raises_if_firetext_rejects_with_unexpected_data(mocker, mo assert type(exc.value.exception) == HTTPError +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 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') + + 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"}}' + + def test_try_send_sms_raises_if_firetext_rejects_with_connect_timeout(rmock, mock_firetext_client): to = content = reference = 'foo' From 7d92a0869a9c61f77e17e526099e0c209e412f5a Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 29 Mar 2022 14:18:00 +0100 Subject: [PATCH 11/12] Remove per-client SMS exception classes In response to: [^1]. The stacktrace conveys the same and more information. We don't do anything different for each exception class, so there's no value in having three of them over one exception. I did think about DRYing-up the duplicate exception behaviour into the base class one. This isn't ideal because the base class would be making assumptions about how inheriting classes make requests, which might change with future providers. Although it might be nice to have more info in the top-level message, we'll still get it in the stacktrace e.g. ValueError: Expected 'code' to be '0' During handling of the above exception, another exception occurred: app.clients.sms.SmsClientResponseException: SMS client error (Invalid response JSON) requests.exceptions.ReadTimeout During handling of the above exception, another exception occurred: app.clients.sms.SmsClientResponseException: SMS client error (Request failed) [^1]: https://github.com/alphagov/notifications-api/pull/3493#discussion_r837363717 --- app/clients/sms/__init__.py | 2 +- app/clients/sms/firetext.py | 22 +++++----------------- app/clients/sms/mmg.py | 8 ++++---- app/clients/sms/reach.py | 21 ++++----------------- tests/app/clients/test_firetext.py | 24 +++++++----------------- tests/app/clients/test_mmg.py | 23 +++++++---------------- tests/app/clients/test_reach.py | 21 ++++++--------------- 7 files changed, 34 insertions(+), 87 deletions(-) diff --git a/app/clients/sms/__init__.py b/app/clients/sms/__init__.py index 3a2ba1367..0aca5b58c 100644 --- a/app/clients/sms/__init__.py +++ b/app/clients/sms/__init__.py @@ -12,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): diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index f2336f2aa..0d8f5a685 100644 --- a/app/clients/sms/firetext.py +++ b/app/clients/sms/firetext.py @@ -44,18 +44,6 @@ 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. @@ -91,10 +79,10 @@ class FiretextClient(SmsClient): try: json.loads(response.text) if response.json()['code'] != 0: - raise ValueError() - except (ValueError, AttributeError) as e: - raise FiretextClientResponseException(response=response, exception=e) - except RequestException as e: - raise FiretextClientResponseException(response=e.response, exception=e) + 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 a1408a16e..b05c275ea 100644 --- a/app/clients/sms/mmg.py +++ b/app/clients/sms/mmg.py @@ -102,9 +102,9 @@ class MMGClient(SmsClient): response.raise_for_status() try: json.loads(response.text) - except (ValueError, AttributeError) as e: - raise MMGClientResponseException(response=response, exception=e) - except RequestException as e: - raise MMGClientResponseException(response=e.response, exception=e) + 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 ad4eee17f..3541f8b5c 100644 --- a/app/clients/sms/reach.py +++ b/app/clients/sms/reach.py @@ -16,19 +16,6 @@ def get_reach_responses(status, detailed_status_code=None): raise KeyError -class ReachClientResponseException(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 ReachClient(SmsClient): def init_app(self, *args, **kwargs): super().init_app(*args, **kwargs) @@ -57,9 +44,9 @@ class ReachClient(SmsClient): response.raise_for_status() try: json.loads(response.text) - except (ValueError, AttributeError) as e: - raise ReachClientResponseException(response=response, exception=e) - except RequestException as e: - raise ReachClientResponseException(response=e.response, exception=e) + except (ValueError, AttributeError): + raise SmsClientResponseException("Invalid response JSON") + except RequestException: + raise SmsClientResponseException("Request failed") return response diff --git a/tests/app/clients/test_firetext.py b/tests/app/clients/test_firetext.py index 089a61fc0..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, ) @@ -116,9 +114,7 @@ def test_try_send_sms_raises_if_firetext_rejects(mocker, mock_firetext_client): request_mock.post('https://example.com/firetext', json=response_dict, status_code=200) 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_try_send_sms_raises_if_firetext_rejects_with_unexpected_data(mocker, mock_firetext_client): @@ -129,9 +125,7 @@ def test_try_send_sms_raises_if_firetext_rejects_with_unexpected_data(mocker, mo request_mock.post('https://example.com/firetext', json=response_dict, status_code=400) 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_try_send_sms_raises_if_firetext_fails_to_return_json(notify_api, mock_firetext_client): @@ -142,28 +136,24 @@ def test_try_send_sms_raises_if_firetext_fails_to_return_json(notify_api, mock_f request_mock.post('https://example.com/firetext', text=response_dict, status_code=200) mock_firetext_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_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.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_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.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 0164af452..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', [ @@ -85,10 +83,7 @@ def test_try_send_sms_raises_if_mmg_rejects(notify_api, mocker): request_mock.post('https://example.com/mmg', json=response_dict, status_code=400) 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_try_send_sms_raises_if_mmg_fails_to_return_json(notify_api, mocker): @@ -99,28 +94,24 @@ def test_try_send_sms_raises_if_mmg_fails_to_return_json(notify_api, mocker): request_mock.post('https://example.com/mmg', text=response_dict, status_code=200) 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_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.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_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.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 7cc8b2bc3..b62a5fd0b 100644 --- a/tests/app/clients/test_reach.py +++ b/tests/app/clients/test_reach.py @@ -1,11 +1,9 @@ import pytest import requests_mock -from requests import HTTPError from requests.exceptions import ConnectTimeout, ReadTimeout from app import reach_client from app.clients.sms import SmsClientResponseException -from app.clients.sms.reach import ReachClientResponseException # TODO: tests for get_reach_responses @@ -52,10 +50,7 @@ def test_try_send_sms_raises_if_reach_rejects(notify_api, mocker): request_mock.post('https://example.com/reach', json=response_dict, status_code=400) reach_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) def test_try_send_sms_raises_if_reach_fails_to_return_json(notify_api, mocker): @@ -66,28 +61,24 @@ def test_try_send_sms_raises_if_reach_fails_to_return_json(notify_api, mocker): request_mock.post('https://example.com/reach', text=response_dict, status_code=200) reach_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_try_send_sms_raises_if_reach_rejects_with_connect_timeout(rmock): to = content = reference = 'foo' - with pytest.raises(ReachClientResponseException) as exc: + 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 exc.value.status_code == 504 - assert exc.value.text == 'Gateway Time-out' + 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(ReachClientResponseException) as exc: + 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 exc.value.status_code == 504 - assert exc.value.text == 'Gateway Time-out' + assert 'Request failed' in str(exc.value) From e6fffc00da22fc5dcdb44d04e910a4ebc62222ce Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 30 Mar 2022 13:36:10 +0100 Subject: [PATCH 12/12] Add temporary log to check if code is in use In response to: [^1]. [^1]: https://github.com/alphagov/notifications-api/pull/3493#discussion_r838477599 --- app/clients/sms/__init__.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/app/clients/sms/__init__.py b/app/clients/sms/__init__.py index 0aca5b58c..38f3c55c9 100644 --- a/app/clients/sms/__init__.py +++ b/app/clients/sms/__init__.py @@ -40,6 +40,14 @@ class SmsClient(Client): 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: