From 732c203d3e224ca4e03b4ea575f1d2b3ceea8715 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 17 Nov 2020 12:35:18 +0000 Subject: [PATCH 1/2] rename clients to notification_provider_clients i think it's causing havoc with my attempts to mock stuff in the `app.clients` directory because it's also accessible at that path. the name's super vague and doesn't explain what it is anyway --- app/__init__.py | 6 +++--- app/clients/__init__.py | 2 +- app/delivery/send_to_providers.py | 4 ++-- tests/app/dao/test_provider_details_dao.py | 4 ++-- tests/app/delivery/test_send_to_providers.py | 7 +++++-- 5 files changed, 13 insertions(+), 10 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index e30655832..c412e4b8f 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -22,7 +22,7 @@ from werkzeug.exceptions import HTTPException as WerkzeugHTTPException from werkzeug.local import LocalProxy from app.celery.celery import NotifyCelery -from app.clients import Clients +from app.clients import NotificationProviderClients from app.clients.cbc_proxy import CBCProxyClient, CBCProxyNoopClient from app.clients.document_download import DocumentDownloadClient from app.clients.email.aws_ses import AwsSesClient @@ -65,7 +65,7 @@ cbc_proxy_client = CBCProxyNoopClient() document_download_client = DocumentDownloadClient() metrics = GDSMetrics() -clients = Clients() +notification_provider_clients = NotificationProviderClients() api_user = LocalProxy(lambda: _request_ctx_stack.top.api_user) authenticated_service = LocalProxy(lambda: _request_ctx_stack.top.authenticated_service) @@ -106,7 +106,7 @@ 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] - clients.init_app(sms_clients=[firetext_client, mmg_client], email_clients=email_clients) + notification_provider_clients.init_app(sms_clients=[firetext_client, mmg_client], email_clients=email_clients) notify_celery.init_app(application) encryption.init_app(application) diff --git a/app/clients/__init__.py b/app/clients/__init__.py index 74750c51a..ecf60204f 100644 --- a/app/clients/__init__.py +++ b/app/clients/__init__.py @@ -17,7 +17,7 @@ STATISTICS_DELIVERED = 'delivered' STATISTICS_FAILURE = 'failure' -class Clients(object): +class NotificationProviderClients(object): sms_clients = {} email_clients = {} diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 3f92e54d1..902126d6d 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -9,7 +9,7 @@ from notifications_utils.recipients import ( ) from notifications_utils.template import HTMLEmailTemplate, PlainTextEmailTemplate, SMSMessageTemplate -from app import clients, statsd_client, create_uuid +from app import notification_provider_clients, statsd_client, create_uuid from app.dao.notifications_dao import ( dao_update_notification ) @@ -144,7 +144,7 @@ def provider_to_use(notification_type, international=False): chosen_provider = random.choices(active_providers, weights=[p.priority for p in active_providers])[0] - return clients.get_client_by_name_and_type(chosen_provider.identifier, notification_type) + return notification_provider_clients.get_client_by_name_and_type(chosen_provider.identifier, notification_type) def get_logo_url(base_url, logo_file): diff --git a/tests/app/dao/test_provider_details_dao.py b/tests/app/dao/test_provider_details_dao.py index 2d42975e2..a25deaa6a 100644 --- a/tests/app/dao/test_provider_details_dao.py +++ b/tests/app/dao/test_provider_details_dao.py @@ -5,7 +5,7 @@ from freezegun import freeze_time from sqlalchemy.sql import desc from app.models import ProviderDetails, ProviderDetailsHistory -from app import clients +from app import notification_provider_clients from app.dao.provider_details_dao import ( get_alternative_sms_provider, get_provider_details_by_identifier, @@ -76,7 +76,7 @@ def test_can_get_email_providers(notify_db_session): def test_should_not_error_if_any_provider_in_code_not_in_database(restore_provider_details): ProviderDetails.query.filter_by(identifier='mmg').delete() - assert clients.get_sms_client('mmg') + assert notification_provider_clients.get_sms_client('mmg') @freeze_time('2000-01-01T00:00:00') diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 367f86059..6cf1013a4 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -9,7 +9,7 @@ from notifications_utils.recipients import validate_and_format_phone_number from requests import HTTPError import app -from app import clients, mmg_client, firetext_client +from app import notification_provider_clients, mmg_client, firetext_client from app.dao import notifications_dao from app.dao.provider_details_dao import get_provider_details_by_identifier from app.delivery import send_to_providers @@ -535,7 +535,10 @@ def test_update_notification_to_sending_does_not_update_status_from_a_final_stat ): template = create_template(sample_service) notification = create_notification(template=template, status=starting_status) - send_to_providers.update_notification_to_sending(notification, clients.get_client_by_name_and_type("mmg", "sms")) + send_to_providers.update_notification_to_sending( + notification, + notification_provider_clients.get_client_by_name_and_type("mmg", "sms") + ) assert notification.status == expected_status From b72640bf5e242ef1cac07d45ab906a9c814dc3f5 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 29 Oct 2020 10:22:50 +0000 Subject: [PATCH 2/2] refactor cbc proxy and fix tests moved the lambda invocation to a separate function to keep DRY asserts on exception types need to be outside of with blocks, or they won't trip (as the exception will stop execution of the inner with block). the asserts were also the wrong way round so fixed that. --- app/clients/cbc_proxy.py | 63 +++++++++++------------------ tests/app/clients/test_cbc_proxy.py | 24 +++++------ 2 files changed, 36 insertions(+), 51 deletions(-) diff --git a/app/clients/cbc_proxy.py b/app/clients/cbc_proxy.py index 919c4a23c..3a82f640e 100644 --- a/app/clients/cbc_proxy.py +++ b/app/clients/cbc_proxy.py @@ -20,6 +20,10 @@ import boto3 # the preceeding Alert message in the references field +class CBCProxyException(Exception): + pass + + # Noop = no operation class CBCProxyNoopClient: @@ -72,72 +76,53 @@ class CBCProxyClient: aws_secret_access_key=app.config['CBC_PROXY_AWS_SECRET_ACCESS_KEY'], ) - def send_canary( - self, - identifier, - ): - payload_bytes = bytes(json.dumps({ - 'identifier': identifier, - }), encoding='utf8') + def _invoke_lambda(self, function_name, payload): + payload_bytes = bytes(json.dumps(payload), encoding='utf8') result = self._lambda_client.invoke( - FunctionName='canary', + FunctionName=function_name, InvocationType='RequestResponse', Payload=payload_bytes, ) if result['StatusCode'] > 299: - raise Exception('Could not invoke lambda') + raise CBCProxyException('Could not invoke lambda') if 'FunctionError' in result: - raise Exception('Function exited with unhandled exception') + raise CBCProxyException('Function exited with unhandled exception') + + return result + + def send_canary( + self, + identifier, + ): + self._invoke_lambda(function_name='canary', payload={'identifier': identifier}) def send_link_test( self, identifier, ): - payload_bytes = bytes(json.dumps({ - 'message_type': 'test', - 'identifier': identifier, - }), encoding='utf8') + payload = {'message_type': 'test', 'identifier': identifier} - result = self._lambda_client.invoke( - FunctionName='bt-ee-1-proxy', - InvocationType='RequestResponse', - Payload=payload_bytes, - ) - - if result['StatusCode'] > 299: - raise Exception('Could not invoke lambda') - - if 'FunctionError' in result: - raise Exception('Function exited with unhandled exception') + self._invoke_lambda(function_name='bt-ee-1-proxy', payload=payload) def create_and_send_broadcast( self, identifier, headline, description, areas, sent, expires, ): - payload_bytes = bytes(json.dumps({ + payload = { 'message_type': 'alert', 'identifier': identifier, 'headline': headline, 'description': description, 'areas': areas, - 'sent': sent, 'expires': expires, - }), encoding='utf8') + 'sent': sent, + 'expires': expires, + } - result = self._lambda_client.invoke( - FunctionName='bt-ee-1-proxy', - InvocationType='RequestResponse', - Payload=payload_bytes, - ) - - if result['StatusCode'] > 299: - raise Exception('Could not invoke lambda') - - if 'FunctionError' in result: - raise Exception('Function exited with unhandled exception') + self._invoke_lambda(function_name='bt-ee-1-proxy', payload=payload) # We have not implementated updating a broadcast def update_and_send_broadcast( diff --git a/tests/app/clients/test_cbc_proxy.py b/tests/app/clients/test_cbc_proxy.py index 3df22bdd4..d888d44b6 100644 --- a/tests/app/clients/test_cbc_proxy.py +++ b/tests/app/clients/test_cbc_proxy.py @@ -3,7 +3,7 @@ import uuid import pytest -from app.clients.cbc_proxy import CBCProxyClient +from app.clients.cbc_proxy import CBCProxyClient, CBCProxyException @pytest.fixture(scope='function') @@ -116,7 +116,7 @@ def test_cbc_proxy_create_and_send_handles_invoke_error(mocker, cbc_proxy): 'StatusCode': 400, } - with pytest.raises(Exception) as e: + with pytest.raises(CBCProxyException) as e: cbc_proxy.create_and_send_broadcast( identifier=identifier, headline=headline, @@ -165,7 +165,7 @@ def test_cbc_proxy_create_and_send_handles_function_error(mocker, cbc_proxy): 'FunctionError': 'something', } - with pytest.raises(Exception) as e: + with pytest.raises(CBCProxyException) as e: cbc_proxy.create_and_send_broadcast( identifier=identifier, headline=headline, @@ -174,7 +174,7 @@ def test_cbc_proxy_create_and_send_handles_function_error(mocker, cbc_proxy): sent=sent, expires=expires, ) - assert e.match('Function exited with unhandled exception') + assert e.match('Function exited with unhandled exception') ld_client_mock.invoke.assert_called_once_with( FunctionName='bt-ee-1-proxy', @@ -226,12 +226,12 @@ def test_cbc_proxy_send_canary_handles_invoke_error(mocker, cbc_proxy): 'StatusCode': 400, } - with pytest.raises(Exception) as e: + with pytest.raises(CBCProxyException) as e: cbc_proxy.send_canary( identifier=identifier, ) - assert e.match('Function exited with unhandled exception') + assert e.match('Could not invoke lambda') ld_client_mock.invoke.assert_called_once_with( FunctionName='canary', @@ -254,12 +254,12 @@ def test_cbc_proxy_send_canary_handles_function_error(mocker, cbc_proxy): 'FunctionError': 'something', } - with pytest.raises(Exception) as e: + with pytest.raises(CBCProxyException) as e: cbc_proxy.send_canary( identifier=identifier, ) - assert e.match('Could not invoke lambda') + assert e.match('Function exited with unhandled exception') ld_client_mock.invoke.assert_called_once_with( FunctionName='canary', @@ -312,12 +312,12 @@ def test_cbc_proxy_send_link_test_handles_invoke_error(mocker, cbc_proxy): 'StatusCode': 400, } - with pytest.raises(Exception) as e: + with pytest.raises(CBCProxyException) as e: cbc_proxy.send_link_test( identifier=identifier, ) - assert e.match('Function exited with unhandled exception') + assert e.match('Could not invoke lambda') ld_client_mock.invoke.assert_called_once_with( FunctionName='bt-ee-1-proxy', @@ -340,12 +340,12 @@ def test_cbc_proxy_send_link_test_handles_function_error(mocker, cbc_proxy): 'FunctionError': 'something', } - with pytest.raises(Exception) as e: + with pytest.raises(CBCProxyException) as e: cbc_proxy.send_link_test( identifier=identifier, ) - assert e.match('Could not invoke lambda') + assert e.match('Function exited with unhandled exception') ld_client_mock.invoke.assert_called_once_with( FunctionName='bt-ee-1-proxy',