From 95ae353e84995c6571ec8bde0ec319ecd758b304 Mon Sep 17 00:00:00 2001 From: William B <7444334+whabanks@users.noreply.github.com> Date: Mon, 18 Dec 2023 16:15:15 -0500 Subject: [PATCH] Revert "Notification stats discrepancy take 2 - Handling UTC window and DST when fetching notification stats (#2064)" (#2067) This reverts commit f0a72724629fdf8f98e0ba8e959d4a12a8f8e633. --- app/dao/date_util.py | 16 ----- app/dao/fact_notification_status_dao.py | 12 ++-- tests/app/dao/test_date_utils.py | 15 ----- .../dao/test_fact_notification_status_dao.py | 60 ++++--------------- 4 files changed, 15 insertions(+), 88 deletions(-) diff --git a/app/dao/date_util.py b/app/dao/date_util.py index a89936202f..921dc14dce 100644 --- a/app/dao/date_util.py +++ b/app/dao/date_util.py @@ -83,22 +83,6 @@ 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. diff --git a/app/dao/fact_notification_status_dao.py b/app/dao/fact_notification_status_dao.py index f15437b295..7c805ed6aa 100644 --- a/app/dao/fact_notification_status_dao.py +++ b/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, tz_aware_utc_now, utc_midnight_n_days_ago +from app.dao.date_util import get_midnight, utc_midnight_n_days_ago from app.models import ( EMAIL_TYPE, KEY_TYPE_NORMAL, @@ -239,10 +239,7 @@ 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): - 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) + start_date = utc_midnight_n_days_ago(limit_days) stats_for_7_days = db.session.query( FactNotificationStatus.notification_type.label("notification_type"), @@ -251,7 +248,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 >= ft_start_date, + FactNotificationStatus.bst_date >= start_date, FactNotificationStatus.key_type != KEY_TYPE_TEST, ) @@ -263,8 +260,7 @@ def fetch_notification_status_for_service_for_today_and_7_previous_days(service_ *([func.count().label("count")]), ) .filter( - Notification.created_at >= start, - Notification.created_at <= end, + Notification.created_at >= get_midnight(datetime.utcnow()), Notification.service_id == service_id, Notification.key_type != KEY_TYPE_TEST, ) diff --git a/tests/app/dao/test_date_utils.py b/tests/app/dao/test_date_utils.py index bd082d4196..4286fb8da7 100644 --- a/tests/app/dao/test_date_utils.py +++ b/tests/app/dao/test_date_utils.py @@ -2,7 +2,6 @@ import pytest import pytz -from freezegun import freeze_time from app.dao.date_util import ( get_april_fools, @@ -10,7 +9,6 @@ get_financial_year_for_datetime, get_midnight, get_month_start_and_end_date_in_utc, - tz_aware_midnight_n_days_ago, ) @@ -82,19 +80,6 @@ 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 diff --git a/tests/app/dao/test_fact_notification_status_dao.py b/tests/app/dao/test_fact_notification_status_dao.py index e9bf5b3a64..f96fab220a 100644 --- a/tests/app/dao/test_fact_notification_status_dao.py +++ b/tests/app/dao/test_fact_notification_status_dao.py @@ -48,7 +48,6 @@ create_notification, create_notification_history, create_service, - create_service_data_retention, create_template, save_notification, ) @@ -240,44 +239,6 @@ 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, @@ -287,13 +248,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) # delivered + create_ft_notification_status(date(2018, 10, 29), "sms", service_1, count=10) 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) # delivered - create_ft_notification_status(date(2018, 10, 29), "email", service_1, count=3) # delivered + 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) - 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, 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, 12, 0, 0), status="delivered")) save_notification(create_notification(email_template, created_at=datetime(2018, 10, 31, 13, 0, 0), status="delivered")) @@ -330,6 +291,7 @@ 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 @@ -344,8 +306,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(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")) + 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")) # too early, shouldn't be included save_notification( @@ -357,12 +319,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), @@ -1202,11 +1164,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] == 3 + assert results[0][2] == 2 # 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] == 8 + assert results[0][2] == 7