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

Adjust start dates when fetching notification stats #2061

Merged
merged 4 commits into from
Dec 14, 2023
Merged

Adjust start dates when fetching notification stats #2061

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c5550f2
Adjust start dates when fetching notification stats
whabanks Dec 10, 2023
25c47fa
Merge branch 'main' into fix/report-notification-discrepancies
whabanks Dec 12, 2023
82abbbe
Fix tests
whabanks Dec 12, 2023
1d280a7
Merge branch 'main' into fix/report-notification-discrepancies
whabanks Dec 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
6 changes: 3 additions & 3 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 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,7 +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):
start_date = utc_midnight_n_days_ago(limit_days - 1)
start_date = utc_midnight_n_days_ago(limit_days)
Copy link
Contributor Author

@whabanks whabanks Dec 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I was hesitant to remove the - 1 here as we added it in this PR for an edge case.

Given this date is only compared to the bst_date in ft_notification_counts, which has no timezone data, we may not need the call to utc_midnight_n_days_ago anymore.


stats_for_7_days = db.session.query(
Copy link
Contributor Author

@whabanks whabanks Dec 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

An idea that crossed my mind to make this code a bit more testable is pulling the stats_for_7_days and stats_for_today out into their own methods. We could then reuse them in other contexts in the future and get a bit more granular and focused with our testing, considering this area of the code continues to be a pain-point.

Had this been the case from the beginning we would have caught on to stats_for_today pulling in more than the current day's notifications.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like that idea, if you want to do it in this PR feel free

FactNotificationStatus.notification_type.label("notification_type"),
Expand All @@ -260,7 +260,7 @@ def fetch_notification_status_for_service_for_today_and_7_previous_days(service_
*([func.count().label("count")]),
)
.filter(
Notification.created_at >= start_date,
Notification.created_at >= get_midnight(datetime.utcnow()),
Copy link
Contributor Author

@whabanks whabanks Dec 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This line was the core of the issue that inflated the notification counts. We weren't pulling just today's notifications in this query, but all notifications from starting from today - days of retention to today, doubling notification counts for those already entered into ft_notification_status.

This line was also why it seemed reasonable to start the investigation with jobs as it wasn't a 1:1 duplication but a duplication of all notifications - today's notifications making it seem like one off's weren't affected.

In retrospect, investigating significantly higher volume services could have helped steer us away from that sooner as it's less likely a service would be sending large volumes of one offs. Certainly possible, but less likely to make sense to do so.

Notification.service_id == service_id,
Notification.key_type != KEY_TYPE_TEST,
)
Expand Down
59 changes: 28 additions & 31 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 @@ -249,10 +249,9 @@ def test_fetch_notification_status_for_service_for_today_and_7_previous_days(
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, 24), "sms", service_1, count=8)
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, 26), "letter", service_1, count=5)

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)))
Expand All @@ -272,42 +271,38 @@ def test_fetch_notification_status_for_service_for_today_and_7_previous_days(
key=lambda x: (x.notification_type, x.status),
)

assert len(results) == 4
assert len(results) == 3

assert results[0].notification_type == "email"
assert results[0].status == "delivered"
assert results[0].count == 4

assert results[1].notification_type == "letter"
assert results[1].status == "delivered"
assert results[1].count == 5
assert results[1].notification_type == "sms"
assert results[1].status == "created"
assert results[1].count == 3

assert results[2].notification_type == "sms"
assert results[2].status == "created"
assert results[2].count == 3

assert results[3].notification_type == "sms"
assert results[3].status == "delivered"
assert results[3].count == 11
assert results[2].status == "delivered"
assert results[2].count == 19


@freeze_time("2018-10-31T18:00:00")
# This test assumes the local timezone is EST
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 Template 1", service=service_1, template_type=SMS_TYPE)
sms_template_2 = create_template(template_name="sms Template 2", service=service_1, template_type=SMS_TYPE)
email_template = create_template(service=service_1, template_type=EMAIL_TYPE)
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
create_template(service=service_1, template_type=EMAIL_TYPE)
create_template(template_name="UNUSED", service=service_1, template_type=EMAIL_TYPE)

# 30 sms
create_ft_notification_status(date(2018, 10, 29), "sms", service_1, count=10, billable_units=20)
create_ft_notification_status(date(2018, 10, 29), "sms", service_1, count=11, billable_units=11)
create_ft_notification_status(date(2018, 10, 28), "sms", service_1, count=11, billable_units=11)
create_ft_notification_status(date(2018, 10, 24), "sms", service_1, count=8)
create_ft_notification_status(date(2018, 10, 29), "sms", service_1, notification_status="created")
create_ft_notification_status(date(2018, 10, 27), "sms", service_1, notification_status="created")
create_ft_notification_status(date(2018, 10, 29), "email", service_1, count=3)
create_ft_notification_status(date(2018, 10, 26), "letter", service_1, count=5)

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"))
Expand All @@ -324,13 +319,13 @@ 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 Template Name", False, mock.ANY, "email", "delivered", 1),
("EMAIL NON-FT", False, mock.ANY, "email", "delivered", 1),
("email Template Name", False, mock.ANY, "email", "delivered", 3),
("letter Template Name", False, mock.ANY, "letter", "delivered", 5),
("sms Template 1", False, mock.ANY, "sms", "created", 1),
("SMS NON-FT", False, mock.ANY, "sms", "created", 1),
("sms Template Name", False, mock.ANY, "sms", "created", 1),
("sms Template 1", False, mock.ANY, "sms", "delivered", 1),
("sms Template 2", False, mock.ANY, "sms", "delivered", 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),
] == sorted(results, key=lambda x: (x.notification_type, x.status, x.template_name, x.count))
Expand Down Expand Up @@ -1143,7 +1138,7 @@ def test_fetch_monthly_notification_statuses_per_service_for_rows_that_should_be
assert len(results) == 0


@freeze_time("2018-10-31T18:00:00")
@freeze_time("2018-11-01T18:00:00")
def test_fetch_notification_status_for_service_for_today_handles_midnight_utc(
notify_db_session,
):
Expand All @@ -1152,13 +1147,16 @@ def test_fetch_notification_status_for_service_for_today_handles_midnight_utc(

# create notifications that should not be included in today's 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, 31, 23, 59, 59), status="delivered"))
save_notification(create_notification(email_template, created_at=datetime(2018, 10, 31, 11, 59, 59), status="delivered"))
save_notification(create_notification(email_template, created_at=datetime(2018, 10, 31, 11, 59, 59), status="delivered"))
save_notification(create_notification(email_template, created_at=datetime(2018, 10, 30, 23, 59, 59), status="delivered"))
save_notification(create_notification(email_template, created_at=datetime(2018, 10, 30, 11, 59, 59), status="delivered"))
save_notification(create_notification(email_template, created_at=datetime(2018, 10, 29, 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"))
create_ft_notification_status(date(2018, 10, 30), "email", service_1, count=5)

save_notification(create_notification(email_template, created_at=datetime(2018, 11, 1, 13, 0, 0), status="delivered"))
save_notification(create_notification(email_template, created_at=datetime(2018, 11, 1, 1, 0, 0), status="delivered"))

# checking the daily stats for this day should give us the 2 created after 12am UTC
results = sorted(
Expand All @@ -1173,5 +1171,4 @@ def test_fetch_notification_status_for_service_for_today_handles_midnight_utc(
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] == 4
assert results[0][2] == 7
Loading