Notification stats discrepancy take 2 - Handling UTC window and DST w…

…hen fetching notification stats (#2064)

* Handle UTC window when fetching notification stats

- Implement timezone aware dates compatible with timedelta date math

* Fix test

* Really fix the tests this time

app/dao/date_util.py

@@ -83,6 +83,22 @@ def get_midnight(datetime: datetime) -> datetime:
     return datetime.replace(hour=0, minute=0, second=0, microsecond=0)
 
 
+def tz_aware_utc_now():
+    """
+    Returns a local (EST/EDT), time zone aware representation of UTC now.
+    Call dst() on the returned object to determine daylight savings status.
+    """
+    return pytz.utc.localize(datetime.utcnow())
+
+
+def tz_aware_midnight_n_days_ago(days_ago: int = 1) -> datetime:
+    """
+    Returns an EST/EDT aware UTC midnight a number of days ago.
+    """
+    est = pytz.timezone("US/Eastern")
+    return est.localize(tz_aware_utc_now().replace(tzinfo=None) - timedelta(days=days_ago))
+
+
 def utc_midnight_n_days_ago(number_of_days):
     """
     Returns utc midnight a number of days ago.

app/dao/fact_notification_status_dao.py

@@ -8,7 +8,7 @@
 from sqlalchemy.types import DateTime, Integer
 
 from app import db
-from app.dao.date_util import get_midnight, utc_midnight_n_days_ago
+from app.dao.date_util import get_midnight, tz_aware_utc_now, utc_midnight_n_days_ago
 from app.models import (
     EMAIL_TYPE,
     KEY_TYPE_NORMAL,
@@ -239,7 +239,10 @@ def fetch_notification_status_for_service_for_day(bst_day, service_id):
 
 
 def fetch_notification_status_for_service_for_today_and_7_previous_days(service_id, by_template=False, limit_days=7):
-    start_date = utc_midnight_n_days_ago(limit_days)
+    ft_start_date = utc_midnight_n_days_ago(limit_days)
+
+    start = tz_aware_utc_now() - timedelta(days=1)
+    end = get_midnight(tz_aware_utc_now()) + timedelta(hours=23, minutes=59, seconds=59)
 
     stats_for_7_days = db.session.query(
         FactNotificationStatus.notification_type.label("notification_type"),
@@ -248,7 +251,7 @@ def fetch_notification_status_for_service_for_today_and_7_previous_days(service_
         *([FactNotificationStatus.notification_count.label("count")]),
     ).filter(
         FactNotificationStatus.service_id == service_id,
-        FactNotificationStatus.bst_date >= start_date,
+        FactNotificationStatus.bst_date >= ft_start_date,
         FactNotificationStatus.key_type != KEY_TYPE_TEST,
     )
 
@@ -260,7 +263,8 @@ def fetch_notification_status_for_service_for_today_and_7_previous_days(service_
             *([func.count().label("count")]),
         )
         .filter(
-            Notification.created_at >= get_midnight(datetime.utcnow()),
+            Notification.created_at >= start,
+            Notification.created_at <= end,
             Notification.service_id == service_id,
             Notification.key_type != KEY_TYPE_TEST,
         )

tests/app/dao/test_date_utils.py

@@ -2,13 +2,15 @@
 
 import pytest
 import pytz
+from freezegun import freeze_time
 
 from app.dao.date_util import (
     get_april_fools,
     get_financial_year,
     get_financial_year_for_datetime,
     get_midnight,
     get_month_start_and_end_date_in_utc,
+    tz_aware_midnight_n_days_ago,
 )
 
 
@@ -80,6 +82,19 @@ def test_get_financial_year_for_datetime(dt, fy):
     assert get_financial_year_for_datetime(dt) == fy
 
 
+@pytest.mark.parametrize(
+    "frozen_datetime, expected_tz",
+    [
+        ("2023-11-04T00:00:00", "EDT"),
+        ("2023-11-08T00:00:00", "EST"),
+    ],
+)
+def test_tz_aware_midnight_n_days_ago_handles_daylight_savings(frozen_datetime, expected_tz):
+    with freeze_time(frozen_datetime):
+        converted_date = tz_aware_midnight_n_days_ago(days_ago=1)
+        assert converted_date.tzinfo._tzname == expected_tz
+
+
 class TestMidnightDateTime:
     eastern = pytz.timezone("US/Eastern")
     utc = pytz.utc

tests/app/dao/test_fact_notification_status_dao.py

@@ -48,6 +48,7 @@
     create_notification,
     create_notification_history,
     create_service,
+    create_service_data_retention,
     create_template,
     save_notification,
 )
@@ -239,6 +240,44 @@ def test_fetch_notification_status_for_service_for_day(notify_db_session):
     assert results[1].count == 1
 
 
+@freeze_time("2023-12-14T19:00:00")
+def test_fetch_notification_status_for_service_for_today_and_7_previous_days_non_default_retention_period(
+    notify_db_session,
+):
+    service = create_service(service_name="service")
+    create_service_data_retention(service, notification_type="sms", days_of_retention=3)
+    create_service_data_retention(service, notification_type="email", days_of_retention=3)
+    sms_template = create_template(service=service, template_type=SMS_TYPE)
+    email_template = create_template(service=service, template_type=EMAIL_TYPE)
+
+    # Should not be counted
+    create_ft_notification_status(date(2023, 12, 10), "sms", service, count=10)
+    create_ft_notification_status(date(2023, 12, 10), "email", service, count=10)
+    save_notification(create_notification(sms_template, created_at=datetime(2023, 12, 10, 18, 0)))
+    save_notification(create_notification(email_template, created_at=datetime(2023, 12, 10, 18, 0)))
+
+    # Should be counted
+    create_ft_notification_status(date(2023, 12, 11), "sms", service, count=5)
+    create_ft_notification_status(date(2023, 12, 11), "email", service, count=5)
+    save_notification(create_notification(sms_template, created_at=datetime(2023, 12, 10, 19, 0)))
+    save_notification(create_notification(email_template, created_at=datetime(2023, 12, 10, 19, 0)))
+
+    results = sorted(
+        fetch_notification_status_for_service_for_today_and_7_previous_days(service.id, False, 3),
+        key=lambda x: (x.notification_type, x.status),
+    )
+
+    assert len(results) == 2
+
+    assert results[0].notification_type == "email"
+    assert results[0].status == "delivered"
+    assert results[0].count == 5
+
+    assert results[1].notification_type == "sms"
+    assert results[1].status == "delivered"
+    assert results[1].count == 5
+
+
 @freeze_time("2018-10-31T18:00:00")
 def test_fetch_notification_status_for_service_for_today_and_7_previous_days(
     notify_db_session,
@@ -248,13 +287,13 @@ def test_fetch_notification_status_for_service_for_today_and_7_previous_days(
     sms_template_2 = create_template(service=service_1, template_type=SMS_TYPE)
     email_template = create_template(service=service_1, template_type=EMAIL_TYPE)
 
-    create_ft_notification_status(date(2018, 10, 29), "sms", service_1, count=10)
+    create_ft_notification_status(date(2018, 10, 29), "sms", service_1, count=10)  # delivered
     create_ft_notification_status(date(2018, 10, 29), "sms", service_1, notification_status="created")
-    create_ft_notification_status(date(2018, 10, 24), "sms", service_1, count=8)
-    create_ft_notification_status(date(2018, 10, 29), "email", service_1, count=3)
+    create_ft_notification_status(date(2018, 10, 24), "sms", service_1, count=8)  # delivered
+    create_ft_notification_status(date(2018, 10, 29), "email", service_1, count=3)  # delivered
 
-    save_notification(create_notification(sms_template, created_at=datetime(2018, 10, 31, 11, 0, 0)))
-    save_notification(create_notification(sms_template_2, created_at=datetime(2018, 10, 31, 11, 0, 0)))
+    save_notification(create_notification(sms_template, created_at=datetime(2018, 10, 31, 11, 0, 0)))  # Created
+    save_notification(create_notification(sms_template_2, created_at=datetime(2018, 10, 31, 11, 0, 0)))  # Created
     save_notification(create_notification(sms_template, created_at=datetime(2018, 10, 31, 12, 0, 0), status="delivered"))
     save_notification(create_notification(email_template, created_at=datetime(2018, 10, 31, 13, 0, 0), status="delivered"))
 
@@ -291,7 +330,6 @@ def test_fetch_notification_status_for_service_for_today_and_7_previous_days(
 def test_fetch_notification_status_by_template_for_service_for_today_and_7_previous_days(notify_db_session, notify_api):
     service_1 = create_service(service_name="service_1")
     sms_template = create_template(template_name="SMS NON-FT", service=service_1, template_type=SMS_TYPE)
-    sms_template_2 = create_template(template_name="SMS1 NON-FT", service=service_1, template_type=SMS_TYPE)
     email_template = create_template(template_name="EMAIL NON-FT", service=service_1, template_type=EMAIL_TYPE)
 
     # create unused email template
@@ -306,8 +344,8 @@ def test_fetch_notification_status_by_template_for_service_for_today_and_7_previ
 
     save_notification(create_notification(sms_template, created_at=datetime(2018, 10, 31, 11, 0, 0)))
     save_notification(create_notification(sms_template, created_at=datetime(2018, 10, 31, 12, 0, 0), status="delivered"))
-    save_notification(create_notification(sms_template_2, created_at=datetime(2018, 10, 31, 12, 0, 0), status="delivered"))
-    save_notification(create_notification(email_template, created_at=datetime(2018, 10, 31, 13, 0, 0), status="delivered"))
+    save_notification(create_notification(email_template, created_at=datetime(2018, 10, 31, 11, 0, 0)))
+    save_notification(create_notification(email_template, created_at=datetime(2018, 10, 31, 12, 0, 0), status="delivered"))
 
     # too early, shouldn't be included
     save_notification(
@@ -319,12 +357,12 @@ def test_fetch_notification_status_by_template_for_service_for_today_and_7_previ
     )
     results = fetch_notification_status_for_service_for_today_and_7_previous_days(service_1.id, by_template=True)
     assert [
+        ("EMAIL NON-FT", False, mock.ANY, "email", "created", 1),
         ("EMAIL NON-FT", False, mock.ANY, "email", "delivered", 1),
         ("email Template Name", False, mock.ANY, "email", "delivered", 3),
         ("SMS NON-FT", False, mock.ANY, "sms", "created", 1),
         ("sms Template Name", False, mock.ANY, "sms", "created", 1),
         ("SMS NON-FT", False, mock.ANY, "sms", "delivered", 1),
-        ("SMS1 NON-FT", False, mock.ANY, "sms", "delivered", 1),
         ("sms Template Name", False, mock.ANY, "sms", "delivered", 8),
         ("sms Template Name", False, mock.ANY, "sms", "delivered", 10),
         ("sms Template Name", False, mock.ANY, "sms", "delivered", 11),
@@ -1164,11 +1202,11 @@ def test_fetch_notification_status_for_service_for_today_handles_midnight_utc(
         key=lambda x: (x.notification_type, x.status),
     )
 
-    assert results[0][2] == 2
+    assert results[0][2] == 3
 
     # checking the daily stats for the last 2 days should give us the 2 created after 12am UTC and the 1 from the day before
     results = sorted(
         fetch_notification_status_for_service_for_today_and_7_previous_days(service_1.id, limit_days=2),
         key=lambda x: (x.notification_type, x.status),
     )
-    assert results[0][2] == 7
+    assert results[0][2] == 8