From 2e665de46d2115c19cfa7ae98eb86c88793f2dd0 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 26 Oct 2020 16:04:39 +0000 Subject: [PATCH 1/6] add broadcast provider message table to DB we need to track the state of sending to different provider separately (and trigger them off separately, refer to references separately, etc) --- app/models.py | 47 +++++++++++++++++- .../versions/0332_broadcast_provider_msg.py | 49 +++++++++++++++++++ 2 files changed, 95 insertions(+), 1 deletion(-) create mode 100644 migrations/versions/0332_broadcast_provider_msg.py diff --git a/app/models.py b/app/models.py index db4a2b3d0..f1e8c0a74 100644 --- a/app/models.py +++ b/app/models.py @@ -2195,6 +2195,10 @@ class BroadcastStatusType(db.Model): class BroadcastMessage(db.Model): + """ + This is for creating a message, viewing it in notify, adding areas, approvals, drafts, etc. Notify logic before + hitting send. + """ __tablename__ = 'broadcast_message' __table_args__ = ( db.ForeignKeyConstraint( @@ -2299,7 +2303,8 @@ class BroadcastEventMessageType: class BroadcastEvent(db.Model): """ - This table represents a single CAP XML blob that we sent to the mobile network providers. + This table represents an instruction that we will send to the broadcast providers. It directly correlates with an + instruction from the admin - to broadcast a message, to cancel an existing message, or to update an existing one. We should be able to create the complete CAP message without joining from this to any other tables, eg template, service, or broadcast_message. @@ -2398,3 +2403,43 @@ class BroadcastEvent(db.Model): 'transmitted_finishes_at': self.transmitted_finishes_at.strftime(DATETIME_FORMAT), } + + +class BroadcastProvider: + EE = 'ee' + VODAFONE = 'vodafone' + THREE = 'three' + O2 = 'o2' + + PROVIDERS = [EE, VODAFONE, THREE, O2] + + +class BroadcastProviderMessageStatus: + TECHNICAL_FAILURE = 'technical-failure' # Couldn’t send (cbc proxy 5xx/4xx) + SENDING = 'sending' # Sent to cbc, awaiting response + ACK = 'returned-ack' # Received ack response + ERR = 'returned-error' # Received error response + + STATES = [TECHNICAL_FAILURE, SENDING, ACK, ERR] + + +class BroadcastProviderMessage(db.Model): + """ + A row in this table represents the XML blob sent to a single provider. + """ + __tablename__ = 'broadcast_provider_message' + + id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) + + broadcast_event_id = db.Column(UUID(as_uuid=True), db.ForeignKey('broadcast_event.id')) + broadcast_event = db.relationship('BroadcastEvent') + + # 'ee', 'three', 'vodafone', etc + provider = db.Column(db.String) + + status = db.Column(db.String) + + created_at = db.Column(db.DateTime, nullable=False, default=datetime.datetime.utcnow) + updated_at = db.Column(db.DateTime, nullable=True, onupdate=datetime.datetime.utcnow) + + UniqueConstraint(broadcast_event_id, provider) diff --git a/migrations/versions/0332_broadcast_provider_msg.py b/migrations/versions/0332_broadcast_provider_msg.py new file mode 100644 index 000000000..088f1c9df --- /dev/null +++ b/migrations/versions/0332_broadcast_provider_msg.py @@ -0,0 +1,49 @@ +""" + +Revision ID: 0332_broadcast_provider_msg +Revises: 0331_add_broadcast_org +Create Date: 2020-10-26 16:28:11.917468 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +revision = '0332_broadcast_provider_msg' +down_revision = '0331_add_broadcast_org' + +STATUSES = [ + 'technical-failure', + 'sending', + 'returned-ack', + 'returned-error', +] + + +def upgrade(): + + broadcast_provider_message_status_type = op.create_table( + 'broadcast_provider_message_status_type', + sa.Column('name', sa.String(), nullable=False), + sa.PrimaryKeyConstraint('name') + ) + op.bulk_insert(broadcast_provider_message_status_type, [{'name': status} for status in STATUSES]) + + # ### commands auto generated by Alembic - please adjust! ### + op.create_table( + 'broadcast_provider_message', + sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('broadcast_event_id', postgresql.UUID(as_uuid=True), nullable=True), + sa.Column('provider', sa.String(), nullable=True), + sa.Column('status', sa.String(), nullable=True), + sa.Column('created_at', sa.DateTime(), nullable=False), + sa.Column('updated_at', sa.DateTime(), nullable=True), + sa.ForeignKeyConstraint(['broadcast_event_id'], ['broadcast_event.id'], ), + sa.PrimaryKeyConstraint('id'), + sa.UniqueConstraint('broadcast_event_id', 'provider') + ) + + +def downgrade(): + op.drop_table('broadcast_provider_message') + op.drop_table('broadcast_provider_message_status_type') From bc3512467b32c82add4ebb6a5b721cd0f4c7513d Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 29 Oct 2020 11:12:28 +0000 Subject: [PATCH 2/6] send messages to multiple providers at the moment only EE is enabled (this is set in app.config, but also, only EE have a function defined for them so even if another provider was enabled without changing the dict in cbc_proxy.py we won't trigger anything). this commit just adds wrapper tasks that check what providers are enabled, and invokes the send function for each provider. The send function doesn't currently distinguish between providers for now - as we only have EE set up. in the future we'll want to separate the cbc_proxy_client into separate clients for separate providers. Different providers have different lambda functions, and have different requirements. For example, we know that the two different CBC software solutions handle references to previous messages differently. --- app/celery/broadcast_message_tasks.py | 39 +++++++++++++++++++++++++-- app/celery/scheduled_tasks.py | 21 +++------------ app/clients/cbc_proxy.py | 15 ++++++++++- app/config.py | 3 +++ 4 files changed, 57 insertions(+), 21 deletions(-) diff --git a/app/celery/broadcast_message_tasks.py b/app/celery/broadcast_message_tasks.py index c90c3e297..50eb7c982 100644 --- a/app/celery/broadcast_message_tasks.py +++ b/app/celery/broadcast_message_tasks.py @@ -1,15 +1,29 @@ +import uuid + from flask import current_app from notifications_utils.statsd_decorators import statsd from app import cbc_proxy_client, notify_celery - -from app.models import BroadcastEventMessageType +from app.config import QueueNames +from app.models import BroadcastEventMessageType, BroadcastProvider from app.dao.broadcast_message_dao import dao_get_broadcast_event_by_id @notify_celery.task(name="send-broadcast-event") @statsd(namespace="tasks") def send_broadcast_event(broadcast_event_id): + for provider in BroadcastProvider.PROVIDERS: + # TODO: Decide whether to send to each provider based on platform admin, service level settings, broadcast + # level settings, etc. + send_broadcast_provider_message.apply_async( + kwargs={'broadcast_event_id': broadcast_event_id, 'provider': provider}, + queue=QueueNames.NOTIFY + ) + + +@notify_celery.task(name="send-broadcast-provider-message") +@statsd(namespace="tasks") +def send_broadcast_provider_message(broadcast_event_id, provider): broadcast_event = dao_get_broadcast_event_by_id(broadcast_event_id) current_app.logger.info( @@ -52,3 +66,24 @@ def send_broadcast_event(broadcast_event_id): sent=broadcast_event.sent_at_as_cap_datetime_string, expires=broadcast_event.transmitted_finishes_at_as_cap_datetime_string, ) + + +@notify_celery.task(name='trigger-link-test') +def trigger_link_test(provider): + """ + Currently we only have one hardcoded CBC Proxy, which corresponds to one + CBC, and so currently we do not specify the CBC Proxy name + + In future we will have multiple CBC proxies, each proxy corresponding to + one MNO's CBC + + This task should invoke other tasks which do the actual link tests, eg: + for cbc_name in app.config.ENABLED_CBCS: + send_link_test_for_cbc(cbc_name) + + Alternatively this task could be configured to be a Celery group + """ + identifier = str(uuid.uuid4()) + message = f"Sending a link test to CBC proxy for provider {provider} with ID {identifier}" + current_app.logger.info(message) + cbc_proxy_client.send_link_test(identifier) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index eaf898ddf..948c572aa 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -17,6 +17,7 @@ from app.celery.tasks import ( process_row, process_incomplete_jobs) from app.celery.letters_pdf_tasks import get_pdf_for_templated_letter +from app.celery.broadcast_message_tasks import trigger_link_test from app.config import QueueNames from app.dao.invited_org_user_dao import delete_org_invitations_created_more_than_two_days_ago from app.dao.invited_user_dao import delete_invitations_created_more_than_two_days_ago @@ -305,21 +306,5 @@ def send_canary_to_cbc_proxy(): @notify_celery.task(name='trigger-link-tests') def trigger_link_tests(): - """ - Currently we only have one hardcoded CBC Proxy, which corresponds to one - CBC, and so currently we do not specify the CBC Proxy name - - In future we will have multiple CBC proxies, each proxy corresponding to - one MNO's CBC - - This task should invoke other tasks which do the actual link tests, eg: - for cbc_name in app.config.ENABLED_CBCS: - send_link_test_for_cbc(cbc_name) - - Alternatively this task could be configured to be a Celery group - """ - for _ in range(1): - identifier = str(uuid.uuid4()) - message = f"Sending a link test to CBC proxy with ID {identifier}" - current_app.logger.info(message) - cbc_proxy_client.send_link_test(identifier) + for cbc_name in current_app.config['ENABLED_CBCS']: + trigger_link_test.apply_async(kwargs={'provider': cbc_name}, queue=QueueNames.NOTIFY) diff --git a/app/clients/cbc_proxy.py b/app/clients/cbc_proxy.py index 3a82f640e..578ddcd66 100644 --- a/app/clients/cbc_proxy.py +++ b/app/clients/cbc_proxy.py @@ -2,6 +2,8 @@ import json import boto3 +from app.models import BroadcastProviders + # The variable names in this file have specific meaning in a CAP message # # identifier is a unique field for each CAP message @@ -67,6 +69,9 @@ class CBCProxyNoopClient: class CBCProxyClient: + provider_function_name_map = { + BroadcastProviders.EE: 'bt-ee-1-proxy', + } def init_app(self, app): self._lambda_client = boto3.client( @@ -97,12 +102,21 @@ class CBCProxyClient: self, identifier, ): + """ + canary - a specific lambda that does not connect to a provider, but just confirms the connectivity between + Notify and the CBC proxy AWS account + """ self._invoke_lambda(function_name='canary', payload={'identifier': identifier}) def send_link_test( self, identifier, + provider, ): + """ + link test - open up a connection to a specific provider, and send them an xml payload with a of + test. + """ payload = {'message_type': 'test', 'identifier': identifier} self._invoke_lambda(function_name='bt-ee-1-proxy', payload=payload) @@ -121,7 +135,6 @@ class CBCProxyClient: 'sent': sent, 'expires': expires, } - self._invoke_lambda(function_name='bt-ee-1-proxy', payload=payload) # We have not implementated updating a broadcast diff --git a/app/config.py b/app/config.py index 3809a3109..793c2473d 100644 --- a/app/config.py +++ b/app/config.py @@ -367,6 +367,9 @@ class Config(object): CBC_PROXY_AWS_ACCESS_KEY_ID = os.environ.get('CBC_PROXY_AWS_ACCESS_KEY_ID', '') CBC_PROXY_AWS_SECRET_ACCESS_KEY = os.environ.get('CBC_PROXY_AWS_SECRET_ACCESS_KEY', '') + # matches up with the strings in models.py::BroadcastProvider + ENABLED_CBCS = ['ee'] + ###################### # Config overrides ### From 7cc83e04eb2b780aa03e81c1d6aa0a7231163925 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 16 Nov 2020 12:47:38 +0000 Subject: [PATCH 3/6] move BroadcastProvider from models.py to config.py It's not something that is tied to a database table, and was causing circular import issues --- app/celery/broadcast_message_tasks.py | 4 +- app/clients/cbc_proxy.py | 5 +- app/config.py | 12 +++- .../celery/test_broadcast_message_tasks.py | 57 ++++++++++++++++--- tests/app/celery/test_scheduled_tasks.py | 26 ++++----- 5 files changed, 74 insertions(+), 30 deletions(-) diff --git a/app/celery/broadcast_message_tasks.py b/app/celery/broadcast_message_tasks.py index 50eb7c982..448e6e183 100644 --- a/app/celery/broadcast_message_tasks.py +++ b/app/celery/broadcast_message_tasks.py @@ -5,14 +5,14 @@ from notifications_utils.statsd_decorators import statsd from app import cbc_proxy_client, notify_celery from app.config import QueueNames -from app.models import BroadcastEventMessageType, BroadcastProvider +from app.models import BroadcastEventMessageType from app.dao.broadcast_message_dao import dao_get_broadcast_event_by_id @notify_celery.task(name="send-broadcast-event") @statsd(namespace="tasks") def send_broadcast_event(broadcast_event_id): - for provider in BroadcastProvider.PROVIDERS: + for provider in current_app.config['ENABLED_CBCS']: # TODO: Decide whether to send to each provider based on platform admin, service level settings, broadcast # level settings, etc. send_broadcast_provider_message.apply_async( diff --git a/app/clients/cbc_proxy.py b/app/clients/cbc_proxy.py index 578ddcd66..c7c231888 100644 --- a/app/clients/cbc_proxy.py +++ b/app/clients/cbc_proxy.py @@ -2,7 +2,7 @@ import json import boto3 -from app.models import BroadcastProviders +from app.config import BroadcastProvider # The variable names in this file have specific meaning in a CAP message # @@ -70,7 +70,7 @@ class CBCProxyNoopClient: class CBCProxyClient: provider_function_name_map = { - BroadcastProviders.EE: 'bt-ee-1-proxy', + BroadcastProvider.EE: 'bt-ee-1-proxy', } def init_app(self, app): @@ -111,7 +111,6 @@ class CBCProxyClient: def send_link_test( self, identifier, - provider, ): """ link test - open up a connection to a specific provider, and send them an xml payload with a of diff --git a/app/config.py b/app/config.py index 793c2473d..c6871a6b5 100644 --- a/app/config.py +++ b/app/config.py @@ -56,6 +56,15 @@ class QueueNames(object): ] +class BroadcastProvider: + EE = 'ee' + VODAFONE = 'vodafone' + THREE = 'three' + O2 = 'o2' + + PROVIDERS = [EE, VODAFONE, THREE, O2] + + class TaskNames(object): PROCESS_INCOMPLETE_JOBS = 'process-incomplete-jobs' ZIP_AND_SEND_LETTER_PDFS = 'zip-and-send-letter-pdfs' @@ -367,8 +376,7 @@ class Config(object): CBC_PROXY_AWS_ACCESS_KEY_ID = os.environ.get('CBC_PROXY_AWS_ACCESS_KEY_ID', '') CBC_PROXY_AWS_SECRET_ACCESS_KEY = os.environ.get('CBC_PROXY_AWS_SECRET_ACCESS_KEY', '') - # matches up with the strings in models.py::BroadcastProvider - ENABLED_CBCS = ['ee'] + ENABLED_CBCS = {BroadcastProvider.EE} ###################### diff --git a/tests/app/celery/test_broadcast_message_tasks.py b/tests/app/celery/test_broadcast_message_tasks.py index d6b5cdb4e..4ae6034b7 100644 --- a/tests/app/celery/test_broadcast_message_tasks.py +++ b/tests/app/celery/test_broadcast_message_tasks.py @@ -1,13 +1,33 @@ +import uuid +from unittest.mock import call + from freezegun import freeze_time import pytest from app.models import BROADCAST_TYPE, BroadcastStatusType, BroadcastEventMessageType -from app.celery.broadcast_message_tasks import send_broadcast_event +from app.celery.broadcast_message_tasks import send_broadcast_event, send_broadcast_provider_message, trigger_link_test + from tests.app.db import create_template, create_broadcast_message, create_broadcast_event +from tests.conftest import set_config + + +def test_send_broadcast_event_queues_up_for_active_providers(mocker, notify_api): + mock_send_broadcast_provider_message = mocker.patch( + 'app.celery.broadcast_message_tasks.send_broadcast_provider_message', + ) + + event_id = uuid.uuid4() + with set_config(notify_api, 'ENABLED_CBCS', ['ee', 'vodafone']): + send_broadcast_event(event_id) + + assert mock_send_broadcast_provider_message.apply_async.call_args_list == [ + call(kwargs={'broadcast_event_id': event_id, 'provider': 'ee'}, queue='notify-internal-tasks'), + call(kwargs={'broadcast_event_id': event_id, 'provider': 'vodafone'}, queue='notify-internal-tasks') + ] @freeze_time('2020-08-01 12:00') -def test_create_broadcast_event_sends_data_correctly(mocker, sample_service): +def test_send_broadcast_provider_message_sends_data_correctly(mocker, sample_service): template = create_template(sample_service, BROADCAST_TYPE) broadcast_message = create_broadcast_message( template, @@ -26,7 +46,7 @@ def test_create_broadcast_event_sends_data_correctly(mocker, sample_service): 'app.cbc_proxy_client.create_and_send_broadcast', ) - send_broadcast_event(broadcast_event_id=str(event.id)) + send_broadcast_provider_message(provider='ee', broadcast_event_id=str(event.id)) mock_create_broadcast.assert_called_once_with( identifier=str(event.id), @@ -46,7 +66,7 @@ def test_create_broadcast_event_sends_data_correctly(mocker, sample_service): ) -def test_update_broadcast_event_sends_references(mocker, sample_service): +def test_send_broadcast_provider_message_sends_update_with_references(mocker, sample_service): template = create_template(sample_service, BROADCAST_TYPE, content='content') broadcast_message = create_broadcast_message( @@ -67,7 +87,7 @@ def test_update_broadcast_event_sends_references(mocker, sample_service): 'app.cbc_proxy_client.update_and_send_broadcast', ) - send_broadcast_event(broadcast_event_id=str(update_event.id)) + send_broadcast_provider_message(provider='ee', broadcast_event_id=str(update_event.id)) mock_update_broadcast.assert_called_once_with( identifier=str(update_event.id), @@ -82,7 +102,7 @@ def test_update_broadcast_event_sends_references(mocker, sample_service): ) -def test_cancel_broadcast_event_sends_references(mocker, sample_service): +def test_send_broadcast_provider_message_sends_cancel_with_references(mocker, sample_service): template = create_template(sample_service, BROADCAST_TYPE, content='content') broadcast_message = create_broadcast_message( @@ -104,7 +124,7 @@ def test_cancel_broadcast_event_sends_references(mocker, sample_service): 'app.cbc_proxy_client.cancel_broadcast', ) - send_broadcast_event(broadcast_event_id=str(cancel_event.id)) + send_broadcast_provider_message(provider='ee', broadcast_event_id=str(cancel_event.id)) mock_cancel_broadcast.assert_called_once_with( identifier=str(cancel_event.id), @@ -119,7 +139,7 @@ def test_cancel_broadcast_event_sends_references(mocker, sample_service): ) -def test_send_broadcast_event_errors(mocker, sample_service): +def test_send_broadcast_provider_message_errors(mocker, sample_service): template = create_template(sample_service, BROADCAST_TYPE) broadcast_message = create_broadcast_message( @@ -141,7 +161,7 @@ def test_send_broadcast_event_errors(mocker, sample_service): ) with pytest.raises(Exception) as ex: - send_broadcast_event(broadcast_event_id=str(event.id)) + send_broadcast_provider_message(provider='ee', broadcast_event_id=str(event.id)) assert ex.match('oh no') @@ -159,3 +179,22 @@ def test_send_broadcast_event_errors(mocker, sample_service): sent=event.sent_at_as_cap_datetime_string, expires=event.transmitted_finishes_at_as_cap_datetime_string, ) + + +def test_trigger_link_tests_invokes_cbc_proxy_client( + mocker, +): + mock_send_link_test = mocker.patch( + 'app.cbc_proxy_client.send_link_test', + ) + + trigger_link_test('some-provider') + + assert mock_send_link_test.called + # the 0th argument of the call to send_link_test + identifier = mock_send_link_test.mock_calls[0][1][0] + + try: + uuid.UUID(identifier) + except BaseException: + pytest.fail(f"{identifier} is not a valid uuid") diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index c76632c2e..a5b4d9cd3 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -19,6 +19,7 @@ from app.celery.scheduled_tasks import ( check_for_missing_rows_in_completed_jobs, check_for_services_with_high_failure_rates_or_sending_to_tv_numbers, switch_current_sms_provider_on_slow_delivery, + trigger_link_tests, ) from app.config import QueueNames, Config from app.dao.jobs_dao import dao_get_job_by_id @@ -30,8 +31,8 @@ from app.models import ( NOTIFICATION_DELIVERED, NOTIFICATION_PENDING_VIRUS_CHECK, ) +from tests.conftest import set_config from tests.app import load_example_csv - from tests.app.db import ( create_notification, create_template, @@ -577,20 +578,17 @@ def test_send_canary_to_cbc_proxy_invokes_cbc_proxy_client( pytest.fail(f"{identifier} is not a valid uuid") -def test_trigger_link_tests_invokes_cbc_proxy_client( - mocker, +def test_trigger_link_tests_calls_for_all_providers( + mocker, notify_api ): - mock_send_link_test = mocker.patch( - 'app.cbc_proxy_client.send_link_test', + mock_trigger_link_test = mocker.patch( + 'app.celery.scheduled_tasks.trigger_link_test', ) - scheduled_tasks.trigger_link_tests() + with set_config(notify_api, 'ENABLED_CBCS', ['ee', 'vodafone']): + trigger_link_tests() - mock_send_link_test.assert_called - # the 0th argument of the call to send_link_test - identifier = mock_send_link_test.mock_calls[0][1][0] - - try: - uuid.UUID(identifier) - except BaseException: - pytest.fail(f"{identifier} is not a valid uuid") + assert mock_trigger_link_test.apply_async.call_args_list == [ + call(kwargs={'provider': 'ee'}, queue='notify-internal-tasks'), + call(kwargs={'provider': 'vodafone'}, queue='notify-internal-tasks') + ] From f12c949ae966c016ba00785ebebfaaad8f06e0a5 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 16 Nov 2020 18:48:00 +0000 Subject: [PATCH 4/6] create broadcast_provider_message and use id from that instead (instead of using the id from broadcast_event) we need every XML blob we send to have a different ID. if we're sending different XML blobs for each provider, then each one should have a different identifier. So, instead of taking the identifier from the broadcast_event, take it from the broadcast_provider_message instead. Note: We're still going to the broadcast_event for most fields, to ensure they stay consistent between different providers. The last thing we want is for different phone networks to get different content --- app/celery/broadcast_message_tasks.py | 10 ++++--- app/dao/broadcast_message_dao.py | 15 ++++++++++- app/models.py | 12 ++++++++- .../celery/test_broadcast_message_tasks.py | 23 +++++++++++----- tests/app/dao/test_broadcast_message_dao.py | 27 ++++++++++++++++--- 5 files changed, 71 insertions(+), 16 deletions(-) diff --git a/app/celery/broadcast_message_tasks.py b/app/celery/broadcast_message_tasks.py index 448e6e183..4a6a9648b 100644 --- a/app/celery/broadcast_message_tasks.py +++ b/app/celery/broadcast_message_tasks.py @@ -6,7 +6,7 @@ from notifications_utils.statsd_decorators import statsd from app import cbc_proxy_client, notify_celery from app.config import QueueNames from app.models import BroadcastEventMessageType -from app.dao.broadcast_message_dao import dao_get_broadcast_event_by_id +from app.dao.broadcast_message_dao import dao_get_broadcast_event_by_id, create_broadcast_provider_message @notify_celery.task(name="send-broadcast-event") @@ -26,6 +26,8 @@ def send_broadcast_event(broadcast_event_id): def send_broadcast_provider_message(broadcast_event_id, provider): broadcast_event = dao_get_broadcast_event_by_id(broadcast_event_id) + broadcast_provider_message = create_broadcast_provider_message(broadcast_event, provider) + current_app.logger.info( f'invoking cbc proxy to send ' f'broadcast_event {broadcast_event.reference} ' @@ -39,7 +41,7 @@ def send_broadcast_provider_message(broadcast_event_id, provider): if broadcast_event.message_type == BroadcastEventMessageType.ALERT: cbc_proxy_client.create_and_send_broadcast( - identifier=str(broadcast_event.id), + identifier=str(broadcast_provider_message.id), headline="GOV.UK Notify Broadcast", description=broadcast_event.transmitted_content['body'], areas=areas, @@ -48,7 +50,7 @@ def send_broadcast_provider_message(broadcast_event_id, provider): ) elif broadcast_event.message_type == BroadcastEventMessageType.UPDATE: cbc_proxy_client.update_and_send_broadcast( - identifier=str(broadcast_event.id), + identifier=str(broadcast_provider_message.id), headline="GOV.UK Notify Broadcast", description=broadcast_event.transmitted_content['body'], areas=areas, @@ -58,7 +60,7 @@ def send_broadcast_provider_message(broadcast_event_id, provider): ) elif broadcast_event.message_type == BroadcastEventMessageType.CANCEL: cbc_proxy_client.cancel_broadcast( - identifier=str(broadcast_event.id), + identifier=str(broadcast_provider_message.id), headline="GOV.UK Notify Broadcast", description=broadcast_event.transmitted_content['body'], areas=areas, diff --git a/app/dao/broadcast_message_dao.py b/app/dao/broadcast_message_dao.py index 0c7f560a7..c24e93997 100644 --- a/app/dao/broadcast_message_dao.py +++ b/app/dao/broadcast_message_dao.py @@ -1,4 +1,6 @@ -from app.models import BroadcastMessage, BroadcastEvent +from app import db +from app.dao.dao_utils import transactional +from app.models import BroadcastMessage, BroadcastEvent, BroadcastProviderMessage, BroadcastProviderMessageStatus def dao_get_broadcast_message_by_id_and_service_id(broadcast_message_id, service_id): @@ -34,3 +36,14 @@ def get_earlier_events_for_broadcast_event(broadcast_event_id): ).order_by( BroadcastEvent.sent_at.asc() ).all() + + +@transactional +def create_broadcast_provider_message(broadcast_event, provider): + provider_message = BroadcastProviderMessage( + broadcast_event=broadcast_event, + provider=provider, + status=BroadcastProviderMessageStatus.SENDING, + ) + db.session.add(provider_message) + return provider_message diff --git a/app/models.py b/app/models.py index f1e8c0a74..5bb8d8b50 100644 --- a/app/models.py +++ b/app/models.py @@ -2377,6 +2377,16 @@ class BroadcastEvent(db.Model): """ return f"{dt.strftime('%Y-%m-%dT%H:%M:%S')}-00:00" + def get_provider_message(self, provider): + return next( + ( + provider_message + for provider_message in self.provider_messages + if provider_message.provider == provider + ), + None + ) + def get_earlier_message_references(self): from app.dao.broadcast_message_dao import get_earlier_events_for_broadcast_event return [event.reference for event in get_earlier_events_for_broadcast_event(self.id)] @@ -2432,7 +2442,7 @@ class BroadcastProviderMessage(db.Model): id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) broadcast_event_id = db.Column(UUID(as_uuid=True), db.ForeignKey('broadcast_event.id')) - broadcast_event = db.relationship('BroadcastEvent') + broadcast_event = db.relationship('BroadcastEvent', backref='provider_messages') # 'ee', 'three', 'vodafone', etc provider = db.Column(db.String) diff --git a/tests/app/celery/test_broadcast_message_tasks.py b/tests/app/celery/test_broadcast_message_tasks.py index 4ae6034b7..9c77dedb1 100644 --- a/tests/app/celery/test_broadcast_message_tasks.py +++ b/tests/app/celery/test_broadcast_message_tasks.py @@ -1,10 +1,10 @@ import uuid -from unittest.mock import call +from unittest.mock import call, ANY from freezegun import freeze_time import pytest -from app.models import BROADCAST_TYPE, BroadcastStatusType, BroadcastEventMessageType +from app.models import BROADCAST_TYPE, BroadcastStatusType, BroadcastEventMessageType, BroadcastProviderMessageStatus from app.celery.broadcast_message_tasks import send_broadcast_event, send_broadcast_provider_message, trigger_link_test from tests.app.db import create_template, create_broadcast_message, create_broadcast_event @@ -46,10 +46,15 @@ def test_send_broadcast_provider_message_sends_data_correctly(mocker, sample_ser 'app.cbc_proxy_client.create_and_send_broadcast', ) + assert event.get_provider_message('ee') is None + send_broadcast_provider_message(provider='ee', broadcast_event_id=str(event.id)) + broadcast_provider_message = event.get_provider_message('ee') + assert broadcast_provider_message.status == BroadcastProviderMessageStatus.SENDING + mock_create_broadcast.assert_called_once_with( - identifier=str(event.id), + identifier=str(broadcast_provider_message.id), headline='GOV.UK Notify Broadcast', description='this is an emergency broadcast message', areas=[{ @@ -89,8 +94,11 @@ def test_send_broadcast_provider_message_sends_update_with_references(mocker, sa send_broadcast_provider_message(provider='ee', broadcast_event_id=str(update_event.id)) + broadcast_provider_message = update_event.get_provider_message('ee') + assert broadcast_provider_message.state == BroadcastProviderMessageStatus.SENDING + mock_update_broadcast.assert_called_once_with( - identifier=str(update_event.id), + identifier=str(broadcast_provider_message.id), headline="GOV.UK Notify Broadcast", description='this is an emergency broadcast message', areas=[{ @@ -126,8 +134,11 @@ def test_send_broadcast_provider_message_sends_cancel_with_references(mocker, sa send_broadcast_provider_message(provider='ee', broadcast_event_id=str(cancel_event.id)) + broadcast_provider_message = cancel_event.get_provider_message('ee') + assert broadcast_provider_message.state == BroadcastProviderMessageStatus.SENDING + mock_cancel_broadcast.assert_called_once_with( - identifier=str(cancel_event.id), + identifier=str(broadcast_provider_message.id), headline="GOV.UK Notify Broadcast", description='this is an emergency broadcast message', areas=[{ @@ -166,7 +177,7 @@ def test_send_broadcast_provider_message_errors(mocker, sample_service): assert ex.match('oh no') mock_create_broadcast.assert_called_once_with( - identifier=str(event.id), + identifier=ANY, headline="GOV.UK Notify Broadcast", description='this is an emergency broadcast message', areas=[{ diff --git a/tests/app/dao/test_broadcast_message_dao.py b/tests/app/dao/test_broadcast_message_dao.py index cefebbbd7..196a3457f 100644 --- a/tests/app/dao/test_broadcast_message_dao.py +++ b/tests/app/dao/test_broadcast_message_dao.py @@ -1,11 +1,12 @@ from datetime import datetime -from app.models import BROADCAST_TYPE -from app.models import BroadcastEventMessageType -from app.dao.broadcast_message_dao import get_earlier_events_for_broadcast_event + +from freezegun import freeze_time + +from app.models import BROADCAST_TYPE, BroadcastStatusType, BroadcastEventMessageType +from app.dao.broadcast_message_dao import get_earlier_events_for_broadcast_event, create_broadcast_provider_message from tests.app.db import create_broadcast_message, create_template, create_broadcast_event - def test_get_earlier_events_for_broadcast_event(sample_service): t = create_template(sample_service, BROADCAST_TYPE) bm = create_broadcast_message(t) @@ -41,3 +42,21 @@ def test_get_earlier_events_for_broadcast_event(sample_service): # only fetches earlier events, and they're in time order earlier_events = get_earlier_events_for_broadcast_event(events[2].id) assert earlier_events == [events[0], events[1]] + + +@freeze_time('2020-02-03 04:05:06') +def test_create_broadcast_provider_message_creates_in_correct_state(sample_broadcast_service): + t = create_template(sample_broadcast_service, BROADCAST_TYPE) + broadcast_message = create_broadcast_message(t, status=BroadcastStatusType.APPROVED) + broadcast_event = create_broadcast_event( + broadcast_message, + sent_at=datetime(2020, 1, 1, 12, 0, 0), + message_type=BroadcastEventMessageType.ALERT, + transmitted_content={'body': 'Initial content'} + ) + + broadcast_provider_message = create_broadcast_provider_message(broadcast_event, 'fake-provider') + + assert broadcast_provider_message.status == 'sending' + assert broadcast_provider_message.broadcast_event_id == broadcast_event.id + assert broadcast_provider_message.created_at == datetime.utcnow() From 0257774cfa8674a4771200690ce8fa2d0ab8e894 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 17 Nov 2020 12:35:10 +0000 Subject: [PATCH 5/6] add get_earlier_provider_message fn to broadcast_event replacing get_earlier_provider_messages. The old function returned the previous references for earlier events for a broadcast_message. However, these depend on the message sent to a specific provider, so the function needs to change. It now takes in a provider, and only returns broadcast_provider_messages sent to that provider. If there are earlier broadcast_events without a provider_message for the chosen provider, it raises an exception - you cannot cancel a message if all the previous events have not been created properly (as we wouldn't know what references to cancel). --- app/celery/broadcast_message_tasks.py | 4 +-- app/clients/cbc_proxy.py | 12 ++++----- app/models.py | 27 ++++++++++++++++--- .../celery/test_broadcast_message_tasks.py | 24 +++++++++++++---- tests/app/dao/test_broadcast_message_dao.py | 11 ++++---- tests/app/db.py | 18 ++++++++++++- 6 files changed, 72 insertions(+), 24 deletions(-) diff --git a/app/celery/broadcast_message_tasks.py b/app/celery/broadcast_message_tasks.py index 4a6a9648b..d13b76cd3 100644 --- a/app/celery/broadcast_message_tasks.py +++ b/app/celery/broadcast_message_tasks.py @@ -54,7 +54,7 @@ def send_broadcast_provider_message(broadcast_event_id, provider): headline="GOV.UK Notify Broadcast", description=broadcast_event.transmitted_content['body'], areas=areas, - references=broadcast_event.get_earlier_message_references(), + previous_provider_messages=broadcast_event.get_earlier_provider_messages(provider), sent=broadcast_event.sent_at_as_cap_datetime_string, expires=broadcast_event.transmitted_finishes_at_as_cap_datetime_string, ) @@ -64,7 +64,7 @@ def send_broadcast_provider_message(broadcast_event_id, provider): headline="GOV.UK Notify Broadcast", description=broadcast_event.transmitted_content['body'], areas=areas, - references=broadcast_event.get_earlier_message_references(), + previous_provider_messages=broadcast_event.get_earlier_provider_messages(provider), sent=broadcast_event.sent_at_as_cap_datetime_string, expires=broadcast_event.transmitted_finishes_at_as_cap_datetime_string, ) diff --git a/app/clients/cbc_proxy.py b/app/clients/cbc_proxy.py index c7c231888..841e6dad4 100644 --- a/app/clients/cbc_proxy.py +++ b/app/clients/cbc_proxy.py @@ -16,10 +16,10 @@ from app.config import BroadcastProvider # * description is a string which populates the areaDesc field # * polygon is a list of lat/long pairs # -# references is a whitespace separated list of message identifiers +# previous_provider_messages is a whitespace separated list of message identifiers # where each identifier is a previous sent message # ie a Cancel message would have a unique identifier but have the identifier of -# the preceeding Alert message in the references field +# the preceeding Alert message in the previous_provider_messages field class CBCProxyException(Exception): @@ -54,7 +54,7 @@ class CBCProxyNoopClient: # We have not implementated updating a broadcast def update_and_send_broadcast( self, - identifier, references, headline, description, areas, + identifier, previous_provider_messages, headline, description, areas, sent, expires, ): pass @@ -62,7 +62,7 @@ class CBCProxyNoopClient: # We have not implemented cancelling a broadcast def cancel_broadcast( self, - identifier, references, headline, description, areas, + identifier, previous_provider_messages, headline, description, areas, sent, expires, ): pass @@ -139,7 +139,7 @@ class CBCProxyClient: # We have not implementated updating a broadcast def update_and_send_broadcast( self, - identifier, references, headline, description, areas, + identifier, previous_provider_messages, headline, description, areas, sent, expires, ): pass @@ -147,7 +147,7 @@ class CBCProxyClient: # We have not implemented cancelling a broadcast def cancel_broadcast( self, - identifier, references, headline, description, areas, + identifier, previous_provider_messages, headline, description, areas, sent, expires, ): pass diff --git a/app/models.py b/app/models.py index 5bb8d8b50..1d5ea5174 100644 --- a/app/models.py +++ b/app/models.py @@ -2387,9 +2387,30 @@ class BroadcastEvent(db.Model): None ) - def get_earlier_message_references(self): + def get_earlier_provider_messages(self, provider): + """ + Get the previous message for a provider. These are differentper provider, as the identifiers are different. + Return the full provider_message object rather than just an identifier, since the different providers expect + reference to contain different things - let the cbc_proxy work out what information is relevant. + """ from app.dao.broadcast_message_dao import get_earlier_events_for_broadcast_event - return [event.reference for event in get_earlier_events_for_broadcast_event(self.id)] + earlier_events = [ + event for event in get_earlier_events_for_broadcast_event(self.id) + ] + ret = [] + for event in earlier_events: + provider_message = event.get_provider_message(provider) + if provider_message is None: + # TODO: We should figure out what to do if a previous message hasn't been sent out yet. + # We don't want to not cancel a message just because it's stuck in a queue somewhere. + # This exception should probably be named, and then should be caught further up and handled + # appropriately. + raise Exception( + f'Cannot get earlier message references for event {self.id}, previous event {event.id} has not ' + + f' been sent to provider "{provider}" yet' + ) + ret.append(provider_message) + return ret def serialize(self): return { @@ -2397,8 +2418,6 @@ class BroadcastEvent(db.Model): 'service_id': str(self.service_id), - 'previous_event_references': self.get_earlier_message_references(), - 'broadcast_message_id': str(self.broadcast_message_id), # sent_at is required by BroadcastMessageTemplate.from_broadcast_event 'sent_at': self.sent_at.strftime(DATETIME_FORMAT), diff --git a/tests/app/celery/test_broadcast_message_tasks.py b/tests/app/celery/test_broadcast_message_tasks.py index 9c77dedb1..92dff4db6 100644 --- a/tests/app/celery/test_broadcast_message_tasks.py +++ b/tests/app/celery/test_broadcast_message_tasks.py @@ -7,7 +7,12 @@ import pytest from app.models import BROADCAST_TYPE, BroadcastStatusType, BroadcastEventMessageType, BroadcastProviderMessageStatus from app.celery.broadcast_message_tasks import send_broadcast_event, send_broadcast_provider_message, trigger_link_test -from tests.app.db import create_template, create_broadcast_message, create_broadcast_event +from tests.app.db import ( + create_template, + create_broadcast_message, + create_broadcast_event, + create_broadcast_provider_message +) from tests.conftest import set_config @@ -86,6 +91,7 @@ def test_send_broadcast_provider_message_sends_update_with_references(mocker, sa ) alert_event = create_broadcast_event(broadcast_message, message_type=BroadcastEventMessageType.ALERT) + create_broadcast_provider_message(alert_event, 'ee') update_event = create_broadcast_event(broadcast_message, message_type=BroadcastEventMessageType.UPDATE) mock_update_broadcast = mocker.patch( @@ -95,7 +101,7 @@ def test_send_broadcast_provider_message_sends_update_with_references(mocker, sa send_broadcast_provider_message(provider='ee', broadcast_event_id=str(update_event.id)) broadcast_provider_message = update_event.get_provider_message('ee') - assert broadcast_provider_message.state == BroadcastProviderMessageStatus.SENDING + assert broadcast_provider_message.status == BroadcastProviderMessageStatus.SENDING mock_update_broadcast.assert_called_once_with( identifier=str(broadcast_provider_message.id), @@ -104,7 +110,9 @@ def test_send_broadcast_provider_message_sends_update_with_references(mocker, sa areas=[{ "polygon": [[50.12, 1.2], [50.13, 1.2], [50.14, 1.21]], }], - references=[alert_event.reference], + previous_provider_messages=[ + alert_event.get_provider_message('ee') + ], sent=update_event.sent_at_as_cap_datetime_string, expires=update_event.transmitted_finishes_at_as_cap_datetime_string, ) @@ -128,6 +136,9 @@ def test_send_broadcast_provider_message_sends_cancel_with_references(mocker, sa update_event = create_broadcast_event(broadcast_message, message_type=BroadcastEventMessageType.UPDATE) cancel_event = create_broadcast_event(broadcast_message, message_type=BroadcastEventMessageType.CANCEL) + create_broadcast_provider_message(alert_event, 'ee') + create_broadcast_provider_message(update_event, 'ee') + mock_cancel_broadcast = mocker.patch( 'app.cbc_proxy_client.cancel_broadcast', ) @@ -135,7 +146,7 @@ def test_send_broadcast_provider_message_sends_cancel_with_references(mocker, sa send_broadcast_provider_message(provider='ee', broadcast_event_id=str(cancel_event.id)) broadcast_provider_message = cancel_event.get_provider_message('ee') - assert broadcast_provider_message.state == BroadcastProviderMessageStatus.SENDING + assert broadcast_provider_message.status == BroadcastProviderMessageStatus.SENDING mock_cancel_broadcast.assert_called_once_with( identifier=str(broadcast_provider_message.id), @@ -144,7 +155,10 @@ def test_send_broadcast_provider_message_sends_cancel_with_references(mocker, sa areas=[{ "polygon": [[50.12, 1.2], [50.13, 1.2], [50.14, 1.21]], }], - references=[alert_event.reference, update_event.reference], + previous_provider_messages=[ + alert_event.get_provider_message('ee'), + update_event.get_provider_message('ee') + ], sent=cancel_event.sent_at_as_cap_datetime_string, expires=cancel_event.transmitted_finishes_at_as_cap_datetime_string, ) diff --git a/tests/app/dao/test_broadcast_message_dao.py b/tests/app/dao/test_broadcast_message_dao.py index 196a3457f..a8fb160b5 100644 --- a/tests/app/dao/test_broadcast_message_dao.py +++ b/tests/app/dao/test_broadcast_message_dao.py @@ -1,12 +1,11 @@ from datetime import datetime -from freezegun import freeze_time - -from app.models import BROADCAST_TYPE, BroadcastStatusType, BroadcastEventMessageType +from app.models import BROADCAST_TYPE, BroadcastEventMessageType from app.dao.broadcast_message_dao import get_earlier_events_for_broadcast_event, create_broadcast_provider_message from tests.app.db import create_broadcast_message, create_template, create_broadcast_event + def test_get_earlier_events_for_broadcast_event(sample_service): t = create_template(sample_service, BROADCAST_TYPE) bm = create_broadcast_message(t) @@ -44,10 +43,9 @@ def test_get_earlier_events_for_broadcast_event(sample_service): assert earlier_events == [events[0], events[1]] -@freeze_time('2020-02-03 04:05:06') def test_create_broadcast_provider_message_creates_in_correct_state(sample_broadcast_service): t = create_template(sample_broadcast_service, BROADCAST_TYPE) - broadcast_message = create_broadcast_message(t, status=BroadcastStatusType.APPROVED) + broadcast_message = create_broadcast_message(t) broadcast_event = create_broadcast_event( broadcast_message, sent_at=datetime(2020, 1, 1, 12, 0, 0), @@ -59,4 +57,5 @@ def test_create_broadcast_provider_message_creates_in_correct_state(sample_broad assert broadcast_provider_message.status == 'sending' assert broadcast_provider_message.broadcast_event_id == broadcast_event.id - assert broadcast_provider_message.created_at == datetime.utcnow() + assert broadcast_provider_message.created_at is not None + assert broadcast_provider_message.updated_at is None diff --git a/tests/app/db.py b/tests/app/db.py index 969a20e28..30f00e7b6 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -62,7 +62,8 @@ from app.models import ( ServiceContactList, BroadcastMessage, BroadcastStatusType, - BroadcastEvent + BroadcastEvent, + BroadcastProviderMessage ) @@ -1050,3 +1051,18 @@ def create_broadcast_event( db.session.add(b_e) db.session.commit() return b_e + + +def create_broadcast_provider_message( + broadcast_event, + provider, + status='sending' +): + provider_message = BroadcastProviderMessage( + broadcast_event=broadcast_event, + provider=provider, + status=status + ) + db.session.add(provider_message) + db.session.commit() + return provider_message From 087cc5053d2df8d9f320e0c500a8ea113421011f Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 17 Nov 2020 12:35:22 +0000 Subject: [PATCH 6/6] separate cbc proxy into separate clients this is a pretty big and convoluted refactor unfortunately. Previously: There was one global `cbc_proxy_client` object in apps. This class has the information about how to invoke the bt-ee lambda, and handles all calls to lambda. This includes calls to the canary too (which is a separate lambda). The future: There's one global `cbc_proxy_client`. This knows about the different provider functions and lambdas, and you'll need to ask this client for a proxy for your chosen provider. call cbc_proxy_client.get_proxy('ee')` and it'll return you a proxy that knows what ee's lambda function is, how to transform any content in a way that is exclusive to ee, and in future how to parse any response from ee. The present: I also cleaned up some duplicate tests. I'm really not sure about the names of some of these variables - in particular `cbc_proxy_client` isn't a client - it's more of a java style factory, where you call a function on it to get the client of your choice. --- app/__init__.py | 7 +- app/celery/broadcast_message_tasks.py | 10 +- app/celery/scheduled_tasks.py | 2 +- app/clients/cbc_proxy.py | 92 ++++----- .../celery/test_broadcast_message_tasks.py | 12 +- tests/app/celery/test_scheduled_tasks.py | 3 +- tests/app/clients/test_cbc_proxy.py | 177 +++++------------- 7 files changed, 110 insertions(+), 193 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index c412e4b8f..5f85ccad6 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -23,7 +23,7 @@ from werkzeug.local import LocalProxy from app.celery.celery import NotifyCelery from app.clients import NotificationProviderClients -from app.clients.cbc_proxy import CBCProxyClient, CBCProxyNoopClient +from app.clients.cbc_proxy import CBCProxyClient from app.clients.document_download import DocumentDownloadClient from app.clients.email.aws_ses import AwsSesClient from app.clients.email.aws_ses_stub import AwsSesStubClient @@ -61,7 +61,7 @@ zendesk_client = ZendeskClient() statsd_client = StatsdClient() redis_store = RedisClient() performance_platform_client = PerformancePlatformClient() -cbc_proxy_client = CBCProxyNoopClient() +cbc_proxy_client = CBCProxyClient() document_download_client = DocumentDownloadClient() metrics = GDSMetrics() @@ -114,9 +114,6 @@ def create_app(application): performance_platform_client.init_app(application) document_download_client.init_app(application) - global cbc_proxy_client - if application.config['CBC_PROXY_AWS_ACCESS_KEY_ID']: - cbc_proxy_client = CBCProxyClient() cbc_proxy_client.init_app(application) register_blueprint(application) diff --git a/app/celery/broadcast_message_tasks.py b/app/celery/broadcast_message_tasks.py index d13b76cd3..65fdceeba 100644 --- a/app/celery/broadcast_message_tasks.py +++ b/app/celery/broadcast_message_tasks.py @@ -39,8 +39,10 @@ def send_broadcast_provider_message(broadcast_event_id, provider): for polygon in broadcast_event.transmitted_areas["simple_polygons"] ] + cbc_proxy_provider_client = cbc_proxy_client.get_proxy(provider) + if broadcast_event.message_type == BroadcastEventMessageType.ALERT: - cbc_proxy_client.create_and_send_broadcast( + cbc_proxy_provider_client.create_and_send_broadcast( identifier=str(broadcast_provider_message.id), headline="GOV.UK Notify Broadcast", description=broadcast_event.transmitted_content['body'], @@ -49,7 +51,7 @@ def send_broadcast_provider_message(broadcast_event_id, provider): expires=broadcast_event.transmitted_finishes_at_as_cap_datetime_string, ) elif broadcast_event.message_type == BroadcastEventMessageType.UPDATE: - cbc_proxy_client.update_and_send_broadcast( + cbc_proxy_provider_client.update_and_send_broadcast( identifier=str(broadcast_provider_message.id), headline="GOV.UK Notify Broadcast", description=broadcast_event.transmitted_content['body'], @@ -59,7 +61,7 @@ def send_broadcast_provider_message(broadcast_event_id, provider): expires=broadcast_event.transmitted_finishes_at_as_cap_datetime_string, ) elif broadcast_event.message_type == BroadcastEventMessageType.CANCEL: - cbc_proxy_client.cancel_broadcast( + cbc_proxy_provider_client.cancel_broadcast( identifier=str(broadcast_provider_message.id), headline="GOV.UK Notify Broadcast", description=broadcast_event.transmitted_content['body'], @@ -88,4 +90,4 @@ def trigger_link_test(provider): identifier = str(uuid.uuid4()) message = f"Sending a link test to CBC proxy for provider {provider} with ID {identifier}" current_app.logger.info(message) - cbc_proxy_client.send_link_test(identifier) + cbc_proxy_client.get_proxy(provider).send_link_test(identifier) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 948c572aa..7fc16a08f 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -301,7 +301,7 @@ def send_canary_to_cbc_proxy(): identifier = str(uuid.uuid4()) message = f"Sending a canary message to CBC proxy with ID {identifier}" current_app.logger.info(message) - cbc_proxy_client.send_canary(identifier) + cbc_proxy_client.get_proxy('canary').send_canary(identifier) @notify_celery.task(name='trigger-link-tests') diff --git a/app/clients/cbc_proxy.py b/app/clients/cbc_proxy.py index 841e6dad4..99174121b 100644 --- a/app/clients/cbc_proxy.py +++ b/app/clients/cbc_proxy.py @@ -1,6 +1,7 @@ import json import boto3 +from flask import current_app from app.config import BroadcastProvider @@ -16,9 +17,8 @@ from app.config import BroadcastProvider # * description is a string which populates the areaDesc field # * polygon is a list of lat/long pairs # -# previous_provider_messages is a whitespace separated list of message identifiers -# where each identifier is a previous sent message -# ie a Cancel message would have a unique identifier but have the identifier of +# previous_provider_messages is a list of previous events (models.py::BroadcastProviderMessage) +# ie a Cancel message would have a unique event but have the event of # the preceeding Alert message in the previous_provider_messages field @@ -26,11 +26,31 @@ class CBCProxyException(Exception): pass -# Noop = no operation -class CBCProxyNoopClient: +class CBCProxyClient: + _lambda_client = None def init_app(self, app): - pass + if app.config.get('CBC_PROXY_AWS_ACCESS_KEY_ID'): + self._lambda_client = boto3.client( + 'lambda', + region_name='eu-west-2', + aws_access_key_id=app.config['CBC_PROXY_AWS_ACCESS_KEY_ID'], + aws_secret_access_key=app.config['CBC_PROXY_AWS_SECRET_ACCESS_KEY'], + ) + + def get_proxy(self, provider): + proxy_classes = { + 'canary': CBCProxyCanary, + BroadcastProvider.EE: CBCProxyEE, + } + return proxy_classes[provider](self._lambda_client) + + +class CBCProxyClientBase: + lambda_name = None + + def __init__(self, lambda_client): + self._lambda_client = lambda_client def send_canary( self, @@ -67,25 +87,17 @@ class CBCProxyNoopClient: ): pass + def _invoke_lambda(self, payload): + if not self.lambda_name: + current_app.logger.warning( + '{self.__class__.__name__} tried to send {payload} but cbc proxy aws env vars not set' + ) + return -class CBCProxyClient: - provider_function_name_map = { - BroadcastProvider.EE: 'bt-ee-1-proxy', - } - - def init_app(self, app): - self._lambda_client = boto3.client( - 'lambda', - region_name='eu-west-2', - aws_access_key_id=app.config['CBC_PROXY_AWS_ACCESS_KEY_ID'], - aws_secret_access_key=app.config['CBC_PROXY_AWS_SECRET_ACCESS_KEY'], - ) - - def _invoke_lambda(self, function_name, payload): payload_bytes = bytes(json.dumps(payload), encoding='utf8') result = self._lambda_client.invoke( - FunctionName=function_name, + FunctionName=self.lambda_name, InvocationType='RequestResponse', Payload=payload_bytes, ) @@ -98,15 +110,23 @@ class CBCProxyClient: return result + +class CBCProxyCanary(CBCProxyClientBase): + """ + The canary is a lambda which tests notify's connectivity to the Cell Broadcast AWS infrastructure. It calls the + canary, a specific lambda that does not open a vpn or connect to a provider but just responds from within AWS. + """ + lambda_name = 'canary' + def send_canary( self, identifier, ): - """ - canary - a specific lambda that does not connect to a provider, but just confirms the connectivity between - Notify and the CBC proxy AWS account - """ - self._invoke_lambda(function_name='canary', payload={'identifier': identifier}) + self._invoke_lambda(payload={'identifier': identifier}) + + +class CBCProxyEE(CBCProxyClientBase): + lambda_name = 'bt-ee-1-proxy' def send_link_test( self, @@ -118,7 +138,7 @@ class CBCProxyClient: """ payload = {'message_type': 'test', 'identifier': identifier} - self._invoke_lambda(function_name='bt-ee-1-proxy', payload=payload) + self._invoke_lambda(payload=payload) def create_and_send_broadcast( self, @@ -134,20 +154,4 @@ class CBCProxyClient: 'sent': sent, 'expires': expires, } - self._invoke_lambda(function_name='bt-ee-1-proxy', payload=payload) - - # We have not implementated updating a broadcast - def update_and_send_broadcast( - self, - identifier, previous_provider_messages, headline, description, areas, - sent, expires, - ): - pass - - # We have not implemented cancelling a broadcast - def cancel_broadcast( - self, - identifier, previous_provider_messages, headline, description, areas, - sent, expires, - ): - pass + self._invoke_lambda(payload=payload) diff --git a/tests/app/celery/test_broadcast_message_tasks.py b/tests/app/celery/test_broadcast_message_tasks.py index 92dff4db6..097f940a0 100644 --- a/tests/app/celery/test_broadcast_message_tasks.py +++ b/tests/app/celery/test_broadcast_message_tasks.py @@ -48,7 +48,7 @@ def test_send_broadcast_provider_message_sends_data_correctly(mocker, sample_ser event = create_broadcast_event(broadcast_message) mock_create_broadcast = mocker.patch( - 'app.cbc_proxy_client.create_and_send_broadcast', + 'app.clients.cbc_proxy.CBCProxyEE.create_and_send_broadcast', ) assert event.get_provider_message('ee') is None @@ -95,7 +95,7 @@ def test_send_broadcast_provider_message_sends_update_with_references(mocker, sa update_event = create_broadcast_event(broadcast_message, message_type=BroadcastEventMessageType.UPDATE) mock_update_broadcast = mocker.patch( - 'app.cbc_proxy_client.update_and_send_broadcast', + 'app.clients.cbc_proxy.CBCProxyEE.update_and_send_broadcast', ) send_broadcast_provider_message(provider='ee', broadcast_event_id=str(update_event.id)) @@ -140,7 +140,7 @@ def test_send_broadcast_provider_message_sends_cancel_with_references(mocker, sa create_broadcast_provider_message(update_event, 'ee') mock_cancel_broadcast = mocker.patch( - 'app.cbc_proxy_client.cancel_broadcast', + 'app.clients.cbc_proxy.CBCProxyEE.cancel_broadcast', ) send_broadcast_provider_message(provider='ee', broadcast_event_id=str(cancel_event.id)) @@ -181,7 +181,7 @@ def test_send_broadcast_provider_message_errors(mocker, sample_service): event = create_broadcast_event(broadcast_message) mock_create_broadcast = mocker.patch( - 'app.cbc_proxy_client.create_and_send_broadcast', + 'app.clients.cbc_proxy.CBCProxyEE.create_and_send_broadcast', side_effect=Exception('oh no'), ) @@ -210,10 +210,10 @@ def test_trigger_link_tests_invokes_cbc_proxy_client( mocker, ): mock_send_link_test = mocker.patch( - 'app.cbc_proxy_client.send_link_test', + 'app.clients.cbc_proxy.CBCProxyEE.send_link_test', ) - trigger_link_test('some-provider') + trigger_link_test('ee') assert mock_send_link_test.called # the 0th argument of the call to send_link_test diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index a5b4d9cd3..8ea0613a6 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -561,9 +561,10 @@ def test_check_for_services_with_high_failure_rates_or_sending_to_tv_numbers( def test_send_canary_to_cbc_proxy_invokes_cbc_proxy_client( mocker, + notify_api ): mock_send_canary = mocker.patch( - 'app.cbc_proxy_client.send_canary', + 'app.clients.cbc_proxy.CBCProxyCanary.send_canary', ) scheduled_tasks.send_canary_to_cbc_proxy() diff --git a/tests/app/clients/test_cbc_proxy.py b/tests/app/clients/test_cbc_proxy.py index d888d44b6..10534855b 100644 --- a/tests/app/clients/test_cbc_proxy.py +++ b/tests/app/clients/test_cbc_proxy.py @@ -1,13 +1,14 @@ import json import uuid +from unittest.mock import Mock import pytest -from app.clients.cbc_proxy import CBCProxyClient, CBCProxyException +from app.clients.cbc_proxy import CBCProxyClient, CBCProxyException, CBCProxyEE, CBCProxyCanary @pytest.fixture(scope='function') -def cbc_proxy(client, mocker): +def cbc_proxy_client(client, mocker): client = CBCProxyClient() current_app = mocker.Mock(config={ 'CBC_PROXY_AWS_ACCESS_KEY_ID': 'cbc-proxy-aws-access-key-id', @@ -17,19 +18,39 @@ def cbc_proxy(client, mocker): return client -def test_cbc_proxy_lambda_client_has_correct_region(cbc_proxy): - assert cbc_proxy._lambda_client._client_config.region_name == 'eu-west-2' +@pytest.fixture +def cbc_proxy_ee(cbc_proxy_client): + return cbc_proxy_client.get_proxy('ee') -def test_cbc_proxy_lambda_client_has_correct_keys(cbc_proxy): - key = cbc_proxy._lambda_client._request_signer._credentials.access_key - secret = cbc_proxy._lambda_client._request_signer._credentials.secret_key +@pytest.mark.parametrize('provider_name, expected_provider_class', [ + ('ee', CBCProxyEE), + ('canary', CBCProxyCanary), +]) +def test_cbc_proxy_client_returns_correct_client(provider_name, expected_provider_class): + mock_lambda = Mock() + cbc_proxy_client = CBCProxyClient() + cbc_proxy_client._lambda_client = mock_lambda + + ret = cbc_proxy_client.get_proxy(provider_name) + + assert type(ret) == expected_provider_class + assert ret._lambda_client == mock_lambda + + +def test_cbc_proxy_lambda_client_has_correct_region(cbc_proxy_ee): + assert cbc_proxy_ee._lambda_client._client_config.region_name == 'eu-west-2' + + +def test_cbc_proxy_lambda_client_has_correct_keys(cbc_proxy_ee): + key = cbc_proxy_ee._lambda_client._request_signer._credentials.access_key + secret = cbc_proxy_ee._lambda_client._request_signer._credentials.secret_key assert key == 'cbc-proxy-aws-access-key-id' assert secret == 'cbc-proxy-aws-secret-access-key' -def test_cbc_proxy_create_and_send_invokes_function(mocker, cbc_proxy): +def test_cbc_proxy_create_and_send_invokes_function(mocker, cbc_proxy_ee): identifier = 'my-identifier' headline = 'my-headline' description = 'my-description' @@ -50,7 +71,7 @@ def test_cbc_proxy_create_and_send_invokes_function(mocker, cbc_proxy): }] ld_client_mock = mocker.patch.object( - cbc_proxy, + cbc_proxy_ee, '_lambda_client', create=True, ) @@ -59,7 +80,7 @@ def test_cbc_proxy_create_and_send_invokes_function(mocker, cbc_proxy): 'StatusCode': 200, } - cbc_proxy.create_and_send_broadcast( + cbc_proxy_ee.create_and_send_broadcast( identifier=identifier, headline=headline, description=description, @@ -86,7 +107,7 @@ def test_cbc_proxy_create_and_send_invokes_function(mocker, cbc_proxy): assert payload['expires'] == expires -def test_cbc_proxy_create_and_send_handles_invoke_error(mocker, cbc_proxy): +def test_cbc_proxy_create_and_send_handles_invoke_error(mocker, cbc_proxy_ee): identifier = 'my-identifier' headline = 'my-headline' description = 'my-description' @@ -107,7 +128,7 @@ def test_cbc_proxy_create_and_send_handles_invoke_error(mocker, cbc_proxy): }] ld_client_mock = mocker.patch.object( - cbc_proxy, + cbc_proxy_ee, '_lambda_client', create=True, ) @@ -117,7 +138,7 @@ def test_cbc_proxy_create_and_send_handles_invoke_error(mocker, cbc_proxy): } with pytest.raises(CBCProxyException) as e: - cbc_proxy.create_and_send_broadcast( + cbc_proxy_ee.create_and_send_broadcast( identifier=identifier, headline=headline, description=description, @@ -134,7 +155,7 @@ def test_cbc_proxy_create_and_send_handles_invoke_error(mocker, cbc_proxy): ) -def test_cbc_proxy_create_and_send_handles_function_error(mocker, cbc_proxy): +def test_cbc_proxy_create_and_send_handles_function_error(mocker, cbc_proxy_ee): identifier = 'my-identifier' headline = 'my-headline' description = 'my-description' @@ -155,7 +176,7 @@ def test_cbc_proxy_create_and_send_handles_function_error(mocker, cbc_proxy): }] ld_client_mock = mocker.patch.object( - cbc_proxy, + cbc_proxy_ee, '_lambda_client', create=True, ) @@ -166,7 +187,7 @@ def test_cbc_proxy_create_and_send_handles_function_error(mocker, cbc_proxy): } with pytest.raises(CBCProxyException) as e: - cbc_proxy.create_and_send_broadcast( + cbc_proxy_ee.create_and_send_broadcast( identifier=identifier, headline=headline, description=description, @@ -183,11 +204,13 @@ def test_cbc_proxy_create_and_send_handles_function_error(mocker, cbc_proxy): ) -def test_cbc_proxy_send_canary_invokes_function(mocker, cbc_proxy): +def test_cbc_proxy_send_canary_invokes_function(mocker, cbc_proxy_client): identifier = str(uuid.uuid4()) + canary_client = cbc_proxy_client.get_proxy('canary') + ld_client_mock = mocker.patch.object( - cbc_proxy, + canary_client, '_lambda_client', create=True, ) @@ -196,7 +219,7 @@ def test_cbc_proxy_send_canary_invokes_function(mocker, cbc_proxy): 'StatusCode': 200, } - cbc_proxy.send_canary( + canary_client.send_canary( identifier=identifier, ) @@ -213,66 +236,11 @@ def test_cbc_proxy_send_canary_invokes_function(mocker, cbc_proxy): assert payload['identifier'] == identifier -def test_cbc_proxy_send_canary_handles_invoke_error(mocker, cbc_proxy): +def test_cbc_proxy_send_link_test_invokes_function(mocker, cbc_proxy_ee): identifier = str(uuid.uuid4()) ld_client_mock = mocker.patch.object( - cbc_proxy, - '_lambda_client', - create=True, - ) - - ld_client_mock.invoke.return_value = { - 'StatusCode': 400, - } - - with pytest.raises(CBCProxyException) as e: - cbc_proxy.send_canary( - identifier=identifier, - ) - - assert e.match('Could not invoke lambda') - - ld_client_mock.invoke.assert_called_once_with( - FunctionName='canary', - InvocationType='RequestResponse', - Payload=mocker.ANY, - ) - - -def test_cbc_proxy_send_canary_handles_function_error(mocker, cbc_proxy): - identifier = str(uuid.uuid4()) - - ld_client_mock = mocker.patch.object( - cbc_proxy, - '_lambda_client', - create=True, - ) - - ld_client_mock.invoke.return_value = { - 'StatusCode': 200, - 'FunctionError': 'something', - } - - with pytest.raises(CBCProxyException) as e: - cbc_proxy.send_canary( - identifier=identifier, - ) - - assert e.match('Function exited with unhandled exception') - - ld_client_mock.invoke.assert_called_once_with( - FunctionName='canary', - InvocationType='RequestResponse', - Payload=mocker.ANY, - ) - - -def test_cbc_proxy_send_link_test_invokes_function(mocker, cbc_proxy): - identifier = str(uuid.uuid4()) - - ld_client_mock = mocker.patch.object( - cbc_proxy, + cbc_proxy_ee, '_lambda_client', create=True, ) @@ -281,7 +249,7 @@ def test_cbc_proxy_send_link_test_invokes_function(mocker, cbc_proxy): 'StatusCode': 200, } - cbc_proxy.send_link_test( + cbc_proxy_ee.send_link_test( identifier=identifier, ) @@ -297,58 +265,3 @@ def test_cbc_proxy_send_link_test_invokes_function(mocker, cbc_proxy): assert payload['identifier'] == identifier assert payload['message_type'] == 'test' - - -def test_cbc_proxy_send_link_test_handles_invoke_error(mocker, cbc_proxy): - identifier = str(uuid.uuid4()) - - ld_client_mock = mocker.patch.object( - cbc_proxy, - '_lambda_client', - create=True, - ) - - ld_client_mock.invoke.return_value = { - 'StatusCode': 400, - } - - with pytest.raises(CBCProxyException) as e: - cbc_proxy.send_link_test( - identifier=identifier, - ) - - assert e.match('Could not invoke lambda') - - ld_client_mock.invoke.assert_called_once_with( - FunctionName='bt-ee-1-proxy', - InvocationType='RequestResponse', - Payload=mocker.ANY, - ) - - -def test_cbc_proxy_send_link_test_handles_function_error(mocker, cbc_proxy): - identifier = str(uuid.uuid4()) - - ld_client_mock = mocker.patch.object( - cbc_proxy, - '_lambda_client', - create=True, - ) - - ld_client_mock.invoke.return_value = { - 'StatusCode': 200, - 'FunctionError': 'something', - } - - with pytest.raises(CBCProxyException) as e: - cbc_proxy.send_link_test( - identifier=identifier, - ) - - assert e.match('Function exited with unhandled exception') - - ld_client_mock.invoke.assert_called_once_with( - FunctionName='bt-ee-1-proxy', - InvocationType='RequestResponse', - Payload=mocker.ANY, - )