From a617ccca9daffb0485e37e301b6ebb0d841a4371 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 21 Feb 2019 15:57:24 +0000 Subject: [PATCH] allow pending notifications to influence switchover. Currently we switch if: * status = delivered and updated_at - sent_at > threshold * status = sending and now - sent_at > threshold firetext can leave notifications in the pending state, which is equivalent to sending in terms of how we should handle it, so this commit changes the second case to allow pending as well as sending. --- app/celery/scheduled_tasks.py | 2 +- app/dao/notifications_dao.py | 2 +- .../dao/notification_dao/test_notification_dao.py | 12 +++++++----- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index af072d91b..f8b966e84 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -97,7 +97,7 @@ def delete_invitations(): @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) + Switch providers if at least 10% of notifications took more than four minutes to be delivered in the last ten minutes. Search from the time we last switched to the current provider. """ current_provider = get_current_provider('sms') diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 534f6d015..d693bde25 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -484,7 +484,7 @@ def is_delivery_slow_for_provider( ).filter( Notification.created_at >= created_at, Notification.sent_at.isnot(None), - Notification.status.in_([NOTIFICATION_DELIVERED, NOTIFICATION_SENDING]), + Notification.status.in_([NOTIFICATION_DELIVERED, NOTIFICATION_PENDING, NOTIFICATION_SENDING]), Notification.sent_by == provider, Notification.key_type != KEY_TYPE_TEST ).group_by("slow").all() diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 90dc73035..0776a45fe 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -45,6 +45,7 @@ from app.models import ( NOTIFICATION_STATUS_TYPES_FAILED, NOTIFICATION_TEMPORARY_FAILURE, NOTIFICATION_SENDING, + NOTIFICATION_PENDING, NOTIFICATION_SENT, NOTIFICATION_DELIVERED, KEY_TYPE_NORMAL, @@ -1267,15 +1268,16 @@ def test_is_delivery_slow_for_provider( @pytest.mark.parametrize("options,expected_result", [ - ({"status": NOTIFICATION_TEMPORARY_FAILURE, "sent_by": "mmg"}, False), - ({"status": NOTIFICATION_DELIVERED, "sent_by": "firetext"}, False), ({"status": NOTIFICATION_DELIVERED, "sent_by": "mmg"}, True), + ({"status": NOTIFICATION_PENDING, "sent_by": "mmg"}, True), + ({"status": NOTIFICATION_SENDING, "sent_by": "mmg"}, True), + + ({"status": NOTIFICATION_TEMPORARY_FAILURE, "sent_by": "mmg"}, False), ({"status": NOTIFICATION_DELIVERED, "sent_by": "mmg", "sent_at": None}, False), ({"status": NOTIFICATION_DELIVERED, "sent_by": "mmg", "key_type": KEY_TYPE_TEST}, False), ({"status": NOTIFICATION_SENDING, "sent_by": "firetext"}, False), - ({"status": NOTIFICATION_SENDING, "sent_by": "mmg"}, True), - ({"status": NOTIFICATION_SENDING, "sent_by": "mmg", "sent_at": None}, False), - ({"status": NOTIFICATION_SENDING, "sent_by": "mmg", "key_type": KEY_TYPE_TEST}, False), + ({"status": NOTIFICATION_DELIVERED, "sent_by": "firetext"}, False), + ]) @freeze_time("2018-12-04 12:00:00.000000") def test_delivery_is_delivery_slow_for_provider_filters_out_notifications_it_should_not_count(