From 53a413e7a5f5618b18bcacdf14aaeebef24cc221 Mon Sep 17 00:00:00 2001 From: Jumana B Date: Thu, 26 Sep 2024 15:32:22 -0400 Subject: [PATCH] Update the fetch for notifications to take the correct datetimes into account (#2302) * Change the datetimes - We want query the facts table for 6 days prev to the current day - We want to query notifications for todays data. due to timezones/ and when the job runs to populate facts, we will take the max existing date in facts from the last 6 days and use that as a starting point to query notifications for - We added a filter for notification_type as there is a part in the frontend that fetches this query only for a singular notification type * Pr comment --- app/dao/fact_notification_status_dao.py | 83 +++++-- .../dao/test_fact_notification_status_dao.py | 229 +++++++++--------- 2 files changed, 179 insertions(+), 133 deletions(-) diff --git a/app/dao/fact_notification_status_dao.py b/app/dao/fact_notification_status_dao.py index 7808b855ed..cd8ceba11d 100644 --- a/app/dao/fact_notification_status_dao.py +++ b/app/dao/fact_notification_status_dao.py @@ -1,4 +1,4 @@ -from datetime import datetime, time, timedelta +from datetime import datetime, time, timedelta, timezone from flask import current_app from sqlalchemy import Date, case, func @@ -7,7 +7,7 @@ from sqlalchemy.types import DateTime, Integer from app import db -from app.dao.date_util import tz_aware_midnight_n_days_ago, utc_midnight_n_days_ago +from app.dao.date_util import get_query_date_based_on_retention_period from app.models import ( EMAIL_TYPE, KEY_TYPE_NORMAL, @@ -246,32 +246,41 @@ 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): - if limit_days == 1: - ft_start_date = utc_midnight_n_days_ago(limit_days - 1) - # For daily stats, service limits reset at 12:00am UTC each night, so we need to fetch the data from 12:00 UTC to now - start = utc_midnight_n_days_ago(0) - end = datetime.utcnow() - else: - ft_start_date = utc_midnight_n_days_ago(limit_days) - - # The nightly task that populates ft_notification_status counts collects notifications from - # 5AM the day before to 5AM of the current day. So we need to match that timeframe when - # we fetch notifications for the current day. - start = (tz_aware_midnight_n_days_ago(1) + timedelta(hours=5)).replace(minute=0, second=0, microsecond=0) - end = (tz_aware_midnight_n_days_ago(0) + timedelta(hours=5)).replace(minute=0, second=0, microsecond=0) +def _stats_for_days_facts(service_id, start_time, by_template=False, notification_type=None): + """ + We want to take the data from the fact_notification_status table for bst_data>=start_date - stats_for_7_days = db.session.query( + Returns: + Aggregate data in a certain format for total notifications + """ + stats_from_facts = db.session.query( FactNotificationStatus.notification_type.label("notification_type"), FactNotificationStatus.notification_status.label("status"), *([FactNotificationStatus.template_id.label("template_id")] if by_template else []), *([FactNotificationStatus.notification_count.label("count")]), ).filter( FactNotificationStatus.service_id == service_id, - FactNotificationStatus.bst_date >= ft_start_date, + FactNotificationStatus.bst_date >= start_time, FactNotificationStatus.key_type != KEY_TYPE_TEST, ) + if notification_type: + stats_from_facts = stats_from_facts.filter(FactNotificationStatus.notification_type == notification_type) + + return stats_from_facts + + +def _timing_notification_table(service_id): + max_date_from_facts = ( + FactNotificationStatus.query.with_entities(func.max(FactNotificationStatus.bst_date)) + .filter(FactNotificationStatus.service_id == service_id) + .scalar() + ) + date_to_use = max_date_from_facts + timedelta(days=1) if max_date_from_facts else datetime.now(timezone.utc) + return datetime.combine(date_to_use, time.min) + + +def _stats_for_today(service_id, start_time, by_template=False, notification_type=None): stats_for_today = ( db.session.query( Notification.notification_type.cast(db.Text), @@ -280,8 +289,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 >= start_time, Notification.service_id == service_id, Notification.key_type != KEY_TYPE_TEST, ) @@ -291,8 +299,41 @@ def fetch_notification_status_for_service_for_today_and_7_previous_days(service_ Notification.status, ) ) + if notification_type: + stats_for_today = stats_for_today.filter(Notification.notification_type == notification_type) + + return stats_for_today + + +def fetch_notification_status_for_service_for_today_and_7_previous_days( + service_id, by_template=False, limit_days=7, notification_type=None +): + """ + We want to take the data from the fact_notification_status table and the notifications table and combine them + We will take the data from notifications ONLY for today and the fact_notification_status for the last 6 days. + In total we will have 7 days worth of data. + + As the data in facts is populated by a job, instead of + keeping track of the job - we will find the max date in the facts table and then use that date to get the + data from the notifications table. + + Args: + service_id (uuid): service_id + by_template (bool, optional): aggregate by template Defaults to False. + limit_days (int, optional): Number of days we want to get data for - it can depend on sensitive services. + Defaults to 7. + notification_type (str, optional): notification type. Defaults to None which means all notification types. + + Returns: + Aggregate data in a certain format for total notifications + """ + facts_notification_start_time = get_query_date_based_on_retention_period(limit_days) + stats_from_facts = _stats_for_days_facts(service_id, facts_notification_start_time, by_template, notification_type) + + start_time_notify_table = _timing_notification_table(service_id) + stats_for_today = _stats_for_today(service_id, start_time_notify_table, by_template, notification_type) - all_stats_table = stats_for_7_days.union_all(stats_for_today).subquery() + all_stats_table = stats_from_facts.union_all(stats_for_today).subquery() query = db.session.query( *( diff --git a/tests/app/dao/test_fact_notification_status_dao.py b/tests/app/dao/test_fact_notification_status_dao.py index 4103a2b62d..3c97bd3bf4 100644 --- a/tests/app/dao/test_fact_notification_status_dao.py +++ b/tests/app/dao/test_fact_notification_status_dao.py @@ -7,6 +7,7 @@ from notifications_utils.timezones import convert_utc_to_local_timezone from app.dao.fact_notification_status_dao import ( + _timing_notification_table, fetch_delivered_notification_stats_by_month, fetch_monthly_notification_statuses_per_service, fetch_monthly_template_usage_for_service, @@ -325,96 +326,133 @@ def test_fetch_notification_status_for_service_for_day(notify_db_session): assert results[1].count == 1 -@freeze_time("2018-10-31T18:00:00") -def test_fetch_notification_status_for_service_for_today_and_7_previous_days( - notify_db_session, -): - service_1 = create_service(service_name="service_1") - sms_template = create_template(service=service_1, template_type=SMS_TYPE) - sms_template_2 = create_template(service=service_1, template_type=SMS_TYPE) - email_template = create_template(service=service_1, template_type=EMAIL_TYPE) +class TestFetchNotificationStatusfortodayand7days: + @freeze_time("2018-10-31T18:00:00") + def test_fetch_notification_status_for_service_for_today_and_7_previous_days( + self, + notify_db_session, + ): + service_1 = create_service(service_name="service_1") + sms_template = create_template(service=service_1, template_type=SMS_TYPE) + 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) - 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, 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) + 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))) - 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")) + 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")) - # too early, shouldn't be included - save_notification( - create_notification( - service_1.templates[0], - created_at=datetime(2018, 10, 23, 12, 0, 0), - status="delivered", + # too early, shouldn't be included + save_notification( + create_notification( + service_1.templates[0], + created_at=datetime(2018, 10, 23, 12, 0, 0), + status="delivered", + ) + ) + results = sorted( + fetch_notification_status_for_service_for_today_and_7_previous_days(service_1.id), + key=lambda x: (x.notification_type, x.status), ) - ) - results = sorted( - fetch_notification_status_for_service_for_today_and_7_previous_days(service_1.id), - key=lambda x: (x.notification_type, x.status), - ) - - 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 == "sms" - assert results[1].status == "created" - assert results[1].count == 3 - assert results[2].notification_type == "sms" - assert results[2].status == "delivered" - assert results[2].count == 19 + assert len(results) == 3 + assert results[0].notification_type == "email" + assert results[0].status == "delivered" + assert results[0].count == 4 -@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 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) + assert results[1].notification_type == "sms" + assert results[1].status == "created" + assert results[1].count == 3 - # create unused email template - create_template(template_name="UNUSED", service=service_1, template_type=EMAIL_TYPE) + assert results[2].notification_type == "sms" + assert results[2].status == "delivered" + assert results[2].count == 11 - # 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, 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, 27), "sms", service_1, notification_status="created") - create_ft_notification_status(date(2018, 10, 29), "email", service_1, count=3) + @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(self, notify_db_session, notify_api): + # based on the freeze time, we will look at data in the facts table from 2018-10-24 + 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 + 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, 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, 27), "sms", service_1, notification_status="created") + 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))) + save_notification(create_notification(sms_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( + create_notification( + service_1.templates[0], + created_at=datetime(2018, 10, 23, 12, 0, 0), + status="delivered", + ) + ) + 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", "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", 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)) + + @freeze_time("2018-11-01T18:00:00") + def test_fetch_notification_status_for_service_for_today_handles_midnight_utc( + self, + 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 today's count + create_ft_notification_status(date(2018, 10, 24), "email", service_1, count=30) + create_ft_notification_status(date(2018, 10, 31), "email", service_1, count=20) + + save_notification(create_notification(email_template, created_at=datetime(2018, 10, 31, 0, 0, 0), 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, 31, 23, 59, 59), status="delivered")) + + # create notifications that should be included in count + 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, 6, 0, 0), status="delivered")) + save_notification(create_notification(email_template, created_at=datetime(2018, 11, 1, 17, 59, 59), status="delivered")) + + # checking the daily stats for this day should give us the 3 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] == 3 - 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(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")) + @freeze_time("2018-10-31T18:00:00") + def test_timing_for_facts(self, notify_db_session): + service_1 = create_service(service_name="service_1") + create_ft_notification_status(date(2018, 10, 24), "email", service_1, count=30) - # too early, shouldn't be included - save_notification( - create_notification( - service_1.templates[0], - created_at=datetime(2018, 10, 23, 12, 0, 0), - status="delivered", - ) - ) - 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", "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), - ] == sorted(results, key=lambda x: (x.notification_type, x.status, x.template_name, x.count)) + assert _timing_notification_table(service_1.id) == datetime(2018, 10, 25, 0, 0, 0) @freeze_time("2018-10-31T18:00:00") @@ -1307,36 +1345,3 @@ 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 - - -# Freezegun is currently unable of handling non-timezone naive dates: -# https://github.com/spulec/freezegun/issues/89 : https://github.com/spulec/freezegun/issues/487 -# So while the timeframe boundaries we're testing here are 5AM to 5AM UTC across 2 days, because the start/end dates -# are timezone aware our boundaries for the purpose of this test are 23h to 23h. -@freeze_time("2018-11-01T18: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 today's count - create_ft_notification_status(date(2018, 10, 24), "email", service_1, count=30) - create_ft_notification_status(date(2018, 10, 31), "email", service_1, count=20) - - save_notification(create_notification(email_template, created_at=datetime(2018, 10, 31, 0, 0, 0), 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, 31, 23, 59, 59), status="delivered")) - - # create notifications that should be included in count - 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, 6, 0, 0), status="delivered")) - save_notification(create_notification(email_template, created_at=datetime(2018, 11, 1, 17, 59, 59), status="delivered")) - - # checking the daily stats for this day should give us the 3 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] == 3