Update switch provider on slow delivery task to change max once evey 10 minutes

app/celery/scheduled_tasks.py

@@ -256,28 +256,25 @@ 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')
+    current_provider = get_current_provider('sms')
-    functional_test_provider_template_id = current_app.config.get('FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID')
+    if current_provider.updated_at > datetime.utcnow() - timedelta(minutes=10):
+        current_app.logger.info("Slow delivery provider switched less than 10 minutes ago.")
+        return
+    slow_delivery_notifications = is_delivery_slow_for_provider(
+        provider=current_provider.identifier,
+        threshold=0.1,
+        created_at=datetime.utcnow() - timedelta(minutes=10),
+        delivery_time=timedelta(minutes=4),
+    )
 
-    if functional_test_provider_service_id and functional_test_provider_template_id:
+    if slow_delivery_notifications:
-        current_provider = get_current_provider('sms')
+        current_app.logger.warning(
-        slow_delivery_notifications = is_delivery_slow_for_provider(
+            'Slow delivery notifications detected for provider {}'.format(
-            provider=current_provider.identifier,
+                current_provider.identifier
-            threshold=2,
+            )
-            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
         )
 
-        if slow_delivery_notifications:
+        dao_toggle_sms_provider(current_provider.identifier)
-            current_app.logger.warning(
-                'Slow delivery notifications detected for provider {}'.format(
-                    current_provider.identifier
-                )
-            )
-
-            dao_toggle_sms_provider(current_provider.identifier)
 
 
 @notify_celery.task(name="delete-inbound-sms")

app/dao/notifications_dao.py

@@ -459,7 +459,6 @@ def is_delivery_slow_for_provider(
         Notification.key_type != KEY_TYPE_TEST
     ).group_by("slow").all()
 
-    print(count)
     counts = {c[0]: c[1] for c in count}
     total_notifications = sum(counts.values())
     if total_notifications:

tests/app/celery/test_scheduled_tasks.py

@@ -47,7 +47,6 @@ from app.dao.provider_details_dao import (
 from app.exceptions import NotificationTechnicalFailureException
 from app.models import (
     NotificationHistory,
-    Service,
     StatsTemplateUsageByMonth,
     JOB_STATUS_IN_PROGRESS,
     JOB_STATUS_ERROR,
@@ -71,33 +70,21 @@ from tests.app.conftest import (
     sample_job as create_sample_job,
     sample_notification_history as create_notification_history,
     sample_template as create_sample_template,
-    create_custom_template,
     datetime_in_past
 )
 from tests.conftest import set_config_values
 
 
-def _create_slow_delivery_notification(provider='mmg'):
+def _create_slow_delivery_notification(template, 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')
-        )
-
-    template = create_custom_template(
-        service=service,
-        user=service.users[0],
-        template_config_name='FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID',
-        template_type='sms'
-    )
 
     create_notification(
         template=template,
         status='delivered',
         sent_by=provider,
-        updated_at=five_minutes_from_now
+        updated_at=five_minutes_from_now,
+        sent_at=now,
     )
 
 
@@ -129,8 +116,9 @@ def test_should_have_decorated_tasks_functions():
 @pytest.fixture(scope='function')
 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)
+    initial_provider.updated_at = datetime.utcnow() - timedelta(minutes=30)
+    db.session.commit()
 
 
 def test_should_call_delete_sms_notifications_more_than_week_in_task(notify_api, mocker):
@@ -413,25 +401,6 @@ def test_send_total_sent_notifications_to_performance_platform_calls_with_correc
         ])
 
 
