From bd06b186843451d60691e38af332412d06294316 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 20 Sep 2016 17:24:28 +0100 Subject: [PATCH 1/9] Refactor of celery tasks. - Aim to move the code that contacts providers into it's own module. - Celery tasks now call this module to send to provider - No exceptions caught in the new module. Celery tasks now use any exception to trigger a retry. - tests moved about - new test directory for the new class, all tests from celery test module moved, excepting the retry logic. --- app/celery/provider_tasks.py | 191 ++------- app/delivery/__init__.py | 0 app/delivery/send_to_providers.py | 139 ++++++ tests/app/celery/test_provider_tasks.py | 423 +------------------ tests/app/delivery/test_send_to_providers.py | 415 ++++++++++++++++++ 5 files changed, 586 insertions(+), 582 deletions(-) create mode 100644 app/delivery/__init__.py create mode 100644 app/delivery/send_to_providers.py create mode 100644 tests/app/delivery/test_send_to_providers.py diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 60ed740b5..ffc5787f9 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -1,33 +1,15 @@ -from datetime import datetime -from monotonic import monotonic -from urllib.parse import urljoin - from flask import current_app -from notifications_utils.recipients import ( - validate_and_format_phone_number -) -from notifications_utils.template import Template, get_sms_fragment_count -from notifications_utils.renderers import HTMLEmail, PlainTextEmail, SMSMessage -from app import notify_celery, statsd_client, clients, create_uuid -from app.clients.email import EmailClientException -from app.clients.sms import SmsClientException -from app.dao.notifications_dao import ( - get_notification_by_id, - dao_update_notification, - update_notification_status_by_id -) -from app.dao.provider_details_dao import get_provider_details_by_notification_type -from app.dao.services_dao import dao_fetch_service_by_id -from app.celery.research_mode_tasks import send_sms_response, send_email_response -from app.dao.templates_dao import dao_get_template_by_id - -from app.models import SMS_TYPE, EMAIL_TYPE, KEY_TYPE_TEST, BRANDING_ORG +from app import notify_celery +from app.dao.notifications_dao import update_notification_status_by_id from app.statsd_decorators import statsd +from app.delivery import send_to_providers + def retry_iteration_to_delay(retry=0): """ + :param retry times we have performed a retry Given current retry calculate some delay before retrying 0: 10 seconds 1: 60 seconds (1 minutes) @@ -52,149 +34,38 @@ def retry_iteration_to_delay(retry=0): @notify_celery.task(bind=True, name="send-sms-to-provider", max_retries=5, default_retry_delay=5) @statsd(namespace="tasks") def send_sms_to_provider(self, service_id, notification_id): - service = dao_fetch_service_by_id(service_id) - provider = provider_to_use(SMS_TYPE, notification_id) - notification = get_notification_by_id(notification_id) - if notification.status == 'created': - template_model = dao_get_template_by_id(notification.template_id, notification.template_version) - template = Template( - template_model.__dict__, - values={} if not notification.personalisation else notification.personalisation, - renderer=SMSMessage(prefix=service.name, sender=service.sms_sender) - ) + try: + send_to_providers.send_sms_to_provider(notification_id) + except Exception as e: try: - if service.research_mode or notification.key_type == KEY_TYPE_TEST: - send_sms_response.apply_async( - (provider.get_name(), str(notification_id), notification.to), queue='research-mode' - ) - notification.billable_units = 0 - else: - provider.send_sms( - to=validate_and_format_phone_number(notification.to), - content=template.replaced, - reference=str(notification_id), - sender=service.sms_sender - ) - notification.billable_units = get_sms_fragment_count(template.replaced_content_count) - - notification.sent_at = datetime.utcnow() - notification.sent_by = provider.get_name() - notification.status = 'sending' - dao_update_notification(notification) - except SmsClientException as e: - try: - current_app.logger.error( - "RETRY: SMS notification {} failed".format(notification_id) - ) - current_app.logger.exception(e) - self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries)) - except self.MaxRetriesExceededError: - current_app.logger.error( - "RETRY FAILED: task send_sms_to_provider failed for notification {}".format(notification.id), - e - ) - update_notification_status_by_id(notification.id, 'technical-failure') - - current_app.logger.info( - "SMS {} sent to provider at {}".format(notification_id, notification.sent_at) - ) - delta_milliseconds = (datetime.utcnow() - notification.created_at).total_seconds() * 1000 - statsd_client.timing("sms.total-time", delta_milliseconds) - - -def provider_to_use(notification_type, notification_id): - active_providers_in_order = [ - provider for provider in get_provider_details_by_notification_type(notification_type) if provider.active - ] - - if not active_providers_in_order: - current_app.logger.error( - "{} {} failed as no active providers".format(notification_type, notification_id) - ) - raise Exception("No active {} providers".format(notification_type)) - - return clients.get_client_by_name_and_type(active_providers_in_order[0].identifier, notification_type) + current_app.logger.error( + "RETRY: SMS notification {} failed".format(notification_id) + ) + current_app.logger.exception(e) + self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries)) + except self.MaxRetriesExceededError: + current_app.logger.error( + "RETRY FAILED: task send_sms_to_provider failed for notification {}".format(notification_id), + e + ) + update_notification_status_by_id(notification_id, 'technical-failure') @notify_celery.task(bind=True, name="send-email-to-provider", max_retries=5, default_retry_delay=5) @statsd(namespace="tasks") def send_email_to_provider(self, service_id, notification_id): - service = dao_fetch_service_by_id(service_id) - provider = provider_to_use(EMAIL_TYPE, notification_id) - notification = get_notification_by_id(notification_id) - if notification.status == 'created': + try: + send_to_providers.send_email_response(notification_id) + except Exception as e: try: - template_dict = dao_get_template_by_id(notification.template_id, notification.template_version).__dict__ - - html_email = Template( - template_dict, - values=notification.personalisation, - renderer=get_html_email_renderer(service) + current_app.logger.error( + "RETRY: Email notification {} failed".format(notification_id) ) - - plain_text_email = Template( - template_dict, - values=notification.personalisation, - renderer=PlainTextEmail() + current_app.logger.exception(e) + self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries)) + except self.MaxRetriesExceededError: + current_app.logger.error( + "RETRY FAILED: task send_email_to_provider failed for notification {}".format(notification_id), + e ) - - if service.research_mode or notification.key_type == KEY_TYPE_TEST: - reference = str(create_uuid()) - send_email_response.apply_async( - (provider.get_name(), reference, notification.to), queue='research-mode' - ) - else: - from_address = '"{}" <{}@{}>'.format(service.name, service.email_from, - current_app.config['NOTIFY_EMAIL_DOMAIN']) - reference = provider.send_email( - from_address, - notification.to, - plain_text_email.replaced_subject, - body=plain_text_email.replaced, - html_body=html_email.replaced, - reply_to_address=service.reply_to_email_address, - ) - - notification.reference = reference - notification.sent_at = datetime.utcnow() - notification.sent_by = provider.get_name(), - notification.status = 'sending' - dao_update_notification(notification) - except EmailClientException as e: - try: - current_app.logger.error( - "RETRY: Email notification {} failed".format(notification_id) - ) - current_app.logger.exception(e) - self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries)) - except self.MaxRetriesExceededError: - current_app.logger.error( - "RETRY FAILED: task send_email_to_provider failed for notification {}".format(notification.id), - e - ) - update_notification_status_by_id(notification.id, 'technical-failure') - - current_app.logger.info( - "Email {} sent to provider at {}".format(notification_id, notification.sent_at) - ) - delta_milliseconds = (datetime.utcnow() - notification.created_at).total_seconds() * 1000 - statsd_client.timing("email.total-time", delta_milliseconds) - - -def get_html_email_renderer(service): - govuk_banner = service.branding != BRANDING_ORG - if service.organisation: - logo = '{}{}{}'.format( - current_app.config['ADMIN_BASE_URL'], - current_app.config['BRANDING_PATH'], - service.organisation.logo - ) - branding = { - 'brand_colour': service.organisation.colour, - 'brand_logo': logo, - 'brand_name': service.organisation.name, - } - else: - branding = {} - - return HTMLEmail(govuk_banner=govuk_banner, **branding) + update_notification_status_by_id(notification_id, 'technical-failure') diff --git a/app/delivery/__init__.py b/app/delivery/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py new file mode 100644 index 000000000..73d7fc150 --- /dev/null +++ b/app/delivery/send_to_providers.py @@ -0,0 +1,139 @@ +from datetime import datetime + +from flask import current_app +from notifications_utils.recipients import ( + validate_and_format_phone_number +) +from notifications_utils.template import Template, get_sms_fragment_count +from notifications_utils.renderers import HTMLEmail, PlainTextEmail, SMSMessage + +from app import clients, statsd_client, create_uuid +from app.dao.notifications_dao import ( + get_notification_by_id, + dao_update_notification +) +from app.dao.provider_details_dao import get_provider_details_by_notification_type +from app.dao.services_dao import dao_fetch_service_by_id +from app.celery.research_mode_tasks import send_sms_response, send_email_response +from app.dao.templates_dao import dao_get_template_by_id + +from app.models import SMS_TYPE, KEY_TYPE_TEST, BRANDING_ORG, EMAIL_TYPE + + +def send_sms_to_provider(notification_id): + notification = get_notification_by_id(notification_id) + service = dao_fetch_service_by_id(notification.service_id) + provider = provider_to_use(SMS_TYPE, notification_id) + if notification.status == 'created': + template_model = dao_get_template_by_id(notification.template_id, notification.template_version) + template = Template( + template_model.__dict__, + values={} if not notification.personalisation else notification.personalisation, + renderer=SMSMessage(prefix=service.name, sender=service.sms_sender) + ) + if service.research_mode or notification.key_type == KEY_TYPE_TEST: + send_sms_response.apply_async( + (provider.get_name(), str(notification_id), notification.to), queue='research-mode' + ) + notification.billable_units = 0 + else: + provider.send_sms( + to=validate_and_format_phone_number(notification.to), + content=template.replaced, + reference=str(notification_id), + sender=service.sms_sender + ) + notification.billable_units = get_sms_fragment_count(template.replaced_content_count) + + notification.sent_at = datetime.utcnow() + notification.sent_by = provider.get_name() + notification.status = 'sending' + dao_update_notification(notification) + + current_app.logger.info( + "SMS {} sent to provider at {}".format(notification_id, notification.sent_at) + ) + delta_milliseconds = (datetime.utcnow() - notification.created_at).total_seconds() * 1000 + statsd_client.timing("sms.total-time", delta_milliseconds) + + +def send_email_to_provider(notification_id): + notification = get_notification_by_id(notification_id) + service = dao_fetch_service_by_id(notification.service_id) + provider = provider_to_use(EMAIL_TYPE, notification_id) + if notification.status == 'created': + template_dict = dao_get_template_by_id(notification.template_id, notification.template_version).__dict__ + + html_email = Template( + template_dict, + values=notification.personalisation, + renderer=get_html_email_renderer(service) + ) + + plain_text_email = Template( + template_dict, + values=notification.personalisation, + renderer=PlainTextEmail() + ) + + if service.research_mode or notification.key_type == KEY_TYPE_TEST: + reference = str(create_uuid()) + send_email_response.apply_async( + (provider.get_name(), reference, notification.to), queue='research-mode' + ) + else: + from_address = '"{}" <{}@{}>'.format(service.name, service.email_from, + current_app.config['NOTIFY_EMAIL_DOMAIN']) + reference = provider.send_email( + from_address, + notification.to, + plain_text_email.replaced_subject, + body=plain_text_email.replaced, + html_body=html_email.replaced, + reply_to_address=service.reply_to_email_address, + ) + + notification.reference = reference + notification.sent_at = datetime.utcnow() + notification.sent_by = provider.get_name(), + notification.status = 'sending' + dao_update_notification(notification) + + current_app.logger.info( + "Email {} sent to provider at {}".format(notification_id, notification.sent_at) + ) + delta_milliseconds = (datetime.utcnow() - notification.created_at).total_seconds() * 1000 + statsd_client.timing("email.total-time", delta_milliseconds) + + +def provider_to_use(notification_type, notification_id): + active_providers_in_order = [ + provider for provider in get_provider_details_by_notification_type(notification_type) if provider.active + ] + + if not active_providers_in_order: + current_app.logger.error( + "{} {} failed as no active providers".format(notification_type, notification_id) + ) + raise Exception("No active {} providers".format(notification_type)) + + return clients.get_client_by_name_and_type(active_providers_in_order[0].identifier, notification_type) + + +def get_html_email_renderer(service): + govuk_banner = service.branding != BRANDING_ORG + if service.organisation: + logo = '{}{}{}'.format( + current_app.config['ADMIN_BASE_URL'], + current_app.config['BRANDING_PATH'], + service.organisation.logo + ) + branding = { + 'brand_colour': service.organisation.colour, + 'brand_logo': logo, + 'brand_name': service.organisation.name, + } + else: + branding = {} + + return HTMLEmail(govuk_banner=govuk_banner, **branding) diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 08a6026e9..310184629 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -62,231 +62,6 @@ def test_should_by_240_minute_delay_on_retry_two(): assert provider_tasks.retry_iteration_to_delay(4) == 14400 -def test_should_return_highest_priority_active_provider(notify_db, notify_db_session): - providers = provider_details_dao.get_provider_details_by_notification_type('sms') - - first = providers[0] - second = providers[1] - - assert provider_tasks.provider_to_use('sms', '1234').name == first.identifier - - first.priority = 20 - second.priority = 10 - - provider_details_dao.dao_update_provider_details(first) - provider_details_dao.dao_update_provider_details(second) - - assert provider_tasks.provider_to_use('sms', '1234').name == second.identifier - - first.priority = 10 - first.active = False - second.priority = 20 - - provider_details_dao.dao_update_provider_details(first) - provider_details_dao.dao_update_provider_details(second) - - assert provider_tasks.provider_to_use('sms', '1234').name == second.identifier - - first.active = True - provider_details_dao.dao_update_provider_details(first) - - assert provider_tasks.provider_to_use('sms', '1234').name == first.identifier - - -def test_should_send_personalised_template_to_correct_sms_provider_and_persist( - notify_db, - notify_db_session, - sample_template_with_placeholders, - mocker -): - db_notification = sample_notification(notify_db, notify_db_session, template=sample_template_with_placeholders, - to_field="+447234123123", personalisation={"name": "Jo"}, - status='created') - - mocker.patch('app.mmg_client.send_sms') - mocker.patch('app.mmg_client.get_name', return_value="mmg") - - send_sms_to_provider( - db_notification.service_id, - db_notification.id - ) - - mmg_client.send_sms.assert_called_once_with( - to=format_phone_number(validate_phone_number("+447234123123")), - content="Sample service: Hello Jo\nYour thing is due soon", - reference=str(db_notification.id), - sender=None - ) - notification = Notification.query.filter_by(id=db_notification.id).one() - - assert notification.status == 'sending' - assert notification.sent_at <= datetime.utcnow() - assert notification.sent_by == 'mmg' - assert notification.billable_units == 1 - assert notification.personalisation == {"name": "Jo"} - - -def test_should_send_personalised_template_to_correct_email_provider_and_persist( - notify_db, - notify_db_session, - sample_email_template_with_placeholders, - mocker -): - db_notification = sample_notification( - notify_db=notify_db, notify_db_session=notify_db_session, - template=sample_email_template_with_placeholders, - to_field="jo.smith@example.com", - personalisation={'name': 'Jo'} - ) - - mocker.patch('app.aws_ses_client.send_email', return_value='reference') - mocker.patch('app.aws_ses_client.get_name', return_value="ses") - - send_email_to_provider( - db_notification.service_id, - db_notification.id - ) - - app.aws_ses_client.send_email.assert_called_once_with( - '"Sample service" ', - 'jo.smith@example.com', - 'Jo', - body='Hello Jo\nThis is an email from GOV.\u200bUK', - html_body=ANY, - reply_to_address=None - ) - assert ' version_on_notification - - send_sms_to_provider( - db_notification.service_id, - db_notification.id - ) - - mmg_client.send_sms.assert_called_once_with( - to=format_phone_number(validate_phone_number("+447234123123")), - content="Sample service: This is a template:\nwith a newline", - reference=str(db_notification.id), - sender=None - ) - - persisted_notification = notifications_dao.get_notification_by_id(db_notification.id) - assert persisted_notification.to == db_notification.to - assert persisted_notification.template_id == sample_template.id - assert persisted_notification.template_version == version_on_notification - assert persisted_notification.template_version != sample_template.version - assert persisted_notification.status == 'sending' - assert not persisted_notification.personalisation - - -@pytest.mark.parametrize('research_mode,key_type', [ - (True, KEY_TYPE_NORMAL), - (False, KEY_TYPE_TEST) -]) -def test_should_call_send_sms_response_task_if_research_mode(notify_db, sample_service, sample_notification, mocker, - research_mode, key_type): - mocker.patch('app.mmg_client.send_sms') - mocker.patch('app.mmg_client.get_name', return_value="mmg") - mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async') - - if research_mode: - sample_service.research_mode = True - notify_db.session.add(sample_service) - notify_db.session.commit() - - sample_notification.key_type = key_type - - send_sms_to_provider( - sample_notification.service_id, - sample_notification.id - ) - assert not mmg_client.send_sms.called - send_sms_response.apply_async.assert_called_once_with( - ('mmg', str(sample_notification.id), sample_notification.to), queue='research-mode' - ) - - persisted_notification = notifications_dao.get_notification_by_id(sample_notification.id) - assert persisted_notification.to == sample_notification.to - assert persisted_notification.template_id == sample_notification.template_id - assert persisted_notification.status == 'sending' - assert persisted_notification.sent_at <= datetime.utcnow() - assert persisted_notification.sent_by == 'mmg' - assert not persisted_notification.personalisation - - -@pytest.mark.parametrize('research_mode,key_type', [ - (True, KEY_TYPE_NORMAL), - (False, KEY_TYPE_TEST) -]) -def test_not_should_update_provider_stats_on_success_in_research_mode(notify_db, sample_service, sample_notification, - mocker, research_mode, key_type): - provider_stats = provider_statistics_dao.get_provider_statistics(sample_service).all() - assert len(provider_stats) == 0 - - mocker.patch('app.mmg_client.send_sms') - mocker.patch('app.mmg_client.get_name', return_value="mmg") - mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async') - if research_mode: - sample_service.research_mode = True - notify_db.session.add(sample_service) - notify_db.session.commit() - sample_notification.key_type = key_type - - send_sms_to_provider( - sample_notification.service_id, - sample_notification.id - ) - - updated_provider_stats = provider_statistics_dao.get_provider_statistics(sample_service).all() - assert len(updated_provider_stats) == 0 - - -def test_should_not_send_to_provider_when_status_is_not_created(notify_db, notify_db_session, - sample_service, - mocker): - notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, - service=sample_service, - status='sending') - mocker.patch('app.mmg_client.send_sms') - mocker.patch('app.mmg_client.get_name', return_value="mmg") - mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async') - - send_sms_to_provider( - notification.service_id, - notification.id - ) - - app.mmg_client.send_sms.assert_not_called() - app.celery.research_mode_tasks.send_sms_response.apply_async.assert_not_called() - - def test_should_go_into_technical_error_if_exceeds_retries( notify_db, notify_db_session, @@ -296,7 +71,7 @@ def test_should_go_into_technical_error_if_exceeds_retries( notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, service=sample_service, status='created') - mocker.patch('app.mmg_client.send_sms', side_effect=SmsClientException("EXPECTED")) + mocker.patch('app.delivery.send_to_providers.send_sms_to_provider', side_effect=Exception("EXPECTED")) mocker.patch('app.celery.provider_tasks.send_sms_to_provider.retry', side_effect=MaxRetriesExceededError()) send_sms_to_provider( @@ -310,90 +85,6 @@ def test_should_go_into_technical_error_if_exceeds_retries( assert db_notification.status == 'technical-failure' -def test_should_send_sms_sender_from_service_if_present( - notify_db, - notify_db_session, - sample_service, - sample_template, - mocker): - db_notification = sample_notification(notify_db, notify_db_session, template=sample_template, - to_field="+447234123123", - status='created') - - sample_service.sms_sender = 'elevenchars' - notify_db.session.add(sample_service) - notify_db.session.commit() - - mocker.patch('app.mmg_client.send_sms') - mocker.patch('app.mmg_client.get_name', return_value="mmg") - - send_sms_to_provider( - db_notification.service_id, - db_notification.id - ) - - mmg_client.send_sms.assert_called_once_with( - to=format_phone_number(validate_phone_number("+447234123123")), - content="This is a template:\nwith a newline", - reference=str(db_notification.id), - sender=sample_service.sms_sender - ) - - -@pytest.mark.parametrize('research_mode,key_type', [ - (True, KEY_TYPE_NORMAL), - (False, KEY_TYPE_TEST) -]) -def test_send_email_to_provider_should_call_research_mode_task_response_task_if_research_mode( - notify_db, - notify_db_session, - sample_service, - sample_email_template, - ses_provider, - mocker, - research_mode, - key_type): - notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, - template=sample_email_template, - to_field="john@smith.com", - key_type=key_type - ) - - reference = uuid.uuid4() - mocker.patch('app.uuid.uuid4', return_value=reference) - mocker.patch('app.aws_ses_client.send_email') - mocker.patch('app.aws_ses_client.get_name', return_value="ses") - mocker.patch('app.celery.research_mode_tasks.send_email_response.apply_async') - - if research_mode: - sample_service.research_mode = True - notify_db.session.add(sample_service) - notify_db.session.commit() - assert not get_provider_statistics( - sample_email_template.service, - providers=[ses_provider.identifier]).first() - send_email_to_provider( - sample_service.id, - notification.id - ) - assert not app.aws_ses_client.send_email.called - send_email_response.apply_async.assert_called_once_with( - ('ses', str(reference), 'john@smith.com'), queue="research-mode" - ) - assert not get_provider_statistics( - sample_email_template.service, - providers=[ses_provider.identifier]).first() - persisted_notification = Notification.query.filter_by(id=notification.id).one() - - assert persisted_notification.to == 'john@smith.com' - assert persisted_notification.template_id == sample_email_template.id - assert persisted_notification.status == 'sending' - assert persisted_notification.sent_at <= datetime.utcnow() - assert persisted_notification.created_at <= datetime.utcnow() - assert persisted_notification.sent_by == 'ses' - assert persisted_notification.reference == str(reference) - - def test_send_email_to_provider_should_go_into_technical_error_if_exceeds_retries( notify_db, notify_db_session, @@ -416,115 +107,3 @@ def test_send_email_to_provider_should_go_into_technical_error_if_exceeds_retrie db_notification = Notification.query.filter_by(id=notification.id).one() assert db_notification.status == 'technical-failure' - - -def test_send_email_to_provider_should_not_send_to_provider_when_status_is_not_created(notify_db, notify_db_session, - sample_service, - sample_email_template, - mocker): - notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, - template=sample_email_template, - service=sample_service, - status='sending') - mocker.patch('app.aws_ses_client.send_email') - mocker.patch('app.aws_ses_client.get_name', return_value="ses") - mocker.patch('app.celery.research_mode_tasks.send_email_response.apply_async') - - send_sms_to_provider( - notification.service_id, - notification.id - ) - - app.aws_ses_client.send_email.assert_not_called() - app.celery.research_mode_tasks.send_email_response.apply_async.assert_not_called() - - -def test_send_email_should_use_service_reply_to_email( - notify_db, notify_db_session, - sample_service, - sample_email_template, - mocker): - mocker.patch('app.aws_ses_client.send_email', return_value='reference') - mocker.patch('app.aws_ses_client.get_name', return_value="ses") - - db_notification = sample_notification(notify_db, notify_db_session, template=sample_email_template) - sample_service.reply_to_email_address = 'foo@bar.com' - - send_email_to_provider( - db_notification.service_id, - db_notification.id, - ) - - app.aws_ses_client.send_email.assert_called_once_with( - ANY, - ANY, - ANY, - body=ANY, - html_body=ANY, - reply_to_address=sample_service.reply_to_email_address - ) - - -def test_should_not_set_billable_units_if_research_mode(notify_db, sample_service, sample_notification, mocker): - mocker.patch('app.mmg_client.send_sms') - mocker.patch('app.mmg_client.get_name', return_value="mmg") - mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async') - - sample_service.research_mode = True - notify_db.session.add(sample_service) - notify_db.session.commit() - - send_sms_to_provider( - sample_notification.service_id, - sample_notification.id - ) - - persisted_notification = notifications_dao.get_notification_by_id(sample_notification.id) - assert persisted_notification.billable_units == 0 - - -def test_get_html_email_renderer_should_return_for_normal_service(sample_service): - renderer = provider_tasks.get_html_email_renderer(sample_service) - assert renderer.govuk_banner - assert renderer.brand_colour is None - assert renderer.brand_logo is None - assert renderer.brand_name is None - - -@pytest.mark.parametrize('branding_type, govuk_banner', [ - (BRANDING_ORG, False), - (BRANDING_BOTH, True) -]) -def test_get_html_email_renderer_with_branding_details(branding_type, govuk_banner, notify_db, sample_service): - sample_service.branding = branding_type - org = Organisation(colour='#000000', logo='justice-league.png', name='Justice League') - sample_service.organisation = org - notify_db.session.add_all([sample_service, org]) - notify_db.session.commit() - - renderer = provider_tasks.get_html_email_renderer(sample_service) - - assert renderer.govuk_banner == govuk_banner - assert renderer.brand_colour == '000000' - assert renderer.brand_name == 'Justice League' - - -def test_get_html_email_renderer_prepends_logo_path(notify_db, sample_service): - sample_service.branding = BRANDING_ORG - org = Organisation(colour='#000000', logo='justice-league.png', name='Justice League') - sample_service.organisation = org - notify_db.session.add_all([sample_service, org]) - notify_db.session.commit() - - renderer = provider_tasks.get_html_email_renderer(sample_service) - - assert renderer.brand_logo == 'http://localhost:6012/static/images/email-template/crests/justice-league.png' - - -def _get_provider_statistics(service, **kwargs): - query = ProviderStatistics.query.filter_by(service=service) - if 'providers' in kwargs: - providers = ProviderDetails.query.filter(ProviderDetails.identifier.in_(kwargs['providers'])).all() - provider_ids = [provider.id for provider in providers] - query = query.filter(ProviderStatistics.provider_id.in_(provider_ids)) - return query diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py new file mode 100644 index 000000000..755f33835 --- /dev/null +++ b/tests/app/delivery/test_send_to_providers.py @@ -0,0 +1,415 @@ +import uuid +from datetime import datetime + +import pytest +from mock import ANY + +import app +from app import mmg_client +from app.dao import (provider_details_dao, notifications_dao, provider_statistics_dao) +from app.dao.provider_statistics_dao import get_provider_statistics +from app.delivery import send_to_providers +from app.models import Notification, KEY_TYPE_NORMAL, KEY_TYPE_TEST, BRANDING_ORG, BRANDING_BOTH, Organisation +from tests.app.conftest import sample_notification + +from notifications_utils.recipients import validate_phone_number, format_phone_number + + +def test_should_return_highest_priority_active_provider(notify_db, notify_db_session): + providers = provider_details_dao.get_provider_details_by_notification_type('sms') + + first = providers[0] + second = providers[1] + + assert send_to_providers.provider_to_use('sms', '1234').name == first.identifier + + first.priority = 20 + second.priority = 10 + + provider_details_dao.dao_update_provider_details(first) + provider_details_dao.dao_update_provider_details(second) + + assert send_to_providers.provider_to_use('sms', '1234').name == second.identifier + + first.priority = 10 + first.active = False + second.priority = 20 + + provider_details_dao.dao_update_provider_details(first) + provider_details_dao.dao_update_provider_details(second) + + assert send_to_providers.provider_to_use('sms', '1234').name == second.identifier + + first.active = True + provider_details_dao.dao_update_provider_details(first) + + assert send_to_providers.provider_to_use('sms', '1234').name == first.identifier + + +def test_should_send_personalised_template_to_correct_sms_provider_and_persist( + notify_db, + notify_db_session, + sample_template_with_placeholders, + mocker +): + db_notification = sample_notification(notify_db, notify_db_session, template=sample_template_with_placeholders, + to_field="+447234123123", personalisation={"name": "Jo"}, + status='created') + + mocker.patch('app.mmg_client.send_sms') + mocker.patch('app.mmg_client.get_name', return_value="mmg") + + send_to_providers.send_sms_to_provider( + db_notification.id + ) + + mmg_client.send_sms.assert_called_once_with( + to=format_phone_number(validate_phone_number("+447234123123")), + content="Sample service: Hello Jo\nYour thing is due soon", + reference=str(db_notification.id), + sender=None + ) + notification = Notification.query.filter_by(id=db_notification.id).one() + + assert notification.status == 'sending' + assert notification.sent_at <= datetime.utcnow() + assert notification.sent_by == 'mmg' + assert notification.billable_units == 1 + assert notification.personalisation == {"name": "Jo"} + + +def test_should_send_personalised_template_to_correct_email_provider_and_persist( + notify_db, + notify_db_session, + sample_email_template_with_placeholders, + mocker +): + db_notification = sample_notification( + notify_db=notify_db, notify_db_session=notify_db_session, + template=sample_email_template_with_placeholders, + to_field="jo.smith@example.com", + personalisation={'name': 'Jo'} + ) + + mocker.patch('app.aws_ses_client.send_email', return_value='reference') + mocker.patch('app.aws_ses_client.get_name', return_value="ses") + + send_to_providers.send_email_to_provider( + db_notification.id + ) + + app.aws_ses_client.send_email.assert_called_once_with( + '"Sample service" ', + 'jo.smith@example.com', + 'Jo', + body='Hello Jo\nThis is an email from GOV.\u200bUK', + html_body=ANY, + reply_to_address=None + ) + assert ' version_on_notification + + send_to_providers.send_sms_to_provider( + db_notification.id + ) + + mmg_client.send_sms.assert_called_once_with( + to=format_phone_number(validate_phone_number("+447234123123")), + content="Sample service: This is a template:\nwith a newline", + reference=str(db_notification.id), + sender=None + ) + + persisted_notification = notifications_dao.get_notification_by_id(db_notification.id) + assert persisted_notification.to == db_notification.to + assert persisted_notification.template_id == sample_template.id + assert persisted_notification.template_version == version_on_notification + assert persisted_notification.template_version != sample_template.version + assert persisted_notification.status == 'sending' + assert not persisted_notification.personalisation + + +@pytest.mark.parametrize('research_mode,key_type', [ + (True, KEY_TYPE_NORMAL), + (False, KEY_TYPE_TEST) +]) +def test_should_call_send_sms_response_task_if_research_mode(notify_db, sample_service, sample_notification, mocker, + research_mode, key_type): + mocker.patch('app.mmg_client.send_sms') + mocker.patch('app.mmg_client.get_name', return_value="mmg") + mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async') + + if research_mode: + sample_service.research_mode = True + notify_db.session.add(sample_service) + notify_db.session.commit() + + sample_notification.key_type = key_type + + send_to_providers.send_sms_to_provider( + sample_notification.id + ) + assert not mmg_client.send_sms.called + send_to_providers.send_sms_response.apply_async.assert_called_once_with( + ('mmg', str(sample_notification.id), sample_notification.to), queue='research-mode' + ) + + persisted_notification = notifications_dao.get_notification_by_id(sample_notification.id) + assert persisted_notification.to == sample_notification.to + assert persisted_notification.template_id == sample_notification.template_id + assert persisted_notification.status == 'sending' + assert persisted_notification.sent_at <= datetime.utcnow() + assert persisted_notification.sent_by == 'mmg' + assert not persisted_notification.personalisation + + +@pytest.mark.parametrize('research_mode,key_type', [ + (True, KEY_TYPE_NORMAL), + (False, KEY_TYPE_TEST) +]) +def test_should_set_billable_units_to_zero_in_research_mode_or_test_key( + notify_db, sample_service, sample_notification, mocker, research_mode, key_type): + provider_stats = provider_statistics_dao.get_provider_statistics(sample_service).all() + assert len(provider_stats) == 0 + + mocker.patch('app.mmg_client.send_sms') + mocker.patch('app.mmg_client.get_name', return_value="mmg") + mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async') + if research_mode: + sample_service.research_mode = True + notify_db.session.add(sample_service) + notify_db.session.commit() + sample_notification.key_type = key_type + + send_to_providers.send_sms_to_provider( + sample_notification.id + ) + + assert notifications_dao.get_notification_by_id(sample_notification.id).billable_units == 0 + + +def test_should_not_send_to_provider_when_status_is_not_created(notify_db, notify_db_session, + sample_service, + mocker): + notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, + service=sample_service, + status='sending') + mocker.patch('app.mmg_client.send_sms') + mocker.patch('app.mmg_client.get_name', return_value="mmg") + mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async') + + send_to_providers.send_sms_to_provider( + notification.id + ) + + app.mmg_client.send_sms.assert_not_called() + app.celery.research_mode_tasks.send_sms_response.apply_async.assert_not_called() + + +def test_should_send_sms_sender_from_service_if_present( + notify_db, + notify_db_session, + sample_service, + sample_template, + mocker): + db_notification = sample_notification(notify_db, notify_db_session, template=sample_template, + to_field="+447234123123", + status='created') + + sample_service.sms_sender = 'elevenchars' + notify_db.session.add(sample_service) + notify_db.session.commit() + + mocker.patch('app.mmg_client.send_sms') + mocker.patch('app.mmg_client.get_name', return_value="mmg") + + send_to_providers.send_sms_to_provider( + db_notification.id + ) + + mmg_client.send_sms.assert_called_once_with( + to=format_phone_number(validate_phone_number("+447234123123")), + content="This is a template:\nwith a newline", + reference=str(db_notification.id), + sender=sample_service.sms_sender + ) + + +@pytest.mark.parametrize('research_mode,key_type', [ + (True, KEY_TYPE_NORMAL), + (False, KEY_TYPE_TEST) +]) +def test_send_email_to_provider_should_call_research_mode_task_response_task_if_research_mode( + notify_db, + notify_db_session, + sample_service, + sample_email_template, + ses_provider, + mocker, + research_mode, + key_type): + notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, + template=sample_email_template, + to_field="john@smith.com", + key_type=key_type + ) + + reference = uuid.uuid4() + mocker.patch('app.uuid.uuid4', return_value=reference) + mocker.patch('app.aws_ses_client.send_email') + mocker.patch('app.aws_ses_client.get_name', return_value="ses") + mocker.patch('app.celery.research_mode_tasks.send_email_response.apply_async') + + if research_mode: + sample_service.research_mode = True + notify_db.session.add(sample_service) + notify_db.session.commit() + assert not get_provider_statistics( + sample_email_template.service, + providers=[ses_provider.identifier]).first() + send_to_providers.send_email_to_provider( + notification.id + ) + assert not app.aws_ses_client.send_email.called + send_to_providers.send_email_response.apply_async.assert_called_once_with( + ('ses', str(reference), 'john@smith.com'), queue="research-mode" + ) + assert not get_provider_statistics( + sample_email_template.service, + providers=[ses_provider.identifier]).first() + persisted_notification = Notification.query.filter_by(id=notification.id).one() + + assert persisted_notification.to == 'john@smith.com' + assert persisted_notification.template_id == sample_email_template.id + assert persisted_notification.status == 'sending' + assert persisted_notification.sent_at <= datetime.utcnow() + assert persisted_notification.created_at <= datetime.utcnow() + assert persisted_notification.sent_by == 'ses' + assert persisted_notification.reference == str(reference) + + +def test_send_email_to_provider_should_not_send_to_provider_when_status_is_not_created(notify_db, notify_db_session, + sample_service, + sample_email_template, + mocker): + notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, + template=sample_email_template, + service=sample_service, + status='sending') + mocker.patch('app.aws_ses_client.send_email') + mocker.patch('app.aws_ses_client.get_name', return_value="ses") + mocker.patch('app.celery.research_mode_tasks.send_email_response.apply_async') + + send_to_providers.send_sms_to_provider( + notification.id + ) + + app.aws_ses_client.send_email.assert_not_called() + app.celery.research_mode_tasks.send_email_response.apply_async.assert_not_called() + + +def test_send_email_should_use_service_reply_to_email( + notify_db, notify_db_session, + sample_service, + sample_email_template, + mocker): + mocker.patch('app.aws_ses_client.send_email', return_value='reference') + mocker.patch('app.aws_ses_client.get_name', return_value="ses") + + db_notification = sample_notification(notify_db, notify_db_session, template=sample_email_template) + sample_service.reply_to_email_address = 'foo@bar.com' + + send_to_providers.send_email_to_provider( + db_notification.id, + ) + + app.aws_ses_client.send_email.assert_called_once_with( + ANY, + ANY, + ANY, + body=ANY, + html_body=ANY, + reply_to_address=sample_service.reply_to_email_address + ) + + +def test_get_html_email_renderer_should_return_for_normal_service(sample_service): + renderer = send_to_providers.get_html_email_renderer(sample_service) + assert renderer.govuk_banner + assert renderer.brand_colour is None + assert renderer.brand_logo is None + assert renderer.brand_name is None + + +@pytest.mark.parametrize('branding_type, govuk_banner', [ + (BRANDING_ORG, False), + (BRANDING_BOTH, True) +]) +def test_get_html_email_renderer_with_branding_details(branding_type, govuk_banner, notify_db, sample_service): + sample_service.branding = branding_type + org = Organisation(colour='#000000', logo='justice-league.png', name='Justice League') + sample_service.organisation = org + notify_db.session.add_all([sample_service, org]) + notify_db.session.commit() + + renderer = send_to_providers.get_html_email_renderer(sample_service) + + assert renderer.govuk_banner == govuk_banner + assert renderer.brand_colour == '000000' + assert renderer.brand_name == 'Justice League' + + +def test_get_html_email_renderer_prepends_logo_path(notify_db, sample_service): + sample_service.branding = BRANDING_ORG + org = Organisation(colour='#000000', logo='justice-league.png', name='Justice League') + sample_service.organisation = org + notify_db.session.add_all([sample_service, org]) + notify_db.session.commit() + + renderer = send_to_providers.get_html_email_renderer(sample_service) + + assert renderer.brand_logo == 'http://localhost:6012/static/images/email-template/crests/justice-league.png' + + +def test_should_not_set_billable_units_if_research_mode(notify_db, sample_service, sample_notification, mocker): + mocker.patch('app.mmg_client.send_sms') + mocker.patch('app.mmg_client.get_name', return_value="mmg") + mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async') + + sample_service.research_mode = True + notify_db.session.add(sample_service) + notify_db.session.commit() + + send_to_providers.send_sms_to_provider( + sample_notification.id + ) + + persisted_notification = notifications_dao.get_notification_by_id(sample_notification.id) + assert persisted_notification.billable_units == 0 From 991cc884b40e6df28b50ecacce75861a17e98db1 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 21 Sep 2016 13:27:32 +0100 Subject: [PATCH 2/9] Refactoring the send to provider code out of the tasks folder - building a rest endpoint to call that code to compliment the existing task based approach. --- app/celery/provider_tasks.py | 41 ++++++++++++++++++++ app/delivery/rest.py | 34 ++++++++++++++++ app/delivery/send_to_providers.py | 7 ++-- tests/app/celery/test_provider_tasks.py | 25 +----------- tests/app/delivery/test_send_to_providers.py | 13 +++++++ 5 files changed, 92 insertions(+), 28 deletions(-) create mode 100644 app/delivery/rest.py diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index ffc5787f9..14c3ba937 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -31,6 +31,47 @@ def retry_iteration_to_delay(retry=0): return delays.get(retry, 10) +@notify_celery.task(bind=True, name="deliver_sms", max_retries=5, default_retry_delay=5) +@statsd(namespace="tasks") +def deliver_sms(self, notification_id): + try: + send_to_providers.send_sms_to_provider(notification_id) + except Exception as e: + try: + current_app.logger.error( + "RETRY: SMS notification {} failed".format(notification_id) + ) + current_app.logger.exception(e) + self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries)) + except self.MaxRetriesExceededError: + current_app.logger.error( + "RETRY FAILED: task send_sms_to_provider failed for notification {}".format(notification_id), + e + ) + update_notification_status_by_id(notification_id, 'technical-failure') + + +@notify_celery.task(bind=True, name="deliver_email", max_retries=5, default_retry_delay=5) +@statsd(namespace="tasks") +def deliver_email(self, notification_id): + try: + send_to_providers.send_email_response(notification_id) + except Exception as e: + try: + current_app.logger.error( + "RETRY: Email notification {} failed".format(notification_id) + ) + current_app.logger.exception(e) + self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries)) + except self.MaxRetriesExceededError: + current_app.logger.error( + "RETRY FAILED: task send_email_to_provider failed for notification {}".format(notification_id), + e + ) + update_notification_status_by_id(notification_id, 'technical-failure') + + + @notify_celery.task(bind=True, name="send-sms-to-provider", max_retries=5, default_retry_delay=5) @statsd(namespace="tasks") def send_sms_to_provider(self, service_id, notification_id): diff --git a/app/delivery/rest.py b/app/delivery/rest.py new file mode 100644 index 000000000..67cca534b --- /dev/null +++ b/app/delivery/rest.py @@ -0,0 +1,34 @@ +from flask import Blueprint, jsonify + +from app.delivery import send_to_providers +from app.models import EMAIL_TYPE +from app.celery import provider_tasks +from sqlalchemy.orm.exc import NoResultFound + +delivery = Blueprint('delivery', __name__) + +from app.errors import ( + register_errors, + InvalidRequest +) + +register_errors(delivery) + + +@delivery.route('/deliver/notification/', methods=['POST']) +def send_notification_to_provider(notification_type, notification_id): + + if notification_type == EMAIL_TYPE: + send_response(send_to_providers.send_email_response, provider_tasks.deliver_email, notification_id, 'send-email') + else: + send_response(send_to_providers.send_sms_response, provider_tasks.deliver_sms, notification_id, 'send-sms') + return jsonify({}), 204 + + +def send_response(send_call, task_call, notification_id, queue): + try: + send_call(notification_id) + except NoResultFound as e: + raise InvalidRequest(e, status_code=404) + except Exception as e: + task_call.apply_async((str(notification_id)), queue=queue) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 73d7fc150..8b43ecec0 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -20,10 +20,9 @@ from app.dao.templates_dao import dao_get_template_by_id from app.models import SMS_TYPE, KEY_TYPE_TEST, BRANDING_ORG, EMAIL_TYPE -def send_sms_to_provider(notification_id): - notification = get_notification_by_id(notification_id) +def send_sms_to_provider(notification): service = dao_fetch_service_by_id(notification.service_id) - provider = provider_to_use(SMS_TYPE, notification_id) + provider = provider_to_use(SMS_TYPE, notification.id) if notification.status == 'created': template_model = dao_get_template_by_id(notification.template_id, notification.template_version) template = Template( @@ -33,7 +32,7 @@ def send_sms_to_provider(notification_id): ) if service.research_mode or notification.key_type == KEY_TYPE_TEST: send_sms_response.apply_async( - (provider.get_name(), str(notification_id), notification.to), queue='research-mode' + (provider.get_name(), str(notification.id), notification.to), queue='research-mode' ) notification.billable_units = 0 else: diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 310184629..982bf612b 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -1,31 +1,8 @@ -import uuid -from datetime import datetime - -import pytest from celery.exceptions import MaxRetriesExceededError -from unittest.mock import ANY, call -from notifications_utils.recipients import validate_phone_number, format_phone_number - -import app -from app import statsd_client, mmg_client from app.celery import provider_tasks from app.celery.provider_tasks import send_sms_to_provider, send_email_to_provider -from app.celery.research_mode_tasks import send_sms_response, send_email_response from app.clients.email import EmailClientException -from app.clients.sms import SmsClientException -from app.dao import notifications_dao, provider_details_dao -from app.dao import provider_statistics_dao -from app.dao.provider_statistics_dao import get_provider_statistics -from app.models import ( - Notification, - NotificationStatistics, - Job, - Organisation, - KEY_TYPE_NORMAL, - KEY_TYPE_TEST, - BRANDING_ORG, - BRANDING_BOTH -) +from app.models import Notification from tests.app.conftest import sample_notification diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 755f33835..af49d64f5 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -5,6 +5,7 @@ import pytest from mock import ANY import app +from sqlalchemy.orm.exc import NoResultFound from app import mmg_client from app.dao import (provider_details_dao, notifications_dao, provider_statistics_dao) from app.dao.provider_statistics_dao import get_provider_statistics @@ -46,6 +47,18 @@ def test_should_return_highest_priority_active_provider(notify_db, notify_db_ses assert send_to_providers.provider_to_use('sms', '1234').name == first.identifier +def test_raises_not_found_exception_if_no_notification_for_id(notify_db, notify_db_session, mocker): + mocker.patch('app.mmg_client.send_sms') + mocker.patch('app.mmg_client.get_name', return_value="mmg") + notification_id = uuid.uuid4() + + with pytest.raises(NoResultFound) as exc: + send_to_providers.send_sms_to_provider(notification_id) + + assert str(exc.value) == "No notification for {}".format(str(notification_id)) + app.mmg_client.send_sms.assert_not_called() + + def test_should_send_personalised_template_to_correct_sms_provider_and_persist( notify_db, notify_db_session, From 2f36e0dbcf33e0ccf752d739314981323fdf836a Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 22 Sep 2016 09:16:58 +0100 Subject: [PATCH 3/9] Refactor to make the new send_to_provider methods take a notification not a notification ID. - Driven by the fact we won't know the type in the API call - hence we need to load notification earlier , so pass it not the id through to the send task to avoid loading it twice. --- app/celery/provider_tasks.py | 8 +++-- app/delivery/rest.py | 23 ++++++------- app/delivery/send_to_providers.py | 11 +++--- tests/app/delivery/test_send_to_providers.py | 35 +++++++------------- 4 files changed, 33 insertions(+), 44 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 14c3ba937..d2a9fb209 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -1,6 +1,7 @@ from flask import current_app from app import notify_celery +from app.dao import notifications_dao from app.dao.notifications_dao import update_notification_status_by_id from app.statsd_decorators import statsd @@ -35,7 +36,8 @@ def retry_iteration_to_delay(retry=0): @statsd(namespace="tasks") def deliver_sms(self, notification_id): try: - send_to_providers.send_sms_to_provider(notification_id) + notification = notifications_dao.get_notification_by_id(notification_id) + send_to_providers.send_sms_to_provider(notification) except Exception as e: try: current_app.logger.error( @@ -55,7 +57,8 @@ def deliver_sms(self, notification_id): @statsd(namespace="tasks") def deliver_email(self, notification_id): try: - send_to_providers.send_email_response(notification_id) + notification = notifications_dao.get_notification_by_id(notification_id) + send_to_providers.send_email_to_provider(notification) except Exception as e: try: current_app.logger.error( @@ -71,7 +74,6 @@ def deliver_email(self, notification_id): update_notification_status_by_id(notification_id, 'technical-failure') - @notify_celery.task(bind=True, name="send-sms-to-provider", max_retries=5, default_retry_delay=5) @statsd(namespace="tasks") def send_sms_to_provider(self, service_id, notification_id): diff --git a/app/delivery/rest.py b/app/delivery/rest.py index 67cca534b..5f534f631 100644 --- a/app/delivery/rest.py +++ b/app/delivery/rest.py @@ -3,32 +3,31 @@ from flask import Blueprint, jsonify from app.delivery import send_to_providers from app.models import EMAIL_TYPE from app.celery import provider_tasks -from sqlalchemy.orm.exc import NoResultFound +from app.dao import notifications_dao delivery = Blueprint('delivery', __name__) -from app.errors import ( - register_errors, - InvalidRequest -) +from app.errors import register_errors register_errors(delivery) @delivery.route('/deliver/notification/', methods=['POST']) -def send_notification_to_provider(notification_type, notification_id): +def send_notification_to_provider(notification_id): - if notification_type == EMAIL_TYPE: + notification = notifications_dao.get_notification_by_id(notification_id) + if not notification: + return jsonify(notification={"result": "error", "message": "No result found"}), 404 + + if notification.notification_type == EMAIL_TYPE: send_response(send_to_providers.send_email_response, provider_tasks.deliver_email, notification_id, 'send-email') else: send_response(send_to_providers.send_sms_response, provider_tasks.deliver_sms, notification_id, 'send-sms') return jsonify({}), 204 -def send_response(send_call, task_call, notification_id, queue): +def send_response(send_call, task_call, notification, queue): try: - send_call(notification_id) - except NoResultFound as e: - raise InvalidRequest(e, status_code=404) + send_call(notification) except Exception as e: - task_call.apply_async((str(notification_id)), queue=queue) + task_call.apply_async((str(notification.id)), queue=queue) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 8b43ecec0..60bca95c4 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -39,7 +39,7 @@ def send_sms_to_provider(notification): provider.send_sms( to=validate_and_format_phone_number(notification.to), content=template.replaced, - reference=str(notification_id), + reference=str(notification.id), sender=service.sms_sender ) notification.billable_units = get_sms_fragment_count(template.replaced_content_count) @@ -50,16 +50,15 @@ def send_sms_to_provider(notification): dao_update_notification(notification) current_app.logger.info( - "SMS {} sent to provider at {}".format(notification_id, notification.sent_at) + "SMS {} sent to provider at {}".format(notification.id, notification.sent_at) ) delta_milliseconds = (datetime.utcnow() - notification.created_at).total_seconds() * 1000 statsd_client.timing("sms.total-time", delta_milliseconds) -def send_email_to_provider(notification_id): - notification = get_notification_by_id(notification_id) +def send_email_to_provider(notification): service = dao_fetch_service_by_id(notification.service_id) - provider = provider_to_use(EMAIL_TYPE, notification_id) + provider = provider_to_use(EMAIL_TYPE, notification.id) if notification.status == 'created': template_dict = dao_get_template_by_id(notification.template_id, notification.template_version).__dict__ @@ -99,7 +98,7 @@ def send_email_to_provider(notification_id): dao_update_notification(notification) current_app.logger.info( - "Email {} sent to provider at {}".format(notification_id, notification.sent_at) + "Email {} sent to provider at {}".format(notification.id, notification.sent_at) ) delta_milliseconds = (datetime.utcnow() - notification.created_at).total_seconds() * 1000 statsd_client.timing("email.total-time", delta_milliseconds) diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index af49d64f5..94e0ff6f5 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -47,18 +47,6 @@ def test_should_return_highest_priority_active_provider(notify_db, notify_db_ses assert send_to_providers.provider_to_use('sms', '1234').name == first.identifier -def test_raises_not_found_exception_if_no_notification_for_id(notify_db, notify_db_session, mocker): - mocker.patch('app.mmg_client.send_sms') - mocker.patch('app.mmg_client.get_name', return_value="mmg") - notification_id = uuid.uuid4() - - with pytest.raises(NoResultFound) as exc: - send_to_providers.send_sms_to_provider(notification_id) - - assert str(exc.value) == "No notification for {}".format(str(notification_id)) - app.mmg_client.send_sms.assert_not_called() - - def test_should_send_personalised_template_to_correct_sms_provider_and_persist( notify_db, notify_db_session, @@ -73,7 +61,7 @@ def test_should_send_personalised_template_to_correct_sms_provider_and_persist( mocker.patch('app.mmg_client.get_name', return_value="mmg") send_to_providers.send_sms_to_provider( - db_notification.id + db_notification ) mmg_client.send_sms.assert_called_once_with( @@ -108,7 +96,7 @@ def test_should_send_personalised_template_to_correct_email_provider_and_persist mocker.patch('app.aws_ses_client.get_name', return_value="ses") send_to_providers.send_email_to_provider( - db_notification.id + db_notification ) app.aws_ses_client.send_email.assert_called_once_with( @@ -150,7 +138,7 @@ def test_send_sms_should_use_template_version_from_notification_not_latest( assert t.version > version_on_notification send_to_providers.send_sms_to_provider( - db_notification.id + db_notification ) mmg_client.send_sms.assert_called_once_with( @@ -187,7 +175,7 @@ def test_should_call_send_sms_response_task_if_research_mode(notify_db, sample_s sample_notification.key_type = key_type send_to_providers.send_sms_to_provider( - sample_notification.id + sample_notification ) assert not mmg_client.send_sms.called send_to_providers.send_sms_response.apply_async.assert_called_once_with( @@ -222,7 +210,7 @@ def test_should_set_billable_units_to_zero_in_research_mode_or_test_key( sample_notification.key_type = key_type send_to_providers.send_sms_to_provider( - sample_notification.id + sample_notification ) assert notifications_dao.get_notification_by_id(sample_notification.id).billable_units == 0 @@ -239,7 +227,7 @@ def test_should_not_send_to_provider_when_status_is_not_created(notify_db, notif mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async') send_to_providers.send_sms_to_provider( - notification.id + notification ) app.mmg_client.send_sms.assert_not_called() @@ -264,7 +252,7 @@ def test_should_send_sms_sender_from_service_if_present( mocker.patch('app.mmg_client.get_name', return_value="mmg") send_to_providers.send_sms_to_provider( - db_notification.id + db_notification ) mmg_client.send_sms.assert_called_once_with( @@ -307,8 +295,9 @@ def test_send_email_to_provider_should_call_research_mode_task_response_task_if_ assert not get_provider_statistics( sample_email_template.service, providers=[ses_provider.identifier]).first() + send_to_providers.send_email_to_provider( - notification.id + notification ) assert not app.aws_ses_client.send_email.called send_to_providers.send_email_response.apply_async.assert_called_once_with( @@ -341,7 +330,7 @@ def test_send_email_to_provider_should_not_send_to_provider_when_status_is_not_c mocker.patch('app.celery.research_mode_tasks.send_email_response.apply_async') send_to_providers.send_sms_to_provider( - notification.id + notification ) app.aws_ses_client.send_email.assert_not_called() @@ -360,7 +349,7 @@ def test_send_email_should_use_service_reply_to_email( sample_service.reply_to_email_address = 'foo@bar.com' send_to_providers.send_email_to_provider( - db_notification.id, + db_notification, ) app.aws_ses_client.send_email.assert_called_once_with( @@ -421,7 +410,7 @@ def test_should_not_set_billable_units_if_research_mode(notify_db, sample_servic notify_db.session.commit() send_to_providers.send_sms_to_provider( - sample_notification.id + sample_notification ) persisted_notification = notifications_dao.get_notification_by_id(sample_notification.id) From 59ab5da5d30622b559743fd58546380958fde585 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 22 Sep 2016 09:52:23 +0100 Subject: [PATCH 4/9] Handle errors where the notification isn't found when executing the tasks - Thows a NoResultFound sqlalchemy exception - Which causes a retry. This means we give it a few goes (5, max 5 hours) for the notification to appear. - Should never happen, only if we get some task overlaps that are unusual that leads to tasks executed in an overlapping nature. --- app/celery/provider_tasks.py | 16 +++- tests/app/celery/test_provider_tasks.py | 110 +++++++++++++++++++++++- 2 files changed, 120 insertions(+), 6 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index d2a9fb209..632b4f937 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -6,7 +6,7 @@ from app.dao.notifications_dao import update_notification_status_by_id from app.statsd_decorators import statsd from app.delivery import send_to_providers - +from sqlalchemy.orm.exc import NoResultFound def retry_iteration_to_delay(retry=0): """ @@ -37,6 +37,8 @@ def retry_iteration_to_delay(retry=0): def deliver_sms(self, notification_id): try: notification = notifications_dao.get_notification_by_id(notification_id) + if not notification: + raise NoResultFound() send_to_providers.send_sms_to_provider(notification) except Exception as e: try: @@ -58,6 +60,8 @@ def deliver_sms(self, notification_id): def deliver_email(self, notification_id): try: notification = notifications_dao.get_notification_by_id(notification_id) + if not notification: + raise NoResultFound() send_to_providers.send_email_to_provider(notification) except Exception as e: try: @@ -78,7 +82,10 @@ def deliver_email(self, notification_id): @statsd(namespace="tasks") def send_sms_to_provider(self, service_id, notification_id): try: - send_to_providers.send_sms_to_provider(notification_id) + notification = notifications_dao.get_notification_by_id(notification_id) + if not notification: + raise NoResultFound() + send_to_providers.send_sms_to_provider(notification) except Exception as e: try: current_app.logger.error( @@ -98,7 +105,10 @@ def send_sms_to_provider(self, service_id, notification_id): @statsd(namespace="tasks") def send_email_to_provider(self, service_id, notification_id): try: - send_to_providers.send_email_response(notification_id) + notification = notifications_dao.get_notification_by_id(notification_id) + if not notification: + raise NoResultFound() + send_to_providers.send_email_to_provider(notification) except Exception as e: try: current_app.logger.error( diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 982bf612b..df5dc8ef6 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -1,9 +1,10 @@ from celery.exceptions import MaxRetriesExceededError from app.celery import provider_tasks -from app.celery.provider_tasks import send_sms_to_provider, send_email_to_provider +from app.celery.provider_tasks import send_sms_to_provider, send_email_to_provider, deliver_sms, deliver_email from app.clients.email import EmailClientException from app.models import Notification from tests.app.conftest import sample_notification +import app def test_should_have_decorated_tasks_functions(): @@ -39,12 +40,116 @@ def test_should_by_240_minute_delay_on_retry_two(): assert provider_tasks.retry_iteration_to_delay(4) == 14400 +def test_should_call_send_sms_to_provider_from_deliver_sms_task( + notify_db, + notify_db_session, + sample_notification, + mocker): + mocker.patch('app.delivery.send_to_providers.send_sms_to_provider') + + deliver_sms(sample_notification.id) + app.delivery.send_to_providers.send_sms_to_provider.assert_called_with(sample_notification) + + +def test_should_add_to_retry_queue_if_notification_not_found_in_deliver_sms_task( + notify_db, + notify_db_session, + mocker): + mocker.patch('app.delivery.send_to_providers.send_sms_to_provider') + mocker.patch('app.celery.provider_tasks.deliver_sms.retry') + + notification_id = app.create_uuid() + + deliver_sms(notification_id) + app.delivery.send_to_providers.send_sms_to_provider.assert_not_called() + app.celery.provider_tasks.deliver_sms.retry.assert_called_with(queue="retry", countdown=10) + + +def test_should_call_send_sms_to_provider_from_send_sms_to_provider_task( + notify_db, + notify_db_session, + sample_notification, + mocker): + + mocker.patch('app.delivery.send_to_providers.send_sms_to_provider') + + send_sms_to_provider(sample_notification.service_id, sample_notification.id) + app.delivery.send_to_providers.send_sms_to_provider.assert_called_with(sample_notification) + + +def test_should_add_to_retry_queue_if_notification_not_found_in_send_sms_to_provider_task( + notify_db, + notify_db_session, + mocker): + mocker.patch('app.delivery.send_to_providers.send_sms_to_provider') + mocker.patch('app.celery.provider_tasks.send_sms_to_provider.retry') + + notification_id = app.create_uuid() + service_id = app.create_uuid() + + send_sms_to_provider(service_id, notification_id) + app.delivery.send_to_providers.send_sms_to_provider.assert_not_called() + app.celery.provider_tasks.send_sms_to_provider.retry.assert_called_with(queue="retry", countdown=10) + + +def test_should_call_send_email_to_provider_from_deliver_email_task( + notify_db, + notify_db_session, + sample_notification, + mocker): + + mocker.patch('app.delivery.send_to_providers.send_email_to_provider') + + deliver_email(sample_notification.id) + app.delivery.send_to_providers.send_email_to_provider.assert_called_with(sample_notification) + + +def test_should_add_to_retry_queue_if_notification_not_found_in_deliver_email_task( + notify_db, + notify_db_session, + mocker): + mocker.patch('app.delivery.send_to_providers.send_email_to_provider') + mocker.patch('app.celery.provider_tasks.deliver_email.retry') + + notification_id = app.create_uuid() + + deliver_email(notification_id) + app.delivery.send_to_providers.send_email_to_provider.assert_not_called() + app.celery.provider_tasks.deliver_email.retry.assert_called_with(queue="retry", countdown=10) + + +def test_should_call_send_email_to_provider_from_email_task( + notify_db, + notify_db_session, + sample_notification, + mocker): + + mocker.patch('app.delivery.send_to_providers.send_email_to_provider') + + send_email_to_provider(sample_notification.service_id, sample_notification.id) + app.delivery.send_to_providers.send_email_to_provider.assert_called_with(sample_notification) + + +def test_should_add_to_retry_queue_if_notification_not_found_in_send_email_to_provider_task( + notify_db, + notify_db_session, + mocker): + mocker.patch('app.delivery.send_to_providers.send_email_to_provider') + mocker.patch('app.celery.provider_tasks.send_email_to_provider.retry') + + notification_id = app.create_uuid() + service_id = app.create_uuid() + + send_email_to_provider(service_id, notification_id) + app.delivery.send_to_providers.send_email_to_provider.assert_not_called() + app.celery.provider_tasks.send_email_to_provider.retry.assert_called_with(queue="retry", countdown=10) + + def test_should_go_into_technical_error_if_exceeds_retries( notify_db, notify_db_session, sample_service, mocker): - notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, service=sample_service, status='created') @@ -68,7 +173,6 @@ def test_send_email_to_provider_should_go_into_technical_error_if_exceeds_retrie sample_service, sample_email_template, mocker): - notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, service=sample_service, status='created', template=sample_email_template) From 37a2dc7925621ee7f608fda32c44dc5d33d837f0 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 22 Sep 2016 14:01:25 +0100 Subject: [PATCH 5/9] Implemented the REST endpoint to communicate with the new send-to-provider code - this allows us to send a notification to a provider by means of an API call - This is in addition to the celery code. - idea is that we can use this method to help speed up throughput by generating API traffic by node/lambda etc to supplement the celery code in times of high load. --- app/__init__.py | 2 + app/delivery/rest.py | 27 ++++-- tests/app/celery/test_provider_tasks.py | 27 +++--- tests/app/delivery/__init__.py | 0 tests/app/delivery/test_rest.py | 105 ++++++++++++++++++++++++ 5 files changed, 139 insertions(+), 22 deletions(-) create mode 100644 tests/app/delivery/__init__.py create mode 100644 tests/app/delivery/test_rest.py diff --git a/app/__init__.py b/app/__init__.py index cf47632f9..c602003fb 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -70,6 +70,7 @@ def create_app(app_name=None): from app.provider_details.rest import provider_details as provider_details_blueprint from app.spec.rest import spec as spec_blueprint from app.organisation.rest import organisation_blueprint + from app.delivery.rest import delivery_blueprint application.register_blueprint(service_blueprint, url_prefix='/service') application.register_blueprint(user_blueprint, url_prefix='/user') @@ -78,6 +79,7 @@ def create_app(app_name=None): application.register_blueprint(notifications_blueprint) application.register_blueprint(job_blueprint) application.register_blueprint(invite_blueprint) + application.register_blueprint(delivery_blueprint) application.register_blueprint(accept_invite, url_prefix='/invite') application.register_blueprint(template_statistics_blueprint) diff --git a/app/delivery/rest.py b/app/delivery/rest.py index 5f534f631..0bacb43bb 100644 --- a/app/delivery/rest.py +++ b/app/delivery/rest.py @@ -4,25 +4,33 @@ from app.delivery import send_to_providers from app.models import EMAIL_TYPE from app.celery import provider_tasks from app.dao import notifications_dao +from flask import current_app -delivery = Blueprint('delivery', __name__) +delivery_blueprint = Blueprint('delivery', __name__) from app.errors import register_errors -register_errors(delivery) +register_errors(delivery_blueprint) -@delivery.route('/deliver/notification/', methods=['POST']) +@delivery_blueprint.route('/deliver/notification/', methods=['POST']) def send_notification_to_provider(notification_id): - notification = notifications_dao.get_notification_by_id(notification_id) if not notification: - return jsonify(notification={"result": "error", "message": "No result found"}), 404 + return jsonify({"result": "error", "message": "No result found"}), 404 if notification.notification_type == EMAIL_TYPE: - send_response(send_to_providers.send_email_response, provider_tasks.deliver_email, notification_id, 'send-email') + send_response( + send_to_providers.send_email_to_provider, + provider_tasks.deliver_email, + notification, + 'send-email') else: - send_response(send_to_providers.send_sms_response, provider_tasks.deliver_sms, notification_id, 'send-sms') + send_response( + send_to_providers.send_sms_to_provider, + provider_tasks.deliver_sms, + notification, + 'send-sms') return jsonify({}), 204 @@ -30,4 +38,9 @@ def send_response(send_call, task_call, notification, queue): try: send_call(notification) except Exception as e: + current_app.logger.exception( + "Failed to send notification, retrying in celery. ID {} type {}".format( + notification.id, + notification.notification_type), + e) task_call.apply_async((str(notification.id)), queue=queue) diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index df5dc8ef6..b2f7f28e7 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -66,11 +66,10 @@ def test_should_add_to_retry_queue_if_notification_not_found_in_deliver_sms_task def test_should_call_send_sms_to_provider_from_send_sms_to_provider_task( - notify_db, - notify_db_session, - sample_notification, - mocker): - + notify_db, + notify_db_session, + sample_notification, + mocker): mocker.patch('app.delivery.send_to_providers.send_sms_to_provider') send_sms_to_provider(sample_notification.service_id, sample_notification.id) @@ -93,11 +92,10 @@ def test_should_add_to_retry_queue_if_notification_not_found_in_send_sms_to_prov def test_should_call_send_email_to_provider_from_deliver_email_task( - notify_db, - notify_db_session, - sample_notification, - mocker): - + notify_db, + notify_db_session, + sample_notification, + mocker): mocker.patch('app.delivery.send_to_providers.send_email_to_provider') deliver_email(sample_notification.id) @@ -119,11 +117,10 @@ def test_should_add_to_retry_queue_if_notification_not_found_in_deliver_email_ta def test_should_call_send_email_to_provider_from_email_task( - notify_db, - notify_db_session, - sample_notification, - mocker): - + notify_db, + notify_db_session, + sample_notification, + mocker): mocker.patch('app.delivery.send_to_providers.send_email_to_provider') send_email_to_provider(sample_notification.service_id, sample_notification.id) diff --git a/tests/app/delivery/__init__.py b/tests/app/delivery/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/app/delivery/test_rest.py b/tests/app/delivery/test_rest.py new file mode 100644 index 000000000..fc51fb508 --- /dev/null +++ b/tests/app/delivery/test_rest.py @@ -0,0 +1,105 @@ +from flask import json + +import app +from tests import create_authorization_header + + +def test_should_reject_if_not_authenticated(notify_api): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + response = client.post('/deliver/notification/{}'.format(app.create_uuid())) + assert response.status_code == 401 + + +def test_should_reject_if_invalid_uuid(notify_api): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + auth = create_authorization_header() + response = client.post( + '/deliver/notification/{}', + headers=[auth] + ) + body = json.loads(response.get_data(as_text=True)) + assert response.status_code == 404 + assert body['message'] == 'The requested URL was not found on the server. If you entered the URL manually please check your spelling and try again.' # noqa + assert body['result'] == 'error' + + +def test_should_reject_if_notification_id_cannot_be_found(notify_api): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + auth = create_authorization_header() + response = client.post( + '/deliver/notification/{}'.format(app.create_uuid()), + headers=[auth] + ) + body = json.loads(response.get_data(as_text=True)) + assert response.status_code == 404 + assert body['message'] == 'No result found' + assert body['result'] == 'error' + + +def test_should_call_send_sms_to_provider_as_primary(notify_api, sample_notification, mocker): + mocker.patch('app.delivery.send_to_providers.send_sms_to_provider') + with notify_api.test_request_context(): + with notify_api.test_client() as client: + auth = create_authorization_header() + response = client.post( + '/deliver/notification/{}'.format(sample_notification.id), + headers=[auth] + ) + app.delivery.send_to_providers.send_sms_to_provider.assert_called_with(sample_notification) + assert response.status_code == 204 + + +def test_should_call_send_email_to_provider_as_primary(notify_api, sample_email_notification, mocker): + mocker.patch('app.delivery.send_to_providers.send_email_to_provider') + with notify_api.test_request_context(): + with notify_api.test_client() as client: + auth = create_authorization_header() + response = client.post( + '/deliver/notification/{}'.format(sample_email_notification.id), + headers=[auth] + ) + app.delivery.send_to_providers.send_email_to_provider.assert_called_with(sample_email_notification) + assert response.status_code == 204 + + +def test_should_call_deliver_sms_task_if_send_sms_to_provider_fails(notify_api, sample_notification, mocker): + mocker.patch('app.delivery.send_to_providers.send_sms_to_provider', side_effect=Exception()) + mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + + with notify_api.test_request_context(): + with notify_api.test_client() as client: + auth = create_authorization_header() + response = client.post( + '/deliver/notification/{}'.format(sample_notification.id), + headers=[auth] + ) + app.delivery.send_to_providers.send_sms_to_provider.assert_called_with(sample_notification) + app.celery.provider_tasks.deliver_sms.apply_async.assert_called_with( + (str(sample_notification.id)), queue='send-sms' + ) + assert response.status_code == 204 + + +def test_should_call_deliver_email_task_if_send_email_to_provider_fails( + notify_api, + sample_email_notification, + mocker +): + mocker.patch('app.delivery.send_to_providers.send_email_to_provider', side_effect=Exception()) + mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') + + with notify_api.test_request_context(): + with notify_api.test_client() as client: + auth = create_authorization_header() + response = client.post( + '/deliver/notification/{}'.format(sample_email_notification.id), + headers=[auth] + ) + app.delivery.send_to_providers.send_email_to_provider.assert_called_with(sample_email_notification) + app.celery.provider_tasks.deliver_email.apply_async.assert_called_with( + (str(sample_email_notification.id)), queue='send-email' + ) + assert response.status_code == 204 From 376f8355cbeb2a8865221f45fef055095375d735 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 22 Sep 2016 17:18:05 +0100 Subject: [PATCH 6/9] Updated clients to have a more robust error handling - fire text and omg much more similar. Ready to be combined. - Error handling now for JSON valid responses --- app/celery/provider_tasks.py | 1 + app/clients/sms/__init__.py | 11 +++-- app/clients/sms/firetext.py | 78 +++++++++++++++++------------- app/clients/sms/loadtesting.py | 5 ++ app/clients/sms/mmg.py | 78 +++++++++++++++++------------- tests/app/clients/test_firetext.py | 23 +++++++-- tests/app/clients/test_mmg.py | 30 ++++++++++-- 7 files changed, 146 insertions(+), 80 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 632b4f937..c94120e79 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -8,6 +8,7 @@ from app.statsd_decorators import statsd from app.delivery import send_to_providers from sqlalchemy.orm.exc import NoResultFound + def retry_iteration_to_delay(retry=0): """ :param retry times we have performed a retry diff --git a/app/clients/sms/__init__.py b/app/clients/sms/__init__.py index 90d63ed0d..e6accdb8a 100644 --- a/app/clients/sms/__init__.py +++ b/app/clients/sms/__init__.py @@ -1,11 +1,16 @@ from app.clients import (Client, ClientException) -class SmsClientException(ClientException): +class SmsClientResponseException(ClientException): ''' - Base Exception for SmsClients + Base Exception for SmsClientsResponses ''' - pass + + def __init__(self, message): + self.message = message + + def __str__(self): + return "Message {}".format(self.message) class SmsClient(Client): diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index e9760a4e6..5ffb749eb 100644 --- a/app/clients/sms/firetext.py +++ b/app/clients/sms/firetext.py @@ -1,12 +1,10 @@ +import json import logging from monotonic import monotonic -from requests import request, RequestException, HTTPError +from requests import request, RequestException -from app.clients.sms import ( - SmsClient, - SmsClientException -) +from app.clients.sms import (SmsClient, SmsClientResponseException) from app.clients import STATISTICS_DELIVERED, STATISTICS_FAILURE logger = logging.getLogger(__name__) @@ -42,13 +40,14 @@ def get_firetext_responses(status): return firetext_responses[status] -class FiretextClientException(SmsClientException): - def __init__(self, response): - self.code = response['code'] - self.description = response['description'] +class FiretextClientResponseException(SmsClientResponseException): + def __init__(self, response, exception): + self.status_code = response.status_code + self.text = response.text + self.exception = exception def __str__(self): - return "Code {} description {}".format(self.code, self.description) + return "Code {} text {} exception {}".format(self.status_code, self.text, str(self.exception)) class FiretextClient(SmsClient): @@ -62,11 +61,35 @@ class FiretextClient(SmsClient): self.api_key = current_app.config.get('FIRETEXT_API_KEY') self.from_number = current_app.config.get('FROM_NUMBER') self.name = 'firetext' + self.url = "https://www.firetext.co.uk/api/sendsms/json" self.statsd_client = statsd_client def get_name(self): return self.name + def record_outcome(self, success, response): + log_message = "API {} request {} on {} response status_code {} response text'{}'".format( + "POST", + "succeeded" if success else "failed", + self.url, + response.status_code, + response.text + ) + + if not success: + self.statsd_client.incr("clients.firetext.error") + self.current_app.logger.error(log_message) + else: + self.current_app.logger.info(log_message) + self.statsd_client.incr("clients.firetext.success") + + @staticmethod + def check_response(response): + response.raise_for_status() + json.loads(response.text) + if response.json()['code'] != 0: + raise ValueError() + def send_sms(self, to, content, reference, sender=None): data = { @@ -81,36 +104,23 @@ class FiretextClient(SmsClient): try: response = request( "POST", - "https://www.firetext.co.uk/api/sendsms/json", + self.url, data=data ) - firetext_response = response.json() - if firetext_response['code'] != 0: - raise FiretextClientException(firetext_response) response.raise_for_status() - self.current_app.logger.info( - "API {} request on {} succeeded with {} '{}'".format( - "POST", - "https://www.firetext.co.uk/api/sendsms", - response.status_code, - firetext_response.items() - ) - ) + try: + json.loads(response.text) + if response.json()['code'] != 0: + raise ValueError() + except (ValueError, AttributeError) as e: + self.record_outcome(False, response) + raise FiretextClientResponseException(response=response, exception=e) + self.record_outcome(True, response) except RequestException as e: - api_error = HTTPError.create(e) - logger.error( - "API {} request on {} failed with {} '{}'".format( - "POST", - "https://www.firetext.co.uk/api/sendsms", - api_error.status_code, - api_error.message - ) - ) - self.statsd_client.incr("clients.firetext.error") - raise api_error + self.record_outcome(False, e.response) + raise FiretextClientResponseException(response=e.response, exception=e) finally: elapsed_time = monotonic() - start_time self.current_app.logger.info("Firetext request finished in {}".format(elapsed_time)) - self.statsd_client.incr("clients.firetext.success") self.statsd_client.timing("clients.firetext.request-time", elapsed_time) return response diff --git a/app/clients/sms/loadtesting.py b/app/clients/sms/loadtesting.py index a3e500ba8..53ebca9f9 100644 --- a/app/clients/sms/loadtesting.py +++ b/app/clients/sms/loadtesting.py @@ -1,4 +1,7 @@ import logging + +from flask import current_app + from app.clients.sms.firetext import ( FiretextClient ) @@ -13,7 +16,9 @@ class LoadtestingClient(FiretextClient): def init_app(self, config, statsd_client, *args, **kwargs): super(FiretextClient, self).__init__(*args, **kwargs) + self.current_app = current_app self.api_key = config.config.get('LOADTESTING_API_KEY') self.from_number = config.config.get('LOADTESTING_NUMBER') self.name = 'loadtesting' + self.url = "https://www.firetext.co.uk/api/sendsms/json" self.statsd_client = statsd_client diff --git a/app/clients/sms/mmg.py b/app/clients/sms/mmg.py index 066c1f259..8919b1358 100644 --- a/app/clients/sms/mmg.py +++ b/app/clients/sms/mmg.py @@ -1,8 +1,9 @@ import json from monotonic import monotonic -from requests import (request, RequestException, HTTPError) +from requests import (request, RequestException) + from app.clients import (STATISTICS_DELIVERED, STATISTICS_FAILURE) -from app.clients.sms import (SmsClient, SmsClientException) +from app.clients.sms import (SmsClient, SmsClientResponseException) mmg_response_map = { '2': { @@ -42,13 +43,14 @@ def get_mmg_responses(status): return mmg_response_map.get(status, mmg_response_map.get('default')) -class MMGClientException(SmsClientException): - def __init__(self, error_response): - self.code = error_response['Error'] - self.description = error_response['Description'] +class MMGClientResponseException(SmsClientResponseException): + def __init__(self, response, exception): + self.status_code = response.status_code + self.text = response.text + self.exception = exception def __str__(self): - return "Code {} description {}".format(self.code, self.description) + return "Code {} text {} exception {}".format(self.status_code, self.text, str(self.exception)) class MMGClient(SmsClient): @@ -65,6 +67,22 @@ class MMGClient(SmsClient): self.statsd_client = statsd_client self.mmg_url = current_app.config.get('MMG_URL') + def record_outcome(self, success, response): + log_message = "API {} request {} on {} response status_code {} response text'{}'".format( + "POST", + "succeeded" if success else "failed", + self.mmg_url, + response.status_code, + response.text + ) + + if not success: + self.statsd_client.incr("clients.mmg.error") + self.current_app.logger.error(log_message) + else: + self.current_app.logger.info(log_message) + self.statsd_client.incr("clients.mmg.success") + def get_name(self): return self.name @@ -79,38 +97,30 @@ class MMGClient(SmsClient): } start_time = monotonic() - try: - response = request("POST", self.mmg_url, - data=json.dumps(data), - headers={'Content-Type': 'application/json', - 'Authorization': 'Basic {}'.format(self.api_key)}) - if response.status_code != 200: - raise MMGClientException(response.json()) + response = request( + "POST", + self.mmg_url, + data=json.dumps(data), + headers={ + 'Content-Type': 'application/json', + 'Authorization': 'Basic {}'.format(self.api_key) + } + ) + response.raise_for_status() - self.current_app.logger.info( - "API {} request on {} succeeded with {} '{}'".format( - "POST", - self.mmg_url, - response.status_code, - response.json().items() - ) - ) + try: + json.loads(response.text) + except (ValueError, AttributeError) as e: + self.record_outcome(False, response) + raise MMGClientResponseException(response=response, exception=e) + self.record_outcome(True, response) except RequestException as e: - api_error = HTTPError.create(e) - self.current_app.logger.error( - "API {} request on {} failed with {} '{}'".format( - "POST", - self.mmg_url, - api_error.status_code, - api_error.message - ) - ) - self.statsd_client.incr("clients.mmg.error") - raise api_error + self.record_outcome(False, e.response) + raise MMGClientResponseException(response=e.response, exception=e) finally: elapsed_time = monotonic() - start_time self.statsd_client.timing("clients.mmg.request-time", elapsed_time) - self.statsd_client.incr("clients.mmg.success") self.current_app.logger.info("MMG request finished in {}".format(elapsed_time)) + return response diff --git a/tests/app/clients/test_firetext.py b/tests/app/clients/test_firetext.py index ea8630913..80737e23d 100644 --- a/tests/app/clients/test_firetext.py +++ b/tests/app/clients/test_firetext.py @@ -1,9 +1,10 @@ +from requests import HTTPError from urllib.parse import parse_qs import pytest import requests_mock -from app.clients.sms.firetext import (get_firetext_responses, FiretextClientException) +from app.clients.sms.firetext import get_firetext_responses, SmsClientResponseException def test_should_return_correct_details_for_delivery(): @@ -88,12 +89,26 @@ def test_send_sms_raises_if_firetext_rejects(mocker, mock_firetext_client): 'responseData': '' } - with pytest.raises(FiretextClientException) as exc, requests_mock.Mocker() as request_mock: + with pytest.raises(SmsClientResponseException) as exc, requests_mock.Mocker() as request_mock: request_mock.post('https://www.firetext.co.uk/api/sendsms/json', json=response_dict, status_code=200) mock_firetext_client.send_sms(to, content, reference) - assert exc.value.code == 1 - assert exc.value.description == 'Some kind of error' + assert exc.value.status_code == 200 + assert '"description": "Some kind of error"' in exc.value.text + assert '"code": 1' in exc.value.text + + +def test_send_sms_raises_if_firetext_rejects(mocker, mock_firetext_client): + to = content = reference = 'foo' + response_dict = {"something": "gone bad"} + + with pytest.raises(SmsClientResponseException) as exc, requests_mock.Mocker() as request_mock: + request_mock.post('https://www.firetext.co.uk/api/sendsms/json', json=response_dict, status_code=400) + mock_firetext_client.send_sms(to, content, reference) + + assert exc.value.status_code == 400 + assert exc.value.text == '{"something": "gone bad"}' + assert type(exc.value.exception) == HTTPError def test_send_sms_override_configured_shortcode_with_sender(mocker, mock_firetext_client): diff --git a/tests/app/clients/test_mmg.py b/tests/app/clients/test_mmg.py index 649764d89..d87f2f914 100644 --- a/tests/app/clients/test_mmg.py +++ b/tests/app/clients/test_mmg.py @@ -1,8 +1,13 @@ +import json + import pytest import requests_mock -from app import mmg_client +from requests import HTTPError -from app.clients.sms.mmg import (get_mmg_responses, MMGClientException) +from app import mmg_client +from app.clients.sms import SmsClientResponseException + +from app.clients.sms.mmg import get_mmg_responses def test_should_return_correct_details_for_delivery(): @@ -72,12 +77,14 @@ def test_send_sms_raises_if_mmg_rejects(notify_api, mocker): 'Description': 'Some kind of error' } - with pytest.raises(MMGClientException) as exc, requests_mock.Mocker() as request_mock: + with pytest.raises(SmsClientResponseException) as exc, requests_mock.Mocker() as request_mock: request_mock.post('https://api.mmg.co.uk/json/api.php', json=response_dict, status_code=400) mmg_client.send_sms(to, content, reference) - assert exc.value.code == 206 - assert exc.value.description == 'Some kind of error' + assert exc.value.status_code == 400 + assert '"Error": 206' in exc.value.text + assert '"Description": "Some kind of error"' in exc.value.text + assert type(exc.value.exception) == HTTPError def test_send_sms_override_configured_shortcode_with_sender(notify_api, mocker): @@ -93,3 +100,16 @@ def test_send_sms_override_configured_shortcode_with_sender(notify_api, mocker): request_args = request_mock.request_history[0].json() assert request_args['sender'] == 'fromservice' + + +def test_send_sms_raises_if_mmg_fails_to_return_json(notify_api, mocker): + to = content = reference = 'foo' + response_dict = 'NOT AT ALL VALID JSON {"key" : "value"}}' + + with pytest.raises(SmsClientResponseException) as exc, requests_mock.Mocker() as request_mock: + request_mock.post('https://api.mmg.co.uk/json/api.php', text=response_dict, status_code=200) + mmg_client.send_sms(to, content, reference) + + assert 'app.clients.sms.mmg.MMGClientResponseException: Code 200 text NOT AT ALL VALID JSON {"key" : "value"}} exception Expecting value: line 1 column 1 (char 0)' in str(exc) # noqa + assert exc.value.status_code == 200 + assert exc.value.text == 'NOT AT ALL VALID JSON {"key" : "value"}}' From 54c28a4bea58f65059c7a699cb7395caf92507ac Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 23 Sep 2016 10:24:12 +0100 Subject: [PATCH 7/9] Updated logging as logger was parsing some JSON and erroring. - Don't write whole JSON response. Safer not to anyway. No need. --- app/clients/sms/firetext.py | 12 ++---------- app/clients/sms/mmg.py | 5 ++--- 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index 5ffb749eb..fcaf175fc 100644 --- a/app/clients/sms/firetext.py +++ b/app/clients/sms/firetext.py @@ -68,12 +68,11 @@ class FiretextClient(SmsClient): return self.name def record_outcome(self, success, response): - log_message = "API {} request {} on {} response status_code {} response text'{}'".format( + log_message = "API {} request {} on {} response status_code {}".format( "POST", "succeeded" if success else "failed", self.url, - response.status_code, - response.text + response.status_code ) if not success: @@ -83,13 +82,6 @@ class FiretextClient(SmsClient): self.current_app.logger.info(log_message) self.statsd_client.incr("clients.firetext.success") - @staticmethod - def check_response(response): - response.raise_for_status() - json.loads(response.text) - if response.json()['code'] != 0: - raise ValueError() - def send_sms(self, to, content, reference, sender=None): data = { diff --git a/app/clients/sms/mmg.py b/app/clients/sms/mmg.py index 8919b1358..fb135a9c3 100644 --- a/app/clients/sms/mmg.py +++ b/app/clients/sms/mmg.py @@ -68,12 +68,11 @@ class MMGClient(SmsClient): self.mmg_url = current_app.config.get('MMG_URL') def record_outcome(self, success, response): - log_message = "API {} request {} on {} response status_code {} response text'{}'".format( + log_message = "API {} request {} on {} response status_code {}".format( "POST", "succeeded" if success else "failed", self.mmg_url, - response.status_code, - response.text + response.status_code ) if not success: From 124487f204c5dedea471bd2c45ad8b929ff7fae0 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 23 Sep 2016 10:31:18 +0100 Subject: [PATCH 8/9] Fix from number on Load testing client --- app/clients/sms/loadtesting.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/clients/sms/loadtesting.py b/app/clients/sms/loadtesting.py index 53ebca9f9..429a6d314 100644 --- a/app/clients/sms/loadtesting.py +++ b/app/clients/sms/loadtesting.py @@ -18,7 +18,7 @@ class LoadtestingClient(FiretextClient): super(FiretextClient, self).__init__(*args, **kwargs) self.current_app = current_app self.api_key = config.config.get('LOADTESTING_API_KEY') - self.from_number = config.config.get('LOADTESTING_NUMBER') + self.from_number = config.config.get('FROM_NUMBER') self.name = 'loadtesting' self.url = "https://www.firetext.co.uk/api/sendsms/json" self.statsd_client = statsd_client From 6f7dd149e217898bde194057eb2bbc912478981c Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 23 Sep 2016 15:59:23 +0100 Subject: [PATCH 9/9] Tidied up pull request comments --- app/clients/sms/firetext.py | 8 ++++---- app/clients/sms/mmg.py | 8 ++++---- app/delivery/send_to_providers.py | 5 +---- tests/app/delivery/test_send_to_providers.py | 3 +-- 4 files changed, 10 insertions(+), 14 deletions(-) diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index fcaf175fc..9617b88bf 100644 --- a/app/clients/sms/firetext.py +++ b/app/clients/sms/firetext.py @@ -75,12 +75,12 @@ class FiretextClient(SmsClient): response.status_code ) - if not success: - self.statsd_client.incr("clients.firetext.error") - self.current_app.logger.error(log_message) - else: + 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.error(log_message) def send_sms(self, to, content, reference, sender=None): diff --git a/app/clients/sms/mmg.py b/app/clients/sms/mmg.py index fb135a9c3..b036d637c 100644 --- a/app/clients/sms/mmg.py +++ b/app/clients/sms/mmg.py @@ -75,12 +75,12 @@ class MMGClient(SmsClient): response.status_code ) - if not success: - self.statsd_client.incr("clients.mmg.error") - self.current_app.logger.error(log_message) - else: + 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.error(log_message) def get_name(self): return self.name diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index ca36a43a5..8c7c021a8 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -8,10 +8,7 @@ from notifications_utils.template import Template, get_sms_fragment_count from notifications_utils.renderers import HTMLEmail, PlainTextEmail, SMSMessage from app import clients, statsd_client, create_uuid -from app.dao.notifications_dao import ( - get_notification_by_id, - dao_update_notification -) +from app.dao.notifications_dao import dao_update_notification from app.dao.provider_details_dao import get_provider_details_by_notification_type from app.dao.services_dao import dao_fetch_service_by_id from app.celery.research_mode_tasks import send_sms_response, send_email_response diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 3f91d36fc..17b16e063 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -2,10 +2,9 @@ import uuid from datetime import datetime import pytest -from mock import ANY +from unittest.mock import ANY import app -from sqlalchemy.orm.exc import NoResultFound from app import mmg_client from app.dao import (provider_details_dao, notifications_dao) from app.delivery import send_to_providers