New query for finding if provider is slow

The delivery for provider is slow if more than threshold (currently
we pass in threshold 10%) either took x (for now 4) minutes to deliver,
or are still sending after that time. We look at all notifications
for current provider which are delivered or sending, and are not under
test key, for the last 10 minutes.

We are using created_at to establish if notifications are from last
10 minutes because we have an index on it, so the query is faster.

Also write tests for new is_delivery_slow_for_provider query

app/dao/notifications_dao.py

@@ -437,22 +437,35 @@ def get_total_sent_notifications_in_date_range(start_date, end_date, notificatio
 
 
 def is_delivery_slow_for_provider(
-        sent_at,
+        created_at,
         provider,
         threshold,
         delivery_time,
-        service_id,
-        template_id
 ):
-    count = db.session.query(Notification).filter(
+    count = db.session.query(
-        Notification.service_id == service_id,
+        case(
-        Notification.template_id == template_id,
+            [(
-        Notification.sent_at >= sent_at,
+                Notification.status == NOTIFICATION_DELIVERED,
-        Notification.status == NOTIFICATION_DELIVERED,
+                (Notification.updated_at - Notification.sent_at) >= delivery_time
+            )],
+            else_=(datetime.utcnow() - Notification.sent_at) >= delivery_time
+        ).label("slow"), func.count()
+
+    ).filter(
+        Notification.created_at >= created_at,
+        Notification.sent_at.isnot(None),
+        Notification.status.in_([NOTIFICATION_DELIVERED, NOTIFICATION_SENDING]),
         Notification.sent_by == provider,
-        (Notification.updated_at - Notification.sent_at) >= delivery_time,
+        Notification.key_type != KEY_TYPE_TEST
-    ).count()
+    ).group_by("slow").all()
-    return count >= threshold
+
+    print(count)
+    counts = {c[0]: c[1] for c in count}
+    total_notifications = sum(counts.values())
+    if total_notifications:
+        return counts.get(True, 0) / total_notifications >= threshold
+    else:
+        return False
 
 
 @statsd(namespace="dao")

tests/app/dao/notification_dao/test_notification_dao.py

@@ -43,6 +43,8 @@ from app.models import (
     ScheduledNotification,
     NOTIFICATION_STATUS_TYPES,
     NOTIFICATION_STATUS_TYPES_FAILED,
+    NOTIFICATION_TEMPORARY_FAILURE,
+    NOTIFICATION_SENDING,
     NOTIFICATION_SENT,
     NOTIFICATION_DELIVERED,
     KEY_TYPE_NORMAL,
@@ -1177,131 +1179,88 @@ def test_get_total_sent_notifications_for_email_excludes_sms_counts(
     assert total_count == 2
 
 
-@freeze_time("2016-01-10 12:00:00.000000")
+def test_is_delivery_slow_for_provider_not_slow_when_no_notifications(notify_db_session):
-def test_slow_provider_delivery_returns_for_sent_notifications(
+    assert not is_delivery_slow_for_provider(datetime.utcnow(), "firetext", 0.1, timedelta(minutes=4))
-    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(
+@pytest.mark.parametrize(
+    "normal_sending,slow_sending,normal_delivered,slow_delivered,threshold,expected_result",
+    [
+        (0, 0, 0, 0, 0.1, False),
+        (1, 0, 0, 0, 0.1, False),
+        (1, 1, 0, 0, 0.1, True),
+        (0, 0, 1, 1, 0.1, True),
+        (1, 1, 1, 1, 0.5, True),
+        (1, 1, 1, 1, 0.6, False),
+        (45, 5, 45, 5, 0.1, True),
+    ]
+)
+@freeze_time("2018-12-04 12:00:00.000000")
+def test_delivery_is_delivery_slow_for_provider(
+    notify_db_session,
+    sample_template,
+    normal_sending,
+    slow_sending,
+    normal_delivered,
+    slow_delivered,
+    threshold,
+    expected_result
+):
+    normal_notification = partial(
         create_notification,
         template=sample_template,
-        status='delivered',
         sent_by='mmg',
-        updated_at=five_minutes_from_now
+        sent_at=datetime.now(),
+        updated_at=datetime.now()
     )
 
-    notification_five_minutes_to_deliver(sent_at=now)
+    slow_notification = partial(
-    notification_five_minutes_to_deliver(sent_at=one_minute_from_now)
-    notification_five_minutes_to_deliver(sent_at=one_minute_from_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=sample_template.service.id,
-        template_id=sample_template.id
-    )
-
-    assert slow_delivery
-
-
-@freeze_time("2016-01-10 12:00:00.000000")
-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(
         create_notification,
         template=sample_template,
-        status='delivered',
-        sent_at=now,
         sent_by='mmg',
-        updated_at=five_minutes_from_now
+        sent_at=datetime.now() - timedelta(minutes=5),
+        updated_at=datetime.now()
     )
 
-    notification_five_minutes_to_deliver()
+    for _ in range(normal_sending):
-    notification_five_minutes_to_deliver()
+        normal_notification(status='sending')
-
+    for _ in range(slow_sending):
-    slow_delivery = is_delivery_slow_for_provider(
+        slow_notification(status='sending')
-        sent_at=now,
+    for _ in range(normal_delivered):
-        provider='mmg',
+        normal_notification(status='delivered')
-        threshold=3,
+    for _ in range(slow_delivered):
-        delivery_time=timedelta(minutes=5),
+        slow_notification(status='delivered')
-        service_id=sample_template.service.id,
-        template_id=sample_template.id
-    )
-
-    assert not slow_delivery
 
 
-@freeze_time("2016-01-10 12:00:00.000000")
+    assert is_delivery_slow_for_provider(datetime.utcnow(), "mmg", threshold, timedelta(minutes=4)) is expected_result
-def test_slow_provider_delivery_returns_for_delivered_notifications_only(
+
-    sample_template
+@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_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),
+])
+@freeze_time("2018-12-04 12:00:00.000000")
+def test_delivery_is_delivery_slow_for_provider_filters_out_notifications_it_should_not_count(
+    notify_db_session,
+    sample_template,
+    options,
+    expected_result
 ):
-    now = datetime.utcnow()
+    create_notification_with = {
-    five_minutes_from_now = now + timedelta(minutes=5)
+        "template": sample_template,
-
+        "sent_at": datetime.now() - timedelta(minutes=5),
-    notification_five_minutes_to_deliver = partial(
+        "updated_at": datetime.now(),
-        create_notification,
+    }
-        template=sample_template,
+    create_notification_with.update(options)
-        sent_at=now,
+    create_notification(
-        sent_by='firetext',
+        **create_notification_with
-        created_at=now,
-        updated_at=five_minutes_from_now
     )
-
+    assert is_delivery_slow_for_provider(datetime.utcnow(), "mmg", 0.1, timedelta(minutes=4)) is expected_result
-    notification_five_minutes_to_deliver(status='sending')
-    notification_five_minutes_to_deliver(status='delivered')
-    notification_five_minutes_to_deliver(status='delivered')
-
-    slow_delivery = is_delivery_slow_for_provider(
-        sent_at=now,
-        provider='firetext',
-        threshold=2,
-        delivery_time=timedelta(minutes=5),
-        service_id=sample_template.service.id,
-        template_id=sample_template.id
-    )
-
-    assert slow_delivery
-
-
-@freeze_time("2016-01-10 12:00:00.000000")
-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(
-        create_notification,
-        template=sample_template,
-        created_at=now,
-        sent_at=now,
-        sent_by='mmg',
-        status='delivered'
-    )
-
-    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)
-
-    slow_delivery = is_delivery_slow_for_provider(
-        sent_at=now,
-        provider='mmg',
-        threshold=2,
-        delivery_time=timedelta(minutes=5),
-        service_id=sample_template.service.id,
-        template_id=sample_template.id
-    )
-
-    assert not slow_delivery
 
 
 def test_dao_get_notifications_by_to_field(sample_template):