From 60f63a30dbdd6725fcfd5d647f0b17d8783ea9cb Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 13 Feb 2017 14:27:32 +0000 Subject: [PATCH 01/10] Add dao method to get count of slow delivery notifications by provider --- app/dao/notifications_dao.py | 32 ++++ tests/app/conftest.py | 41 +++-- tests/app/dao/test_notification_dao.py | 230 ++++++++++++++++++++++++- 3 files changed, 284 insertions(+), 19 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 13317afd7..13fcee367 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -11,13 +11,16 @@ from sqlalchemy.orm import joinedload from app import db, create_uuid from app.dao import days_ago +from app.dao.provider_details_dao import get_provider_details_by_identifier from app.models import ( Service, Notification, NotificationHistory, NotificationStatistics, + ProviderDetails, Template, NOTIFICATION_CREATED, + NOTIFICATION_DELIVERED, NOTIFICATION_SENDING, NOTIFICATION_PENDING, NOTIFICATION_TECHNICAL_FAILURE, @@ -421,3 +424,32 @@ def get_total_sent_notifications_in_date_range(start_date, end_date, notificatio ).scalar() return result or 0 + + +def get_count_of_slow_delivery_sms_notifications_for_provider( + created_at, + sent_at, + provider, + threshold, + delivery_time, + service_id, + template_id +): + count = db.session.query( + func.count().label('total'), + Notification.sent_by + ).filter( + Notification.service_id == service_id, + Notification.template_id == template_id, + Notification.created_at >= created_at, + Notification.sent_at >= sent_at, + Notification.status == NOTIFICATION_DELIVERED, + Notification.sent_by == provider, + (Notification.updated_at - Notification.sent_at) >= delivery_time, + ).group_by( + Notification.sent_by + ).having( + func.count().label('total') >= threshold, + ).first() + + return count diff --git a/tests/app/conftest.py b/tests/app/conftest.py index d58cfef70..ea7319cce 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -421,29 +421,34 @@ def sample_notification_with_job( @pytest.fixture(scope='function') -def sample_notification(notify_db, - notify_db_session, - service=None, - template=None, - job=None, - job_row_number=None, - to_field=None, - status='created', - reference=None, - created_at=None, - sent_at=None, - billable_units=1, - personalisation=None, - api_key_id=None, - key_type=KEY_TYPE_NORMAL, - sent_by=None, - client_reference=None): +def sample_notification( + notify_db, + notify_db_session, + service=None, + template=None, + job=None, + job_row_number=None, + to_field=None, + status='created', + reference=None, + created_at=None, + sent_at=None, + billable_units=1, + personalisation=None, + api_key_id=None, + key_type=KEY_TYPE_NORMAL, + sent_by=None, + client_reference=None, + updated_at=None +): if created_at is None: created_at = datetime.utcnow() if service is None: service = sample_service(notify_db, notify_db_session) if template is None: template = sample_template(notify_db, notify_db_session, service=service) + if not updated_at and status in NOTIFICATION_STATUS_TYPES_COMPLETED: + updated_at = created_at notification_id = uuid.uuid4() @@ -472,7 +477,7 @@ def sample_notification(notify_db, 'api_key_id': api_key_id, 'key_type': key_type, 'sent_by': sent_by, - 'updated_at': created_at if status in NOTIFICATION_STATUS_TYPES_COMPLETED else None, + 'updated_at': updated_at, 'client_reference': client_reference } if job_row_number: diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 83e8d1132..d9aa249a4 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -40,7 +40,9 @@ from app.dao.notifications_dao import ( dao_delete_notifications_and_history_by_id, dao_timeout_notifications, get_financial_year, - get_april_fools) + get_april_fools, + get_count_of_slow_delivery_sms_notifications_for_provider +) from app.dao.services_dao import dao_update_service from tests.app.conftest import ( @@ -1422,3 +1424,229 @@ def test_get_total_sent_notifications_for_email_excludes_sms_counts( total_count = get_total_sent_notifications_in_date_range(start_date, end_date, 'email') assert total_count == 2 + + +@freeze_time("2016-01-10 12:00:00.000000") +def test_get_count_of_slow_delivery_sms_notifications_returns_after_created_time( + notify_db, + notify_db_session, + sample_template +): + now = datetime.utcnow() + a_second_ago = now - timedelta(seconds=1) + five_minutes_from_now = now + timedelta(minutes=5) + + notification_five_minutes_to_deliver = partial( + sample_notification, + notify_db, + notify_db_session, + template=sample_template, + status='delivered', + sent_at=now, + sent_by='mmg', + updated_at=five_minutes_from_now + ) + + noti1 = notification_five_minutes_to_deliver(created_at=a_second_ago) + notification_five_minutes_to_deliver(created_at=a_second_ago) + notification_five_minutes_to_deliver(created_at=now) + + count = get_count_of_slow_delivery_sms_notifications_for_provider( + created_at=now, + sent_at=now, + provider='mmg', + threshold=1, + delivery_time=timedelta(minutes=5), + service_id=noti1.service_id, + template_id=noti1.template_id + ) + + assert count.total == 1 + + +@freeze_time("2016-01-10 12:00:00.000000") +def test_get_count_of_slow_delivery_sms_notifications_returns_after_sent_time( + notify_db, + notify_db_session, + sample_template +): + now = datetime.utcnow() + one_minute_from_now = now + timedelta(minutes=1) + five_minutes_from_now = now + timedelta(minutes=5) + + notification_five_minutes_to_deliver = partial( + sample_notification, + notify_db, + notify_db_session, + template=sample_template, + status='delivered', + sent_by='mmg', + updated_at=five_minutes_from_now + ) + + noti1 = notification_five_minutes_to_deliver(created_at=now, sent_at=now) + notification_five_minutes_to_deliver(created_at=now, sent_at=one_minute_from_now) + notification_five_minutes_to_deliver(created_at=now, sent_at=one_minute_from_now) + + count = get_count_of_slow_delivery_sms_notifications_for_provider( + created_at=now, + sent_at=one_minute_from_now, + provider='mmg', + threshold=2, + delivery_time=timedelta(minutes=3), + service_id=noti1.service_id, + template_id=noti1.template_id + ) + + assert count.total == 2 + + +@freeze_time("2016-01-10 12:00:00.000000") +def test_get_count_of_slow_delivery_sms_notifications_observes_threshold( + notify_db, + notify_db_session, + sample_template +): + now = datetime.utcnow() + five_minutes_from_now = now + timedelta(minutes=5) + + notification_five_minutes_to_deliver = partial( + sample_notification, + notify_db, + notify_db_session, + template=sample_template, + status='delivered', + sent_at=now, + sent_by='mmg', + updated_at=five_minutes_from_now + ) + + noti1 = notification_five_minutes_to_deliver(created_at=now) + notification_five_minutes_to_deliver(created_at=now) + + count = get_count_of_slow_delivery_sms_notifications_for_provider( + created_at=now, + sent_at=now, + provider='mmg', + threshold=3, + delivery_time=timedelta(minutes=5), + service_id=noti1.service_id, + template_id=noti1.template_id + ) + + assert not count + + +@freeze_time("2016-01-10 12:00:00.000000") +def test_get_count_of_slow_delivery_sms_notifications_returns_status_delivered_only( + notify_db, + notify_db_session, + sample_template +): + now = datetime.utcnow() + five_minutes_from_now = now + timedelta(minutes=5) + + notification_five_minutes_to_deliver = partial( + sample_notification, + notify_db, + notify_db_session, + template=sample_template, + sent_at=now, + sent_by='firetext', + created_at=now, + updated_at=five_minutes_from_now + ) + + noti1 = notification_five_minutes_to_deliver(status='created') + notification_five_minutes_to_deliver(status='sending') + notification_five_minutes_to_deliver(status='delivered') + notification_five_minutes_to_deliver(status='delivered') + + count = get_count_of_slow_delivery_sms_notifications_for_provider( + created_at=now, + sent_at=now, + provider='firetext', + threshold=1, + delivery_time=timedelta(minutes=5), + service_id=noti1.service_id, + template_id=noti1.template_id + ) + + assert count.total == 2 + + +@freeze_time("2016-01-10 12:00:00.000000") +def test_get_count_of_slow_delivery_sms_notifications_returns_slow_delivery_time_only( + notify_db, + notify_db_session, + sample_template +): + now = datetime.utcnow() + five_minutes_from_now = now + timedelta(minutes=5) + + notification = partial( + sample_notification, + notify_db, + notify_db_session, + template=sample_template, + created_at=now, + sent_at=now, + sent_by='mmg', + status='delivered' + ) + + noti1 = notification(updated_at=five_minutes_from_now - timedelta(seconds=1)) + noti1 = notification(updated_at=five_minutes_from_now - timedelta(seconds=1)) + notification(updated_at=five_minutes_from_now) + notification(updated_at=five_minutes_from_now) + notification(updated_at=five_minutes_from_now) + + count = get_count_of_slow_delivery_sms_notifications_for_provider( + created_at=now, + sent_at=now, + provider='mmg', + threshold=1, + delivery_time=timedelta(minutes=5), + service_id=noti1.service_id, + template_id=noti1.template_id + ) + + assert count.total == 3 + + +@freeze_time("2016-01-10 12:00:00.000000") +def test_get_count_of_slow_delivery_sms_notifications_returns_provider_only( + notify_db, + notify_db_session, + sample_template +): + now = datetime.utcnow() + five_minutes_from_now = now + timedelta(minutes=5) + + notification = partial( + sample_notification, + notify_db, + notify_db_session, + template=sample_template, + created_at=now, + sent_at=now, + updated_at=five_minutes_from_now, + status='delivered' + ) + + noti1 = notification(sent_by='mmg') + notification(sent_by='firetext') + notification(sent_by='loadtesting') + notification(sent_by='loadtesting') + + count = get_count_of_slow_delivery_sms_notifications_for_provider( + created_at=now, + sent_at=now, + provider='mmg', + threshold=1, + delivery_time=timedelta(minutes=5), + service_id=noti1.service_id, + template_id=noti1.template_id + ) + + assert count.sent_by == 'mmg' From d058626119ad2d0cc0b7941ce029f697411ca285 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 13 Feb 2017 15:05:39 +0000 Subject: [PATCH 02/10] Add task to run every minute that will switch provider on slow delivery notifications --- app/celery/scheduled_tasks.py | 45 +++++++++++++++++++++++++++++++++-- app/config.py | 5 ++++ 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 921f12fdc..1afe1aed9 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -1,4 +1,7 @@ -from datetime import datetime +from datetime import ( + datetime, + timedelta +) from flask import current_app from sqlalchemy.exc import SQLAlchemyError @@ -10,7 +13,12 @@ from app.dao.invited_user_dao import delete_invitations_created_more_than_two_da from app.dao.jobs_dao import dao_set_scheduled_jobs_to_pending, dao_get_jobs_older_than from app.dao.notifications_dao import ( delete_notifications_created_more_than_a_week_ago, - dao_timeout_notifications + dao_timeout_notifications, + get_count_of_slow_delivery_sms_notifications_for_provider +) +from app.dao.provider_details_dao import ( + get_current_provider, + dao_toggle_sms_provider ) from app.dao.users_dao import delete_codes_older_created_more_than_a_day_ago from app.statsd_decorators import statsd @@ -141,3 +149,36 @@ def send_daily_performance_platform_stats(): email_sent_count, 'day' ) + + +@notify_celery.task(name='switch-current-sms-provider-on-slow-delivery') +@statsd(namespace="tasks") +def switch_current_sms_provider_on_slow_delivery(): + """ + Switch providers if there are at least two slow delivery notifications (more than four minutes) + in the last ten minutes. Search from the time we last switched to the current provider. + """ + functional_test_provider_service_id = current_app.config.get('FUNCTIONAL_TEST_PROVIDER_SERVICE_ID') + functional_test_provider_template_id = current_app.config.get('FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID') + + if functional_test_provider_service_id and functional_test_provider_template_id: + current_provider = get_current_provider('sms') + slow_delivery_notifications = get_count_of_slow_delivery_sms_notifications_for_provider( + provider=current_provider.identifier, + threshold=2, + created_at=current_provider.updated_at, + sent_at=datetime.utcnow() - timedelta(minutes=10), + delivery_time=timedelta(minutes=4), + service_id=functional_test_provider_service_id, + template_id=functional_test_provider_template_id + ) + + if slow_delivery_notifications: + current_app.logger.warning( + '{} slow delivery notifications detected for provider {}'.format( + slow_delivery_notifications.total, + slow_delivery_notifications.sent_by + ) + ) + + dao_toggle_sms_provider(current_provider.identifier) diff --git a/app/config.py b/app/config.py index a5e7c8240..303e18943 100644 --- a/app/config.py +++ b/app/config.py @@ -131,6 +131,11 @@ class Config(object): 'schedule': crontab(minute=30, hour=0), # 00:30 'options': {'queue': 'periodic'} }, + 'switch-current-sms-provider-on-slow-delivery': { + 'task': 'switch-current-sms-provider-on-slow-delivery', + 'schedule': crontab(), # Every minute + 'options': {'queue': 'periodic'} + }, 'timeout-sending-notifications': { 'task': 'timeout-sending-notifications', 'schedule': crontab(minute=0, hour='0,1,2'), From 73d5ce4f8bb35987ea8f5cccd183bb9a3278c71e Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 13 Feb 2017 15:46:06 +0000 Subject: [PATCH 03/10] Add tests to verify correctness of the switching provider task --- app/dao/services_dao.py | 4 +- app/dao/templates_dao.py | 4 +- tests/app/celery/test_scheduled_tasks.py | 173 ++++++++++++++++++++++- tests/app/conftest.py | 77 +++++----- tests/conftest.py | 14 ++ 5 files changed, 232 insertions(+), 40 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 554b3675f..7868ef86a 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -110,11 +110,11 @@ def dao_fetch_service_by_id_and_user(service_id, user_id): @transactional @version_class(Service) -def dao_create_service(service, user): +def dao_create_service(service, user, service_id=None): from app.dao.permissions_dao import permission_dao service.users.append(user) permission_dao.add_default_service_permissions_for_user(user, service) - service.id = uuid.uuid4() # must be set now so version history model can use same id + service.id = service_id or uuid.uuid4() # must be set now so version history model can use same id service.active = True service.research_mode = False db.session.add(service) diff --git a/app/dao/templates_dao.py b/app/dao/templates_dao.py index 567e763f3..096b48887 100644 --- a/app/dao/templates_dao.py +++ b/app/dao/templates_dao.py @@ -13,8 +13,8 @@ from app.dao.dao_utils import ( @transactional @version_class(Template, TemplateHistory) -def dao_create_template(template): - template.id = uuid.uuid4() # must be set now so version history model can use same id +def dao_create_template(template, template_id=None): + template.id = template_id or uuid.uuid4() # must be set now so version history model can use same id template.archived = False db.session.add(template) diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 4f7b44288..4bc6d5f58 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -1,3 +1,5 @@ +import pytest + from datetime import datetime, timedelta from functools import partial @@ -13,19 +15,63 @@ from app.celery.scheduled_tasks import ( delete_invitations, timeout_notifications, run_scheduled_jobs, - send_daily_performance_platform_stats + send_daily_performance_platform_stats, + switch_current_sms_provider_on_slow_delivery ) from app.clients.performance_platform.performance_platform_client import PerformancePlatformClient from app.dao.jobs_dao import dao_get_job_by_id +from app.dao.provider_details_dao import ( + dao_update_provider_details, + get_current_provider +) from app.utils import get_london_midnight_in_utc from tests.app.conftest import ( sample_notification as create_sample_notification, sample_job as create_sample_job, - sample_notification_history as create_notification_history + sample_notification_history as create_notification_history, + sample_service as create_sample_service, + sample_template as create_sample_template ) +from tests.conftest import set_config_values from unittest.mock import call, patch, PropertyMock +def _create_slow_delivery_notification(notify_db, notify_db_session, provider='mmg'): + now = datetime.utcnow() + five_minutes_from_now = now + timedelta(minutes=5) + + service = create_sample_service( + notify_db, + notify_db_session, + service_id=current_app.config.get('FUNCTIONAL_TEST_PROVIDER_SERVICE_ID') + ) + + template = create_sample_template( + notify_db, + notify_db_session, + template_id=current_app.config.get('FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID'), + service=service + ) + + create_sample_notification( + notify_db, + notify_db_session, + service=service, + template=template, + status='delivered', + created_at=now, + sent_at=now, + sent_by=provider, + updated_at=five_minutes_from_now + ) + + +@pytest.fixture(scope='function') +def set_provider_updated_at(current_sms_provider): + current_sms_provider.updated_at = datetime.utcnow() - timedelta(minutes=30) + dao_update_provider_details(current_sms_provider) + + def test_should_have_decorated_tasks_functions(): assert delete_verify_codes.__wrapped__.__name__ == 'delete_verify_codes' assert delete_successful_notifications.__wrapped__.__name__ == 'delete_successful_notifications' @@ -35,6 +81,8 @@ def test_should_have_decorated_tasks_functions(): assert run_scheduled_jobs.__wrapped__.__name__ == 'run_scheduled_jobs' assert remove_csv_files.__wrapped__.__name__ == 'remove_csv_files' assert send_daily_performance_platform_stats.__wrapped__.__name__ == 'send_daily_performance_platform_stats' + assert switch_current_sms_provider_on_slow_delivery.__wrapped__.__name__ == \ + 'switch_current_sms_provider_on_slow_delivery' def test_should_call_delete_successful_notifications_more_than_week_in_task(notify_api, mocker): @@ -244,3 +292,124 @@ def test_send_daily_performance_stats_calls_with_correct_totals( call(get_london_midnight_in_utc(yesterday), 'sms', 2, 'day'), call(get_london_midnight_in_utc(yesterday), 'email', 3, 'day') ]) + + +def test_switch_current_sms_provider_on_slow_delivery_does_not_run_if_config_unset(client, mocker): + get_notifications_mock = mocker.patch( + 'app.celery.scheduled_tasks.get_count_of_slow_delivery_sms_notifications_for_provider' + ) + toggle_sms_mock = mocker.patch('app.celery.scheduled_tasks.dao_toggle_sms_provider') + + with set_config_values(client.application, { + 'FUNCTIONAL_TEST_PROVIDER_SERVICE_ID': None, + 'FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID': None + }): + switch_current_sms_provider_on_slow_delivery() + + assert get_notifications_mock.called is False + assert toggle_sms_mock.called is False + + +def test_switch_providers_on_slow_delivery_runs_if_config_set( + notify_db, + notify_db_session, + notify_api, + mocker +): + get_notifications_mock = mocker.patch( + 'app.celery.scheduled_tasks.get_count_of_slow_delivery_sms_notifications_for_provider', + return_value=[] + ) + + with set_config_values(notify_api, { + 'FUNCTIONAL_TEST_PROVIDER_SERVICE_ID': '7954469d-8c6d-43dc-b8f7-86be2d69f5f3', + 'FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID': '331a63e6-f1aa-4588-ad3f-96c268788ae7' + }): + switch_current_sms_provider_on_slow_delivery() + + assert get_notifications_mock.called is True + + +def test_switch_providers_on_slow_delivery_triggers_switch_on_slow_notification_delivery( + notify_db, + notify_db_session, + notify_api, + restore_provider_details, + current_sms_provider, + set_provider_updated_at +): + with set_config_values(notify_api, { + 'FUNCTIONAL_TEST_PROVIDER_SERVICE_ID': '7954469d-8c6d-43dc-b8f7-86be2d69f5f3', + 'FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID': '331a63e6-f1aa-4588-ad3f-96c268788ae7' + }): + _create_slow_delivery_notification(notify_db, notify_db_session, current_sms_provider.identifier) + _create_slow_delivery_notification(notify_db, notify_db_session, current_sms_provider.identifier) + + switch_current_sms_provider_on_slow_delivery() + + new_provider = get_current_provider('sms') + assert new_provider.identifier != current_sms_provider.identifier + assert new_provider.priority < current_sms_provider.priority + + +def test_switch_providers_on_slow_delivery_does_not_switch_if_already_switched( + notify_db, + notify_db_session, + notify_api, + restore_provider_details, + current_sms_provider, + set_provider_updated_at +): + with set_config_values(notify_api, { + 'FUNCTIONAL_TEST_PROVIDER_SERVICE_ID': '7954469d-8c6d-43dc-b8f7-86be2d69f5f3', + 'FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID': '331a63e6-f1aa-4588-ad3f-96c268788ae7' + }): + _create_slow_delivery_notification(notify_db, notify_db_session) + _create_slow_delivery_notification(notify_db, notify_db_session) + + switch_current_sms_provider_on_slow_delivery() + switch_current_sms_provider_on_slow_delivery() + + new_provider = get_current_provider('sms') + assert new_provider.identifier != current_sms_provider.identifier + assert new_provider.priority < current_sms_provider.priority + + +def test_switch_providers_on_slow_delivery_does_not_switch_based_on_older_notifications( + notify_db, + notify_db_session, + notify_api, + restore_provider_details, + current_sms_provider, + set_provider_updated_at +): + """ + Assume we have three slow delivery notifications for the current provider x. This triggers + a switch to provider y. If we experience some slow delivery notifications on this provider, + we switch back to provider x. + + Provider x had three slow deliveries initially, but we do not want to trigger another switch + based on these as they are old. We only want to look for slow notifications after the point at + which we switched back to provider x. + """ + with set_config_values(notify_api, { + 'FUNCTIONAL_TEST_PROVIDER_SERVICE_ID': '7954469d-8c6d-43dc-b8f7-86be2d69f5f3', + 'FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID': '331a63e6-f1aa-4588-ad3f-96c268788ae7' + }): + # Provider x -> y + _create_slow_delivery_notification(notify_db, notify_db_session, current_sms_provider.identifier) + _create_slow_delivery_notification(notify_db, notify_db_session, current_sms_provider.identifier) + _create_slow_delivery_notification(notify_db, notify_db_session, current_sms_provider.identifier) + switch_current_sms_provider_on_slow_delivery() + + current_provider = get_current_provider('sms') + + # Provider y -> x + _create_slow_delivery_notification(notify_db, notify_db_session, current_provider.identifier) + _create_slow_delivery_notification(notify_db, notify_db_session, current_provider.identifier) + switch_current_sms_provider_on_slow_delivery() + + # Expect to stay on provider x + switch_current_sms_provider_on_slow_delivery() + current_provider = get_current_provider('sms') + assert current_sms_provider.identifier == current_provider.identifier diff --git a/tests/app/conftest.py b/tests/app/conftest.py index ea7319cce..c946153ec 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -117,13 +117,16 @@ def sample_sms_code(notify_db, @pytest.fixture(scope='function') -def sample_service(notify_db, - notify_db_session, - service_name="Sample service", - user=None, - restricted=False, - limit=1000, - email_from=None): +def sample_service( + notify_db, + notify_db_session, + service_name="Sample service", + user=None, + restricted=False, + limit=1000, + email_from=None, + service_id=None +): if user is None: user = create_user() if email_from is None: @@ -138,7 +141,7 @@ def sample_service(notify_db, service = Service.query.filter_by(name=service_name).first() if not service: service = Service(**data) - dao_create_service(service, user) + dao_create_service(service, user, service_id) else: if user not in service.users: dao_add_user_to_service(service, user) @@ -146,38 +149,44 @@ def sample_service(notify_db, @pytest.fixture(scope='function') -def sample_template(notify_db, - notify_db_session, - template_name="Template Name", - template_type="sms", - content="This is a template:\nwith a newline", - archived=False, - subject_line='Subject', - user=None, - service=None, - created_by=None, - process_type='normal'): +def sample_template( + notify_db, + notify_db_session, + template_name="Template Name", + template_type="sms", + content="This is a template:\nwith a newline", + archived=False, + subject_line='Subject', + user=None, + service=None, + created_by=None, + process_type='normal', + template_id=None +): if user is None: user = create_user() if service is None: service = sample_service(notify_db, notify_db_session) if created_by is None: created_by = create_user() - data = { - 'name': template_name, - 'template_type': template_type, - 'content': content, - 'service': service, - 'created_by': created_by, - 'archived': archived, - 'process_type': process_type - } - if template_type in ['email', 'letter']: - data.update({ - 'subject': subject_line - }) - template = Template(**data) - dao_create_template(template) + + template = Template.query.filter_by(id=template_id).first() + if not template: + data = { + 'name': template_name, + 'template_type': template_type, + 'content': content, + 'service': service, + 'created_by': created_by, + 'archived': archived, + 'process_type': process_type + } + if template_type in ['email', 'letter']: + data.update({ + 'subject': subject_line + }) + template = Template(**data) + dao_create_template(template, template_id) return template diff --git a/tests/conftest.py b/tests/conftest.py index 047e0cd35..41fec2d91 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -113,3 +113,17 @@ def set_config(app, name, value): app.config[name] = value yield app.config[name] = old_val + + +@contextmanager +def set_config_values(app, dict): + old_values = {} + + for key in dict: + old_values[key] = app.config.get(key) + app.config[key] = dict[key] + + yield + + for key in dict: + app.config[key] = old_values[key] From ec65575f5024d86ef35e2c3dfdd07dd80e4fe5fd Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 13 Feb 2017 15:51:01 +0000 Subject: [PATCH 04/10] Set provider switch task to run on prod according to functional tests service --- app/config.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/config.py b/app/config.py index 303e18943..13b4c9063 100644 --- a/app/config.py +++ b/app/config.py @@ -171,6 +171,9 @@ class Config(object): SIMULATED_SMS_NUMBERS = ('+447700900000', '+447700900111', '+447700900222') + FUNCTIONAL_TEST_PROVIDER_SERVICE_ID = None + FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID = None + ###################### # Config overrides ### @@ -236,6 +239,8 @@ class Live(Config): CSV_UPLOAD_BUCKET_NAME = 'live-notifications-csv-upload' STATSD_ENABLED = True FROM_NUMBER = '40604' + FUNCTIONAL_TEST_PROVIDER_SERVICE_ID = '6c1d81bb-dae2-4ee9-80b0-89a4aae9f649' + FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID = '285bb62d-08e7-414b-b5a3-6372ba320e06' class CloudFoundryConfig(Config): From eafe8269ef94446b77397689ff3e75ff08ceb491 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Fri, 24 Feb 2017 13:39:58 +0000 Subject: [PATCH 05/10] Simplify dao method and update tests and fixtures --- app/dao/notifications_dao.py | 20 +-- tests/app/conftest.py | 48 +++---- tests/app/dao/test_notification_dao.py | 167 ++++++------------------- tests/app/db.py | 19 ++- 4 files changed, 73 insertions(+), 181 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 13fcee367..bc05a5509 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -11,13 +11,11 @@ from sqlalchemy.orm import joinedload from app import db, create_uuid from app.dao import days_ago -from app.dao.provider_details_dao import get_provider_details_by_identifier from app.models import ( Service, Notification, NotificationHistory, NotificationStatistics, - ProviderDetails, Template, NOTIFICATION_CREATED, NOTIFICATION_DELIVERED, @@ -426,8 +424,7 @@ def get_total_sent_notifications_in_date_range(start_date, end_date, notificatio return result or 0 -def get_count_of_slow_delivery_sms_notifications_for_provider( - created_at, +def is_delivery_slow_for_provider( sent_at, provider, threshold, @@ -435,21 +432,12 @@ def get_count_of_slow_delivery_sms_notifications_for_provider( service_id, template_id ): - count = db.session.query( - func.count().label('total'), - Notification.sent_by - ).filter( + count = db.session.query(Notification).filter( Notification.service_id == service_id, Notification.template_id == template_id, - Notification.created_at >= created_at, Notification.sent_at >= sent_at, Notification.status == NOTIFICATION_DELIVERED, Notification.sent_by == provider, (Notification.updated_at - Notification.sent_at) >= delivery_time, - ).group_by( - Notification.sent_by - ).having( - func.count().label('total') >= threshold, - ).first() - - return count + ).count() + return count >= threshold diff --git a/tests/app/conftest.py b/tests/app/conftest.py index c946153ec..23e7bcede 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -124,8 +124,7 @@ def sample_service( user=None, restricted=False, limit=1000, - email_from=None, - service_id=None + email_from=None ): if user is None: user = create_user() @@ -141,7 +140,7 @@ def sample_service( service = Service.query.filter_by(name=service_name).first() if not service: service = Service(**data) - dao_create_service(service, user, service_id) + dao_create_service(service, user) else: if user not in service.users: dao_add_user_to_service(service, user) @@ -160,8 +159,7 @@ def sample_template( user=None, service=None, created_by=None, - process_type='normal', - template_id=None + process_type='normal' ): if user is None: user = create_user() @@ -169,24 +167,21 @@ def sample_template( service = sample_service(notify_db, notify_db_session) if created_by is None: created_by = create_user() - - template = Template.query.filter_by(id=template_id).first() - if not template: - data = { - 'name': template_name, - 'template_type': template_type, - 'content': content, - 'service': service, - 'created_by': created_by, - 'archived': archived, - 'process_type': process_type - } - if template_type in ['email', 'letter']: - data.update({ - 'subject': subject_line - }) - template = Template(**data) - dao_create_template(template, template_id) + data = { + 'name': template_name, + 'template_type': template_type, + 'content': content, + 'service': service, + 'created_by': created_by, + 'archived': archived, + 'process_type': process_type + } + if template_type in ['email', 'letter']: + data.update({ + 'subject': subject_line + }) + template = Template(**data) + dao_create_template(template) return template @@ -447,8 +442,7 @@ def sample_notification( api_key_id=None, key_type=KEY_TYPE_NORMAL, sent_by=None, - client_reference=None, - updated_at=None + client_reference=None ): if created_at is None: created_at = datetime.utcnow() @@ -456,8 +450,6 @@ def sample_notification( service = sample_service(notify_db, notify_db_session) if template is None: template = sample_template(notify_db, notify_db_session, service=service) - if not updated_at and status in NOTIFICATION_STATUS_TYPES_COMPLETED: - updated_at = created_at notification_id = uuid.uuid4() @@ -486,7 +478,7 @@ def sample_notification( 'api_key_id': api_key_id, 'key_type': key_type, 'sent_by': sent_by, - 'updated_at': updated_at, + 'updated_at': created_at if status in NOTIFICATION_STATUS_TYPES_COMPLETED else None, 'client_reference': client_reference } if job_row_number: diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index d9aa249a4..bdcef6dcd 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -41,10 +41,11 @@ from app.dao.notifications_dao import ( dao_timeout_notifications, get_financial_year, get_april_fools, - get_count_of_slow_delivery_sms_notifications_for_provider + is_delivery_slow_for_provider ) from app.dao.services_dao import dao_update_service +from tests.app.db import create_notification from tests.app.conftest import ( sample_notification, sample_template, @@ -1427,47 +1428,7 @@ def test_get_total_sent_notifications_for_email_excludes_sms_counts( @freeze_time("2016-01-10 12:00:00.000000") -def test_get_count_of_slow_delivery_sms_notifications_returns_after_created_time( - notify_db, - notify_db_session, - sample_template -): - now = datetime.utcnow() - a_second_ago = now - timedelta(seconds=1) - five_minutes_from_now = now + timedelta(minutes=5) - - notification_five_minutes_to_deliver = partial( - sample_notification, - notify_db, - notify_db_session, - template=sample_template, - status='delivered', - sent_at=now, - sent_by='mmg', - updated_at=five_minutes_from_now - ) - - noti1 = notification_five_minutes_to_deliver(created_at=a_second_ago) - notification_five_minutes_to_deliver(created_at=a_second_ago) - notification_five_minutes_to_deliver(created_at=now) - - count = get_count_of_slow_delivery_sms_notifications_for_provider( - created_at=now, - sent_at=now, - provider='mmg', - threshold=1, - delivery_time=timedelta(minutes=5), - service_id=noti1.service_id, - template_id=noti1.template_id - ) - - assert count.total == 1 - - -@freeze_time("2016-01-10 12:00:00.000000") -def test_get_count_of_slow_delivery_sms_notifications_returns_after_sent_time( - notify_db, - notify_db_session, +def test_slow_provider_delivery_returns_for_sent_notifications( sample_template ): now = datetime.utcnow() @@ -1475,45 +1436,38 @@ def test_get_count_of_slow_delivery_sms_notifications_returns_after_sent_time( five_minutes_from_now = now + timedelta(minutes=5) notification_five_minutes_to_deliver = partial( - sample_notification, - notify_db, - notify_db_session, + create_notification, template=sample_template, status='delivered', sent_by='mmg', updated_at=five_minutes_from_now ) - noti1 = notification_five_minutes_to_deliver(created_at=now, sent_at=now) - notification_five_minutes_to_deliver(created_at=now, sent_at=one_minute_from_now) - notification_five_minutes_to_deliver(created_at=now, sent_at=one_minute_from_now) + notification_five_minutes_to_deliver(sent_at=now) + notification_five_minutes_to_deliver(sent_at=one_minute_from_now) + notification_five_minutes_to_deliver(sent_at=one_minute_from_now) - count = get_count_of_slow_delivery_sms_notifications_for_provider( - created_at=now, + slow_delivery = is_delivery_slow_for_provider( sent_at=one_minute_from_now, provider='mmg', threshold=2, delivery_time=timedelta(minutes=3), - service_id=noti1.service_id, - template_id=noti1.template_id + service_id=sample_template.service.id, + template_id=sample_template.id ) - assert count.total == 2 + assert slow_delivery @freeze_time("2016-01-10 12:00:00.000000") -def test_get_count_of_slow_delivery_sms_notifications_observes_threshold( - notify_db, - notify_db_session, +def test_slow_provider_delivery_observes_threshold( sample_template ): now = datetime.utcnow() five_minutes_from_now = now + timedelta(minutes=5) notification_five_minutes_to_deliver = partial( - sample_notification, - notify_db, - notify_db_session, + create_notification, template=sample_template, status='delivered', sent_at=now, @@ -1521,35 +1475,30 @@ def test_get_count_of_slow_delivery_sms_notifications_observes_threshold( updated_at=five_minutes_from_now ) - noti1 = notification_five_minutes_to_deliver(created_at=now) - notification_five_minutes_to_deliver(created_at=now) + notification_five_minutes_to_deliver() + notification_five_minutes_to_deliver() - count = get_count_of_slow_delivery_sms_notifications_for_provider( - created_at=now, + slow_delivery = is_delivery_slow_for_provider( sent_at=now, provider='mmg', threshold=3, delivery_time=timedelta(minutes=5), - service_id=noti1.service_id, - template_id=noti1.template_id + service_id=sample_template.service.id, + template_id=sample_template.id ) - assert not count + assert not slow_delivery @freeze_time("2016-01-10 12:00:00.000000") -def test_get_count_of_slow_delivery_sms_notifications_returns_status_delivered_only( - notify_db, - notify_db_session, +def test_slow_provider_delivery_returns_for_delivered_notifications_only( sample_template ): now = datetime.utcnow() five_minutes_from_now = now + timedelta(minutes=5) notification_five_minutes_to_deliver = partial( - sample_notification, - notify_db, - notify_db_session, + create_notification, template=sample_template, sent_at=now, sent_by='firetext', @@ -1557,37 +1506,32 @@ def test_get_count_of_slow_delivery_sms_notifications_returns_status_delivered_o updated_at=five_minutes_from_now ) - noti1 = notification_five_minutes_to_deliver(status='created') + notification_five_minutes_to_deliver(status='created') notification_five_minutes_to_deliver(status='sending') notification_five_minutes_to_deliver(status='delivered') notification_five_minutes_to_deliver(status='delivered') - count = get_count_of_slow_delivery_sms_notifications_for_provider( - created_at=now, + slow_delivery = is_delivery_slow_for_provider( sent_at=now, provider='firetext', threshold=1, delivery_time=timedelta(minutes=5), - service_id=noti1.service_id, - template_id=noti1.template_id + service_id=sample_template.service.id, + template_id=sample_template.id ) - assert count.total == 2 + assert slow_delivery @freeze_time("2016-01-10 12:00:00.000000") -def test_get_count_of_slow_delivery_sms_notifications_returns_slow_delivery_time_only( - notify_db, - notify_db_session, +def test_slow_provider_delivery_does_not_return_for_standard_delivery_time( sample_template ): now = datetime.utcnow() five_minutes_from_now = now + timedelta(minutes=5) notification = partial( - sample_notification, - notify_db, - notify_db_session, + create_notification, template=sample_template, created_at=now, sent_at=now, @@ -1595,58 +1539,17 @@ def test_get_count_of_slow_delivery_sms_notifications_returns_slow_delivery_time status='delivered' ) - noti1 = notification(updated_at=five_minutes_from_now - timedelta(seconds=1)) - noti1 = notification(updated_at=five_minutes_from_now - timedelta(seconds=1)) - notification(updated_at=five_minutes_from_now) - notification(updated_at=five_minutes_from_now) + notification(updated_at=five_minutes_from_now - timedelta(seconds=1)) + notification(updated_at=five_minutes_from_now - timedelta(seconds=1)) notification(updated_at=five_minutes_from_now) - count = get_count_of_slow_delivery_sms_notifications_for_provider( - created_at=now, + slow_delivery = is_delivery_slow_for_provider( sent_at=now, provider='mmg', - threshold=1, + threshold=2, delivery_time=timedelta(minutes=5), - service_id=noti1.service_id, - template_id=noti1.template_id + service_id=sample_template.service.id, + template_id=sample_template.id ) - assert count.total == 3 - - -@freeze_time("2016-01-10 12:00:00.000000") -def test_get_count_of_slow_delivery_sms_notifications_returns_provider_only( - notify_db, - notify_db_session, - sample_template -): - now = datetime.utcnow() - five_minutes_from_now = now + timedelta(minutes=5) - - notification = partial( - sample_notification, - notify_db, - notify_db_session, - template=sample_template, - created_at=now, - sent_at=now, - updated_at=five_minutes_from_now, - status='delivered' - ) - - noti1 = notification(sent_by='mmg') - notification(sent_by='firetext') - notification(sent_by='loadtesting') - notification(sent_by='loadtesting') - - count = get_count_of_slow_delivery_sms_notifications_for_provider( - created_at=now, - sent_at=now, - provider='mmg', - threshold=1, - delivery_time=timedelta(minutes=5), - service_id=noti1.service_id, - template_id=noti1.template_id - ) - - assert count.sent_by == 'mmg' + assert not slow_delivery diff --git a/tests/app/db.py b/tests/app/db.py index b30eaa7bb..4b0e9185c 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -23,7 +23,7 @@ def create_user(mobile_number="+447700900986", email="notify@digital.cabinet-off return user -def create_service(user=None, service_name="Sample service"): +def create_service(user=None, service_name="Sample service", service_id=None): service = Service( name=service_name, message_limit=1000, @@ -31,14 +31,15 @@ def create_service(user=None, service_name="Sample service"): email_from=service_name.lower().replace(' ', '.'), created_by=user or create_user() ) - dao_create_service(service, service.created_by) + dao_create_service(service, service.created_by, service_id) return service def create_template( service, template_type=SMS_TYPE, - content='Dear Sir/Madam, Hello. Yours Truly, The Government.' + content='Dear Sir/Madam, Hello. Yours Truly, The Government.', + template_id=None ): data = { 'name': '{} Template Name'.format(template_type), @@ -50,7 +51,7 @@ def create_template( if template_type != SMS_TYPE: data['subject'] = 'Template subject' template = Template(**data) - dao_create_template(template) + dao_create_template(template, template_id) return template @@ -63,6 +64,7 @@ def create_notification( reference=None, created_at=None, sent_at=None, + updated_at=None, billable_units=1, personalisation=None, api_key_id=None, @@ -72,6 +74,13 @@ def create_notification( ): if created_at is None: created_at = datetime.utcnow() + + if sent_at is None: + sent_at = datetime.utcnow() + + if updated_at is None: + updated_at = datetime.utcnow() + data = { 'id': uuid.uuid4(), 'to': to_field, @@ -91,7 +100,7 @@ def create_notification( 'api_key_id': api_key_id, 'key_type': key_type, 'sent_by': sent_by, - 'updated_at': None, + 'updated_at': updated_at, 'client_reference': client_reference, 'job_row_number': job_row_number } From 204d72830fd3fcff71554b8619d160b872f3f213 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Fri, 24 Feb 2017 13:41:32 +0000 Subject: [PATCH 06/10] Update switch task to use sent_at and newer db helpers --- app/celery/scheduled_tasks.py | 12 ++- tests/app/celery/test_scheduled_tasks.py | 102 +++++++++-------------- 2 files changed, 44 insertions(+), 70 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 1afe1aed9..b8e4eb3e8 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -14,7 +14,7 @@ from app.dao.jobs_dao import dao_set_scheduled_jobs_to_pending, dao_get_jobs_old from app.dao.notifications_dao import ( delete_notifications_created_more_than_a_week_ago, dao_timeout_notifications, - get_count_of_slow_delivery_sms_notifications_for_provider + is_delivery_slow_for_provider ) from app.dao.provider_details_dao import ( get_current_provider, @@ -163,11 +163,10 @@ def switch_current_sms_provider_on_slow_delivery(): if functional_test_provider_service_id and functional_test_provider_template_id: current_provider = get_current_provider('sms') - slow_delivery_notifications = get_count_of_slow_delivery_sms_notifications_for_provider( + slow_delivery_notifications = is_delivery_slow_for_provider( provider=current_provider.identifier, threshold=2, - created_at=current_provider.updated_at, - sent_at=datetime.utcnow() - timedelta(minutes=10), + sent_at=max(datetime.utcnow() - timedelta(minutes=10), current_provider.updated_at), delivery_time=timedelta(minutes=4), service_id=functional_test_provider_service_id, template_id=functional_test_provider_template_id @@ -175,9 +174,8 @@ def switch_current_sms_provider_on_slow_delivery(): if slow_delivery_notifications: current_app.logger.warning( - '{} slow delivery notifications detected for provider {}'.format( - slow_delivery_notifications.total, - slow_delivery_notifications.sent_by + 'Slow delivery notifications detected for provider {}'.format( + current_provider.identifier ) ) diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 4bc6d5f58..c7d78d53b 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -24,43 +24,36 @@ from app.dao.provider_details_dao import ( dao_update_provider_details, get_current_provider ) +from app.models import Service, Template from app.utils import get_london_midnight_in_utc +from tests.app.db import create_notification, create_service, create_template from tests.app.conftest import ( - sample_notification as create_sample_notification, sample_job as create_sample_job, - sample_notification_history as create_notification_history, - sample_service as create_sample_service, - sample_template as create_sample_template + sample_notification_history as create_notification_history ) from tests.conftest import set_config_values from unittest.mock import call, patch, PropertyMock -def _create_slow_delivery_notification(notify_db, notify_db_session, provider='mmg'): +def _create_slow_delivery_notification(provider='mmg'): now = datetime.utcnow() five_minutes_from_now = now + timedelta(minutes=5) + service = Service.query.get(current_app.config['FUNCTIONAL_TEST_PROVIDER_SERVICE_ID']) + if not service: + service = create_service( + service_id=current_app.config.get('FUNCTIONAL_TEST_PROVIDER_SERVICE_ID') + ) - service = create_sample_service( - notify_db, - notify_db_session, - service_id=current_app.config.get('FUNCTIONAL_TEST_PROVIDER_SERVICE_ID') - ) + template = Template.query.get(current_app.config['FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID']) + if not template: + template = create_template( + template_id=current_app.config.get('FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID'), + service=service + ) - template = create_sample_template( - notify_db, - notify_db_session, - template_id=current_app.config.get('FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID'), - service=service - ) - - create_sample_notification( - notify_db, - notify_db_session, - service=service, + create_notification( template=template, status='delivered', - created_at=now, - sent_at=now, sent_by=provider, updated_at=five_minutes_from_now ) @@ -116,26 +109,17 @@ def test_update_status_of_notifications_after_timeout(notify_api, sample_template, mmg_provider): with notify_api.test_request_context(): - not1 = create_sample_notification( - notify_db, - notify_db_session, - service=sample_service, + not1 = create_notification( template=sample_template, status='sending', created_at=datetime.utcnow() - timedelta( seconds=current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD') + 10)) - not2 = create_sample_notification( - notify_db, - notify_db_session, - service=sample_service, + not2 = create_notification( template=sample_template, status='created', created_at=datetime.utcnow() - timedelta( seconds=current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD') + 10)) - not3 = create_sample_notification( - notify_db, - notify_db_session, - service=sample_service, + not3 = create_notification( template=sample_template, status='pending', created_at=datetime.utcnow() - timedelta( @@ -153,10 +137,7 @@ def test_not_update_status_of_notification_before_timeout(notify_api, sample_template, mmg_provider): with notify_api.test_request_context(): - not1 = create_sample_notification( - notify_db, - notify_db_session, - service=sample_service, + not1 = create_notification( template=sample_template, status='sending', created_at=datetime.utcnow() - timedelta( @@ -294,13 +275,16 @@ def test_send_daily_performance_stats_calls_with_correct_totals( ]) -def test_switch_current_sms_provider_on_slow_delivery_does_not_run_if_config_unset(client, mocker): +def test_switch_current_sms_provider_on_slow_delivery_does_not_run_if_config_unset( + notify_api, + mocker +): get_notifications_mock = mocker.patch( - 'app.celery.scheduled_tasks.get_count_of_slow_delivery_sms_notifications_for_provider' + 'app.celery.scheduled_tasks.is_delivery_slow_for_provider' ) toggle_sms_mock = mocker.patch('app.celery.scheduled_tasks.dao_toggle_sms_provider') - with set_config_values(client.application, { + with set_config_values(notify_api, { 'FUNCTIONAL_TEST_PROVIDER_SERVICE_ID': None, 'FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID': None }): @@ -311,13 +295,12 @@ def test_switch_current_sms_provider_on_slow_delivery_does_not_run_if_config_uns def test_switch_providers_on_slow_delivery_runs_if_config_set( - notify_db, - notify_db_session, notify_api, - mocker + mocker, + set_provider_updated_at ): get_notifications_mock = mocker.patch( - 'app.celery.scheduled_tasks.get_count_of_slow_delivery_sms_notifications_for_provider', + 'app.celery.scheduled_tasks.is_delivery_slow_for_provider', return_value=[] ) @@ -330,9 +313,7 @@ def test_switch_providers_on_slow_delivery_runs_if_config_set( assert get_notifications_mock.called is True -def test_switch_providers_on_slow_delivery_triggers_switch_on_slow_notification_delivery( - notify_db, - notify_db_session, +def test_switch_providers_triggers_on_slow_notification_delivery( notify_api, restore_provider_details, current_sms_provider, @@ -342,9 +323,8 @@ def test_switch_providers_on_slow_delivery_triggers_switch_on_slow_notification_ 'FUNCTIONAL_TEST_PROVIDER_SERVICE_ID': '7954469d-8c6d-43dc-b8f7-86be2d69f5f3', 'FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID': '331a63e6-f1aa-4588-ad3f-96c268788ae7' }): - _create_slow_delivery_notification(notify_db, notify_db_session, current_sms_provider.identifier) - _create_slow_delivery_notification(notify_db, notify_db_session, current_sms_provider.identifier) - + _create_slow_delivery_notification(current_sms_provider.identifier) + _create_slow_delivery_notification(current_sms_provider.identifier) switch_current_sms_provider_on_slow_delivery() new_provider = get_current_provider('sms') @@ -353,8 +333,6 @@ def test_switch_providers_on_slow_delivery_triggers_switch_on_slow_notification_ def test_switch_providers_on_slow_delivery_does_not_switch_if_already_switched( - notify_db, - notify_db_session, notify_api, restore_provider_details, current_sms_provider, @@ -364,8 +342,8 @@ def test_switch_providers_on_slow_delivery_does_not_switch_if_already_switched( 'FUNCTIONAL_TEST_PROVIDER_SERVICE_ID': '7954469d-8c6d-43dc-b8f7-86be2d69f5f3', 'FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID': '331a63e6-f1aa-4588-ad3f-96c268788ae7' }): - _create_slow_delivery_notification(notify_db, notify_db_session) - _create_slow_delivery_notification(notify_db, notify_db_session) + _create_slow_delivery_notification() + _create_slow_delivery_notification() switch_current_sms_provider_on_slow_delivery() switch_current_sms_provider_on_slow_delivery() @@ -376,8 +354,6 @@ def test_switch_providers_on_slow_delivery_does_not_switch_if_already_switched( def test_switch_providers_on_slow_delivery_does_not_switch_based_on_older_notifications( - notify_db, - notify_db_session, notify_api, restore_provider_details, current_sms_provider, @@ -397,16 +373,16 @@ def test_switch_providers_on_slow_delivery_does_not_switch_based_on_older_notifi 'FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID': '331a63e6-f1aa-4588-ad3f-96c268788ae7' }): # Provider x -> y - _create_slow_delivery_notification(notify_db, notify_db_session, current_sms_provider.identifier) - _create_slow_delivery_notification(notify_db, notify_db_session, current_sms_provider.identifier) - _create_slow_delivery_notification(notify_db, notify_db_session, current_sms_provider.identifier) + _create_slow_delivery_notification(current_sms_provider.identifier) + _create_slow_delivery_notification(current_sms_provider.identifier) + _create_slow_delivery_notification(current_sms_provider.identifier) switch_current_sms_provider_on_slow_delivery() current_provider = get_current_provider('sms') # Provider y -> x - _create_slow_delivery_notification(notify_db, notify_db_session, current_provider.identifier) - _create_slow_delivery_notification(notify_db, notify_db_session, current_provider.identifier) + _create_slow_delivery_notification(current_provider.identifier) + _create_slow_delivery_notification(current_provider.identifier) switch_current_sms_provider_on_slow_delivery() # Expect to stay on provider x From 17b6c13c46021661966a9ea2b91441b049f5468a Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 27 Feb 2017 13:16:48 +0000 Subject: [PATCH 07/10] Small updates: * Make config use new provider sms template id * create_notification to account for created status * Small robustness addition to test --- app/config.py | 2 +- tests/app/dao/test_notification_dao.py | 2 +- tests/app/db.py | 8 +++----- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/app/config.py b/app/config.py index 13b4c9063..07c1c419e 100644 --- a/app/config.py +++ b/app/config.py @@ -240,7 +240,7 @@ class Live(Config): STATSD_ENABLED = True FROM_NUMBER = '40604' FUNCTIONAL_TEST_PROVIDER_SERVICE_ID = '6c1d81bb-dae2-4ee9-80b0-89a4aae9f649' - FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID = '285bb62d-08e7-414b-b5a3-6372ba320e06' + FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID = 'ba9e1789-a804-40b8-871f-cc60d4c1286f' class CloudFoundryConfig(Config): diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index bdcef6dcd..7081c3c1b 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -1514,7 +1514,7 @@ def test_slow_provider_delivery_returns_for_delivered_notifications_only( slow_delivery = is_delivery_slow_for_provider( sent_at=now, provider='firetext', - threshold=1, + threshold=2, delivery_time=timedelta(minutes=5), service_id=sample_template.service.id, template_id=sample_template.id diff --git a/tests/app/db.py b/tests/app/db.py index 4b0e9185c..093bd828e 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -75,11 +75,9 @@ def create_notification( if created_at is None: created_at = datetime.utcnow() - if sent_at is None: - sent_at = datetime.utcnow() - - if updated_at is None: - updated_at = datetime.utcnow() + if status != 'created': + sent_at = sent_at or datetime.utcnow() + updated_at = updated_at or datetime.utcnow() data = { 'id': uuid.uuid4(), From d805985a4ecd4cef40b43288c76fcafbcd312cbd Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 27 Feb 2017 13:18:42 +0000 Subject: [PATCH 08/10] Refactor tests to use cleaner fixture and be more verbose in tests --- tests/app/celery/test_scheduled_tasks.py | 50 ++++++++++++---------- tests/app/dao/test_provider_details_dao.py | 28 +++++++----- 2 files changed, 44 insertions(+), 34 deletions(-) diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index c7d78d53b..06ab343fa 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -60,9 +60,10 @@ def _create_slow_delivery_notification(provider='mmg'): @pytest.fixture(scope='function') -def set_provider_updated_at(current_sms_provider): - current_sms_provider.updated_at = datetime.utcnow() - timedelta(minutes=30) - dao_update_provider_details(current_sms_provider) +def prepare_current_provider(restore_provider_details): + initial_provider = get_current_provider('sms') + initial_provider.updated_at = datetime.utcnow() - timedelta(minutes=30) + dao_update_provider_details(initial_provider) def test_should_have_decorated_tasks_functions(): @@ -297,7 +298,7 @@ def test_switch_current_sms_provider_on_slow_delivery_does_not_run_if_config_uns def test_switch_providers_on_slow_delivery_runs_if_config_set( notify_api, mocker, - set_provider_updated_at + prepare_current_provider ): get_notifications_mock = mocker.patch( 'app.celery.scheduled_tasks.is_delivery_slow_for_provider', @@ -315,29 +316,29 @@ def test_switch_providers_on_slow_delivery_runs_if_config_set( def test_switch_providers_triggers_on_slow_notification_delivery( notify_api, - restore_provider_details, - current_sms_provider, - set_provider_updated_at + prepare_current_provider ): + starting_provider = get_current_provider('sms') + with set_config_values(notify_api, { 'FUNCTIONAL_TEST_PROVIDER_SERVICE_ID': '7954469d-8c6d-43dc-b8f7-86be2d69f5f3', 'FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID': '331a63e6-f1aa-4588-ad3f-96c268788ae7' }): - _create_slow_delivery_notification(current_sms_provider.identifier) - _create_slow_delivery_notification(current_sms_provider.identifier) + _create_slow_delivery_notification(starting_provider.identifier) + _create_slow_delivery_notification(starting_provider.identifier) switch_current_sms_provider_on_slow_delivery() new_provider = get_current_provider('sms') - assert new_provider.identifier != current_sms_provider.identifier - assert new_provider.priority < current_sms_provider.priority + assert new_provider.identifier != starting_provider.identifier + assert new_provider.priority < starting_provider.priority def test_switch_providers_on_slow_delivery_does_not_switch_if_already_switched( notify_api, - restore_provider_details, - current_sms_provider, - set_provider_updated_at + prepare_current_provider ): + starting_provider = get_current_provider('sms') + with set_config_values(notify_api, { 'FUNCTIONAL_TEST_PROVIDER_SERVICE_ID': '7954469d-8c6d-43dc-b8f7-86be2d69f5f3', 'FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID': '331a63e6-f1aa-4588-ad3f-96c268788ae7' @@ -349,15 +350,13 @@ def test_switch_providers_on_slow_delivery_does_not_switch_if_already_switched( switch_current_sms_provider_on_slow_delivery() new_provider = get_current_provider('sms') - assert new_provider.identifier != current_sms_provider.identifier - assert new_provider.priority < current_sms_provider.priority + assert new_provider.identifier != starting_provider.identifier + assert new_provider.priority < starting_provider.priority def test_switch_providers_on_slow_delivery_does_not_switch_based_on_older_notifications( notify_api, - restore_provider_details, - current_sms_provider, - set_provider_updated_at + prepare_current_provider ): """ Assume we have three slow delivery notifications for the current provider x. This triggers @@ -368,24 +367,29 @@ def test_switch_providers_on_slow_delivery_does_not_switch_based_on_older_notifi based on these as they are old. We only want to look for slow notifications after the point at which we switched back to provider x. """ + starting_provider = get_current_provider('sms') with set_config_values(notify_api, { 'FUNCTIONAL_TEST_PROVIDER_SERVICE_ID': '7954469d-8c6d-43dc-b8f7-86be2d69f5f3', 'FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID': '331a63e6-f1aa-4588-ad3f-96c268788ae7' }): # Provider x -> y - _create_slow_delivery_notification(current_sms_provider.identifier) - _create_slow_delivery_notification(current_sms_provider.identifier) - _create_slow_delivery_notification(current_sms_provider.identifier) + _create_slow_delivery_notification(starting_provider.identifier) + _create_slow_delivery_notification(starting_provider.identifier) + _create_slow_delivery_notification(starting_provider.identifier) switch_current_sms_provider_on_slow_delivery() current_provider = get_current_provider('sms') + assert current_provider.identifier != starting_provider.identifier # Provider y -> x _create_slow_delivery_notification(current_provider.identifier) _create_slow_delivery_notification(current_provider.identifier) switch_current_sms_provider_on_slow_delivery() + new_provider = get_current_provider('sms') + assert new_provider.identifier != current_provider.identifier + # Expect to stay on provider x switch_current_sms_provider_on_slow_delivery() current_provider = get_current_provider('sms') - assert current_sms_provider.identifier == current_provider.identifier + assert starting_provider.identifier == current_provider.identifier diff --git a/tests/app/dao/test_provider_details_dao.py b/tests/app/dao/test_provider_details_dao.py index b9569e37e..9add4fb9c 100644 --- a/tests/app/dao/test_provider_details_dao.py +++ b/tests/app/dao/test_provider_details_dao.py @@ -19,7 +19,7 @@ from app.dao.provider_details_dao import ( ) -def set_primary_sms_provider(identifier='mmg'): +def set_primary_sms_provider(restore_provider_details, identifier='mmg'): primary_provider = get_provider_details_by_identifier(identifier) secondary_provider = get_alternative_sms_provider(identifier) @@ -153,22 +153,28 @@ def test_toggle_sms_provider_switches_provider( ): dao_toggle_sms_provider(current_sms_provider.identifier) new_provider = get_current_provider('sms') - assert new_provider.identifier != current_sms_provider.identifier - assert new_provider.priority < current_sms_provider.priority + + old_starting_provider = get_provider_details_by_identifier(current_sms_provider.identifier) + + assert new_provider.identifier != old_starting_provider.identifier + assert new_provider.priority < old_starting_provider.priority def test_toggle_sms_provider_switches_when_provider_priorities_are_equal( - restore_provider_details, - current_sms_provider + restore_provider_details ): - new_provider = get_alternative_sms_provider(current_sms_provider.identifier) - current_sms_provider.priority = new_provider.priority - dao_update_provider_details(current_sms_provider) + current_provider = get_current_provider('sms') + new_provider = get_alternative_sms_provider(current_provider.identifier) - dao_toggle_sms_provider(current_sms_provider.identifier) + current_provider.priority = new_provider.priority + dao_update_provider_details(current_provider) - assert new_provider.identifier != current_sms_provider.identifier - assert new_provider.priority < current_sms_provider.priority + dao_toggle_sms_provider(current_provider.identifier) + + old_starting_provider = get_provider_details_by_identifier(current_provider.identifier) + + assert new_provider.identifier != old_starting_provider.identifier + assert new_provider.priority < old_starting_provider.priority def test_toggle_sms_provider_updates_provider_history( From ed65150f3bedf257a66f6ba43516f5e3f3a743e7 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 27 Feb 2017 13:45:53 +0000 Subject: [PATCH 09/10] Update set primary sms helper to use compulsory identifier --- tests/app/dao/test_provider_details_dao.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/dao/test_provider_details_dao.py b/tests/app/dao/test_provider_details_dao.py index 9add4fb9c..c4f77ed29 100644 --- a/tests/app/dao/test_provider_details_dao.py +++ b/tests/app/dao/test_provider_details_dao.py @@ -19,7 +19,7 @@ from app.dao.provider_details_dao import ( ) -def set_primary_sms_provider(restore_provider_details, identifier='mmg'): +def set_primary_sms_provider(identifier): primary_provider = get_provider_details_by_identifier(identifier) secondary_provider = get_alternative_sms_provider(identifier) From d7e4ca2a437f2e3f9e0400cb487e1a6f8cc31158 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 27 Feb 2017 15:01:44 +0000 Subject: [PATCH 10/10] Remove created notification - not needed for test --- tests/app/dao/test_notification_dao.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 7081c3c1b..5591d3f14 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -1506,7 +1506,6 @@ def test_slow_provider_delivery_returns_for_delivered_notifications_only( updated_at=five_minutes_from_now ) - notification_five_minutes_to_deliver(status='created') notification_five_minutes_to_deliver(status='sending') notification_five_minutes_to_deliver(status='delivered') notification_five_minutes_to_deliver(status='delivered')