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

Update daily notification stats to use 12am utc #1976

Merged
merged 9 commits into from
Sep 14, 2023
Merged

Update daily notification stats to use 12am utc #1976

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e92badf
fix(fact_notification_status_dao): update daily stats query to only l…
andrewleith Sep 12, 2023
6364093
chore: formatting
andrewleith Sep 12, 2023
f427a0b
test: add test to check that daily stats function only returns since …
andrewleith Sep 12, 2023
8bd0b70
fix: make fetch_notification_status_for_service_for_today_and_7_previ…
andrewleith Sep 12, 2023
74c1754
test: update the test to be a little more complicated
andrewleith Sep 12, 2023
4809ec6
chore: formatting
andrewleith Sep 12, 2023
d7f18fe
Merge branch 'main' into fix/update-daily-stats-query
andrewleith Sep 12, 2023
80982f7
Merge branch 'main' into fix/update-daily-stats-query
andrewleith Sep 13, 2023
9162cfd
Merge branch 'main' into fix/update-daily-stats-query
andrewleith Sep 14, 2023
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
10 changes: 8 additions & 2 deletions app/dao/fact_notification_status_dao.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from datetime import datetime, time, timedelta

import pytz
Fixed Show fixed Hide fixed
from flask import current_app
from notifications_utils.timezones import convert_local_timezone_to_utc
from sqlalchemy import Date, case, func
Expand All @@ -8,6 +9,7 @@
from sqlalchemy.types import DateTime, Integer

from app import db
from app.dao.date_util import get_midnight
from app.models import (
EMAIL_TYPE,
KEY_TYPE_NORMAL,
Expand Down Expand Up @@ -239,7 +241,11 @@ 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 = midnight_n_days_ago(limit_days)
if limit_days == 1:
start_date = get_midnight(datetime.now(tz=pytz.utc))
else:
start_date = midnight_n_days_ago(limit_days)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will return the correct datetime for the case we are trying to fix, but I think there are a couple issues with this change:

  • start_date is a timezone aware datetime if limit_days = 1, and it is timezone naive if limit_days > 1. That makes the code harder to understand
  • the way start_date is calculated is very different if limit_days = 1 vs if limit_days > 1. This might cause issues in the future when we forget this detail and we are calling the endpoint via notification-admin

Could we just always use utc midnight for this function? I'm thinking something like:

def utc_midnight_n_days_ago(number_of_days):
    """
    Returns utc midnight a number of days ago.
    """
    return get_midnight(datetime.utcnow() - timedelta(days=number_of_days))

in app/utils.py, and then:

start_date = utc_midnight_n_days_ago(limit_days)

in this file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah for sure, I specifically targeted the 1 day case, but you make good points here. Let's chat!


stats_for_7_days = db.session.query(
FactNotificationStatus.notification_type.label("notification_type"),
FactNotificationStatus.notification_status.label("status"),
Expand All @@ -259,7 +265,7 @@ def fetch_notification_status_for_service_for_today_and_7_previous_days(service_
*([func.count().label("count")]),
)
.filter(
Notification.created_at >= midnight_n_days_ago(limit_days),
Notification.created_at >= start_date,
Notification.service_id == service_id,
Notification.key_type != KEY_TYPE_TEST,
)
Expand Down
32 changes: 32 additions & 0 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 @@ -1141,3 +1141,35 @@ def test_fetch_monthly_notification_statuses_per_service_for_rows_that_should_be

results = fetch_monthly_notification_statuses_per_service(date(2019, 3, 1), date(2019, 3, 31))
assert len(results) == 0


@freeze_time("2018-10-31T18:00:00")
def test_fetch_notification_status_for_service_for_today_handles_midnight_utc(
notify_db_session,
):
service_1 = create_service(service_name="service_1")
email_template = create_template(service=service_1, template_type=EMAIL_TYPE)

# create notifications that should not be included in count
create_ft_notification_status(date(2018, 10, 29), "email", service_1, count=30)
save_notification(create_notification(email_template, created_at=datetime(2018, 10, 30, 11, 59, 59), status="delivered"))

# create notifications that should be included in count
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, 1, 0, 0), status="delivered"))

# checking the daily stats for this day should give us the 2 created after 12am UTC
results = sorted(
fetch_notification_status_for_service_for_today_and_7_previous_days(service_1.id, limit_days=1),
key=lambda x: (x.notification_type, x.status),
)

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] == 3