Skip to content

Commit

Permalink
Revert "Notification stats discrepancy take 2 - Handling UTC window a…
Browse files Browse the repository at this point in the history 
…nd DST when fetching notification stats (#2064)" (#2067)

This reverts commit f0a7272.
  • Loading branch information
@whabanks
whabanks authored Dec 18, 2023
1 parent f0a7272 commit 95ae353
Show file tree
Hide file tree
Showing 4 changed files with 15 additions and 88 deletions.
16 changes: 0 additions & 16 deletions app/dao/date_util.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
12 changes: 4 additions & 8 deletions app/dao/fact_notification_status_dao.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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"),
Expand All @@ -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,
)

Expand All @@ -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,
)
Expand Down
15 changes: 0 additions & 15 deletions tests/app/dao/test_date_utils.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,13 @@

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,
)


Expand Down Expand Up @@ -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
Expand Down
60 changes: 11 additions & 49 deletions tests/app/dao/test_fact_notification_status_dao.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@
create_notification,
create_notification_history,
create_service,
create_service_data_retention,
create_template,
save_notification,
)
Expand Down Expand Up @@ -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,
Expand All @@ -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"))

Expand Down Expand Up @@ -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
Expand All @@ -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(
Expand All @@ -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),
Expand Down Expand Up @@ -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

0 comments on commit 95ae353

Please sign in to comment.