From bd06b186843451d60691e38af332412d06294316 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 20 Sep 2016 17:24:28 +0100 Subject: [PATCH 01/26] 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 02/26] 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 94b280af6321813e21ac1082559b166598e90486 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 21 Sep 2016 14:34:38 +0100 Subject: [PATCH 03/26] ensure conftest sets up datetimes within function bodies this makes sure it respects any freezegun.freeze_times that might be in place --- tests/app/conftest.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index d16adcc87..57f08ef11 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -257,7 +257,7 @@ def sample_job(notify_db, service=None, template=None, notification_count=1, - created_at=datetime.utcnow(), + created_at=None, job_status='pending', scheduled_for=None): if service is None: @@ -273,7 +273,7 @@ def sample_job(notify_db, 'template_version': template.version, 'original_file_name': 'some.csv', 'notification_count': notification_count, - 'created_at': created_at, + 'created_at': created_at or datetime.utcnow(), 'created_by': service.created_by, 'job_status': job_status, 'scheduled_for': scheduled_for From 29abd4c382b848e1a512bcfc0b3139d1fd3f7595 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 21 Sep 2016 14:35:23 +0100 Subject: [PATCH 04/26] make dao_get_jobs_by_service_id paginated it can return a pretty long list, especially when we run lots of smoke tests, so make it accept pagination parameters, and return a pagination object --- app/dao/jobs_dao.py | 14 +++++++---- tests/app/dao/test_jobs_dao.py | 44 +++++++++++++++++++++++++++------- 2 files changed, 44 insertions(+), 14 deletions(-) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index b4cfbde4d..8e7d7b3c2 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -1,10 +1,11 @@ -from datetime import date, timedelta, datetime -from sqlalchemy import desc, asc, cast, Date as sql_date +from datetime import datetime + +from sqlalchemy import func, desc, asc, cast, Date as sql_date + from app import db from app.dao import days_ago from app.models import Job, NotificationHistory, JOB_STATUS_SCHEDULED from app.statsd_decorators import statsd -from sqlalchemy import func, asc @statsd(namespace="dao") @@ -26,11 +27,14 @@ def dao_get_job_by_service_id_and_job_id(service_id, job_id): return Job.query.filter_by(service_id=service_id, id=job_id).one() -def dao_get_jobs_by_service_id(service_id, limit_days=None): +def dao_get_jobs_by_service_id(service_id, limit_days=None, page=1, page_size=50): query_filter = [Job.service_id == service_id] if limit_days is not None: query_filter.append(cast(Job.created_at, sql_date) >= days_ago(limit_days)) - return Job.query.filter(*query_filter).order_by(desc(Job.created_at)).all() + return Job.query \ + .filter(*query_filter) \ + .order_by(desc(Job.created_at)) \ + .paginate(page=page, per_page=page_size) def dao_get_job_by_id(job_id): diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index ee116c8ca..b983c7cbf 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -1,5 +1,6 @@ -from datetime import (datetime, timedelta) +from datetime import datetime, timedelta import uuid + from freezegun import freeze_time from app.dao.jobs_dao import ( @@ -10,9 +11,10 @@ from app.dao.jobs_dao import ( dao_get_scheduled_jobs, dao_get_future_scheduled_job_by_id_and_service_id, dao_get_notification_outcomes_for_job, - dao_get_jobs_older_than) - + dao_get_jobs_older_than +) from app.models import Job + from tests.app.conftest import sample_notification, sample_job, sample_service @@ -143,8 +145,8 @@ def test_get_jobs_for_service(notify_db, notify_db_session, sample_template): other_template = create_template(notify_db, notify_db_session, service=other_service) other_job = create_job(notify_db, notify_db_session, service=other_service, template=other_template) - one_job_from_db = dao_get_jobs_by_service_id(one_job.service_id) - other_job_from_db = dao_get_jobs_by_service_id(other_job.service_id) + one_job_from_db = dao_get_jobs_by_service_id(one_job.service_id).items + other_job_from_db = dao_get_jobs_by_service_id(other_job.service_id).items assert len(one_job_from_db) == 1 assert one_job == one_job_from_db[0] @@ -162,13 +164,13 @@ def test_get_jobs_for_service_with_limit_days_param(notify_db, notify_db_session old_job = create_job(notify_db, notify_db_session, sample_template.service, sample_template, created_at=datetime.now() - timedelta(days=8)) - jobs = dao_get_jobs_by_service_id(one_job.service_id) + jobs = dao_get_jobs_by_service_id(one_job.service_id).items assert len(jobs) == 2 assert one_job in jobs assert old_job in jobs - jobs_limit_days = dao_get_jobs_by_service_id(one_job.service_id, limit_days=7) + jobs_limit_days = dao_get_jobs_by_service_id(one_job.service_id, limit_days=7).items assert len(jobs_limit_days) == 1 assert one_job in jobs_limit_days assert old_job not in jobs_limit_days @@ -187,7 +189,7 @@ def test_get_jobs_for_service_with_limit_days_edge_case(notify_db, notify_db_ses job_eight_days_old = create_job(notify_db, notify_db_session, sample_template.service, sample_template, created_at=datetime.now() - timedelta(days=8)) - jobs_limit_days = dao_get_jobs_by_service_id(one_job.service_id, limit_days=7) + jobs_limit_days = dao_get_jobs_by_service_id(one_job.service_id, limit_days=7).items assert len(jobs_limit_days) == 3 assert one_job in jobs_limit_days assert job_two in jobs_limit_days @@ -207,7 +209,7 @@ def test_get_jobs_for_service_in_created_at_order(notify_db, notify_db_session, job_4 = create_job( notify_db, notify_db_session, sample_template.service, sample_template, created_at=datetime.utcnow()) - jobs = dao_get_jobs_by_service_id(sample_template.service.id) + jobs = dao_get_jobs_by_service_id(sample_template.service.id).items assert len(jobs) == 4 assert jobs[0].id == job_4.id @@ -271,3 +273,27 @@ def test_should_get_jobs_older_than_seven_days(notify_db, notify_db_session): jobs = dao_get_jobs_older_than(7) assert len(jobs) == 1 assert jobs[0].id == job_1.id + + +def test_get_jobs_for_service_is_paginated(notify_db, notify_db_session, sample_service, sample_template): + + from tests.app.conftest import sample_job as create_job + + with freeze_time('2015-01-01T00:00:00') as the_time: + for _ in range(10): + the_time.tick(timedelta(hours=1)) + create_job(notify_db, notify_db_session, sample_service, sample_template) + + res = dao_get_jobs_by_service_id(sample_service.id, page=1, page_size=2) + + assert res.per_page == 2 + assert res.total == 10 + assert len(res.items) == 2 + assert res.items[0].created_at == datetime(2015, 1, 1, 10) + assert res.items[1].created_at == datetime(2015, 1, 1, 9) + + res = dao_get_jobs_by_service_id(sample_service.id, page=2, page_size=2) + + assert len(res.items) == 2 + assert res.items[0].created_at == datetime(2015, 1, 1, 8) + assert res.items[1].created_at == datetime(2015, 1, 1, 7) From 4bfcfcd2fb7322a9005cf871c909e5ec6be7f8a2 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 21 Sep 2016 15:45:26 +0100 Subject: [PATCH 05/26] clean up test imports --- tests/app/dao/test_jobs_dao.py | 86 +++++++++++++++------------------- 1 file changed, 38 insertions(+), 48 deletions(-) diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index b983c7cbf..f9a77192f 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -15,7 +15,11 @@ from app.dao.jobs_dao import ( ) from app.models import Job -from tests.app.conftest import sample_notification, sample_job, sample_service +from tests.app.conftest import sample_notification as create_notification +from tests.app.conftest import sample_job as create_job +from tests.app.conftest import sample_service as create_service +from tests.app.conftest import sample_template as create_template +from tests.app.conftest import sample_user as create_user def test_should_have_decorated_notifications_dao_functions(): @@ -28,14 +32,14 @@ def test_should_get_all_statuses_for_notifications_associated_with_job( sample_service, sample_job): - sample_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='created') - sample_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='sending') - sample_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='delivered') - sample_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='pending') - sample_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='failed') - sample_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='technical-failure') # noqa - sample_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='temporary-failure') # noqa - sample_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='permanent-failure') # noqa + create_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='created') + create_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='sending') + create_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='delivered') + create_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='pending') + create_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='failed') + create_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='technical-failure') # noqa + create_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='temporary-failure') # noqa + create_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='permanent-failure') # noqa results = dao_get_notification_outcomes_for_job(sample_service.id, sample_job.id) assert [(row.count, row.status) for row in results] == [ @@ -55,14 +59,14 @@ def test_should_count_of_statuses_for_notifications_associated_with_job( notify_db_session, sample_service, sample_job): - sample_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='created') - sample_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='created') - sample_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='sending') - sample_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='sending') - sample_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='sending') - sample_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='sending') - sample_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='delivered') - sample_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='delivered') + create_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='created') + create_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='created') + create_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='sending') + create_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='sending') + create_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='sending') + create_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='sending') + create_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='delivered') + create_notification(notify_db, notify_db_session, service=sample_service, job=sample_job, status='delivered') results = dao_get_notification_outcomes_for_job(sample_service.id, sample_job.id) assert [(row.count, row.status) for row in results] == [ @@ -77,11 +81,11 @@ def test_should_return_zero_length_array_if_no_notifications_for_job(sample_serv def test_should_return_notifications_only_for_this_job(notify_db, notify_db_session, sample_service): - job_1 = sample_job(notify_db, notify_db_session, service=sample_service) - job_2 = sample_job(notify_db, notify_db_session, service=sample_service) + job_1 = create_job(notify_db, notify_db_session, service=sample_service) + job_2 = create_job(notify_db, notify_db_session, service=sample_service) - sample_notification(notify_db, notify_db_session, service=sample_service, job=job_1, status='created') - sample_notification(notify_db, notify_db_session, service=sample_service, job=job_2, status='created') + create_notification(notify_db, notify_db_session, service=sample_service, job=job_1, status='created') + create_notification(notify_db, notify_db_session, service=sample_service, job=job_2, status='created') results = dao_get_notification_outcomes_for_job(sample_service.id, job_1.id) assert [(row.count, row.status) for row in results] == [ @@ -90,14 +94,14 @@ def test_should_return_notifications_only_for_this_job(notify_db, notify_db_sess def test_should_return_notifications_only_for_this_service(notify_db, notify_db_session): - service_1 = sample_service(notify_db, notify_db_session, service_name="one", email_from="one") - service_2 = sample_service(notify_db, notify_db_session, service_name="two", email_from="two") + service_1 = create_service(notify_db, notify_db_session, service_name="one", email_from="one") + service_2 = create_service(notify_db, notify_db_session, service_name="two", email_from="two") - job_1 = sample_job(notify_db, notify_db_session, service=service_1) - job_2 = sample_job(notify_db, notify_db_session, service=service_2) + job_1 = create_job(notify_db, notify_db_session, service=service_1) + job_2 = create_job(notify_db, notify_db_session, service=service_2) - sample_notification(notify_db, notify_db_session, service=service_1, job=job_1, status='created') - sample_notification(notify_db, notify_db_session, service=service_2, job=job_2, status='created') + create_notification(notify_db, notify_db_session, service=service_1, job=job_1, status='created') + create_notification(notify_db, notify_db_session, service=service_2, job=job_2, status='created') assert len(dao_get_notification_outcomes_for_job(service_1.id, job_2.id)) == 0 @@ -132,11 +136,6 @@ def test_get_job_by_id(sample_job): def test_get_jobs_for_service(notify_db, notify_db_session, sample_template): - from tests.app.conftest import sample_job as create_job - from tests.app.conftest import sample_service as create_service - from tests.app.conftest import sample_template as create_template - from tests.app.conftest import sample_user as create_user - one_job = create_job(notify_db, notify_db_session, sample_template.service, sample_template) other_user = create_user(notify_db, notify_db_session, email="test@digital.cabinet-office.gov.uk") @@ -158,8 +157,6 @@ def test_get_jobs_for_service(notify_db, notify_db_session, sample_template): def test_get_jobs_for_service_with_limit_days_param(notify_db, notify_db_session, sample_template): - from tests.app.conftest import sample_job as create_job - one_job = create_job(notify_db, notify_db_session, sample_template.service, sample_template) old_job = create_job(notify_db, notify_db_session, sample_template.service, sample_template, created_at=datetime.now() - timedelta(days=8)) @@ -177,8 +174,6 @@ def test_get_jobs_for_service_with_limit_days_param(notify_db, notify_db_session def test_get_jobs_for_service_with_limit_days_edge_case(notify_db, notify_db_session, sample_template): - from tests.app.conftest import sample_job as create_job - one_job = create_job(notify_db, notify_db_session, sample_template.service, sample_template) job_two = create_job(notify_db, notify_db_session, sample_template.service, sample_template, created_at=(datetime.now() - timedelta(days=7)).date()) @@ -198,8 +193,6 @@ def test_get_jobs_for_service_with_limit_days_edge_case(notify_db, notify_db_ses def test_get_jobs_for_service_in_created_at_order(notify_db, notify_db_session, sample_template): - from tests.app.conftest import sample_job as create_job - job_1 = create_job( notify_db, notify_db_session, sample_template.service, sample_template, created_at=datetime.utcnow()) job_2 = create_job( @@ -233,8 +226,8 @@ def test_update_job(sample_job): def test_get_scheduled_jobs_gets_all_jobs_in_scheduled_state_scheduled_before_now(notify_db, notify_db_session): one_minute_ago = datetime.utcnow() - timedelta(minutes=1) one_hour_ago = datetime.utcnow() - timedelta(minutes=60) - job_new = sample_job(notify_db, notify_db_session, scheduled_for=one_minute_ago, job_status='scheduled') - job_old = sample_job(notify_db, notify_db_session, scheduled_for=one_hour_ago, job_status='scheduled') + job_new = create_job(notify_db, notify_db_session, scheduled_for=one_minute_ago, job_status='scheduled') + job_old = create_job(notify_db, notify_db_session, scheduled_for=one_hour_ago, job_status='scheduled') jobs = dao_get_scheduled_jobs() assert len(jobs) == 2 assert jobs[0].id == job_old.id @@ -243,8 +236,8 @@ def test_get_scheduled_jobs_gets_all_jobs_in_scheduled_state_scheduled_before_no def test_get_scheduled_jobs_gets_ignores_jobs_not_scheduled(notify_db, notify_db_session): one_minute_ago = datetime.utcnow() - timedelta(minutes=1) - sample_job(notify_db, notify_db_session) - job_scheduled = sample_job(notify_db, notify_db_session, scheduled_for=one_minute_ago, job_status='scheduled') + create_job(notify_db, notify_db_session) + job_scheduled = create_job(notify_db, notify_db_session, scheduled_for=one_minute_ago, job_status='scheduled') jobs = dao_get_scheduled_jobs() assert len(jobs) == 1 assert jobs[0].id == job_scheduled.id @@ -265,9 +258,9 @@ def test_should_get_jobs_older_than_seven_days(notify_db, notify_db_session): midnight = datetime(2016, 10, 10, 0, 0, 0, 0) one_millisecond_past_midnight = datetime(2016, 10, 10, 0, 0, 0, 1) - job_1 = sample_job(notify_db, notify_db_session, created_at=one_millisecond_before_midnight) - sample_job(notify_db, notify_db_session, created_at=midnight) - sample_job(notify_db, notify_db_session, created_at=one_millisecond_past_midnight) + job_1 = create_job(notify_db, notify_db_session, created_at=one_millisecond_before_midnight) + create_job(notify_db, notify_db_session, created_at=midnight) + create_job(notify_db, notify_db_session, created_at=one_millisecond_past_midnight) with freeze_time('2016-10-17T00:00:00'): jobs = dao_get_jobs_older_than(7) @@ -276,9 +269,6 @@ def test_should_get_jobs_older_than_seven_days(notify_db, notify_db_session): def test_get_jobs_for_service_is_paginated(notify_db, notify_db_session, sample_service, sample_template): - - from tests.app.conftest import sample_job as create_job - with freeze_time('2015-01-01T00:00:00') as the_time: for _ in range(10): the_time.tick(timedelta(hours=1)) From 24903a19ef697cfbf1e50e6eff26734e8708f994 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 21 Sep 2016 16:54:02 +0100 Subject: [PATCH 06/26] add pagination to GET /service/{}/job accepts a page parameter to control what page of data returns additional pagination fields in the response dict * page_size: will always be 50. defined by Config.PAGE_SIZE * total: the total amount of unpaginated records * links: dict containing optionally prev, next, and last, links to other relevant pagination pages also cleaned up some test imports --- app/job/rest.py | 37 +++++-- app/utils.py | 2 +- tests/app/job/test_rest.py | 222 +++++++++++++++++++++++-------------- 3 files changed, 169 insertions(+), 92 deletions(-) diff --git a/app/job/rest.py b/app/job/rest.py index 9f9ef0047..ced973dba 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -96,20 +96,14 @@ def get_jobs_by_service(service_id): if request.args.get('limit_days'): try: limit_days = int(request.args['limit_days']) - except ValueError as e: + except ValueError: errors = {'limit_days': ['{} is not an integer'.format(request.args['limit_days'])]} raise InvalidRequest(errors, status_code=400) else: limit_days = None - jobs = dao_get_jobs_by_service_id(service_id, limit_days) - data = job_schema.dump(jobs, many=True).data - - for job_data in data: - statistics = dao_get_notification_outcomes_for_job(service_id, job_data['id']) - job_data['statistics'] = [{'status': statistic[1], 'count': statistic[0]} for statistic in statistics] - - return jsonify(data=data) + page = int(request.args.get('page', 1)) + return jsonify(**get_paginated_jobs(service_id, limit_days, page)) @job.route('', methods=['POST']) @@ -144,3 +138,28 @@ def create_job(service_id): job_json['statistics'] = [] return jsonify(data=job_json), 201 + + +def get_paginated_jobs(service_id, limit_days, page): + pagination = dao_get_jobs_by_service_id( + service_id, + limit_days=limit_days, + page=page, + page_size=current_app.config['PAGE_SIZE'] + ) + + data = job_schema.dump(pagination.items, many=True).data + for job_data in data: + statistics = dao_get_notification_outcomes_for_job(service_id, job_data['id']) + job_data['statistics'] = [{'status': statistic[1], 'count': statistic[0]} for statistic in statistics] + + return { + 'data': data, + 'page_size': pagination.per_page, + 'total': pagination.total, + 'links': pagination_links( + pagination, + '.get_jobs_by_service', + service_id=service_id + ) + } diff --git a/app/utils.py b/app/utils.py index fb65fef48..f67643290 100644 --- a/app/utils.py +++ b/app/utils.py @@ -4,7 +4,7 @@ from flask import url_for def pagination_links(pagination, endpoint, **kwargs): if 'page' in kwargs: kwargs.pop('page', None) - links = dict() + links = {} if pagination.has_prev: links['prev'] = url_for(endpoint, page=pagination.prev_num, **kwargs) if pagination.has_next: diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index cef1fe793..5daa58595 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -1,8 +1,8 @@ import json import uuid from datetime import datetime, timedelta -from freezegun import freeze_time +from freezegun import freeze_time import pytest import pytz import app.celery.tasks @@ -10,52 +10,12 @@ import app.celery.tasks from tests import create_authorization_header from tests.app.conftest import ( sample_job as create_job, - sample_notification as create_sample_notification, sample_notification, sample_job) + sample_notification as create_notification +) from app.dao.templates_dao import dao_update_template from app.models import NOTIFICATION_STATUS_TYPES -def test_get_jobs(notify_api, notify_db, notify_db_session, sample_template): - _setup_jobs(notify_db, notify_db_session, sample_template) - - service_id = sample_template.service.id - - with notify_api.test_request_context(): - with notify_api.test_client() as client: - path = '/service/{}/job'.format(service_id) - auth_header = create_authorization_header(service_id=service_id) - response = client.get(path, headers=[auth_header]) - assert response.status_code == 200 - resp_json = json.loads(response.get_data(as_text=True)) - assert len(resp_json['data']) == 5 - - -def test_get_jobs_with_limit_days(notify_api, notify_db, notify_db_session, sample_template): - create_job( - notify_db, - notify_db_session, - service=sample_template.service, - template=sample_template, - ) - create_job( - notify_db, - notify_db_session, - service=sample_template.service, - template=sample_template, - created_at=datetime.now() - timedelta(days=7)) - - service_id = sample_template.service.id - - with notify_api.test_request_context(): - with notify_api.test_client() as client: - path = '/service/{}/job'.format(service_id) - auth_header = create_authorization_header(service_id=service_id) - response = client.get(path, headers=[auth_header], query_string={'limit_days': 5}) - assert response.status_code == 200 - resp_json = json.loads(response.get_data(as_text=True)) - assert len(resp_json['data']) == 1 - - def test_get_job_with_invalid_service_id_returns404(notify_api, sample_api_key, sample_service): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -391,7 +351,7 @@ def test_get_all_notifications_for_job_in_order_of_job_number(notify_api, main_job = create_job(notify_db, notify_db_session, service=sample_service) another_job = create_job(notify_db, notify_db_session, service=sample_service) - notification_1 = create_sample_notification( + notification_1 = create_notification( notify_db, notify_db_session, job=main_job, @@ -399,7 +359,7 @@ def test_get_all_notifications_for_job_in_order_of_job_number(notify_api, created_at=datetime.utcnow(), job_row_number=1 ) - notification_2 = create_sample_notification( + notification_2 = create_notification( notify_db, notify_db_session, job=main_job, @@ -407,7 +367,7 @@ def test_get_all_notifications_for_job_in_order_of_job_number(notify_api, created_at=datetime.utcnow(), job_row_number=2 ) - notification_3 = create_sample_notification( + notification_3 = create_notification( notify_db, notify_db_session, job=main_job, @@ -415,7 +375,7 @@ def test_get_all_notifications_for_job_in_order_of_job_number(notify_api, created_at=datetime.utcnow(), job_row_number=3 ) - create_sample_notification(notify_db, notify_db_session, job=another_job) + create_notification(notify_db, notify_db_session, job=another_job) auth_header = create_authorization_header() @@ -454,7 +414,7 @@ def test_get_all_notifications_for_job_filtered_by_status( with notify_api.test_request_context(), notify_api.test_client() as client: job = create_job(notify_db, notify_db_session, service=sample_service) - create_sample_notification( + create_notification( notify_db, notify_db_session, job=job, @@ -492,14 +452,14 @@ def test_get_job_by_id_should_return_statistics(notify_db, notify_db_session, no job_id = str(sample_job.id) service_id = sample_job.service.id - sample_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='created') - sample_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='sending') - sample_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='delivered') - sample_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='pending') - sample_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='failed') - sample_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='technical-failure') # noqa - sample_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='temporary-failure') # noqa - sample_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='permanent-failure') # noqa + create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='created') + create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='sending') + create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='delivered') + create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='pending') + create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='failed') + create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='technical-failure') # noqa + create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='temporary-failure') # noqa + create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='permanent-failure') # noqa with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -524,16 +484,16 @@ def test_get_job_by_id_should_return_summed_statistics(notify_db, notify_db_sess job_id = str(sample_job.id) service_id = sample_job.service.id - sample_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='created') - sample_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='created') - sample_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='created') - sample_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='sending') - sample_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='failed') - sample_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='failed') - sample_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='failed') - sample_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='technical-failure') # noqa - sample_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='temporary-failure') # noqa - sample_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='temporary-failure') # noqa + create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='created') + create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='created') + create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='created') + create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='sending') + create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='failed') + create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='failed') + create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='failed') + create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='technical-failure') # noqa + create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='temporary-failure') # noqa + create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='temporary-failure') # noqa with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -551,22 +511,63 @@ def test_get_job_by_id_should_return_summed_statistics(notify_db, notify_db_sess assert resp_json['data']['created_by']['name'] == 'Test User' -def test_get_jobs_for_service_should_return_statistics(notify_db, notify_db_session, notify_api, sample_service): - now = datetime.utcnow() - earlier = datetime.utcnow() - timedelta(days=1) - job_1 = sample_job(notify_db, notify_db_session, service=sample_service, created_at=earlier) - job_2 = sample_job(notify_db, notify_db_session, service=sample_service, created_at=now) +def test_get_jobs(notify_api, notify_db, notify_db_session, sample_template): + _setup_jobs(notify_db, notify_db_session, sample_template) - sample_notification(notify_db, notify_db_session, service=sample_service, job=job_1, status='created') - sample_notification(notify_db, notify_db_session, service=sample_service, job=job_1, status='created') - sample_notification(notify_db, notify_db_session, service=sample_service, job=job_1, status='created') - sample_notification(notify_db, notify_db_session, service=sample_service, job=job_2, status='sending') - sample_notification(notify_db, notify_db_session, service=sample_service, job=job_2, status='sending') - sample_notification(notify_db, notify_db_session, service=sample_service, job=job_2, status='sending') + service_id = sample_template.service.id with notify_api.test_request_context(): with notify_api.test_client() as client: - path = '/service/{}/job'.format(str(sample_service.id)) + path = '/service/{}/job'.format(service_id) + auth_header = create_authorization_header(service_id=service_id) + response = client.get(path, headers=[auth_header]) + assert response.status_code == 200 + resp_json = json.loads(response.get_data(as_text=True)) + assert len(resp_json['data']) == 5 + + +def test_get_jobs_with_limit_days(notify_api, notify_db, notify_db_session, sample_template): + create_job( + notify_db, + notify_db_session, + service=sample_template.service, + template=sample_template, + ) + create_job( + notify_db, + notify_db_session, + service=sample_template.service, + template=sample_template, + created_at=datetime.now() - timedelta(days=7)) + + service_id = sample_template.service.id + + with notify_api.test_request_context(): + with notify_api.test_client() as client: + path = '/service/{}/job'.format(service_id) + auth_header = create_authorization_header(service_id=service_id) + response = client.get(path, headers=[auth_header], query_string={'limit_days': 5}) + assert response.status_code == 200 + resp_json = json.loads(response.get_data(as_text=True)) + assert len(resp_json['data']) == 1 + + +def test_get_jobs_should_return_statistics(notify_db, notify_db_session, notify_api, sample_service): + now = datetime.utcnow() + earlier = datetime.utcnow() - timedelta(days=1) + job_1 = create_job(notify_db, notify_db_session, service=sample_service, created_at=earlier) + job_2 = create_job(notify_db, notify_db_session, service=sample_service, created_at=now) + + create_notification(notify_db, notify_db_session, service=sample_service, job=job_1, status='created') + create_notification(notify_db, notify_db_session, service=sample_service, job=job_1, status='created') + create_notification(notify_db, notify_db_session, service=sample_service, job=job_1, status='created') + create_notification(notify_db, notify_db_session, service=sample_service, job=job_2, status='sending') + create_notification(notify_db, notify_db_session, service=sample_service, job=job_2, status='sending') + create_notification(notify_db, notify_db_session, service=sample_service, job=job_2, status='sending') + + with notify_api.test_request_context(): + with notify_api.test_client() as client: + path = '/service/{}/job'.format(sample_service.id) auth_header = create_authorization_header(service_id=str(sample_service.id)) response = client.get(path, headers=[auth_header]) assert response.status_code == 200 @@ -578,7 +579,7 @@ def test_get_jobs_for_service_should_return_statistics(notify_db, notify_db_sess assert {'status': 'created', 'count': 3} in resp_json['data'][1]['statistics'] -def test_get_jobs_for_service_should_return_no_stats_if_no_rows_in_notifications( +def test_get_jobs_should_return_no_stats_if_no_rows_in_notifications( notify_db, notify_db_session, notify_api, @@ -586,12 +587,12 @@ def test_get_jobs_for_service_should_return_no_stats_if_no_rows_in_notifications now = datetime.utcnow() earlier = datetime.utcnow() - timedelta(days=1) - job_1 = sample_job(notify_db, notify_db_session, service=sample_service, created_at=earlier) - job_2 = sample_job(notify_db, notify_db_session, service=sample_service, created_at=now) + job_1 = create_job(notify_db, notify_db_session, service=sample_service, created_at=earlier) + job_2 = create_job(notify_db, notify_db_session, service=sample_service, created_at=now) with notify_api.test_request_context(): with notify_api.test_client() as client: - path = '/service/{}/job'.format(str(sample_service.id)) + path = '/service/{}/job'.format(sample_service.id) auth_header = create_authorization_header(service_id=str(sample_service.id)) response = client.get(path, headers=[auth_header]) assert response.status_code == 200 @@ -601,3 +602,60 @@ def test_get_jobs_for_service_should_return_no_stats_if_no_rows_in_notifications assert resp_json['data'][0]['statistics'] == [] assert resp_json['data'][1]['id'] == str(job_1.id) assert resp_json['data'][1]['statistics'] == [] + + +def test_get_jobs_should_paginate( + notify_db, + notify_db_session, + client, + sample_template +): + create_10_jobs(notify_db, notify_db_session, sample_template.service, sample_template) + + client.application.config['PAGE_SIZE'] = 2 + path = '/service/{}/job'.format(sample_template.service_id) + auth_header = create_authorization_header(service_id=str(sample_template.service_id)) + + response = client.get(path, headers=[auth_header]) + + assert response.status_code == 200 + resp_json = json.loads(response.get_data(as_text=True)) + assert len(resp_json['data']) == 2 + assert resp_json['data'][0]['created_at'] == '2015-01-01T10:00:00+00:00' + assert resp_json['data'][1]['created_at'] == '2015-01-01T09:00:00+00:00' + assert resp_json['page_size'] == 2 + assert resp_json['total'] == 10 + assert 'links' in resp_json + assert set(resp_json['links'].keys()) == {'next', 'last'} + + +def test_get_jobs_accepts_page_parameter( + notify_db, + notify_db_session, + client, + sample_template +): + create_10_jobs(notify_db, notify_db_session, sample_template.service, sample_template) + + client.application.config['PAGE_SIZE'] = 2 + path = '/service/{}/job'.format(sample_template.service_id) + auth_header = create_authorization_header(service_id=str(sample_template.service_id)) + + response = client.get(path, headers=[auth_header], query_string={'page': 2}) + + assert response.status_code == 200 + resp_json = json.loads(response.get_data(as_text=True)) + assert len(resp_json['data']) == 2 + assert resp_json['data'][0]['created_at'] == '2015-01-01T08:00:00+00:00' + assert resp_json['data'][1]['created_at'] == '2015-01-01T07:00:00+00:00' + assert resp_json['page_size'] == 2 + assert resp_json['total'] == 10 + assert 'links' in resp_json + assert set(resp_json['links'].keys()) == {'prev', 'next', 'last'} + + +def create_10_jobs(db, session, service, template): + with freeze_time('2015-01-01T00:00:00') as the_time: + for _ in range(10): + the_time.tick(timedelta(hours=1)) + create_job(db, session, service, template) From 00b905bef07271ff0c9fdc7998403f8858301a97 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 21 Sep 2016 17:13:26 +0100 Subject: [PATCH 07/26] use a contextmanager to set config values so that when later tests run, the config has been restored to its defaults and we don't end up failing later tests in confusing ways --- tests/app/job/test_rest.py | 9 +++++---- tests/conftest.py | 9 +++++++++ 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index 5daa58595..f4dc01392 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -8,6 +8,7 @@ import pytz import app.celery.tasks from tests import create_authorization_header +from tests.conftest import set_config from tests.app.conftest import ( sample_job as create_job, sample_notification as create_notification @@ -612,11 +613,11 @@ def test_get_jobs_should_paginate( ): create_10_jobs(notify_db, notify_db_session, sample_template.service, sample_template) - client.application.config['PAGE_SIZE'] = 2 path = '/service/{}/job'.format(sample_template.service_id) auth_header = create_authorization_header(service_id=str(sample_template.service_id)) - response = client.get(path, headers=[auth_header]) + with set_config(client.application, 'PAGE_SIZE', 2): + response = client.get(path, headers=[auth_header]) assert response.status_code == 200 resp_json = json.loads(response.get_data(as_text=True)) @@ -637,11 +638,11 @@ def test_get_jobs_accepts_page_parameter( ): create_10_jobs(notify_db, notify_db_session, sample_template.service, sample_template) - client.application.config['PAGE_SIZE'] = 2 path = '/service/{}/job'.format(sample_template.service_id) auth_header = create_authorization_header(service_id=str(sample_template.service_id)) - response = client.get(path, headers=[auth_header], query_string={'page': 2}) + with set_config(client.application, 'PAGE_SIZE', 2): + response = client.get(path, headers=[auth_header], query_string={'page': 2}) assert response.status_code == 200 resp_json = json.loads(response.get_data(as_text=True)) diff --git a/tests/conftest.py b/tests/conftest.py index 3da2dead3..11f570de5 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,3 +1,4 @@ +from contextlib import contextmanager import os import boto3 @@ -83,3 +84,11 @@ def pytest_generate_tests(metafunc): argnames, testdata = idparametrize.args ids, argvalues = zip(*sorted(testdata.items())) metafunc.parametrize(argnames, argvalues, ids=ids) + + +@contextmanager +def set_config(app, name, value): + old_val = app.config.get(name) + app.config[name] = value + yield + app.config[name] = old_val From 2f36e0dbcf33e0ccf752d739314981323fdf836a Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 22 Sep 2016 09:16:58 +0100 Subject: [PATCH 08/26] 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 09/26] 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 10/26] 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 11/26] 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 b36d0ff5523067cb847ba31deaf34efbb8b0afd2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 23 Sep 2016 09:43:25 +0100 Subject: [PATCH 12/26] Make indentation more sensible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Starting arguments on their own line and putting the closing parenthesis on it’s own line because any subsequent changes to the arguments diff cleanly (ie without touching any other lines). --- app/dao/notifications_dao.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index fa65e8a72..2453db6c5 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -228,14 +228,16 @@ def get_notifications(filter_dict=None): @statsd(namespace="dao") -def get_notifications_for_service(service_id, - filter_dict=None, - page=1, - page_size=None, - limit_days=None, - key_type=None, - personalisation=False, - include_jobs=False): +def get_notifications_for_service( + service_id, + filter_dict=None, + page=1, + page_size=None, + limit_days=None, + key_type=None, + personalisation=False, + include_jobs=False +): if page_size is None: page_size = current_app.config['PAGE_SIZE'] From 54c28a4bea58f65059c7a699cb7395caf92507ac Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 23 Sep 2016 10:24:12 +0100 Subject: [PATCH 13/26] 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 14/26] 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 f9f3bb8370c3e24a5fee7ea026fdf37179a6a9ce Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 23 Sep 2016 10:27:10 +0100 Subject: [PATCH 15/26] Make DAO optionally return test key notifications Developers need visibility of what their integration is doing within the app. This includes notifications sent with a test key. This commit adds an optional, defaults-to-false parameter to include notifications sent from a test API key when getting notifications. --- app/dao/notifications_dao.py | 5 +++-- tests/app/dao/test_notification_dao.py | 8 ++++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 2453db6c5..c289f60ed 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -236,7 +236,8 @@ def get_notifications_for_service( limit_days=None, key_type=None, personalisation=False, - include_jobs=False + include_jobs=False, + include_from_test_key=False ): if page_size is None: page_size = current_app.config['PAGE_SIZE'] @@ -252,7 +253,7 @@ def get_notifications_for_service( if key_type is not None: filters.append(Notification.key_type == key_type) - else: + elif not include_from_test_key: filters.append(Notification.key_type != KEY_TYPE_TEST) query = Notification.query.filter(*filters) diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 39c91912c..97885f50a 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -1000,11 +1000,15 @@ def test_get_notifications_created_by_api_or_csv_are_returned_correctly_excludin all_notifications = Notification.query.all() assert len(all_notifications) == 4 - # returns all API derived notifications + # returns all real API derived notifications all_notifications = get_notifications_for_service(sample_service.id).items assert len(all_notifications) == 2 - # all notifications including jobs + # returns all API derived notifications, including those created with test key + all_notifications = get_notifications_for_service(sample_service.id, include_from_test_key=True).items + assert len(all_notifications) == 3 + + # all real notifications including jobs all_notifications = get_notifications_for_service(sample_service.id, limit_days=1, include_jobs=True).items assert len(all_notifications) == 3 From d7bb83fadfa1fddfa80f6b193438c90ec853925e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 23 Sep 2016 10:35:31 +0100 Subject: [PATCH 16/26] Optionally get notifications created w/ test key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is only for the method that the admin app uses; it doesn’t affect the public get notifications endpoint. --- app/schemas.py | 1 + app/service/rest.py | 5 ++++- tests/app/service/test_rest.py | 25 +++++++++++++++++++++---- 3 files changed, 26 insertions(+), 5 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index ccabfc9d6..bb4aca8e5 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -422,6 +422,7 @@ class NotificationsFilterSchema(ma.Schema): page_size = fields.Int(required=False) limit_days = fields.Int(required=False) include_jobs = fields.Boolean(required=False) + include_from_test_key = fields.Boolean(required=False) @pre_load def handle_multidict(self, in_data): diff --git a/app/service/rest.py b/app/service/rest.py index 011977a84..d50326805 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -214,6 +214,7 @@ def get_all_notifications_for_service(service_id): page_size = data['page_size'] if 'page_size' in data else current_app.config.get('PAGE_SIZE') limit_days = data.get('limit_days') include_jobs = data.get('include_jobs', True) + include_from_test_key = data.get('include_from_test_key', False) pagination = notifications_dao.get_notifications_for_service( service_id, @@ -221,7 +222,9 @@ def get_all_notifications_for_service(service_id): page=page, page_size=page_size, limit_days=limit_days, - include_jobs=include_jobs) + include_jobs=include_jobs, + include_from_test_key=include_from_test_key + ) kwargs = request.args.to_dict() kwargs['service_id'] = service_id return jsonify( diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index ac8c58e3f..6c5287058 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -16,6 +16,7 @@ from tests.app.conftest import ( sample_user as create_sample_user, sample_notification as create_sample_notification, sample_notification_with_job) +from app.models import KEY_TYPE_TEST def test_get_service_list(notify_api, service_factory): @@ -1037,23 +1038,39 @@ def test_get_all_notifications_for_service_in_order(notify_api, notify_db, notif assert response.status_code == 200 +@pytest.mark.parametrize( + 'include_from_test_key, expected_count_of_notifications', + [ + (False, 2), + (True, 3) + ] +) def test_get_all_notifications_for_service_including_ones_made_by_jobs( notify_api, notify_db, notify_db_session, - sample_service): + sample_service, + include_from_test_key, + expected_count_of_notifications +): with notify_api.test_request_context(), notify_api.test_client() as client: with_job = sample_notification_with_job(notify_db, notify_db_session, service=sample_service) without_job = create_sample_notification(notify_db, notify_db_session, service=sample_service) + from_test_api_key = create_sample_notification( + notify_db, notify_db_session, service=sample_service, key_type=KEY_TYPE_TEST + ) auth_header = create_authorization_header() response = client.get( - path='/service/{}/notifications'.format(sample_service.id), - headers=[auth_header]) + path='/service/{}/notifications?include_from_test_key={}'.format( + sample_service.id, include_from_test_key + ), + headers=[auth_header] + ) resp = json.loads(response.get_data(as_text=True)) - assert len(resp['notifications']) == 2 + assert len(resp['notifications']) == expected_count_of_notifications assert resp['notifications'][0]['to'] == with_job.to assert resp['notifications'][1]['to'] == without_job.to assert response.status_code == 200 From ba71079f22800f9ed0ce992652d9a17ea0af94cb Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 23 Sep 2016 10:35:46 +0100 Subject: [PATCH 17/26] Reduce indentation --- tests/app/service/test_rest.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 6c5287058..e3a4ea141 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1046,12 +1046,12 @@ def test_get_all_notifications_for_service_in_order(notify_api, notify_db, notif ] ) def test_get_all_notifications_for_service_including_ones_made_by_jobs( - notify_api, - notify_db, - notify_db_session, - sample_service, - include_from_test_key, - expected_count_of_notifications + notify_api, + notify_db, + notify_db_session, + sample_service, + include_from_test_key, + expected_count_of_notifications ): with notify_api.test_request_context(), notify_api.test_client() as client: with_job = sample_notification_with_job(notify_db, notify_db_session, service=sample_service) From 21f3448fbc1fd3703759654614fe2213913cd2fc Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 23 Sep 2016 10:36:28 +0100 Subject: [PATCH 18/26] Use client fixture --- tests/app/service/test_rest.py | 37 +++++++++++++++++----------------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index e3a4ea141..179d9383a 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1046,34 +1046,33 @@ def test_get_all_notifications_for_service_in_order(notify_api, notify_db, notif ] ) def test_get_all_notifications_for_service_including_ones_made_by_jobs( - notify_api, + client, notify_db, notify_db_session, sample_service, include_from_test_key, expected_count_of_notifications ): - with notify_api.test_request_context(), notify_api.test_client() as client: - with_job = sample_notification_with_job(notify_db, notify_db_session, service=sample_service) - without_job = create_sample_notification(notify_db, notify_db_session, service=sample_service) - from_test_api_key = create_sample_notification( - notify_db, notify_db_session, service=sample_service, key_type=KEY_TYPE_TEST - ) + with_job = sample_notification_with_job(notify_db, notify_db_session, service=sample_service) + without_job = create_sample_notification(notify_db, notify_db_session, service=sample_service) + from_test_api_key = create_sample_notification( + notify_db, notify_db_session, service=sample_service, key_type=KEY_TYPE_TEST + ) - auth_header = create_authorization_header() + auth_header = create_authorization_header() - response = client.get( - path='/service/{}/notifications?include_from_test_key={}'.format( - sample_service.id, include_from_test_key - ), - headers=[auth_header] - ) + response = client.get( + path='/service/{}/notifications?include_from_test_key={}'.format( + sample_service.id, include_from_test_key + ), + headers=[auth_header] + ) - resp = json.loads(response.get_data(as_text=True)) - assert len(resp['notifications']) == expected_count_of_notifications - assert resp['notifications'][0]['to'] == with_job.to - assert resp['notifications'][1]['to'] == without_job.to - assert response.status_code == 200 + resp = json.loads(response.get_data(as_text=True)) + assert len(resp['notifications']) == expected_count_of_notifications + assert resp['notifications'][0]['to'] == with_job.to + assert resp['notifications'][1]['to'] == without_job.to + assert response.status_code == 200 def test_get_only_api_created_notifications_for_service( From f5aac5796cf497ed44e274bd21a250a7dcec8b22 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 23 Sep 2016 11:07:49 +0100 Subject: [PATCH 19/26] Improve the error message when the service id is not the right data type. Improve the error message is the api key is not valid. --- app/authentication/auth.py | 15 ++++---- .../app/authentication/test_authentication.py | 36 ++++++++++--------- 2 files changed, 29 insertions(+), 22 deletions(-) diff --git a/app/authentication/auth.py b/app/authentication/auth.py index 5e8cb2d4e..09ccadb66 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -1,4 +1,5 @@ -from flask import request, jsonify, _request_ctx_stack, current_app +from flask import request, _request_ctx_stack, current_app +from sqlalchemy.exc import DataError from sqlalchemy.orm.exc import NoResultFound from notifications_python_client.authentication import decode_jwt_token, get_token_issuer @@ -37,8 +38,10 @@ def requires_auth(): if client == current_app.config.get('ADMIN_CLIENT_USER_NAME'): return handle_admin_key(auth_token, current_app.config.get('ADMIN_CLIENT_SECRET')) - api_keys = get_model_api_keys(client) - + try: + api_keys = get_model_api_keys(client) + except DataError: + raise AuthError("Invalid token: service id is not the right data type", 403) for api_key in api_keys: try: get_decode_errors(auth_token, api_key.unsigned_secret) @@ -59,19 +62,19 @@ def requires_auth(): if not api_keys: raise AuthError("Invalid token: service has no API keys", 403) else: - raise AuthError("Invalid token: signature", 403) + raise AuthError("Invalid token: signature, api token is not valid", 403) def handle_admin_key(auth_token, secret): try: get_decode_errors(auth_token, secret) return - except TokenDecodeError as e: + except TokenDecodeError: raise AuthError("Invalid token: signature", 403) def get_decode_errors(auth_token, unsigned_secret): try: decode_jwt_token(auth_token, unsigned_secret) - except TokenExpiredError as e: + except TokenExpiredError: raise AuthError("Invalid token: expired", 403) diff --git a/tests/app/authentication/test_authentication.py b/tests/app/authentication/test_authentication.py index 057760585..3b9ff1009 100644 --- a/tests/app/authentication/test_authentication.py +++ b/tests/app/authentication/test_authentication.py @@ -53,7 +53,7 @@ def test_should_not_allow_invalid_secret(notify_api, sample_api_key): ) assert response.status_code == 403 data = json.loads(response.get_data()) - assert data['message'] == {"token": ['Invalid token: signature']} + assert data['message'] == {"token": ['Invalid token: signature, api token is not valid']} def test_should_allow_valid_token(notify_api, sample_api_key): @@ -67,6 +67,20 @@ def test_should_allow_valid_token(notify_api, sample_api_key): assert response.status_code == 200 +def test_should_not_allow_service_id_that_is_not_the_wrong_data_type(notify_api, sample_api_key): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + token = create_jwt_token(secret=get_unsigned_secrets(sample_api_key.service_id)[0], + client_id=str('not-a-valid-id')) + response = client.get( + '/service', + headers={'Authorization': "Bearer {}".format(token)} + ) + assert response.status_code == 403 + data = json.loads(response.get_data()) + assert data['message'] == {"token": ['Invalid token: service id is not the right data type']} + + def test_should_allow_valid_token_for_request_with_path_params(notify_api, sample_api_key): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -95,8 +109,6 @@ def test_should_allow_valid_token_when_service_has_multiple_keys(notify_api, sam def test_authentication_passes_admin_client_token(notify_api, - notify_db, - notify_db_session, sample_api_key): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -173,8 +185,7 @@ def test_authentication_returns_token_expired_when_service_uses_expired_key_and_ def test_authentication_returns_error_when_admin_client_has_no_secrets(notify_api, - notify_db, - notify_db_session): + sample_service): with notify_api.test_request_context(): with notify_api.test_client() as client: api_secret = notify_api.config.get('ADMIN_CLIENT_SECRET') @@ -194,17 +205,14 @@ def test_authentication_returns_error_when_admin_client_has_no_secrets(notify_ap def test_authentication_returns_error_when_service_doesnt_exit( notify_api, - notify_db, - notify_db_session, - sample_service, - fake_uuid + sample_api_key ): with notify_api.test_request_context(), notify_api.test_client() as client: # get service ID and secret the wrong way around token = create_jwt_token( - secret=str(sample_service.id), - client_id=fake_uuid - ) + secret=str(sample_api_key.service_id), + client_id=str(sample_api_key.id)) + response = client.get( '/service', headers={'Authorization': 'Bearer {}'.format(token)} @@ -215,8 +223,6 @@ def test_authentication_returns_error_when_service_doesnt_exit( def test_authentication_returns_error_when_service_has_no_secrets(notify_api, - notify_db, - notify_db_session, sample_service, fake_uuid): with notify_api.test_request_context(): @@ -248,8 +254,6 @@ def test_should_attach_the_current_api_key_to_current_app(notify_api, sample_ser def test_should_return_403_when_token_is_expired(notify_api, - notify_db, - notify_db_session, sample_api_key): with notify_api.test_request_context(): with notify_api.test_client() as client: From 130f42fed27f66f900de9999e6ab4d80297f3e79 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 23 Sep 2016 14:44:15 +0100 Subject: [PATCH 20/26] Add details of API key to notifications MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We want to show developers a log of notifications they’ve sent using the API in the admin app. In order to indentify a notification it’s probably helpful to know: - who the notification was sent to (which we expose) - when the notification was created (which we expose) - which key was used to create the notification (which we expose, but only as the `id` of the key) Developers don’t see the `id` of the API key in the app anywhere, so this isn’t useful information. Much more useful is the `type` and `name` of the key. So this commit changes the schema to also return these. This commit does some slightly hacky stuff with conftest because it breaks a lot of other tests if the sample notification has a real API key or an API key with a non-unique name. --- app/schemas.py | 10 ++++++++++ tests/app/conftest.py | 16 ++++++++++++++-- tests/app/test_schemas.py | 14 ++++++++++++++ 3 files changed, 38 insertions(+), 2 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index ccabfc9d6..91e585026 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -315,6 +315,16 @@ class NotificationWithTemplateSchema(BaseSchema): ) job = fields.Nested(JobSchema, only=["id", "original_file_name"], dump_only=True) personalisation = fields.Dict(required=False) + key_type = field_for(models.Notification, 'key_type', required=True) + key_name = fields.String() + + @pre_dump + def add_api_key_name(self, in_data): + if in_data.api_key: + in_data.key_name = in_data.api_key.name + else: + in_data.key_name = None + return in_data class NotificationWithPersonalisationSchema(NotificationWithTemplateSchema): diff --git a/tests/app/conftest.py b/tests/app/conftest.py index d16adcc87..5c6e45094 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -232,10 +232,11 @@ def sample_email_template_with_placeholders(notify_db, notify_db_session): def sample_api_key(notify_db, notify_db_session, service=None, - key_type=KEY_TYPE_NORMAL): + key_type=KEY_TYPE_NORMAL, + name=None): if service is None: service = sample_service(notify_db, notify_db_session) - data = {'service': service, 'name': uuid.uuid4(), 'created_by': service.created_by, 'key_type': key_type} + data = {'service': service, 'name': name or uuid.uuid4(), 'created_by': service.created_by, 'key_type': key_type} api_key = ApiKey(**data) save_model_api_key(api_key) return api_key @@ -441,6 +442,17 @@ def sample_notification(notify_db, return notification +@pytest.fixture(scope='function') +def sample_notification_with_api_key(notify_db, notify_db_session): + notification = sample_notification(notify_db, notify_db_session) + notification.api_key_id = sample_api_key( + notify_db, + notify_db_session, + name='Test key' + ).id + return notification + + @pytest.fixture(scope='function') def sample_email_notification(notify_db, notify_db_session): created_at = datetime.utcnow() diff --git a/tests/app/test_schemas.py b/tests/app/test_schemas.py index e7d037311..4ba1c98b5 100644 --- a/tests/app/test_schemas.py +++ b/tests/app/test_schemas.py @@ -8,3 +8,17 @@ def test_job_schema_doesnt_return_notifications(sample_notification_with_job): assert not errors assert 'notifications' not in data + + +def test_notification_schema_ignores_absent_api_key(sample_notification_with_job): + from app.schemas import notification_with_template_schema + + data = notification_with_template_schema.dump(sample_notification_with_job).data + assert data['key_name'] is None + + +def test_notification_schema_adds_api_key_name(sample_notification_with_api_key): + from app.schemas import notification_with_template_schema + + data = notification_with_template_schema.dump(sample_notification_with_api_key).data + assert data['key_name'] == 'Test key' From 09e6e6198b0b7a05aec06513f5ce5ac290f2e46a Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 23 Sep 2016 15:46:48 +0100 Subject: [PATCH 21/26] Fixing a bug that allows a sms notification to be sent with an email template and vice versa. This has been resolved for the post notifications endpoint --- app/notifications/rest.py | 11 +++++-- .../rest/test_send_notification.py | 33 +++++++++++++++++-- 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index d47f9f27c..233ceb065 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -205,6 +205,8 @@ def send_notification(notification_type): notification, errors = ( sms_template_notification_schema if notification_type == SMS_TYPE else email_notification_schema ).load(request.get_json()) + if errors: + raise InvalidRequest(errors, status_code=400) if all((api_user.key_type != KEY_TYPE_TEST, service.restricted)): service_stats = sum(row.count for row in dao_fetch_todays_stats_for_service(service.id)) @@ -212,13 +214,16 @@ def send_notification(notification_type): error = 'Exceeded send limits ({}) for today'.format(service.message_limit) raise InvalidRequest(error, status_code=429) - if errors: - raise InvalidRequest(errors, status_code=400) - template = templates_dao.dao_get_template_by_id_and_service_id( template_id=notification['template'], service_id=service_id ) + + if notification_type != template.template_type: + raise InvalidRequest("{0} template is not suitable for a {1} notification".format(template.template_type, + notification_type), + status_code=400) + errors = unarchived_template_schema.validate({'archived': template.archived}) if errors: raise InvalidRequest(errors, status_code=400) diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index 0b5e5eaf0..59aa8754c 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -1,16 +1,13 @@ -import uuid from datetime import datetime import random import string import pytest -from unittest.mock import ANY from flask import (json, current_app) from freezegun import freeze_time from notifications_python_client.authentication import create_jwt_token import app -from app import encryption from app.dao import notifications_dao from app.models import ApiKey, KEY_TYPE_TEAM, KEY_TYPE_TEST, Notification, NotificationHistory from app.dao.templates_dao import dao_get_all_templates_for_service, dao_update_template @@ -999,3 +996,33 @@ def test_should_not_persist_notification_or_send_sms_if_simulated_number( assert response.status_code == 201 apply_async.assert_not_called() assert Notification.query.count() == 0 + + +@pytest.mark.parametrize( + 'notification_type, template_type, to', [ + ('email', 'sms', 'notify@digital.cabinet-office.gov.uk'), + ('sms', 'email', '+447700900986') + ]) +def test_should_error_if_notification_type_does_not_match_template_type( + client, + notify_db, + notify_db_session, + template_type, + notification_type, + to +): + template = create_sample_template(notify_db, notify_db_session, template_type=template_type) + data = { + 'to': to, + 'template': template.id + } + auth_header = create_authorization_header(service_id=template.service_id) + response = client.post("/notifications/{}".format(notification_type), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 400 + json_resp = json.loads(response.get_data(as_text=True)) + assert json_resp['result'] == 'error' + assert '{0} template is not suitable for a {1} notification'.format(template_type, notification_type) \ + in json_resp['message'] From 6f7dd149e217898bde194057eb2bbc912478981c Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 23 Sep 2016 15:59:23 +0100 Subject: [PATCH 22/26] 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 From 0ef43ab1dcca43d7abc8657fbce140f7b89f3678 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 23 Sep 2016 16:03:11 +0100 Subject: [PATCH 23/26] Fix the wording on the message a little bit. --- app/notifications/rest.py | 4 ++-- tests/app/notifications/rest/test_send_notification.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 233ceb065..c960a6aef 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -220,8 +220,8 @@ def send_notification(notification_type): ) if notification_type != template.template_type: - raise InvalidRequest("{0} template is not suitable for a {1} notification".format(template.template_type, - notification_type), + raise InvalidRequest("{0} template is not suitable for {1} notification".format(template.template_type, + notification_type), status_code=400) errors = unarchived_template_schema.validate({'archived': template.archived}) diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index 59aa8754c..2d7cd43ab 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -1024,5 +1024,5 @@ def test_should_error_if_notification_type_does_not_match_template_type( assert response.status_code == 400 json_resp = json.loads(response.get_data(as_text=True)) assert json_resp['result'] == 'error' - assert '{0} template is not suitable for a {1} notification'.format(template_type, notification_type) \ + assert '{0} template is not suitable for {1} notification'.format(template_type, notification_type) \ in json_resp['message'] From fb6cb5f2364257d420460b794aebbcfed7262353 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 23 Sep 2016 16:34:13 +0100 Subject: [PATCH 24/26] add statuses filter to GET /service/{}/job can now pass in a query string `?statuses=x,y,z` to filter jobs based on `Job.job_status`. Not passing in a status or passing in an empty string is equivalent to selecting every filter type at once. --- app/dao/jobs_dao.py | 6 +++++- app/job/rest.py | 10 ++++++---- app/models.py | 9 ++++++++- tests/app/job/test_rest.py | 38 +++++++++++++++++++++++++++++++++++++- 4 files changed, 56 insertions(+), 7 deletions(-) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index 8e7d7b3c2..2074d6ab6 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -27,10 +27,14 @@ def dao_get_job_by_service_id_and_job_id(service_id, job_id): return Job.query.filter_by(service_id=service_id, id=job_id).one() -def dao_get_jobs_by_service_id(service_id, limit_days=None, page=1, page_size=50): +def dao_get_jobs_by_service_id(service_id, limit_days=None, page=1, page_size=50, statuses=''): query_filter = [Job.service_id == service_id] if limit_days is not None: query_filter.append(cast(Job.created_at, sql_date) >= days_ago(limit_days)) + if statuses != ['']: + query_filter.append( + Job.job_status.in_(statuses) + ) return Job.query \ .filter(*query_filter) \ .order_by(desc(Job.created_at)) \ diff --git a/app/job/rest.py b/app/job/rest.py index ced973dba..3a8a60219 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -102,8 +102,10 @@ def get_jobs_by_service(service_id): else: limit_days = None + statuses = [x.strip() for x in request.args.get('statuses', '').split(',')] + page = int(request.args.get('page', 1)) - return jsonify(**get_paginated_jobs(service_id, limit_days, page)) + return jsonify(**get_paginated_jobs(service_id, limit_days, statuses, page)) @job.route('', methods=['POST']) @@ -140,14 +142,14 @@ def create_job(service_id): return jsonify(data=job_json), 201 -def get_paginated_jobs(service_id, limit_days, page): +def get_paginated_jobs(service_id, limit_days, statuses, page): pagination = dao_get_jobs_by_service_id( service_id, limit_days=limit_days, page=page, - page_size=current_app.config['PAGE_SIZE'] + page_size=current_app.config['PAGE_SIZE'], + statuses=statuses ) - data = job_schema.dump(pagination.items, many=True).data for job_data in data: statistics = dao_get_notification_outcomes_for_job(service_id, job_data['id']) diff --git a/app/models.py b/app/models.py index 91294c15d..0a6457667 100644 --- a/app/models.py +++ b/app/models.py @@ -304,7 +304,14 @@ JOB_STATUS_FINISHED = 'finished' JOB_STATUS_SENDING_LIMITS_EXCEEDED = 'sending limits exceeded' JOB_STATUS_SCHEDULED = 'scheduled' JOB_STATUS_CANCELLED = 'cancelled' - +JOB_STATUS_TYPES = [ + JOB_STATUS_PENDING, + JOB_STATUS_IN_PROGRESS, + JOB_STATUS_FINISHED, + JOB_STATUS_SENDING_LIMITS_EXCEEDED, + JOB_STATUS_SCHEDULED, + JOB_STATUS_CANCELLED +] class JobStatus(db.Model): __tablename__ = 'job_status' diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index f4dc01392..20d6a4248 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -14,7 +14,7 @@ from tests.app.conftest import ( sample_notification as create_notification ) from app.dao.templates_dao import dao_update_template -from app.models import NOTIFICATION_STATUS_TYPES +from app.models import NOTIFICATION_STATUS_TYPES, JOB_STATUS_TYPES, JOB_STATUS_PENDING def test_get_job_with_invalid_service_id_returns404(notify_api, sample_api_key, sample_service): @@ -655,6 +655,42 @@ def test_get_jobs_accepts_page_parameter( assert set(resp_json['links'].keys()) == {'prev', 'next', 'last'} +@pytest.mark.parametrize('statuses_filter, expected_statuses', [ + ('', JOB_STATUS_TYPES), + ('pending', [JOB_STATUS_PENDING]), + ('pending, in progress, finished, sending limits exceeded, scheduled, cancelled', JOB_STATUS_TYPES), + # bad statuses are accepted, just return no data + ('foo', []) +]) +def test_get_jobs_can_filter_on_statuses( + notify_db, + notify_db_session, + client, + sample_service, + statuses_filter, + expected_statuses +): + create_job(notify_db, notify_db_session, job_status='pending') + create_job(notify_db, notify_db_session, job_status='in progress') + create_job(notify_db, notify_db_session, job_status='finished') + create_job(notify_db, notify_db_session, job_status='sending limits exceeded') + create_job(notify_db, notify_db_session, job_status='scheduled') + create_job(notify_db, notify_db_session, job_status='cancelled') + + path = '/service/{}/job'.format(sample_service.id) + response = client.get( + path, + headers=[create_authorization_header()], + query_string={'statuses': statuses_filter} + ) + + assert response.status_code == 200 + resp_json = json.loads(response.get_data(as_text=True)) + from pprint import pprint + pprint(resp_json) + assert {x['job_status'] for x in resp_json['data']} == set(expected_statuses) + + def create_10_jobs(db, session, service, template): with freeze_time('2015-01-01T00:00:00') as the_time: for _ in range(10): From d3c0c4840124cf624cc8f30bffacdbddffb912b3 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 23 Sep 2016 17:05:42 +0100 Subject: [PATCH 25/26] fix dao_get_jobs_by_service_id statuses now defaults to correct val --- app/dao/jobs_dao.py | 4 ++-- app/models.py | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index 2074d6ab6..41ba00940 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -27,11 +27,11 @@ def dao_get_job_by_service_id_and_job_id(service_id, job_id): return Job.query.filter_by(service_id=service_id, id=job_id).one() -def dao_get_jobs_by_service_id(service_id, limit_days=None, page=1, page_size=50, statuses=''): +def dao_get_jobs_by_service_id(service_id, limit_days=None, page=1, page_size=50, statuses=None): query_filter = [Job.service_id == service_id] if limit_days is not None: query_filter.append(cast(Job.created_at, sql_date) >= days_ago(limit_days)) - if statuses != ['']: + if statuses is not None and statuses != ['']: query_filter.append( Job.job_status.in_(statuses) ) diff --git a/app/models.py b/app/models.py index 0a6457667..d1f0eda5f 100644 --- a/app/models.py +++ b/app/models.py @@ -313,6 +313,7 @@ JOB_STATUS_TYPES = [ JOB_STATUS_CANCELLED ] + class JobStatus(db.Model): __tablename__ = 'job_status' From 36425cd0351edf6e35b4392956ae94bd2ebce2b5 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 27 Sep 2016 12:39:30 +0100 Subject: [PATCH 26/26] Removed travis image --- README.md | 1 - 1 file changed, 1 deletion(-) diff --git a/README.md b/README.md index 5c6143b4d..dc29d8fb1 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,3 @@ -![](https://travis-ci.org/alphagov/notifications-api.svg) [![Requirements Status](https://requires.io/github/alphagov/notifications-api/requirements.svg?branch=master)](https://requires.io/github/alphagov/notifications-api/requirements/?branch=master) # notifications-api