-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.is_delivery_slow_for_provider'
-    )
-    toggle_sms_mock = mocker.patch('app.celery.scheduled_tasks.dao_toggle_sms_provider')
-
-    with set_config_values(notify_api, {
-        '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_api,
         mocker,
@@ -451,96 +420,31 @@ def test_switch_providers_on_slow_delivery_runs_if_config_set(
     assert get_notifications_mock.called is True
 
 
-def test_switch_providers_triggers_on_slow_notification_delivery(
+def test_switch_providers_on_slow_delivery_switches_once_then_does_not_switch_if_already_switched(
-        notify_api,
-        mocker,
-        prepare_current_provider,
-        sample_user
-):
-    mocker.patch('app.provider_details.switch_providers.get_user_by_id', return_value=sample_user)
-    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(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 != 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,
-        mocker,
-        prepare_current_provider,
-        sample_user
-):
-    mocker.patch('app.provider_details.switch_providers.get_user_by_id', return_value=sample_user)
-    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()
-        _create_slow_delivery_notification()
-
-        switch_current_sms_provider_on_slow_delivery()
-        switch_current_sms_provider_on_slow_delivery()
-
-    new_provider = get_current_provider('sms')
-    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,
         mocker,
         prepare_current_provider,
         sample_user,
-
+        sample_template
 ):
-    """
-    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.
-    """
     mocker.patch('app.provider_details.switch_providers.get_user_by_id', return_value=sample_user)
     starting_provider = get_current_provider('sms')
 
-    with set_config_values(notify_api, {
+    _create_slow_delivery_notification(sample_template)
-        'FUNCTIONAL_TEST_PROVIDER_SERVICE_ID': '7954469d-8c6d-43dc-b8f7-86be2d69f5f3',
+    _create_slow_delivery_notification(sample_template)
-        'FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID': '331a63e6-f1aa-4588-ad3f-96c268788ae7'
-    }):
-        # Provider x -> y
-        _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')
+    switch_current_sms_provider_on_slow_delivery()
-        assert current_provider.identifier != starting_provider.identifier
 
-        # Provider y -> x
+    new_provider = get_current_provider('sms')
-        _create_slow_delivery_notification(current_provider.identifier)
+    _create_slow_delivery_notification(sample_template, new_provider.identifier)
-        _create_slow_delivery_notification(current_provider.identifier)
+    _create_slow_delivery_notification(sample_template, new_provider.identifier)
-        switch_current_sms_provider_on_slow_delivery()
+    switch_current_sms_provider_on_slow_delivery()
 
-        new_provider = get_current_provider('sms')
+    final_provider = get_current_provider('sms')
-        assert new_provider.identifier != current_provider.identifier
 
-        # Expect to stay on provider x
+    assert new_provider.identifier != starting_provider.identifier
-        switch_current_sms_provider_on_slow_delivery()
+    assert new_provider.priority < starting_provider.priority
-        current_provider = get_current_provider('sms')
+    assert final_provider.identifier == new_provider.identifier
-        assert starting_provider.identifier == current_provider.identifier
 
 
 @freeze_time("2017-05-01 14:00:00")

tests/app/dao/notification_dao/test_notification_dao.py

@@ -1179,9 +1179,6 @@ def test_get_total_sent_notifications_for_email_excludes_sms_counts(
     assert total_count == 2
 
 
-def test_is_delivery_slow_for_provider_not_slow_when_no_notifications(notify_db_session):
-    assert not is_delivery_slow_for_provider(datetime.utcnow(), "firetext", 0.1, timedelta(minutes=4))
-
 @pytest.mark.parametrize(
     "normal_sending,slow_sending,normal_delivered,slow_delivered,threshold,expected_result",
     [
@@ -1195,7 +1192,7 @@ def test_is_delivery_slow_for_provider_not_slow_when_no_notifications(notify_db_
     ]
 )
 @freeze_time("2018-12-04 12:00:00.000000")
-def test_delivery_is_delivery_slow_for_provider(
+def test_is_delivery_slow_for_provider(
     notify_db_session,
     sample_template,
     normal_sending,
@@ -1230,9 +1227,9 @@ def test_delivery_is_delivery_slow_for_provider(
     for _ in range(slow_delivered):
         slow_notification(status='delivered')
 
-
     assert is_delivery_slow_for_provider(datetime.utcnow(), "mmg", threshold, timedelta(minutes=4)) is expected_result
 
+
 @pytest.mark.parametrize("options,expected_result", [
     ({"status": NOTIFICATION_TEMPORARY_FAILURE, "sent_by": "mmg"}, False),
     ({"status": NOTIFICATION_DELIVERED, "sent_by": "firetext"}, False),