Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Notification stats discrepancy take 2 - Handling UTC window and DST when fetching notification stats" #2067

Merged
merged 1 commit into from
Dec 18, 2023
Merged

Revert "Notification stats discrepancy take 2 - Handling UTC window and DST when fetching notification stats" #2067

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 0 additions & 16 deletions app/dao/date_util.py
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading