From f4e68d790318b73782681408b92862301af35fe4 Mon Sep 17 00:00:00 2001 From: Beverly Nguyen Date: Thu, 19 Dec 2024 17:53:04 -0800 Subject: [PATCH 01/48] add pending stats --- app/enums.py | 1 + app/service/statistics.py | 2 ++ 2 files changed, 3 insertions(+) diff --git a/app/enums.py b/app/enums.py index a0dfbb467..37b3b6892 100644 --- a/app/enums.py +++ b/app/enums.py @@ -211,3 +211,4 @@ class StatisticsType(StrEnum): REQUESTED = "requested" DELIVERED = "delivered" FAILURE = "failure" + PENDING = "pending" diff --git a/app/service/statistics.py b/app/service/statistics.py index a6b58e067..042927c3f 100644 --- a/app/service/statistics.py +++ b/app/service/statistics.py @@ -96,6 +96,8 @@ def _update_statuses_from_row(update_dict, row): NotificationStatus.VIRUS_SCAN_FAILED, ): update_dict[StatisticsType.FAILURE] += row.count + elif row.status == NotificationStatus.PENDING: + update_dict[StatisticsType.PENDING] += row.count def create_empty_monthly_notification_status_stats_dict(year): From 67a56626a4ecf6d412883c93460da895b5682c7e Mon Sep 17 00:00:00 2001 From: Beverly Nguyen Date: Mon, 30 Dec 2024 20:18:56 -0800 Subject: [PATCH 02/48] update pending count --- app/service/statistics.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/service/statistics.py b/app/service/statistics.py index 042927c3f..12814b970 100644 --- a/app/service/statistics.py +++ b/app/service/statistics.py @@ -96,7 +96,7 @@ def _update_statuses_from_row(update_dict, row): NotificationStatus.VIRUS_SCAN_FAILED, ): update_dict[StatisticsType.FAILURE] += row.count - elif row.status == NotificationStatus.PENDING: + elif row.status in (NotificationStatus.PENDING, NotificationStatus.CREATED, NotificationStatus.SENDING): update_dict[StatisticsType.PENDING] += row.count From 05055aa60a986c157cf980de9afdca095ebea1b3 Mon Sep 17 00:00:00 2001 From: Cliff Hill Date: Wed, 8 Jan 2025 15:41:26 -0500 Subject: [PATCH 03/48] Updated query Signed-off-by: Cliff Hill --- app/dao/services_dao.py | 21 +++++++++++++++++++-- app/service/statistics.py | 6 +++++- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 260008193..b81e54bc2 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -455,24 +455,41 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): start_date = get_midnight_in_utc(start_date) end_date = get_midnight_in_utc(end_date + timedelta(days=1)) - stmt = ( + sub_stmt = ( select( + Job.id, + Job.notification_count, NotificationAllTimeView.notification_type, NotificationAllTimeView.status, func.date_trunc("day", NotificationAllTimeView.created_at).label("day"), func.count(NotificationAllTimeView.id).label("count"), ) - .filter( + .join_from( + Notification, + Job, + ) + .where( NotificationAllTimeView.service_id == service_id, NotificationAllTimeView.key_type != KeyType.TEST, NotificationAllTimeView.created_at >= start_date, NotificationAllTimeView.created_at < end_date, ) .group_by( + Job.id, + Job.notification_count, NotificationAllTimeView.notification_type, NotificationAllTimeView.status, func.date_trunc("day", NotificationAllTimeView.created_at), ) + .subquery() + ) + + stmt = select( + func.sum(sub_stmt.notification_count).label("total_notifications"), + sub_stmt.notification_type, + sub_stmt.status, + sub_stmt.day, + func.sum(sub_stmt.count).label("count"), ) return db.session.execute(stmt).all() diff --git a/app/service/statistics.py b/app/service/statistics.py index 12814b970..8bd41da25 100644 --- a/app/service/statistics.py +++ b/app/service/statistics.py @@ -96,7 +96,11 @@ def _update_statuses_from_row(update_dict, row): NotificationStatus.VIRUS_SCAN_FAILED, ): update_dict[StatisticsType.FAILURE] += row.count - elif row.status in (NotificationStatus.PENDING, NotificationStatus.CREATED, NotificationStatus.SENDING): + elif row.status in ( + NotificationStatus.PENDING, + NotificationStatus.CREATED, + NotificationStatus.SENDING, + ): update_dict[StatisticsType.PENDING] += row.count From 37fa1187acdae5838a8bf18f115511a0a692f4f0 Mon Sep 17 00:00:00 2001 From: Cliff Hill Date: Thu, 9 Jan 2025 08:29:36 -0500 Subject: [PATCH 04/48] Adding group by clause to outer query for function. Signed-off-by: Cliff Hill --- app/dao/services_dao.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index b81e54bc2..099cb0da1 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -484,12 +484,19 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): .subquery() ) - stmt = select( - func.sum(sub_stmt.notification_count).label("total_notifications"), - sub_stmt.notification_type, - sub_stmt.status, - sub_stmt.day, - func.sum(sub_stmt.count).label("count"), + stmt = ( + select( + func.sum(sub_stmt.notification_count).label("total_notifications"), + sub_stmt.notification_type, + sub_stmt.status, + sub_stmt.day, + func.sum(sub_stmt.count).label("count"), + ) + .group_by( + sub_stmt.notification_type, + sub_stmt.status, + sub_stmt.day, + ) ) return db.session.execute(stmt).all() From 8656c44757855d3f2943739384ee0f7b5dd09850 Mon Sep 17 00:00:00 2001 From: Cliff Hill Date: Thu, 9 Jan 2025 13:02:21 -0500 Subject: [PATCH 05/48] Make sure join is correct table. Signed-off-by: Cliff Hill --- app/dao/services_dao.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 099cb0da1..2aa810504 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -465,7 +465,7 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): func.count(NotificationAllTimeView.id).label("count"), ) .join_from( - Notification, + NotificationAllTimeView, Job, ) .where( From b6337ad8074d4764ac77cea612d8800c4df49326 Mon Sep 17 00:00:00 2001 From: Beverly Nguyen Date: Fri, 10 Jan 2025 10:28:28 -0800 Subject: [PATCH 06/48] updating pending count --- app/dao/services_dao.py | 30 ++++++++++++++---------------- app/service/statistics.py | 21 ++++++++++++++------- 2 files changed, 28 insertions(+), 23 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 2aa810504..841434a62 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -2,7 +2,7 @@ import uuid from datetime import timedelta from flask import current_app -from sqlalchemy import Float, cast, delete, select +from sqlalchemy import Float, cast, delete, select, Integer from sqlalchemy.orm import joinedload from sqlalchemy.sql.expression import and_, asc, case, func @@ -458,16 +458,13 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): sub_stmt = ( select( Job.id, - Job.notification_count, + cast(Job.notification_count, Integer).label("notification_count"), # <-- i added cast here NotificationAllTimeView.notification_type, NotificationAllTimeView.status, func.date_trunc("day", NotificationAllTimeView.created_at).label("day"), - func.count(NotificationAllTimeView.id).label("count"), - ) - .join_from( - NotificationAllTimeView, - Job, + cast(func.count(NotificationAllTimeView.id), Integer).label("count"), # <-- i added cast here ) + .join_from(NotificationAllTimeView, Job, NotificationAllTimeView.job_id == Job.id) # <-- i changed this to NotificationAllTimeView from notifications .where( NotificationAllTimeView.service_id == service_id, NotificationAllTimeView.key_type != KeyType.TEST, @@ -486,21 +483,22 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): stmt = ( select( - func.sum(sub_stmt.notification_count).label("total_notifications"), - sub_stmt.notification_type, - sub_stmt.status, - sub_stmt.day, - func.sum(sub_stmt.count).label("count"), + cast(func.sum(sub_stmt.c.notification_count), Integer).label("total_notifications"), # <-- i added cast here + sub_stmt.c.notification_type, + sub_stmt.c.status, + sub_stmt.c.day, + cast(func.sum(sub_stmt.c.count), Integer).label("count"), # <-- i added cast here ) - .group_by( - sub_stmt.notification_type, - sub_stmt.status, - sub_stmt.day, + .group_by( # <-- i added this group here + sub_stmt.c.notification_type, + sub_stmt.c.status, + sub_stmt.c.day ) ) return db.session.execute(stmt).all() + def dao_fetch_stats_for_service_from_days_for_user( service_id, start_date, end_date, user_id ): diff --git a/app/service/statistics.py b/app/service/statistics.py index 8bd41da25..d2e6f9976 100644 --- a/app/service/statistics.py +++ b/app/service/statistics.py @@ -83,11 +83,21 @@ def create_zeroed_stats_dicts(): def _update_statuses_from_row(update_dict, row): + # Initialize pending_count to total_notifications + pending_count = row.total_notifications + + # Update requested count if row.status != NotificationStatus.CANCELLED: update_dict[StatisticsType.REQUESTED] += row.count + pending_count -= row.count # Subtract from pending_count + + # Update delivered count if row.status in (NotificationStatus.DELIVERED, NotificationStatus.SENT): update_dict[StatisticsType.DELIVERED] += row.count - elif row.status in ( + pending_count -= row.count # Subtract from pending_count + + # Update failure count + if row.status in ( NotificationStatus.FAILED, NotificationStatus.TECHNICAL_FAILURE, NotificationStatus.TEMPORARY_FAILURE, @@ -96,13 +106,10 @@ def _update_statuses_from_row(update_dict, row): NotificationStatus.VIRUS_SCAN_FAILED, ): update_dict[StatisticsType.FAILURE] += row.count - elif row.status in ( - NotificationStatus.PENDING, - NotificationStatus.CREATED, - NotificationStatus.SENDING, - ): - update_dict[StatisticsType.PENDING] += row.count + pending_count -= row.count # Subtract from pending_count + # Update pending count directly + update_dict[StatisticsType.PENDING] = pending_count def create_empty_monthly_notification_status_stats_dict(year): utc_month_starts = get_months_for_financial_year(year) From f1cc6aa400d4963313edd3be87159a4890261ed0 Mon Sep 17 00:00:00 2001 From: Cliff Hill Date: Fri, 10 Jan 2025 13:47:43 -0500 Subject: [PATCH 07/48] estructuring how to get total_notifications. Signed-off-by: Cliff Hill --- app/dao/services_dao.py | 26 ++++++++++++++++++++++---- app/service/rest.py | 7 ++++--- app/service/statistics.py | 23 ++++++++++++++--------- 3 files changed, 40 insertions(+), 16 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 841434a62..c4eba56a0 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -457,7 +457,7 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): sub_stmt = ( select( - Job.id, + Job.id.label("job_id"), cast(Job.notification_count, Integer).label("notification_count"), # <-- i added cast here NotificationAllTimeView.notification_type, NotificationAllTimeView.status, @@ -481,6 +481,24 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): .subquery() ) + # Getting the total notifications through this query. + + total_stmt = ( + select( + sub_stmt.c.job_id, + sub_stmt.c.notification_count, + ) + .group_by( + sub_stmt.c.job_id, + sub_stmt.c.notification_count, + ) + ) + + total_notifications = sum( + count + for __, count in db.session.execute(total_stmt).all() + ) + stmt = ( select( cast(func.sum(sub_stmt.c.notification_count), Integer).label("total_notifications"), # <-- i added cast here @@ -495,7 +513,7 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): sub_stmt.c.day ) ) - return db.session.execute(stmt).all() + return total_notifications, db.session.execute(stmt).all() @@ -742,7 +760,7 @@ def fetch_notification_stats_for_service_by_month_by_user( return db.session.execute(stmt).all() -def get_specific_days_stats(data, start_date, days=None, end_date=None): +def get_specific_days_stats(data, start_date, days=None, end_date=None, total_notifications=None): if days is not None and end_date is not None: raise ValueError("Only set days OR set end_date, not both.") elif days is not None: @@ -758,7 +776,7 @@ def get_specific_days_stats(data, start_date, days=None, end_date=None): } stats = { - day.strftime("%Y-%m-%d"): statistics.format_statistics(rows) + day.strftime("%Y-%m-%d"): statistics.format_statistics(rows, total_notifications=total_notifications,) for day, rows in grouped_data.items() } diff --git a/app/service/rest.py b/app/service/rest.py index 7dd614058..3bc27ccb3 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -230,9 +230,9 @@ def get_service_statistics_for_specific_days(service_id, start, days=1): end_date = datetime.strptime(start, "%Y-%m-%d") start_date = end_date - timedelta(days=days - 1) - results = dao_fetch_stats_for_service_from_days(service_id, start_date, end_date) + total_notifications, results = dao_fetch_stats_for_service_from_days(service_id, start_date, end_date,) - stats = get_specific_days_stats(results, start_date, days=days) + stats = get_specific_days_stats(results, start_date, days=days, total_notifications=total_notifications,) return stats @@ -678,7 +678,8 @@ def get_single_month_notification_stats_for_service(service_id): month_year = datetime(year, month, 10, 00, 00, 00) start_date, end_date = get_month_start_and_end_date_in_utc(month_year) - results = dao_fetch_stats_for_service_from_days(service_id, start_date, end_date) + # First element is total notifications used elsewhere. + __, results = dao_fetch_stats_for_service_from_days(service_id, start_date, end_date) stats = get_specific_days_stats(results, start_date, end_date=end_date) return jsonify(stats) diff --git a/app/service/statistics.py b/app/service/statistics.py index d2e6f9976..4103daba8 100644 --- a/app/service/statistics.py +++ b/app/service/statistics.py @@ -5,7 +5,7 @@ from app.dao.date_util import get_months_for_financial_year from app.enums import KeyType, NotificationStatus, StatisticsType, TemplateType -def format_statistics(statistics): +def format_statistics(statistics, total_notifications=None): # statistics come in a named tuple with uniqueness from 'notification_type', 'status' - however missing # statuses/notification types won't be represented and the status types need to be simplified/summed up # so we can return emails/sms * created, sent, and failed @@ -14,7 +14,7 @@ def format_statistics(statistics): # any row could be null, if the service either has no notifications in the notifications table, # or no historical data in the ft_notification_status table. if row.notification_type: - _update_statuses_from_row(counts[row.notification_type], row) + _update_statuses_from_row(counts[row.notification_type], row, total_notifications=total_notifications,) return counts @@ -82,19 +82,22 @@ def create_zeroed_stats_dicts(): } -def _update_statuses_from_row(update_dict, row): +def _update_statuses_from_row(update_dict, row, total_notifications=None): # Initialize pending_count to total_notifications - pending_count = row.total_notifications + if total_notifications is not None: + pending_count = total_notifications # Update requested count if row.status != NotificationStatus.CANCELLED: update_dict[StatisticsType.REQUESTED] += row.count - pending_count -= row.count # Subtract from pending_count + if total_notifications is not None: + pending_count -= row.count # Subtract from pending_count # Update delivered count if row.status in (NotificationStatus.DELIVERED, NotificationStatus.SENT): update_dict[StatisticsType.DELIVERED] += row.count - pending_count -= row.count # Subtract from pending_count + if total_notifications is not None: + pending_count -= row.count # Subtract from pending_count # Update failure count if row.status in ( @@ -106,10 +109,12 @@ def _update_statuses_from_row(update_dict, row): NotificationStatus.VIRUS_SCAN_FAILED, ): update_dict[StatisticsType.FAILURE] += row.count - pending_count -= row.count # Subtract from pending_count + if total_notifications is not None: + pending_count -= row.count # Subtract from pending_count - # Update pending count directly - update_dict[StatisticsType.PENDING] = pending_count + if total_notifications is not None: + # Update pending count directly + update_dict[StatisticsType.PENDING] = pending_count def create_empty_monthly_notification_status_stats_dict(year): utc_month_starts = get_months_for_financial_year(year) From ef61069101a37546d4be379a0f7ed3e5762c7894 Mon Sep 17 00:00:00 2001 From: Cliff Hill Date: Fri, 10 Jan 2025 13:55:01 -0500 Subject: [PATCH 08/48] black cleanup. Signed-off-by: Cliff Hill --- app/dao/services_dao.py | 64 ++++++++++++++++++++------------------- app/service/rest.py | 17 +++++++++-- app/service/statistics.py | 7 ++++- 3 files changed, 53 insertions(+), 35 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index c4eba56a0..16d16d4c6 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -2,7 +2,7 @@ import uuid from datetime import timedelta from flask import current_app -from sqlalchemy import Float, cast, delete, select, Integer +from sqlalchemy import Float, Integer, cast, delete, select from sqlalchemy.orm import joinedload from sqlalchemy.sql.expression import and_, asc, case, func @@ -458,13 +458,19 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): sub_stmt = ( select( Job.id.label("job_id"), - cast(Job.notification_count, Integer).label("notification_count"), # <-- i added cast here + cast(Job.notification_count, Integer).label( + "notification_count" + ), # <-- i added cast here NotificationAllTimeView.notification_type, NotificationAllTimeView.status, func.date_trunc("day", NotificationAllTimeView.created_at).label("day"), - cast(func.count(NotificationAllTimeView.id), Integer).label("count"), # <-- i added cast here + cast(func.count(NotificationAllTimeView.id), Integer).label( + "count" + ), # <-- i added cast here ) - .join_from(NotificationAllTimeView, Job, NotificationAllTimeView.job_id == Job.id) # <-- i changed this to NotificationAllTimeView from notifications + .join_from( + NotificationAllTimeView, Job, NotificationAllTimeView.job_id == Job.id + ) # <-- i changed this to NotificationAllTimeView from notifications .where( NotificationAllTimeView.service_id == service_id, NotificationAllTimeView.key_type != KeyType.TEST, @@ -483,40 +489,31 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): # Getting the total notifications through this query. - total_stmt = ( - select( - sub_stmt.c.job_id, - sub_stmt.c.notification_count, - ) - .group_by( - sub_stmt.c.job_id, - sub_stmt.c.notification_count, - ) + total_stmt = select( + sub_stmt.c.job_id, + sub_stmt.c.notification_count, + ).group_by( + sub_stmt.c.job_id, + sub_stmt.c.notification_count, ) total_notifications = sum( - count - for __, count in db.session.execute(total_stmt).all() + count for __, count in db.session.execute(total_stmt).all() ) - stmt = ( - select( - cast(func.sum(sub_stmt.c.notification_count), Integer).label("total_notifications"), # <-- i added cast here - sub_stmt.c.notification_type, - sub_stmt.c.status, - sub_stmt.c.day, - cast(func.sum(sub_stmt.c.count), Integer).label("count"), # <-- i added cast here - ) - .group_by( # <-- i added this group here - sub_stmt.c.notification_type, - sub_stmt.c.status, - sub_stmt.c.day - ) + stmt = select( + sub_stmt.c.notification_type, + sub_stmt.c.status, + sub_stmt.c.day, + cast(func.sum(sub_stmt.c.count), Integer).label( + "count" + ), # <-- i added cast here + ).group_by( # <-- i added this group here + sub_stmt.c.notification_type, sub_stmt.c.status, sub_stmt.c.day ) return total_notifications, db.session.execute(stmt).all() - def dao_fetch_stats_for_service_from_days_for_user( service_id, start_date, end_date, user_id ): @@ -760,7 +757,9 @@ def fetch_notification_stats_for_service_by_month_by_user( return db.session.execute(stmt).all() -def get_specific_days_stats(data, start_date, days=None, end_date=None, total_notifications=None): +def get_specific_days_stats( + data, start_date, days=None, end_date=None, total_notifications=None +): if days is not None and end_date is not None: raise ValueError("Only set days OR set end_date, not both.") elif days is not None: @@ -776,7 +775,10 @@ def get_specific_days_stats(data, start_date, days=None, end_date=None, total_no } stats = { - day.strftime("%Y-%m-%d"): statistics.format_statistics(rows, total_notifications=total_notifications,) + day.strftime("%Y-%m-%d"): statistics.format_statistics( + rows, + total_notifications=total_notifications, + ) for day, rows in grouped_data.items() } diff --git a/app/service/rest.py b/app/service/rest.py index 3bc27ccb3..748da7df1 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -230,9 +230,18 @@ def get_service_statistics_for_specific_days(service_id, start, days=1): end_date = datetime.strptime(start, "%Y-%m-%d") start_date = end_date - timedelta(days=days - 1) - total_notifications, results = dao_fetch_stats_for_service_from_days(service_id, start_date, end_date,) + total_notifications, results = dao_fetch_stats_for_service_from_days( + service_id, + start_date, + end_date, + ) - stats = get_specific_days_stats(results, start_date, days=days, total_notifications=total_notifications,) + stats = get_specific_days_stats( + results, + start_date, + days=days, + total_notifications=total_notifications, + ) return stats @@ -679,7 +688,9 @@ def get_single_month_notification_stats_for_service(service_id): start_date, end_date = get_month_start_and_end_date_in_utc(month_year) # First element is total notifications used elsewhere. - __, results = dao_fetch_stats_for_service_from_days(service_id, start_date, end_date) + __, results = dao_fetch_stats_for_service_from_days( + service_id, start_date, end_date + ) stats = get_specific_days_stats(results, start_date, end_date=end_date) return jsonify(stats) diff --git a/app/service/statistics.py b/app/service/statistics.py index 4103daba8..d359d5f04 100644 --- a/app/service/statistics.py +++ b/app/service/statistics.py @@ -14,7 +14,11 @@ def format_statistics(statistics, total_notifications=None): # any row could be null, if the service either has no notifications in the notifications table, # or no historical data in the ft_notification_status table. if row.notification_type: - _update_statuses_from_row(counts[row.notification_type], row, total_notifications=total_notifications,) + _update_statuses_from_row( + counts[row.notification_type], + row, + total_notifications=total_notifications, + ) return counts @@ -116,6 +120,7 @@ def _update_statuses_from_row(update_dict, row, total_notifications=None): # Update pending count directly update_dict[StatisticsType.PENDING] = pending_count + def create_empty_monthly_notification_status_stats_dict(year): utc_month_starts = get_months_for_financial_year(year) # nested dicts - data[month][template type][status] = count From bc8084121227353399d4dd6df5a83efdc126ab55 Mon Sep 17 00:00:00 2001 From: Cliff Hill Date: Fri, 10 Jan 2025 14:29:46 -0500 Subject: [PATCH 09/48] Changing the queries again. Signed-off-by: Cliff Hill --- app/dao/services_dao.py | 59 ++++++++++++++++++----------------------- 1 file changed, 26 insertions(+), 33 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 16d16d4c6..41e920cf5 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -455,18 +455,13 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): start_date = get_midnight_in_utc(start_date) end_date = get_midnight_in_utc(end_date + timedelta(days=1)) - sub_stmt = ( + # Getting the total notifications through this query. + + total_stmt = select( select( - Job.id.label("job_id"), cast(Job.notification_count, Integer).label( "notification_count" ), # <-- i added cast here - NotificationAllTimeView.notification_type, - NotificationAllTimeView.status, - func.date_trunc("day", NotificationAllTimeView.created_at).label("day"), - cast(func.count(NotificationAllTimeView.id), Integer).label( - "count" - ), # <-- i added cast here ) .join_from( NotificationAllTimeView, Job, NotificationAllTimeView.job_id == Job.id @@ -480,38 +475,36 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): .group_by( Job.id, Job.notification_count, + ) + ) + + total_notifications = sum(db.session.execute(total_stmt).scalars()) + + stmt = ( + select( + NotificationAllTimeView.notification_type, + NotificationAllTimeView.status, + func.date_trunc("day", NotificationAllTimeView.created_at).label("day"), + cast(func.count(NotificationAllTimeView.id), Integer).label( + "count" + ), # <-- i added cast here + ) + .where( + NotificationAllTimeView.service_id == service_id, + NotificationAllTimeView.key_type != KeyType.TEST, + NotificationAllTimeView.created_at >= start_date, + NotificationAllTimeView.created_at < end_date, + ) + .group_by( NotificationAllTimeView.notification_type, NotificationAllTimeView.status, func.date_trunc("day", NotificationAllTimeView.created_at), ) - .subquery() ) - # Getting the total notifications through this query. + data = db.session.execute(stmt).all() - total_stmt = select( - sub_stmt.c.job_id, - sub_stmt.c.notification_count, - ).group_by( - sub_stmt.c.job_id, - sub_stmt.c.notification_count, - ) - - total_notifications = sum( - count for __, count in db.session.execute(total_stmt).all() - ) - - stmt = select( - sub_stmt.c.notification_type, - sub_stmt.c.status, - sub_stmt.c.day, - cast(func.sum(sub_stmt.c.count), Integer).label( - "count" - ), # <-- i added cast here - ).group_by( # <-- i added this group here - sub_stmt.c.notification_type, sub_stmt.c.status, sub_stmt.c.day - ) - return total_notifications, db.session.execute(stmt).all() + return total_notifications, data def dao_fetch_stats_for_service_from_days_for_user( From 162823e59bf896b9b10393881f02e899b3ac35d5 Mon Sep 17 00:00:00 2001 From: Cliff Hill Date: Fri, 10 Jan 2025 14:46:59 -0500 Subject: [PATCH 10/48] Query fixing. Signed-off-by: Cliff Hill --- app/dao/services_dao.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 41e920cf5..e9171d1fc 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -457,7 +457,7 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): # Getting the total notifications through this query. - total_stmt = select( + total_stmt = ( select( cast(Job.notification_count, Integer).label( "notification_count" From 92190491504d7a0000a9a3bc28fd0d076fdceed9 Mon Sep 17 00:00:00 2001 From: Cliff Hill Date: Fri, 10 Jan 2025 15:08:44 -0500 Subject: [PATCH 11/48] Cleaning up function a bit. Signed-off-by: Cliff Hill --- app/service/statistics.py | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/app/service/statistics.py b/app/service/statistics.py index d359d5f04..11e19f16b 100644 --- a/app/service/statistics.py +++ b/app/service/statistics.py @@ -87,21 +87,19 @@ def create_zeroed_stats_dicts(): def _update_statuses_from_row(update_dict, row, total_notifications=None): - # Initialize pending_count to total_notifications - if total_notifications is not None: - pending_count = total_notifications + requested_count = 0 + delivered_count = 0 + failed_count = 0 # Update requested count if row.status != NotificationStatus.CANCELLED: update_dict[StatisticsType.REQUESTED] += row.count - if total_notifications is not None: - pending_count -= row.count # Subtract from pending_count + requested_count += row.count # Update delivered count if row.status in (NotificationStatus.DELIVERED, NotificationStatus.SENT): update_dict[StatisticsType.DELIVERED] += row.count - if total_notifications is not None: - pending_count -= row.count # Subtract from pending_count + delivered_count += row.count # Update failure count if row.status in ( @@ -113,11 +111,11 @@ def _update_statuses_from_row(update_dict, row, total_notifications=None): NotificationStatus.VIRUS_SCAN_FAILED, ): update_dict[StatisticsType.FAILURE] += row.count - if total_notifications is not None: - pending_count -= row.count # Subtract from pending_count + failed_count += row.count if total_notifications is not None: # Update pending count directly + pending_count = total_notifications - (requested_count + delivered_count + failed_count) update_dict[StatisticsType.PENDING] = pending_count From b543db474fc33bca9a2efa7994a4fb7799927c94 Mon Sep 17 00:00:00 2001 From: Cliff Hill Date: Fri, 10 Jan 2025 16:07:49 -0500 Subject: [PATCH 12/48] Moving where the pending calculation is done. Signed-off-by: Cliff Hill --- app/dao/services_dao.py | 11 +++++++++-- app/service/statistics.py | 19 +++++++++++-------- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index e9171d1fc..93b2c93ac 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -457,8 +457,9 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): # Getting the total notifications through this query. - total_stmt = ( + total_substmt = ( select( + func.date_trunc("day", NotificationAllTimeView.created_at).label("day"), cast(Job.notification_count, Integer).label( "notification_count" ), # <-- i added cast here @@ -475,10 +476,16 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): .group_by( Job.id, Job.notification_count, + func.date_trunc("day", NotificationAllTimeView.created_at), ) + .subquery() ) - total_notifications = sum(db.session.execute(total_stmt).scalars()) + total_stmt = select( + func.sum(total_substmt.c.notification_count).label("total_notifications") + ) + + total_notifications = db.session.execute(total_stmt).scalar_one() stmt = ( select( diff --git a/app/service/statistics.py b/app/service/statistics.py index 11e19f16b..593067745 100644 --- a/app/service/statistics.py +++ b/app/service/statistics.py @@ -2,7 +2,7 @@ from collections import defaultdict from datetime import datetime from app.dao.date_util import get_months_for_financial_year -from app.enums import KeyType, NotificationStatus, StatisticsType, TemplateType +from app.enums import KeyType, NotificationStatus, NotificationType, StatisticsType, TemplateType def format_statistics(statistics, total_notifications=None): @@ -17,9 +17,17 @@ def format_statistics(statistics, total_notifications=None): _update_statuses_from_row( counts[row.notification_type], row, - total_notifications=total_notifications, ) + # Update pending count directly + if NotificationType.SMS in counts and total_notifications is not None: + sms_dict = counts[NotificationType.SMS] + requested_count = sms_dict[StatisticsType.REQUESTED] + delivered_count = sms_dict[StatisticsType.DELIVERED] + failed_count = sms_dict[StatisticsType.FAILURE] + pending_count = total_notifications - (requested_count + delivered_count + failed_count) + sms_dict[StatisticsType.PENDING] = pending_count + return counts @@ -86,7 +94,7 @@ def create_zeroed_stats_dicts(): } -def _update_statuses_from_row(update_dict, row, total_notifications=None): +def _update_statuses_from_row(update_dict, row): requested_count = 0 delivered_count = 0 failed_count = 0 @@ -113,11 +121,6 @@ def _update_statuses_from_row(update_dict, row, total_notifications=None): update_dict[StatisticsType.FAILURE] += row.count failed_count += row.count - if total_notifications is not None: - # Update pending count directly - pending_count = total_notifications - (requested_count + delivered_count + failed_count) - update_dict[StatisticsType.PENDING] = pending_count - def create_empty_monthly_notification_status_stats_dict(year): utc_month_starts = get_months_for_financial_year(year) From 467357c0319fff965b300be6ee61e9680c43eb6d Mon Sep 17 00:00:00 2001 From: Cliff Hill Date: Fri, 10 Jan 2025 16:21:15 -0500 Subject: [PATCH 13/48] Getting total notifications per day made. Signed-off-by: Cliff Hill --- app/dao/services_dao.py | 13 ++++++++++--- app/service/statistics.py | 12 ++++++++++-- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 93b2c93ac..739f385ac 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -482,10 +482,13 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): ) total_stmt = select( - func.sum(total_substmt.c.notification_count).label("total_notifications") + func.date_trunc("day", NotificationAllTimeView.created_at).label("day"), + func.sum(total_substmt.c.notification_count).label("total_notifications"), + ).group_by( + func.date_trunc("day", NotificationAllTimeView.created_at), ) - total_notifications = db.session.execute(total_stmt).scalar_one() + total_notifications = {day: count for day, count in db.session.execute(total_stmt)} stmt = ( select( @@ -777,7 +780,11 @@ def get_specific_days_stats( stats = { day.strftime("%Y-%m-%d"): statistics.format_statistics( rows, - total_notifications=total_notifications, + total_notifications=( + total_notifications.get(day, 0) + if total_notifications is not None + else None + ), ) for day, rows in grouped_data.items() } diff --git a/app/service/statistics.py b/app/service/statistics.py index 593067745..68ba4f3ca 100644 --- a/app/service/statistics.py +++ b/app/service/statistics.py @@ -2,7 +2,13 @@ from collections import defaultdict from datetime import datetime from app.dao.date_util import get_months_for_financial_year -from app.enums import KeyType, NotificationStatus, NotificationType, StatisticsType, TemplateType +from app.enums import ( + KeyType, + NotificationStatus, + NotificationType, + StatisticsType, + TemplateType, +) def format_statistics(statistics, total_notifications=None): @@ -25,7 +31,9 @@ def format_statistics(statistics, total_notifications=None): requested_count = sms_dict[StatisticsType.REQUESTED] delivered_count = sms_dict[StatisticsType.DELIVERED] failed_count = sms_dict[StatisticsType.FAILURE] - pending_count = total_notifications - (requested_count + delivered_count + failed_count) + pending_count = total_notifications - ( + requested_count + delivered_count + failed_count + ) sms_dict[StatisticsType.PENDING] = pending_count return counts From 3f8c49d829483bdb661c7a94ab8f0da7b3436906 Mon Sep 17 00:00:00 2001 From: Cliff Hill Date: Fri, 10 Jan 2025 16:52:30 -0500 Subject: [PATCH 14/48] Removing total_notification calculations. Signed-off-by: Cliff Hill --- app/dao/services_dao.py | 50 +++------------------------------------ app/service/rest.py | 9 ++----- app/service/statistics.py | 14 +---------- 3 files changed, 6 insertions(+), 67 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 739f385ac..7fdac4213 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -455,41 +455,6 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): start_date = get_midnight_in_utc(start_date) end_date = get_midnight_in_utc(end_date + timedelta(days=1)) - # Getting the total notifications through this query. - - total_substmt = ( - select( - func.date_trunc("day", NotificationAllTimeView.created_at).label("day"), - cast(Job.notification_count, Integer).label( - "notification_count" - ), # <-- i added cast here - ) - .join_from( - NotificationAllTimeView, Job, NotificationAllTimeView.job_id == Job.id - ) # <-- i changed this to NotificationAllTimeView from notifications - .where( - NotificationAllTimeView.service_id == service_id, - NotificationAllTimeView.key_type != KeyType.TEST, - NotificationAllTimeView.created_at >= start_date, - NotificationAllTimeView.created_at < end_date, - ) - .group_by( - Job.id, - Job.notification_count, - func.date_trunc("day", NotificationAllTimeView.created_at), - ) - .subquery() - ) - - total_stmt = select( - func.date_trunc("day", NotificationAllTimeView.created_at).label("day"), - func.sum(total_substmt.c.notification_count).label("total_notifications"), - ).group_by( - func.date_trunc("day", NotificationAllTimeView.created_at), - ) - - total_notifications = {day: count for day, count in db.session.execute(total_stmt)} - stmt = ( select( NotificationAllTimeView.notification_type, @@ -514,7 +479,7 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): data = db.session.execute(stmt).all() - return total_notifications, data + return data def dao_fetch_stats_for_service_from_days_for_user( @@ -760,9 +725,7 @@ def fetch_notification_stats_for_service_by_month_by_user( return db.session.execute(stmt).all() -def get_specific_days_stats( - data, start_date, days=None, end_date=None, total_notifications=None -): +def get_specific_days_stats(data, start_date, days=None, end_date=None): if days is not None and end_date is not None: raise ValueError("Only set days OR set end_date, not both.") elif days is not None: @@ -778,14 +741,7 @@ def get_specific_days_stats( } stats = { - day.strftime("%Y-%m-%d"): statistics.format_statistics( - rows, - total_notifications=( - total_notifications.get(day, 0) - if total_notifications is not None - else None - ), - ) + day.strftime("%Y-%m-%d"): statistics.format_statistics(rows) for day, rows in grouped_data.items() } diff --git a/app/service/rest.py b/app/service/rest.py index 748da7df1..d49142788 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -230,18 +230,13 @@ def get_service_statistics_for_specific_days(service_id, start, days=1): end_date = datetime.strptime(start, "%Y-%m-%d") start_date = end_date - timedelta(days=days - 1) - total_notifications, results = dao_fetch_stats_for_service_from_days( + results = dao_fetch_stats_for_service_from_days( service_id, start_date, end_date, ) - stats = get_specific_days_stats( - results, - start_date, - days=days, - total_notifications=total_notifications, - ) + stats = get_specific_days_stats(results, start_date, days=days) return stats diff --git a/app/service/statistics.py b/app/service/statistics.py index 68ba4f3ca..68d137876 100644 --- a/app/service/statistics.py +++ b/app/service/statistics.py @@ -5,13 +5,12 @@ from app.dao.date_util import get_months_for_financial_year from app.enums import ( KeyType, NotificationStatus, - NotificationType, StatisticsType, TemplateType, ) -def format_statistics(statistics, total_notifications=None): +def format_statistics(statistics): # statistics come in a named tuple with uniqueness from 'notification_type', 'status' - however missing # statuses/notification types won't be represented and the status types need to be simplified/summed up # so we can return emails/sms * created, sent, and failed @@ -25,17 +24,6 @@ def format_statistics(statistics, total_notifications=None): row, ) - # Update pending count directly - if NotificationType.SMS in counts and total_notifications is not None: - sms_dict = counts[NotificationType.SMS] - requested_count = sms_dict[StatisticsType.REQUESTED] - delivered_count = sms_dict[StatisticsType.DELIVERED] - failed_count = sms_dict[StatisticsType.FAILURE] - pending_count = total_notifications - ( - requested_count + delivered_count + failed_count - ) - sms_dict[StatisticsType.PENDING] = pending_count - return counts From 7ef808196eacd2eb32081df08c9ed666eed6124a Mon Sep 17 00:00:00 2001 From: Beverly Nguyen Date: Fri, 10 Jan 2025 18:18:20 -0800 Subject: [PATCH 15/48] pending count --- app/dao/services_dao.py | 60 +++++++++++++++++++++++++++++++++------ app/enums.py | 2 ++ app/service/rest.py | 9 ++++-- app/service/statistics.py | 17 ++++++++--- 4 files changed, 73 insertions(+), 15 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 7fdac4213..a2ec31f34 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -455,14 +455,49 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): start_date = get_midnight_in_utc(start_date) end_date = get_midnight_in_utc(end_date + timedelta(days=1)) + # Subquery for daily total notifications + total_substmt = ( + select( + func.date_trunc("day", NotificationAllTimeView.created_at).label("day"), + cast(Job.notification_count, Integer).label("notification_count") + ) + .join( + Job, NotificationAllTimeView.job_id == Job.id + ) + .where( + NotificationAllTimeView.service_id == service_id, + NotificationAllTimeView.key_type != KeyType.TEST, + NotificationAllTimeView.created_at >= start_date, + NotificationAllTimeView.created_at < end_date, + ) + .group_by( + Job.id, + Job.notification_count, + func.date_trunc("day", NotificationAllTimeView.created_at), + ) + .subquery() + ) + + # Query for daily total notifications + total_stmt = select( + func.date_trunc("day", total_substmt.c.day).label("day"), + func.sum(total_substmt.c.notification_count).label("total_notifications"), + ).group_by( + total_substmt.c.day + ) + + # Execute both queries + total_notifications = { + row.day: row.total_notifications for row in db.session.execute(total_stmt).all() + } + + # Query for breakdown by notification type and status stmt = ( select( NotificationAllTimeView.notification_type, NotificationAllTimeView.status, func.date_trunc("day", NotificationAllTimeView.created_at).label("day"), - cast(func.count(NotificationAllTimeView.id), Integer).label( - "count" - ), # <-- i added cast here + cast(func.count(NotificationAllTimeView.id), Integer).label("count"), ) .where( NotificationAllTimeView.service_id == service_id, @@ -479,8 +514,9 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): data = db.session.execute(stmt).all() - return data + print("Daily Total Notifications:", total_notifications) + return total_notifications, data def dao_fetch_stats_for_service_from_days_for_user( service_id, start_date, end_date, user_id @@ -725,7 +761,7 @@ def fetch_notification_stats_for_service_by_month_by_user( return db.session.execute(stmt).all() -def get_specific_days_stats(data, start_date, days=None, end_date=None): +def get_specific_days_stats(data, start_date, days=None, end_date=None,total_notifications=None): if days is not None and end_date is not None: raise ValueError("Only set days OR set end_date, not both.") elif days is not None: @@ -736,13 +772,19 @@ def get_specific_days_stats(data, start_date, days=None, end_date=None): raise ValueError("Either days or end_date must be set.") grouped_data = {date: [] for date in gen_range} | { - day: [row for row in data if row.day.date() == day] - for day in {item.day.date() for item in data} + day: [row for row in data if row.day == day] + for day in {item.day for item in data} } stats = { - day.strftime("%Y-%m-%d"): statistics.format_statistics(rows) + day.strftime("%Y-%m-%d"): statistics.format_statistics( + rows, + total_notifications=( + total_notifications.get(day, 0) + if total_notifications is not None + else None + ), + ) for day, rows in grouped_data.items() } - return stats diff --git a/app/enums.py b/app/enums.py index 37b3b6892..e69678671 100644 --- a/app/enums.py +++ b/app/enums.py @@ -212,3 +212,5 @@ class StatisticsType(StrEnum): DELIVERED = "delivered" FAILURE = "failure" PENDING = "pending" + SENDING = "sending" + CREATED = "created" diff --git a/app/service/rest.py b/app/service/rest.py index d49142788..748da7df1 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -230,13 +230,18 @@ def get_service_statistics_for_specific_days(service_id, start, days=1): end_date = datetime.strptime(start, "%Y-%m-%d") start_date = end_date - timedelta(days=days - 1) - results = dao_fetch_stats_for_service_from_days( + total_notifications, results = dao_fetch_stats_for_service_from_days( service_id, start_date, end_date, ) - stats = get_specific_days_stats(results, start_date, days=days) + stats = get_specific_days_stats( + results, + start_date, + days=days, + total_notifications=total_notifications, + ) return stats diff --git a/app/service/statistics.py b/app/service/statistics.py index 68d137876..41ced13ec 100644 --- a/app/service/statistics.py +++ b/app/service/statistics.py @@ -7,10 +7,11 @@ from app.enums import ( NotificationStatus, StatisticsType, TemplateType, + NotificationType ) -def format_statistics(statistics): +def format_statistics(statistics, total_notifications=None): # statistics come in a named tuple with uniqueness from 'notification_type', 'status' - however missing # statuses/notification types won't be represented and the status types need to be simplified/summed up # so we can return emails/sms * created, sent, and failed @@ -24,8 +25,18 @@ def format_statistics(statistics): row, ) - return counts + if NotificationType.SMS in counts and total_notifications is not None: + sms_dict = counts[NotificationType.SMS] + delivered_count = sms_dict[StatisticsType.DELIVERED] + failed_count = sms_dict[StatisticsType.FAILURE] + print('total_notifications',total_notifications) + pending_count = total_notifications - (delivered_count + failed_count) + pending_count = max(0, pending_count) + + sms_dict[StatisticsType.PENDING] = pending_count + + return counts def format_admin_stats(statistics): counts = create_stats_dict() @@ -98,12 +109,10 @@ def _update_statuses_from_row(update_dict, row): # Update requested count if row.status != NotificationStatus.CANCELLED: update_dict[StatisticsType.REQUESTED] += row.count - requested_count += row.count # Update delivered count if row.status in (NotificationStatus.DELIVERED, NotificationStatus.SENT): update_dict[StatisticsType.DELIVERED] += row.count - delivered_count += row.count # Update failure count if row.status in ( From 30dfc6a571113be60ebc12f1336a82d3ad3f0692 Mon Sep 17 00:00:00 2001 From: Beverly Nguyen Date: Fri, 10 Jan 2025 18:29:16 -0800 Subject: [PATCH 16/48] pending count --- app/dao/services_dao.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index a2ec31f34..f480b4852 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -2,7 +2,7 @@ import uuid from datetime import timedelta from flask import current_app -from sqlalchemy import Float, Integer, cast, delete, select +from sqlalchemy import Float, cast, delete, select from sqlalchemy.orm import joinedload from sqlalchemy.sql.expression import and_, asc, case, func @@ -459,7 +459,7 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): total_substmt = ( select( func.date_trunc("day", NotificationAllTimeView.created_at).label("day"), - cast(Job.notification_count, Integer).label("notification_count") + Job.notification_count.label("notification_count") ) .join( Job, NotificationAllTimeView.job_id == Job.id @@ -497,7 +497,7 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): NotificationAllTimeView.notification_type, NotificationAllTimeView.status, func.date_trunc("day", NotificationAllTimeView.created_at).label("day"), - cast(func.count(NotificationAllTimeView.id), Integer).label("count"), + func.count(NotificationAllTimeView.id).label("count"), ) .where( NotificationAllTimeView.service_id == service_id, From af46a671f9dbc5217ab214106a75b6d946195de6 Mon Sep 17 00:00:00 2001 From: Beverly Nguyen Date: Fri, 10 Jan 2025 18:41:09 -0800 Subject: [PATCH 17/48] cleaningu up pending --- app/dao/services_dao.py | 4 +--- app/enums.py | 2 -- app/service/statistics.py | 13 +------------ 3 files changed, 2 insertions(+), 17 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index f480b4852..09322a464 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -480,7 +480,7 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): # Query for daily total notifications total_stmt = select( - func.date_trunc("day", total_substmt.c.day).label("day"), + total_substmt.c.day, func.sum(total_substmt.c.notification_count).label("total_notifications"), ).group_by( total_substmt.c.day @@ -514,8 +514,6 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): data = db.session.execute(stmt).all() - print("Daily Total Notifications:", total_notifications) - return total_notifications, data def dao_fetch_stats_for_service_from_days_for_user( diff --git a/app/enums.py b/app/enums.py index e69678671..37b3b6892 100644 --- a/app/enums.py +++ b/app/enums.py @@ -212,5 +212,3 @@ class StatisticsType(StrEnum): DELIVERED = "delivered" FAILURE = "failure" PENDING = "pending" - SENDING = "sending" - CREATED = "created" diff --git a/app/service/statistics.py b/app/service/statistics.py index 41ced13ec..a6d87da9f 100644 --- a/app/service/statistics.py +++ b/app/service/statistics.py @@ -29,7 +29,6 @@ def format_statistics(statistics, total_notifications=None): sms_dict = counts[NotificationType.SMS] delivered_count = sms_dict[StatisticsType.DELIVERED] failed_count = sms_dict[StatisticsType.FAILURE] - print('total_notifications',total_notifications) pending_count = total_notifications - (delivered_count + failed_count) pending_count = max(0, pending_count) @@ -102,20 +101,11 @@ def create_zeroed_stats_dicts(): def _update_statuses_from_row(update_dict, row): - requested_count = 0 - delivered_count = 0 - failed_count = 0 - - # Update requested count if row.status != NotificationStatus.CANCELLED: update_dict[StatisticsType.REQUESTED] += row.count - - # Update delivered count if row.status in (NotificationStatus.DELIVERED, NotificationStatus.SENT): update_dict[StatisticsType.DELIVERED] += row.count - - # Update failure count - if row.status in ( + elif row.status in ( NotificationStatus.FAILED, NotificationStatus.TECHNICAL_FAILURE, NotificationStatus.TEMPORARY_FAILURE, @@ -124,7 +114,6 @@ def _update_statuses_from_row(update_dict, row): NotificationStatus.VIRUS_SCAN_FAILED, ): update_dict[StatisticsType.FAILURE] += row.count - failed_count += row.count def create_empty_monthly_notification_status_stats_dict(year): From 50a183f05defac5efe89c321bfe9b44353b47dea Mon Sep 17 00:00:00 2001 From: Beverly Nguyen Date: Fri, 10 Jan 2025 18:42:43 -0800 Subject: [PATCH 18/48] flake --- app/dao/services_dao.py | 3 ++- app/service/statistics.py | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 09322a464..a29dd464a 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -516,6 +516,7 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): return total_notifications, data + def dao_fetch_stats_for_service_from_days_for_user( service_id, start_date, end_date, user_id ): @@ -759,7 +760,7 @@ def fetch_notification_stats_for_service_by_month_by_user( return db.session.execute(stmt).all() -def get_specific_days_stats(data, start_date, days=None, end_date=None,total_notifications=None): +def get_specific_days_stats(data, start_date, days=None, end_date=None, total_notifications=None): if days is not None and end_date is not None: raise ValueError("Only set days OR set end_date, not both.") elif days is not None: diff --git a/app/service/statistics.py b/app/service/statistics.py index a6d87da9f..a1c34ea65 100644 --- a/app/service/statistics.py +++ b/app/service/statistics.py @@ -37,6 +37,7 @@ def format_statistics(statistics, total_notifications=None): return counts + def format_admin_stats(statistics): counts = create_stats_dict() From 2eef7ab206a2d4ce4ba598a5b7c55b6b3af882f5 Mon Sep 17 00:00:00 2001 From: Beverly Nguyen Date: Fri, 10 Jan 2025 18:45:05 -0800 Subject: [PATCH 19/48] remove comments --- app/dao/services_dao.py | 4 ---- app/service/rest.py | 1 - 2 files changed, 5 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index a29dd464a..ba3dbf717 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -455,7 +455,6 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): start_date = get_midnight_in_utc(start_date) end_date = get_midnight_in_utc(end_date + timedelta(days=1)) - # Subquery for daily total notifications total_substmt = ( select( func.date_trunc("day", NotificationAllTimeView.created_at).label("day"), @@ -478,7 +477,6 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): .subquery() ) - # Query for daily total notifications total_stmt = select( total_substmt.c.day, func.sum(total_substmt.c.notification_count).label("total_notifications"), @@ -486,12 +484,10 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): total_substmt.c.day ) - # Execute both queries total_notifications = { row.day: row.total_notifications for row in db.session.execute(total_stmt).all() } - # Query for breakdown by notification type and status stmt = ( select( NotificationAllTimeView.notification_type, diff --git a/app/service/rest.py b/app/service/rest.py index 748da7df1..718e3bd33 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -687,7 +687,6 @@ def get_single_month_notification_stats_for_service(service_id): month_year = datetime(year, month, 10, 00, 00, 00) start_date, end_date = get_month_start_and_end_date_in_utc(month_year) - # First element is total notifications used elsewhere. __, results = dao_fetch_stats_for_service_from_days( service_id, start_date, end_date ) From fa00bd14bf352a5a7fdb7d876dcca12def3e9254 Mon Sep 17 00:00:00 2001 From: Beverly Nguyen Date: Fri, 10 Jan 2025 18:47:39 -0800 Subject: [PATCH 20/48] isort --- app/service/statistics.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/service/statistics.py b/app/service/statistics.py index a1c34ea65..b67107ab1 100644 --- a/app/service/statistics.py +++ b/app/service/statistics.py @@ -5,9 +5,9 @@ from app.dao.date_util import get_months_for_financial_year from app.enums import ( KeyType, NotificationStatus, + NotificationType, StatisticsType, TemplateType, - NotificationType ) From dd3c2779858e561e13ca8dd8fc09773bd03129cf Mon Sep 17 00:00:00 2001 From: Beverly Nguyen Date: Fri, 10 Jan 2025 19:26:46 -0800 Subject: [PATCH 21/48] fix testing --- app/dao/services_dao.py | 14 ++++++-------- tests/app/service/test_rest.py | 23 +++++++++++++++++++++-- 2 files changed, 27 insertions(+), 10 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index ba3dbf717..6bd2e8620 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -458,11 +458,9 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): total_substmt = ( select( func.date_trunc("day", NotificationAllTimeView.created_at).label("day"), - Job.notification_count.label("notification_count") - ) - .join( - Job, NotificationAllTimeView.job_id == Job.id + Job.notification_count.label("notification_count"), ) + .join(Job, NotificationAllTimeView.job_id == Job.id) .where( NotificationAllTimeView.service_id == service_id, NotificationAllTimeView.key_type != KeyType.TEST, @@ -480,9 +478,7 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): total_stmt = select( total_substmt.c.day, func.sum(total_substmt.c.notification_count).label("total_notifications"), - ).group_by( - total_substmt.c.day - ) + ).group_by(total_substmt.c.day) total_notifications = { row.day: row.total_notifications for row in db.session.execute(total_stmt).all() @@ -756,7 +752,9 @@ def fetch_notification_stats_for_service_by_month_by_user( return db.session.execute(stmt).all() -def get_specific_days_stats(data, start_date, days=None, end_date=None, total_notifications=None): +def get_specific_days_stats( + data, start_date, days=None, end_date=None, total_notifications=None +): if days is not None and end_date is not None: raise ValueError("Only set days OR set end_date, not both.") elif days is not None: diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 132de48e9..f4057d0db 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -2200,6 +2200,7 @@ def test_set_sms_prefixing_for_service_cant_be_none( StatisticsType.REQUESTED: 2, StatisticsType.DELIVERED: 1, StatisticsType.FAILURE: 0, + StatisticsType.PENDING: 0, }, ), ( @@ -2208,6 +2209,7 @@ def test_set_sms_prefixing_for_service_cant_be_none( StatisticsType.REQUESTED: 1, StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, + StatisticsType.PENDING: 0, }, ), ], @@ -2256,11 +2258,13 @@ def test_get_services_with_detailed_flag(client, sample_template): NotificationType.EMAIL: { StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, + StatisticsType.PENDING: 0, StatisticsType.REQUESTED: 0, }, NotificationType.SMS: { StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, + StatisticsType.PENDING: 0, StatisticsType.REQUESTED: 3, }, } @@ -2287,11 +2291,13 @@ def test_get_services_with_detailed_flag_excluding_from_test_key( NotificationType.EMAIL: { StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, + StatisticsType.PENDING: 0, StatisticsType.REQUESTED: 0, }, NotificationType.SMS: { StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, + StatisticsType.PENDING: 0, StatisticsType.REQUESTED: 2, }, } @@ -2363,11 +2369,13 @@ def test_get_detailed_services_groups_by_service(notify_db_session): NotificationType.EMAIL: { StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, + StatisticsType.PENDING: 0, StatisticsType.REQUESTED: 0, }, NotificationType.SMS: { StatisticsType.DELIVERED: 1, StatisticsType.FAILURE: 0, + StatisticsType.PENDING: 0, StatisticsType.REQUESTED: 3, }, } @@ -2376,11 +2384,13 @@ def test_get_detailed_services_groups_by_service(notify_db_session): NotificationType.EMAIL: { StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, + StatisticsType.PENDING: 0, StatisticsType.REQUESTED: 0, }, NotificationType.SMS: { StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, + StatisticsType.PENDING: 0, StatisticsType.REQUESTED: 1, }, } @@ -2406,11 +2416,13 @@ def test_get_detailed_services_includes_services_with_no_notifications( NotificationType.EMAIL: { StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, + StatisticsType.PENDING: 0, StatisticsType.REQUESTED: 0, }, NotificationType.SMS: { StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, + StatisticsType.PENDING: 0, StatisticsType.REQUESTED: 1, }, } @@ -2419,11 +2431,13 @@ def test_get_detailed_services_includes_services_with_no_notifications( NotificationType.EMAIL: { StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, + StatisticsType.PENDING: 0, StatisticsType.REQUESTED: 0, }, NotificationType.SMS: { StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, + StatisticsType.PENDING: 0, StatisticsType.REQUESTED: 0, }, } @@ -2448,12 +2462,15 @@ def test_get_detailed_services_only_includes_todays_notifications(sample_templat NotificationType.EMAIL: { StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, - StatisticsType.REQUESTED: 0, + StatisticsType.PENDING: 0, + StatisticsType.REQUESTED: 0 + }, NotificationType.SMS: { StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, - StatisticsType.REQUESTED: 3, + StatisticsType.PENDING: 0, + StatisticsType.REQUESTED: 3 }, } @@ -2501,11 +2518,13 @@ def test_get_detailed_services_for_date_range( assert data[0]["statistics"][NotificationType.EMAIL] == { StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, + StatisticsType.PENDING: 0, StatisticsType.REQUESTED: 0, } assert data[0]["statistics"][NotificationType.SMS] == { StatisticsType.DELIVERED: 2, StatisticsType.FAILURE: 0, + StatisticsType.PENDING: 0, StatisticsType.REQUESTED: 2, } From b2240659ce2092c473a73af3c7313a9d99297f85 Mon Sep 17 00:00:00 2001 From: Beverly Nguyen Date: Mon, 13 Jan 2025 10:16:38 -0800 Subject: [PATCH 22/48] fix testing --- app/enums.py | 1 - 1 file changed, 1 deletion(-) diff --git a/app/enums.py b/app/enums.py index 37b3b6892..a0dfbb467 100644 --- a/app/enums.py +++ b/app/enums.py @@ -211,4 +211,3 @@ class StatisticsType(StrEnum): REQUESTED = "requested" DELIVERED = "delivered" FAILURE = "failure" - PENDING = "pending" From 88c3da5579099600e66ae869a29a033b27b34036 Mon Sep 17 00:00:00 2001 From: Beverly Nguyen Date: Mon, 13 Jan 2025 13:39:34 -0800 Subject: [PATCH 23/48] add testing --- app/enums.py | 1 + app/service/statistics.py | 13 +++-- tests/app/dao/test_services_dao.py | 38 ++++++++++++++- tests/app/service/test_rest.py | 5 +- tests/app/service/test_statistics.py | 59 ++++++++++++++++++----- tests/app/service/test_statistics_rest.py | 4 ++ 6 files changed, 100 insertions(+), 20 deletions(-) diff --git a/app/enums.py b/app/enums.py index a0dfbb467..37b3b6892 100644 --- a/app/enums.py +++ b/app/enums.py @@ -211,3 +211,4 @@ class StatisticsType(StrEnum): REQUESTED = "requested" DELIVERED = "delivered" FAILURE = "failure" + PENDING = "pending" diff --git a/app/service/statistics.py b/app/service/statistics.py index b67107ab1..d6d776539 100644 --- a/app/service/statistics.py +++ b/app/service/statistics.py @@ -29,15 +29,18 @@ def format_statistics(statistics, total_notifications=None): sms_dict = counts[NotificationType.SMS] delivered_count = sms_dict[StatisticsType.DELIVERED] failed_count = sms_dict[StatisticsType.FAILURE] - pending_count = total_notifications - (delivered_count + failed_count) - - pending_count = max(0, pending_count) - - sms_dict[StatisticsType.PENDING] = pending_count + sms_dict[StatisticsType.PENDING] = calculate_pending_stats( + delivered_count, failed_count, total_notifications + ) return counts +def calculate_pending_stats(delivered_count, failed_count, total_notifications): + pending_count = total_notifications - (delivered_count + failed_count) + return max(0, pending_count) + + def format_admin_stats(statistics): counts = create_stats_dict() diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 61fe99419..8cd8a11fd 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -1638,11 +1638,13 @@ _this_date = utc_now() - timedelta(days=4) StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, StatisticsType.REQUESTED: 0, + StatisticsType.PENDING: 0, }, TemplateType.SMS: { StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, StatisticsType.REQUESTED: 2, + StatisticsType.PENDING: 2, }, }, (_this_date.date() + timedelta(days=1)).strftime("%Y-%m-%d"): { @@ -1650,11 +1652,13 @@ _this_date = utc_now() - timedelta(days=4) StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, StatisticsType.REQUESTED: 0, + StatisticsType.PENDING: 0, }, TemplateType.SMS: { StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, StatisticsType.REQUESTED: 1, + StatisticsType.PENDING: 0, }, }, (_this_date.date() + timedelta(days=2)).strftime("%Y-%m-%d"): { @@ -1662,11 +1666,13 @@ _this_date = utc_now() - timedelta(days=4) StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, StatisticsType.REQUESTED: 0, + StatisticsType.PENDING: 0, }, TemplateType.SMS: { StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, StatisticsType.REQUESTED: 1, + StatisticsType.PENDING: 0, }, }, (_this_date.date() + timedelta(days=3)).strftime("%Y-%m-%d"): { @@ -1674,11 +1680,13 @@ _this_date = utc_now() - timedelta(days=4) StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, StatisticsType.REQUESTED: 0, + StatisticsType.PENDING: 0, }, TemplateType.SMS: { StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, StatisticsType.REQUESTED: 0, + StatisticsType.PENDING: 0, }, }, (_this_date.date() + timedelta(days=4)).strftime("%Y-%m-%d"): { @@ -1686,11 +1694,13 @@ _this_date = utc_now() - timedelta(days=4) StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, StatisticsType.REQUESTED: 0, + StatisticsType.PENDING: 0, }, TemplateType.SMS: { StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, StatisticsType.REQUESTED: 1, + StatisticsType.PENDING: 0, }, }, }, @@ -1713,11 +1723,13 @@ _this_date = utc_now() - timedelta(days=4) StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, StatisticsType.REQUESTED: 0, + StatisticsType.PENDING: 0, }, TemplateType.SMS: { StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, StatisticsType.REQUESTED: 2, + StatisticsType.PENDING: 2, }, }, (_this_date.date() + timedelta(days=1)).strftime("%Y-%m-%d"): { @@ -1725,11 +1737,13 @@ _this_date = utc_now() - timedelta(days=4) StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, StatisticsType.REQUESTED: 0, + StatisticsType.PENDING: 0, }, TemplateType.SMS: { StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, StatisticsType.REQUESTED: 1, + StatisticsType.PENDING: 0, }, }, (_this_date.date() + timedelta(days=2)).strftime("%Y-%m-%d"): { @@ -1737,11 +1751,13 @@ _this_date = utc_now() - timedelta(days=4) StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, StatisticsType.REQUESTED: 0, + StatisticsType.PENDING: 0, }, TemplateType.SMS: { StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, StatisticsType.REQUESTED: 1, + StatisticsType.PENDING: 0, }, }, (_this_date.date() + timedelta(days=3)).strftime("%Y-%m-%d"): { @@ -1749,11 +1765,13 @@ _this_date = utc_now() - timedelta(days=4) StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, StatisticsType.REQUESTED: 0, + StatisticsType.PENDING: 0, }, TemplateType.SMS: { StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, StatisticsType.REQUESTED: 0, + StatisticsType.PENDING: 0, }, }, (_this_date.date() + timedelta(days=4)).strftime("%Y-%m-%d"): { @@ -1761,11 +1779,13 @@ _this_date = utc_now() - timedelta(days=4) StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, StatisticsType.REQUESTED: 0, + StatisticsType.PENDING: 0, }, TemplateType.SMS: { StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, StatisticsType.REQUESTED: 1, + StatisticsType.PENDING: 0, }, }, }, @@ -1786,5 +1806,21 @@ def test_get_specific_days(data, start_date, days, end_date, expected, is_error) new_line.count = 1 new_line.something = line["something"] new_data.append(new_line) - results = get_specific_days_stats(new_data, start_date, days, end_date) + + total_notifications = None + + date_key = _this_date.date().strftime("%Y-%m-%d") + if expected and date_key in expected: + sms_stats = expected[date_key].get(TemplateType.SMS, {}) + requested = sms_stats.get(StatisticsType.REQUESTED, 0) + if requested > 0: + total_notifications = {_this_date: requested} + + results = get_specific_days_stats( + new_data, + start_date, + days, + end_date, + total_notifications=total_notifications, + ) assert results == expected diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index f4057d0db..4dc48140e 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -2463,14 +2463,13 @@ def test_get_detailed_services_only_includes_todays_notifications(sample_templat StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, StatisticsType.PENDING: 0, - StatisticsType.REQUESTED: 0 - + StatisticsType.REQUESTED: 0, }, NotificationType.SMS: { StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, StatisticsType.PENDING: 0, - StatisticsType.REQUESTED: 3 + StatisticsType.REQUESTED: 3, }, } diff --git a/tests/app/service/test_statistics.py b/tests/app/service/test_statistics.py index b3534fed3..a16625361 100644 --- a/tests/app/service/test_statistics.py +++ b/tests/app/service/test_statistics.py @@ -9,6 +9,7 @@ from freezegun import freeze_time from app.enums import KeyType, NotificationStatus, NotificationType, StatisticsType from app.service.statistics import ( add_monthly_notification_status_stats, + calculate_pending_stats, create_empty_monthly_notification_status_stats_dict, create_stats_dict, create_zeroed_stats_dicts, @@ -27,22 +28,22 @@ NewStatsRow = collections.namedtuple( @pytest.mark.idparametrize( "stats, email_counts, sms_counts", { - "empty": ([], [0, 0, 0], [0, 0, 0]), + "empty": ([], [0, 0, 0, 0], [0, 0, 0, 0]), "always_increment_requested": ( [ StatsRow(NotificationType.EMAIL, NotificationStatus.DELIVERED, 1), StatsRow(NotificationType.EMAIL, NotificationStatus.FAILED, 1), ], - [2, 1, 1], - [0, 0, 0], + [2, 1, 1, 0], + [0, 0, 0, 0], ), "dont_mix_template_types": ( [ StatsRow(NotificationType.EMAIL, NotificationStatus.DELIVERED, 1), StatsRow(NotificationType.SMS, NotificationStatus.DELIVERED, 1), ], - [1, 1, 0], - [1, 1, 0], + [1, 1, 0, 0], + [1, 1, 0, 0], ), "convert_fail_statuses_to_failed": ( [ @@ -57,8 +58,8 @@ NewStatsRow = collections.namedtuple( NotificationType.EMAIL, NotificationStatus.PERMANENT_FAILURE, 1 ), ], - [4, 0, 4], - [0, 0, 0], + [4, 0, 4, 0], + [0, 0, 0, 0], ), "convert_sent_to_delivered": ( [ @@ -66,16 +67,16 @@ NewStatsRow = collections.namedtuple( StatsRow(NotificationType.SMS, NotificationStatus.DELIVERED, 1), StatsRow(NotificationType.SMS, NotificationStatus.SENT, 1), ], - [0, 0, 0], - [3, 2, 0], + [0, 0, 0, 0], + [3, 2, 0, 0], ), "handles_none_rows": ( [ StatsRow(NotificationType.SMS, NotificationStatus.SENDING, 1), StatsRow(None, None, None), ], - [0, 0, 0], - [1, 0, 0], + [0, 0, 0, 0], + [1, 0, 0, 0], ), }, ) @@ -89,6 +90,7 @@ def test_format_statistics(stats, email_counts, sms_counts): StatisticsType.REQUESTED, StatisticsType.DELIVERED, StatisticsType.FAILURE, + StatisticsType.PENDING, ], email_counts, ) @@ -101,23 +103,58 @@ def test_format_statistics(stats, email_counts, sms_counts): StatisticsType.REQUESTED, StatisticsType.DELIVERED, StatisticsType.FAILURE, + StatisticsType.PENDING, ], sms_counts, ) } +def test_format_statistics_with_pending(): + stats = [ + StatsRow(NotificationType.SMS, NotificationStatus.DELIVERED, 10), + StatsRow(NotificationType.SMS, NotificationStatus.FAILED, 2), + ] + + total_notifications_for_sms = 20 + + result = format_statistics(stats, total_notifications=total_notifications_for_sms) + + expected_sms_counts = { + StatisticsType.REQUESTED: 12, + StatisticsType.DELIVERED: 10, + StatisticsType.FAILURE: 2, + StatisticsType.PENDING: 8, + } + + assert result[NotificationType.SMS] == expected_sms_counts + + +@pytest.mark.parametrize( + "delivered, failed, total, expected", + [ + (10, 2, 20, 8), + (10, 10, 20, 0), + (15, 10, 20, 0), + ], +) +def test_calculate_pending(delivered, failed, total, expected): + assert calculate_pending_stats(delivered, failed, total) == expected + + def test_create_zeroed_stats_dicts(): assert create_zeroed_stats_dicts() == { NotificationType.SMS: { StatisticsType.REQUESTED: 0, StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, + StatisticsType.PENDING: 0, }, NotificationType.EMAIL: { StatisticsType.REQUESTED: 0, StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, + StatisticsType.PENDING: 0, }, } diff --git a/tests/app/service/test_statistics_rest.py b/tests/app/service/test_statistics_rest.py index 6d20cacc3..254736bc9 100644 --- a/tests/app/service/test_statistics_rest.py +++ b/tests/app/service/test_statistics_rest.py @@ -119,6 +119,7 @@ def test_get_template_usage_by_month_returns_two_templates( StatisticsType.REQUESTED: 2, StatisticsType.DELIVERED: 1, StatisticsType.FAILURE: 0, + StatisticsType.PENDING: 0, }, ), ( @@ -127,6 +128,7 @@ def test_get_template_usage_by_month_returns_two_templates( StatisticsType.REQUESTED: 1, StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, + StatisticsType.PENDING: 0, }, ), ], @@ -163,11 +165,13 @@ def test_get_service_notification_statistics_with_unknown_service(admin_request) StatisticsType.REQUESTED: 0, StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, + StatisticsType.PENDING: 0, }, NotificationType.EMAIL: { StatisticsType.REQUESTED: 0, StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, + StatisticsType.PENDING: 0, }, } From 880237f55d55f78599910f282a248b6a27088f1d Mon Sep 17 00:00:00 2001 From: Beverly Nguyen Date: Tue, 14 Jan 2025 16:57:43 -0800 Subject: [PATCH 24/48] fix tuples --- app/dao/services_dao.py | 38 +++++++++++++++++++++++++++++++++++--- app/service/rest.py | 14 +++++++++----- 2 files changed, 44 insertions(+), 8 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 6bd2e8620..7a8d73578 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -515,6 +515,36 @@ def dao_fetch_stats_for_service_from_days_for_user( start_date = get_midnight_in_utc(start_date) end_date = get_midnight_in_utc(end_date + timedelta(days=1)) + total_substmt = ( + select( + func.date_trunc("day", NotificationAllTimeView.created_at).label("day"), + Job.notification_count.label("notification_count"), + ) + .join(Job, NotificationAllTimeView.job_id == Job.id) + .where( + NotificationAllTimeView.service_id == service_id, + NotificationAllTimeView.key_type != KeyType.TEST, + NotificationAllTimeView.created_at >= start_date, + NotificationAllTimeView.created_at < end_date, + NotificationAllTimeView.created_by_id == user_id, + ) + .group_by( + Job.id, + Job.notification_count, + func.date_trunc("day", NotificationAllTimeView.created_at), + ) + .subquery() + ) + + total_stmt = select( + total_substmt.c.day, + func.sum(total_substmt.c.notification_count).label("total_notifications"), + ).group_by(total_substmt.c.day) + + total_notifications = { + row.day: row.total_notifications for row in db.session.execute(total_stmt).all() + } + stmt = ( select( NotificationAllTimeView.notification_type, @@ -522,8 +552,7 @@ def dao_fetch_stats_for_service_from_days_for_user( func.date_trunc("day", NotificationAllTimeView.created_at).label("day"), func.count(NotificationAllTimeView.id).label("count"), ) - .select_from(NotificationAllTimeView) - .filter( + .where( NotificationAllTimeView.service_id == service_id, NotificationAllTimeView.key_type != KeyType.TEST, NotificationAllTimeView.created_at >= start_date, @@ -536,7 +565,10 @@ def dao_fetch_stats_for_service_from_days_for_user( func.date_trunc("day", NotificationAllTimeView.created_at), ) ) - return db.session.execute(stmt).scalars().all() + + data = db.session.execute(stmt).all() + + return total_notifications, data def dao_fetch_todays_stats_for_all_services( diff --git a/app/service/rest.py b/app/service/rest.py index 718e3bd33..81c5eb5c5 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -268,12 +268,16 @@ def get_service_statistics_for_specific_days_by_user( end_date = datetime.strptime(start, "%Y-%m-%d") start_date = end_date - timedelta(days=days - 1) - results = dao_fetch_stats_for_service_from_days_for_user( + total_notifications, results = dao_fetch_stats_for_service_from_days_for_user( service_id, start_date, end_date, user_id ) - stats = get_specific_days_stats(results, start_date, days=days) - + stats = get_specific_days_stats( + results, + start_date, + days=days, + total_notifications=total_notifications, + ) return stats @@ -663,11 +667,11 @@ def get_single_month_notification_stats_by_user(service_id, user_id): month_year = datetime(year, month, 10, 00, 00, 00) start_date, end_date = get_month_start_and_end_date_in_utc(month_year) - results = dao_fetch_stats_for_service_from_days_for_user( + total_notifications, results = dao_fetch_stats_for_service_from_days_for_user( service_id, start_date, end_date, user_id ) - stats = get_specific_days_stats(results, start_date, end_date=end_date) + stats = get_specific_days_stats(results, start_date, end_date=end_date, total_notifications=total_notifications,) return jsonify(stats) From 669f4e9bfd815283381ce094bc5795d46f94352b Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 22 Jan 2025 09:40:59 -0800 Subject: [PATCH 25/48] bulk_update_mappings adr --- ...0-adr-celery-pool-support-best-practice.md | 8 +++-- .../0011-adr-delivery-receipts-updates.md | 31 +++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) create mode 100644 docs/adrs/0011-adr-delivery-receipts-updates.md diff --git a/docs/adrs/0010-adr-celery-pool-support-best-practice.md b/docs/adrs/0010-adr-celery-pool-support-best-practice.md index b8525e654..4c63f6d08 100644 --- a/docs/adrs/0010-adr-celery-pool-support-best-practice.md +++ b/docs/adrs/0010-adr-celery-pool-support-best-practice.md @@ -1,7 +1,7 @@ # Make best use of celery worker pools -Status: N/A -Date: N/A +Status: Accepted +Date: 7 January 2025 ### Context Our API application started with initial celery pool support of 'prefork' (the default) and concurrency of 4. We continuously encountered instability, which we initially attributed to a resource leak. As a result of this we added the configuration `worker-max-tasks-per-child=500` which is a best practice. When we ran a load test of 25000 simulated messages, however, we continued to see stability issues, amounting to a crash of the app after 4 hours requiring a restage. Based on running `cf app notify-api-production` and observing that `cpu entitlement` was off the charts at 10000% to 12000% for the works, and after doing some further reading, we came to the conclusion that perhaps `prefork` pool support is not the best type of pool support for the API application. @@ -10,8 +10,12 @@ The problem with `prefork` is that each process has a tendency to hang onto the ### Decision +We decided to try to the 'threads' pool support with increased concurrency. + ### Consequences +We saw an immediate decrease in CPU usage of about 70% with no adverse consequences. + ### Author @kenkehl diff --git a/docs/adrs/0011-adr-delivery-receipts-updates.md b/docs/adrs/0011-adr-delivery-receipts-updates.md new file mode 100644 index 000000000..a42ea4223 --- /dev/null +++ b/docs/adrs/0011-adr-delivery-receipts-updates.md @@ -0,0 +1,31 @@ +# Optimize processing of delivery receipts + +Status: Accepted +Date: 22 January 2025 + +### Context +Our original effort to get delivery receipts for text messages was very object oriented and conformed to other patterns in the app. After an individual message was sent, we would kick off a new task on a delay, and this task would go search the cloudwatch logs for the given phone number. +On paper this looked good, but when one customer did a big send of 25k messages, we realized suddenly this was a bad idea. We overloaded the AWS api call and got massive throttling as a result. Although we ultimately did get most of the delivery receipts, it took hours and the logs were filled with errors. + +In refactoring this, there were two possible approaches we considered: + +1. Batch updates in the db (up to 1000 messages at a time). This involved running update queries with case statements and there is some theoretical limit on how large these statements can get and still be efficient. + +2. bulk_update_mappings(). This would be a raw updating similar to COPY where we could do millions of rows at a time. + +### Decision + +We decided to try to use batch updates. Even though they don't theoretically scale to the same level as bulk_update_mappings(), our app has a potential problem with using bulk_update_mappings(). In order for it to work, we would need to know the "id" for each notification, which is the primary key into the notifications table. We do NOT know the "id" when we process the delivery receipts. We do know the "message_id", but in order to get the "id" we would either have to a select query, or we would have to maintain some mapping in redis, etc. + +It is not clear, given the extra work necessary, that bulk_update_mappings() would be greatly superior to batch updates for our purposes. And batch updates currently allow us to scale at least 100x above where we are now. + +### Consequences + +Batch updates greatly cleaned up the logs (no more errors for throttling) and reduced CPU consumption. It was a very positive change. + +### Author +@kenkehl + +### Stakeholders +@ccostino +@stvnrlly From d051955d659f4f95b5c61180655650f77f95e691 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 23 Jan 2025 08:01:51 -0800 Subject: [PATCH 26/48] change total message limit to 100000 --- app/config.py | 2 +- tests/app/conftest.py | 2 +- tests/app/db.py | 2 +- tests/app/service/test_rest.py | 22 +++++++++++----------- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/app/config.py b/app/config.py index 13d9daf9d..2cabab9b7 100644 --- a/app/config.py +++ b/app/config.py @@ -339,7 +339,7 @@ class Config(object): FREE_SMS_TIER_FRAGMENT_COUNT = 250000 - TOTAL_MESSAGE_LIMIT = 250000 + TOTAL_MESSAGE_LIMIT = 100000 DAILY_MESSAGE_LIMIT = notifications_utils.DAILY_MESSAGE_LIMIT diff --git a/tests/app/conftest.py b/tests/app/conftest.py index b0bbf132b..d402aa8cb 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -224,7 +224,7 @@ def sample_service(sample_user): data = { "name": service_name, "message_limit": 1000, - "total_message_limit": 250000, + "total_message_limit": 100000, "restricted": False, "email_from": email_from, "created_by": sample_user, diff --git a/tests/app/db.py b/tests/app/db.py index 56a778406..4177c6b05 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -121,7 +121,7 @@ def create_service( email_from=None, prefix_sms=True, message_limit=1000, - total_message_limit=250000, + total_message_limit=100000, organization_type=OrganizationType.FEDERAL, check_if_service_exists=False, go_live_user=None, diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 9aacf2c21..2019eab95 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -397,7 +397,7 @@ def test_create_service( "name": "created service", "user_id": str(sample_user.id), "message_limit": 1000, - "total_message_limit": 250000, + "total_message_limit": 100000, "restricted": False, "active": False, "email_from": "created.service", @@ -468,7 +468,7 @@ def test_create_service_with_domain_sets_organization( "name": "created service", "user_id": str(sample_user.id), "message_limit": 1000, - "total_message_limit": 250000, + "total_message_limit": 100000, "restricted": False, "active": False, "email_from": "created.service", @@ -495,7 +495,7 @@ def test_create_service_should_create_annual_billing_for_service( "name": "created service", "user_id": str(sample_user.id), "message_limit": 1000, - "total_message_limit": 250000, + "total_message_limit": 100000, "restricted": False, "active": False, "email_from": "created.service", @@ -520,7 +520,7 @@ def test_create_service_should_raise_exception_and_not_create_service_if_annual_ "name": "created service", "user_id": str(sample_user.id), "message_limit": 1000, - "total_message_limit": 250000, + "total_message_limit": 100000, "restricted": False, "active": False, "email_from": "created.service", @@ -557,7 +557,7 @@ def test_create_service_inherits_branding_from_organization( "name": "created service", "user_id": str(sample_user.id), "message_limit": 1000, - "total_message_limit": 250000, + "total_message_limit": 100000, "restricted": False, "active": False, "email_from": "created.service", @@ -576,7 +576,7 @@ def test_should_not_create_service_with_missing_user_id_field(notify_api, fake_u "email_from": "service", "name": "created service", "message_limit": 1000, - "total_message_limit": 250000, + "total_message_limit": 100000, "restricted": False, "active": False, "created_by": str(fake_uuid), @@ -597,7 +597,7 @@ def test_should_error_if_created_by_missing(notify_api, sample_user): "email_from": "service", "name": "created service", "message_limit": 1000, - "total_message_limit": 250000, + "total_message_limit": 100000, "restricted": False, "active": False, "user_id": str(sample_user.id), @@ -623,7 +623,7 @@ def test_should_not_create_service_with_missing_if_user_id_is_not_in_database( "user_id": fake_uuid, "name": "created service", "message_limit": 1000, - "total_message_limit": 250000, + "total_message_limit": 100000, "restricted": False, "active": False, "created_by": str(fake_uuid), @@ -666,7 +666,7 @@ def test_should_not_create_service_with_duplicate_name( "name": sample_service.name, "user_id": str(sample_service.users[0].id), "message_limit": 1000, - "total_message_limit": 250000, + "total_message_limit": 100000, "restricted": False, "active": False, "email_from": "sample.service2", @@ -694,7 +694,7 @@ def test_create_service_should_throw_duplicate_key_constraint_for_existing_email "name": service_name, "user_id": str(first_service.users[0].id), "message_limit": 1000, - "total_message_limit": 250000, + "total_message_limit": 100000, "restricted": False, "active": False, "email_from": "first.service", @@ -1220,7 +1220,7 @@ def test_default_permissions_are_added_for_user_service( "name": "created service", "user_id": str(sample_user.id), "message_limit": 1000, - "total_message_limit": 250000, + "total_message_limit": 100000, "restricted": False, "active": False, "email_from": "created.service", From 3002dbf46f1a2b0024ef64f57a9a15cb6e6cf613 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 23 Jan 2025 08:11:17 -0800 Subject: [PATCH 27/48] change total message limit to 100000 --- tests/app/notifications/test_validators.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index f9df6fb91..f95313fde 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -62,13 +62,13 @@ def test_check_service_over_total_message_limit_fails( service = create_service() mocker.patch( "app.redis_store.get", - return_value="250001", + return_value="100001", ) with pytest.raises(TotalRequestsError) as e: check_service_over_total_message_limit(key_type, service) assert e.value.status_code == 429 - assert e.value.message == "Exceeded total application limits (250000) for today" + assert e.value.message == "Exceeded total application limits (100000) for today" assert e.value.fields == [] From 2d2a54bda63a16bf9b0f5b5f46c9b983db6ad3cf Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 23 Jan 2025 10:12:17 -0800 Subject: [PATCH 28/48] change the redis limit tracker to annual --- app/celery/tasks.py | 7 ++++++ app/notifications/validators.py | 11 ++++++-- .../0414_change_total_message_limit.py | 25 +++++++++++++++++++ 3 files changed, 41 insertions(+), 2 deletions(-) create mode 100644 migrations/versions/0414_change_total_message_limit.py diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 331d95364..6fea63e1b 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -159,7 +159,14 @@ def process_row(row, template, job, service, sender_id=None): return notification_id +# TODO +# Originally this was checking a daily limit +# It is now checking an overall limit (annual?) for the free tier +# Is there any limit for the paid tier? +# Assuming the limit is annual, is it calendar year, fiscal year, MOU year? +# Do we need a command to run to clear the redis value, or should it happen automatically? def __total_sending_limits_for_job_exceeded(service, job, job_id): + try: total_sent = check_service_over_total_message_limit(KeyType.NORMAL, service) if total_sent + job.notification_count > service.total_message_limit: diff --git a/app/notifications/validators.py b/app/notifications/validators.py index f0a7f2a8f..48d35dfe9 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -45,10 +45,17 @@ def check_service_over_total_message_limit(key_type, service): cache_key = total_limit_cache_key(service.id) service_stats = redis_store.get(cache_key) + + ## Originally this was a daily limit check. It is now a free-tier limit check. + ## TODO is this annual or forever for each service? + ## TODO do we need a way to clear this out? How do we determine if it is + ## free-tier or paid? What are the limits for paid? Etc. + ## TODO + ## setting expiration to one year for now on the assume that the free tier + ## limit resets annually. if service_stats is None: - # first message of the day, set the cache to 0 and the expiry to 24 hours service_stats = 0 - redis_store.set(cache_key, service_stats, ex=86400) + redis_store.set(cache_key, service_stats, ex=365*24*60*60) return service_stats if int(service_stats) >= service.total_message_limit: current_app.logger.warning( diff --git a/migrations/versions/0414_change_total_message_limit.py b/migrations/versions/0414_change_total_message_limit.py new file mode 100644 index 000000000..1ee9adb08 --- /dev/null +++ b/migrations/versions/0414_change_total_message_limit.py @@ -0,0 +1,25 @@ +""" + +Revision ID: 0414_change_total_message_limit +Revises: 413_add_message_id +Create Date: 2025-01-23 11:35:22.873930 + +""" + +import sqlalchemy as sa +from alembic import op + +down_revision = "0413_add_message_id" +revision = "0414_change_total_message_limit" + + +def upgrade(): + """ + This limit is only used + """ + op.execute("UPDATE services set total_message_limit=100000") + + + +def downgrade(): + op.execute("UPDATE services set total_message_limit=250000") From a5a952205640b27a025dead2512a528c59ce4b5f Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 23 Jan 2025 10:26:11 -0800 Subject: [PATCH 29/48] automate formatting and import sorting --- .github/workflows/checks.yml | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index 5244276bd..19641cf8f 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -47,10 +47,13 @@ jobs: NOTIFY_E2E_TEST_HTTP_AUTH_PASSWORD: ${{ secrets.NOTIFY_E2E_TEST_HTTP_AUTH_PASSWORD }} NOTIFY_E2E_TEST_HTTP_AUTH_USER: ${{ secrets.NOTIFY_E2E_TEST_HTTP_AUTH_USER }} NOTIFY_E2E_TEST_PASSWORD: ${{ secrets.NOTIFY_E2E_TEST_PASSWORD }} + + - name: Check imports alphabetized + run: poetry run isort ./app ./tests + - name: Run formatting + run: poetry run black . - name: Run style checks run: poetry run flake8 . - - name: Check imports alphabetized - run: poetry run isort --check-only ./app ./tests - name: Check for dead code run: make dead-code - name: Run tests with coverage From 7e913983a4886d1e27e8f0416938bfbb0effab82 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 23 Jan 2025 10:32:12 -0800 Subject: [PATCH 30/48] ugh fix flake 8 --- app/notifications/validators.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 48d35dfe9..51c77b577 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -46,13 +46,13 @@ def check_service_over_total_message_limit(key_type, service): cache_key = total_limit_cache_key(service.id) service_stats = redis_store.get(cache_key) - ## Originally this was a daily limit check. It is now a free-tier limit check. - ## TODO is this annual or forever for each service? - ## TODO do we need a way to clear this out? How do we determine if it is - ## free-tier or paid? What are the limits for paid? Etc. - ## TODO - ## setting expiration to one year for now on the assume that the free tier - ## limit resets annually. + # Originally this was a daily limit check. It is now a free-tier limit check. + # TODO is this annual or forever for each service? + # TODO do we need a way to clear this out? How do we determine if it is + # free-tier or paid? What are the limits for paid? Etc. + # TODO + # setting expiration to one year for now on the assume that the free tier + # limit resets annually. if service_stats is None: service_stats = 0 redis_store.set(cache_key, service_stats, ex=365*24*60*60) From 0d874828e42f45ca0679aaa02dd89e3386f0c022 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 23 Jan 2025 13:28:26 -0800 Subject: [PATCH 31/48] fix total message limit so it works --- app/celery/provider_tasks.py | 4 +++ app/celery/tasks.py | 2 +- app/delivery/send_to_providers.py | 11 ++++++-- app/notifications/validators.py | 27 +++++++------------ .../0414_change_total_message_limit.py | 8 +++--- 5 files changed, 27 insertions(+), 25 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 3bdd2d9c0..a3ed1f9ef 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -14,6 +14,7 @@ from app.dao.notifications_dao import update_notification_status_by_id from app.delivery import send_to_providers from app.enums import NotificationStatus from app.exceptions import NotificationTechnicalFailureException +from notifications_utils.clients.redis import total_limit_cache_key @notify_celery.task( @@ -41,6 +42,9 @@ def deliver_sms(self, notification_id): # Code branches off to send_to_providers.py send_to_providers.send_sms_to_provider(notification) + cache_key = total_limit_cache_key(notification.service_id) + redis_store.incr(cache_key) + except Exception as e: update_notification_status_by_id( notification_id, diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 6fea63e1b..8b3d9a353 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -166,7 +166,7 @@ def process_row(row, template, job, service, sender_id=None): # Assuming the limit is annual, is it calendar year, fiscal year, MOU year? # Do we need a command to run to clear the redis value, or should it happen automatically? def __total_sending_limits_for_job_exceeded(service, job, job_id): - + print(hilite("ENTER __total_sending_limits_for_job_exceeded")) try: total_sent = check_service_over_total_message_limit(KeyType.NORMAL, service) if total_sent + job.notification_count > service.total_message_limit: diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index e41062b41..6d9961ab6 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -26,6 +26,7 @@ from app.enums import BrandType, KeyType, NotificationStatus, NotificationType from app.exceptions import NotificationTechnicalFailureException from app.serialised_models import SerialisedService, SerialisedTemplate from app.utils import hilite, utc_now +from notifications_utils.clients.redis import total_limit_cache_key from notifications_utils.template import ( HTMLEmailTemplate, PlainTextEmailTemplate, @@ -118,8 +119,10 @@ def send_sms_to_provider(notification): } db.session.close() # no commit needed as no changes to objects have been made above + message_id = provider.send_sms(**send_sms_kwargs) - current_app.logger.info(f"got message_id {message_id}") + + update_notification_message_id(notification.id, message_id) except Exception as e: n = notification @@ -132,10 +135,14 @@ def send_sms_to_provider(notification): else: # Here we map the job_id and row number to the aws message_id n = notification - msg = f"Send to aws for job_id {n.job_id} row_number {n.job_row_number} message_id {message_id}" + msg = f"Send to AWS!!! for job_id {n.job_id} row_number {n.job_row_number} message_id {message_id}" current_app.logger.info(hilite(msg)) notification.billable_units = template.fragment_count + current_app.logger.info("GOING TO UPDATE NOTI TO SENDING") update_notification_to_sending(notification, provider) + + cache_key = total_limit_cache_key(service.id) + redis_store.incr(cache_key) return message_id diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 51c77b577..da4b9b290 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -11,7 +11,7 @@ from app.models import ServicePermission from app.notifications.process_notifications import create_content_for_notification from app.serialised_models import SerialisedTemplate from app.service.utils import service_allowed_to_send_to -from app.utils import get_public_notify_type_text +from app.utils import get_public_notify_type_text, hilite from notifications_utils import SMS_CHAR_COUNT_LIMIT from notifications_utils.clients.redis import ( rate_limit_cache_key, @@ -24,26 +24,13 @@ from notifications_utils.recipients import ( ) -def check_service_over_api_rate_limit(service, api_key): - if ( - current_app.config["API_RATE_LIMIT_ENABLED"] - and current_app.config["REDIS_ENABLED"] - ): - cache_key = rate_limit_cache_key(service.id, api_key.key_type) - rate_limit = service.rate_limit - interval = 60 - if redis_store.exceeded_rate_limit(cache_key, rate_limit, interval): - current_app.logger.info( - "service {} has been rate limited for throughput".format(service.id) - ) - raise RateLimitError(rate_limit, interval, api_key.key_type) - - def check_service_over_total_message_limit(key_type, service): + print(hilite("ENTER check_service_over_total_message_limit")) if key_type == KeyType.TEST or not current_app.config["REDIS_ENABLED"]: return 0 cache_key = total_limit_cache_key(service.id) + print(hilite(f"CACHE_KEY = {cache_key}")) service_stats = redis_store.get(cache_key) # Originally this was a daily limit check. It is now a free-tier limit check. @@ -53,17 +40,23 @@ def check_service_over_total_message_limit(key_type, service): # TODO # setting expiration to one year for now on the assume that the free tier # limit resets annually. + + # add column for actual charges to notifications and notifification_history table + # add service api to return total_message_limit and actual number of messages for service if service_stats is None: service_stats = 0 redis_store.set(cache_key, service_stats, ex=365*24*60*60) return service_stats - if int(service_stats) >= service.total_message_limit: + if int(service_stats) >= 5: + #if int(service_stats) >= service.total_message_limit: current_app.logger.warning( "service {} has been rate limited for total use sent {} limit {}".format( service.id, int(service_stats), service.total_message_limit ) ) raise TotalRequestsError(service.total_message_limit) + else: + print(hilite(f"TOTAL MESSAGE LIMIT {service.total_message_limit} CURRENT {service_stats}")) return int(service_stats) diff --git a/migrations/versions/0414_change_total_message_limit.py b/migrations/versions/0414_change_total_message_limit.py index 1ee9adb08..f4cb775d0 100644 --- a/migrations/versions/0414_change_total_message_limit.py +++ b/migrations/versions/0414_change_total_message_limit.py @@ -14,12 +14,10 @@ revision = "0414_change_total_message_limit" def upgrade(): - """ - This limit is only used - """ - op.execute("UPDATE services set total_message_limit=100000") + # TODO This needs updating when the agreement model is ready. We only want free tier at 100k + op.execute("UPDATE services set total_message_limit=100000 where total_message_limit=250000") def downgrade(): - op.execute("UPDATE services set total_message_limit=250000") + op.execute("UPDATE services set total_message_limit=250000 where total_message_limit=100000") From 49f4129e5ba17df12fccfc572549d95960c27043 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 23 Jan 2025 13:41:13 -0800 Subject: [PATCH 32/48] add tada to makefile --- .github/workflows/checks.yml | 4 +--- Makefile | 8 +++++++ app/delivery/send_to_providers.py | 2 -- app/notifications/validators.py | 22 ++++++++----------- .../0414_change_total_message_limit.py | 9 +++++--- 5 files changed, 24 insertions(+), 21 deletions(-) diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index 19641cf8f..0de22c0fd 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -49,9 +49,7 @@ jobs: NOTIFY_E2E_TEST_PASSWORD: ${{ secrets.NOTIFY_E2E_TEST_PASSWORD }} - name: Check imports alphabetized - run: poetry run isort ./app ./tests - - name: Run formatting - run: poetry run black . + run: poetry run isort --check-only ./app ./tests - name: Run style checks run: poetry run flake8 . - name: Check for dead code diff --git a/Makefile b/Makefile index 3d29046cb..741ceae5b 100644 --- a/Makefile +++ b/Makefile @@ -31,6 +31,14 @@ bootstrap-with-docker: ## Build the image to run the app in Docker run-procfile: poetry run honcho start -f Procfile.dev + + +.PHONY: tada +tada: + poetry run isort . + poetry run black . + poetry run flake8 . + .PHONY: avg-complexity avg-complexity: echo "*** Shows average complexity in radon of all code ***" diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 6d9961ab6..e34847397 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -119,10 +119,8 @@ def send_sms_to_provider(notification): } db.session.close() # no commit needed as no changes to objects have been made above - message_id = provider.send_sms(**send_sms_kwargs) - update_notification_message_id(notification.id, message_id) except Exception as e: n = notification diff --git a/app/notifications/validators.py b/app/notifications/validators.py index da4b9b290..615e87dfb 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -6,17 +6,14 @@ from app.dao.notifications_dao import dao_get_notification_count_for_service from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_id from app.dao.service_sms_sender_dao import dao_get_service_sms_senders_by_id from app.enums import KeyType, NotificationType, ServicePermissionType, TemplateType -from app.errors import BadRequestError, RateLimitError, TotalRequestsError +from app.errors import BadRequestError, TotalRequestsError from app.models import ServicePermission from app.notifications.process_notifications import create_content_for_notification from app.serialised_models import SerialisedTemplate from app.service.utils import service_allowed_to_send_to from app.utils import get_public_notify_type_text, hilite from notifications_utils import SMS_CHAR_COUNT_LIMIT -from notifications_utils.clients.redis import ( - rate_limit_cache_key, - total_limit_cache_key, -) +from notifications_utils.clients.redis import total_limit_cache_key from notifications_utils.recipients import ( get_international_phone_info, validate_and_format_email_address, @@ -45,10 +42,10 @@ def check_service_over_total_message_limit(key_type, service): # add service api to return total_message_limit and actual number of messages for service if service_stats is None: service_stats = 0 - redis_store.set(cache_key, service_stats, ex=365*24*60*60) + redis_store.set(cache_key, service_stats, ex=365 * 24 * 60 * 60) return service_stats if int(service_stats) >= 5: - #if int(service_stats) >= service.total_message_limit: + # if int(service_stats) >= service.total_message_limit: current_app.logger.warning( "service {} has been rate limited for total use sent {} limit {}".format( service.id, int(service_stats), service.total_message_limit @@ -56,7 +53,11 @@ def check_service_over_total_message_limit(key_type, service): ) raise TotalRequestsError(service.total_message_limit) else: - print(hilite(f"TOTAL MESSAGE LIMIT {service.total_message_limit} CURRENT {service_stats}")) + print( + hilite( + f"TOTAL MESSAGE LIMIT {service.total_message_limit} CURRENT {service_stats}" + ) + ) return int(service_stats) @@ -77,11 +78,6 @@ def check_application_over_retention_limit(key_type, service): return int(total_stats) -def check_rate_limiting(service, api_key): - check_service_over_api_rate_limit(service, api_key) - check_application_over_retention_limit(api_key.key_type, service) - - def check_template_is_for_notification_type(notification_type, template_type): if notification_type != template_type: message = "{0} template is not suitable for {1} notification".format( diff --git a/migrations/versions/0414_change_total_message_limit.py b/migrations/versions/0414_change_total_message_limit.py index f4cb775d0..8a3d9b3e2 100644 --- a/migrations/versions/0414_change_total_message_limit.py +++ b/migrations/versions/0414_change_total_message_limit.py @@ -15,9 +15,12 @@ revision = "0414_change_total_message_limit" def upgrade(): # TODO This needs updating when the agreement model is ready. We only want free tier at 100k - op.execute("UPDATE services set total_message_limit=100000 where total_message_limit=250000") - + op.execute( + "UPDATE services set total_message_limit=100000 where total_message_limit=250000" + ) def downgrade(): - op.execute("UPDATE services set total_message_limit=250000 where total_message_limit=100000") + op.execute( + "UPDATE services set total_message_limit=250000 where total_message_limit=100000" + ) From 2ac436f8470bf10692b4bf00a4dbda715ce4aeaf Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 23 Jan 2025 13:46:25 -0800 Subject: [PATCH 33/48] add tada to makefile --- .ds.baseline | 4 +- tests/app/notifications/test_validators.py | 100 +-------------------- 2 files changed, 4 insertions(+), 100 deletions(-) diff --git a/.ds.baseline b/.ds.baseline index 2baf278e1..18fb40388 100644 --- a/.ds.baseline +++ b/.ds.baseline @@ -295,7 +295,7 @@ "filename": "tests/app/notifications/test_validators.py", "hashed_secret": "6c1a8443963d02d13ffe575a71abe19ea731fb66", "is_verified": false, - "line_number": 768, + "line_number": 672, "is_secret": false } ], @@ -384,5 +384,5 @@ } ] }, - "generated_at": "2024-12-19T19:09:50Z" + "generated_at": "2025-01-23T21:46:22Z" } diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index f95313fde..5cf9f2de0 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -1,11 +1,8 @@ import pytest -from flask import current_app -from freezegun import freeze_time -import app from app.dao import templates_dao from app.enums import KeyType, NotificationType, ServicePermissionType, TemplateType -from app.errors import BadRequestError, RateLimitError, TotalRequestsError +from app.errors import BadRequestError, TotalRequestsError from app.notifications.process_notifications import create_content_for_notification from app.notifications.sns_cert_validator import ( VALID_SNS_TOPICS, @@ -17,10 +14,8 @@ from app.notifications.validators import ( check_if_service_can_send_files_by_email, check_is_message_too_long, check_notification_content_is_not_empty, - check_rate_limiting, check_reply_to, check_service_email_reply_to_id, - check_service_over_api_rate_limit, check_service_over_total_message_limit, check_service_sms_sender_id, check_template_is_active, @@ -29,16 +24,11 @@ from app.notifications.validators import ( validate_and_format_recipient, validate_template, ) -from app.serialised_models import ( - SerialisedAPIKeyCollection, - SerialisedService, - SerialisedTemplate, -) +from app.serialised_models import SerialisedService, SerialisedTemplate from app.service.utils import service_allowed_to_send_to from app.utils import get_template_instance from notifications_utils import SMS_CHAR_COUNT_LIMIT from tests.app.db import ( - create_api_key, create_reply_to_email, create_service, create_service_guest_list, @@ -482,92 +472,6 @@ def test_validate_template_calls_all_validators_exception_message_too_long( assert not mock_check_message_is_too_long.called -@pytest.mark.parametrize("key_type", [KeyType.TEAM, KeyType.NORMAL, KeyType.TEST]) -def test_check_service_over_api_rate_limit_when_exceed_rate_limit_request_fails_raises_error( - key_type, sample_service, mocker -): - with freeze_time("2016-01-01 12:00:00.000000"): - mocker.patch("app.redis_store.exceeded_rate_limit", return_value=True) - - sample_service.restricted = True - api_key = create_api_key(sample_service, key_type=key_type) - serialised_service = SerialisedService.from_id(sample_service.id) - serialised_api_key = SerialisedAPIKeyCollection.from_service_id( - serialised_service.id - )[0] - - with pytest.raises(RateLimitError) as e: - check_service_over_api_rate_limit(serialised_service, serialised_api_key) - - app.redis_store.exceeded_rate_limit.assert_called_with( - f"{sample_service.id}-{api_key.key_type}", - sample_service.rate_limit, - 60, - ) - assert e.value.status_code == 429 - assert e.value.message == ( - f"Exceeded rate limit for key type " - f"{key_type.name if key_type != KeyType.NORMAL else 'LIVE'} of " - f"{sample_service.rate_limit} requests per {60} seconds" - ) - assert e.value.fields == [] - - -def test_check_service_over_api_rate_limit_when_rate_limit_has_not_exceeded_limit_succeeds( - sample_service, - mocker, -): - with freeze_time("2016-01-01 12:00:00.000000"): - mocker.patch("app.redis_store.exceeded_rate_limit", return_value=False) - - sample_service.restricted = True - api_key = create_api_key(sample_service) - serialised_service = SerialisedService.from_id(sample_service.id) - serialised_api_key = SerialisedAPIKeyCollection.from_service_id( - serialised_service.id - )[0] - - check_service_over_api_rate_limit(serialised_service, serialised_api_key) - app.redis_store.exceeded_rate_limit.assert_called_with( - f"{sample_service.id}-{api_key.key_type}", - 3000, - 60, - ) - - -def test_check_service_over_api_rate_limit_should_do_nothing_if_limiting_is_disabled( - sample_service, mocker -): - with freeze_time("2016-01-01 12:00:00.000000"): - current_app.config["API_RATE_LIMIT_ENABLED"] = False - - mocker.patch("app.redis_store.exceeded_rate_limit", return_value=False) - - sample_service.restricted = True - create_api_key(sample_service) - serialised_service = SerialisedService.from_id(sample_service.id) - serialised_api_key = SerialisedAPIKeyCollection.from_service_id( - serialised_service.id - )[0] - - check_service_over_api_rate_limit(serialised_service, serialised_api_key) - app.redis_store.exceeded_rate_limit.assert_not_called() - - -def test_check_rate_limiting_validates_api_rate_limit_and_daily_limit( - notify_db_session, mocker -): - mock_rate_limit = mocker.patch( - "app.notifications.validators.check_service_over_api_rate_limit" - ) - service = create_service() - api_key = create_api_key(service=service) - - check_rate_limiting(service, api_key) - - mock_rate_limit.assert_called_once_with(service, api_key) - - @pytest.mark.parametrize("key_type", [KeyType.TEST, KeyType.NORMAL]) @pytest.mark.skip( "We currently don't support international numbers, our validation fails before here" From 481f27d443f18968dfa9f02b78059a2d67712e9f Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 28 Jan 2025 09:01:24 -0800 Subject: [PATCH 34/48] remove check_rate_limiting --- app/v2/notifications/post_notifications.py | 3 -- .../test_send_notification.py | 47 +------------------ 2 files changed, 1 insertion(+), 49 deletions(-) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index a5ad17646..a8dc894c7 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -18,7 +18,6 @@ from app.notifications.process_notifications import ( from app.notifications.validators import ( check_if_service_can_send_files_by_email, check_is_message_too_long, - check_rate_limiting, check_service_email_reply_to_id, check_service_has_permission, check_service_sms_sender_id, @@ -54,8 +53,6 @@ def post_notification(notification_type): check_service_has_permission(notification_type, authenticated_service.permissions) - check_rate_limiting(authenticated_service, api_user) - template, template_with_content = validate_template( form["template_id"], form.get("personalisation", {}), diff --git a/tests/app/service/send_notification/test_send_notification.py b/tests/app/service/send_notification/test_send_notification.py index 4c4a1792a..14802b56e 100644 --- a/tests/app/service/send_notification/test_send_notification.py +++ b/tests/app/service/send_notification/test_send_notification.py @@ -14,7 +14,7 @@ from app.dao.api_key_dao import save_model_api_key from app.dao.services_dao import dao_update_service from app.dao.templates_dao import dao_get_all_templates_for_service, dao_update_template from app.enums import KeyType, NotificationType, TemplateType -from app.errors import InvalidRequest, RateLimitError +from app.errors import InvalidRequest from app.models import ApiKey, Notification, NotificationHistory, Template from app.service.send_notification import send_one_off_notification from notifications_utils import SMS_CHAR_COUNT_LIMIT @@ -1124,51 +1124,6 @@ def test_create_template_raises_invalid_request_when_content_too_large( } -@pytest.mark.parametrize( - "notification_type, send_to", - [ - (NotificationType.SMS, "2028675309"), - ( - NotificationType.EMAIL, - "sample@email.com", - ), - ], -) -def test_returns_a_429_limit_exceeded_if_rate_limit_exceeded( - client, sample_service, mocker, notification_type, send_to -): - sample = create_template(sample_service, template_type=notification_type) - persist_mock = mocker.patch("app.notifications.rest.persist_notification") - deliver_mock = mocker.patch("app.notifications.rest.send_notification_to_queue") - - mocker.patch( - "app.notifications.rest.check_rate_limiting", - side_effect=RateLimitError("LIMIT", "INTERVAL", "TYPE"), - ) - - data = {"to": send_to, "template": str(sample.id)} - - auth_header = create_service_authorization_header(service_id=sample.service_id) - - response = client.post( - path=f"/notifications/{notification_type}", - data=json.dumps(data), - headers=[("Content-Type", "application/json"), auth_header], - ) - - message = json.loads(response.data)["message"] - result = json.loads(response.data)["result"] - assert response.status_code == 429 - assert result == "error" - assert message == ( - "Exceeded rate limit for key type TYPE of LIMIT " - "requests per INTERVAL seconds" - ) - - assert not persist_mock.called - assert not deliver_mock.called - - def test_should_allow_store_original_number_on_sms_notification( client, sample_template, mocker ): From 6b318b80212a8c7fc69b0b079967d7ede5d25b3e Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 28 Jan 2025 10:22:23 -0800 Subject: [PATCH 35/48] add api for message limit, messages_sent --- app/notifications/validators.py | 14 +++----------- app/service/rest.py | 25 ++++++++++++++++++++++++- 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 615e87dfb..9ec572e8b 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -22,28 +22,20 @@ from notifications_utils.recipients import ( def check_service_over_total_message_limit(key_type, service): - print(hilite("ENTER check_service_over_total_message_limit")) if key_type == KeyType.TEST or not current_app.config["REDIS_ENABLED"]: return 0 cache_key = total_limit_cache_key(service.id) - print(hilite(f"CACHE_KEY = {cache_key}")) service_stats = redis_store.get(cache_key) - # Originally this was a daily limit check. It is now a free-tier limit check. - # TODO is this annual or forever for each service? - # TODO do we need a way to clear this out? How do we determine if it is - # free-tier or paid? What are the limits for paid? Etc. # TODO - # setting expiration to one year for now on the assume that the free tier - # limit resets annually. - - # add column for actual charges to notifications and notifification_history table - # add service api to return total_message_limit and actual number of messages for service + # For now we are using calendar year + # Switch to using service agreement dates when the Agreement model is ready if service_stats is None: service_stats = 0 redis_store.set(cache_key, service_stats, ex=365 * 24 * 60 * 60) return service_stats + # TODO CHANGE THIS BACK TO SERVICE TOTAL MESSAGE LIMIT if int(service_stats) >= 5: # if int(service_stats) >= service.total_message_limit: current_app.logger.warning( diff --git a/app/service/rest.py b/app/service/rest.py index 533bf1bff..083ea23c4 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -7,7 +7,7 @@ from sqlalchemy.exc import IntegrityError from sqlalchemy.orm.exc import NoResultFound from werkzeug.datastructures import MultiDict -from app import db +from app import db, redis_store from app.aws.s3 import get_personalisation_from_s3, get_phone_number_from_s3 from app.config import QueueNames from app.dao import fact_notification_status_dao, notifications_dao @@ -109,6 +109,7 @@ from app.service.service_senders_schema import ( from app.service.utils import get_guest_list_objects from app.user.users_schema import post_set_permissions_schema from app.utils import get_prev_next_pagination_links, utc_now +from notifications_utils.clients.redis import total_limit_cache_key service_blueprint = Blueprint("service", __name__) @@ -1120,6 +1121,28 @@ def modify_service_data_retention(service_id, data_retention_id): return "", 204 +@service_blueprint.route("/get-service-message-ratio") +def get_service_message_ratio(): + service_id = request.args.get("service_id") + + my_service = dao_fetch_service_by_id(service_id) + + cache_key = total_limit_cache_key(service_id) + messages_sent = redis_store.get(cache_key) + if messages_sent is None: + messages_sent = 0 + current_app.logger.warning( + f"Messages sent was not being tracked for service {service_id}" + ) + else: + messages_sent = int(messages_sent) + + return { + "messages_sent": messages_sent, + "total_message_limit": my_service.total_message_limit, + } + + @service_blueprint.route("/monthly-data-by-service") def get_monthly_notification_data_by_service(): start_date = request.args.get("start_date") From c7553b1f21028784812c40c8906e15a1381ed05d Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 28 Jan 2025 10:42:46 -0800 Subject: [PATCH 36/48] add test --- tests/app/service/test_rest.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 2019eab95..c00daf0cf 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1665,6 +1665,27 @@ def test_remove_user_from_service(client, sample_user_service_permission): assert resp.status_code == 204 +def test_get_service_message_ratio(mocker, client, sample_user_service_permission): + service = sample_user_service_permission.service + + mock_redis = mocker.patch("app.service.rest.redis_store.get") + mock_redis.return_value = 1 + + endpoint = url_for( + "service.get_service_message_ratio", + service_id=str(service.id), + ) + auth_header = create_admin_authorization_header() + + resp = client.get( + endpoint, headers=[("Content-Type", "application/json"), auth_header] + ) + assert resp.status_code == 200 + result = resp.json + assert result["total_message_limit"] == 100000 + assert result["message_count"] == 1 + + def test_remove_non_existant_user_from_service(client, sample_user_service_permission): second_user = create_user(email="new@digital.fake.gov") endpoint = url_for( From c52cbe8456d7fc0942924db77a012cc4d3e1e6f2 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 28 Jan 2025 10:51:08 -0800 Subject: [PATCH 37/48] add test --- tests/app/service/test_rest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index c00daf0cf..6dd488ab9 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1683,7 +1683,7 @@ def test_get_service_message_ratio(mocker, client, sample_user_service_permissio assert resp.status_code == 200 result = resp.json assert result["total_message_limit"] == 100000 - assert result["message_count"] == 1 + assert result["messages_sent"] == 1 def test_remove_non_existant_user_from_service(client, sample_user_service_permission): From efe902444226c499c8c93e8c70b2e5e3f079c09a Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 28 Jan 2025 12:11:37 -0800 Subject: [PATCH 38/48] add tests --- .ds.baseline | 4 ++-- app/notifications/validators.py | 13 ++++++++++++- tests/app/service/test_rest.py | 24 +++++++++++++++++++++++- 3 files changed, 37 insertions(+), 4 deletions(-) diff --git a/.ds.baseline b/.ds.baseline index 18fb40388..1a39bc074 100644 --- a/.ds.baseline +++ b/.ds.baseline @@ -305,7 +305,7 @@ "filename": "tests/app/service/test_rest.py", "hashed_secret": "5baa61e4c9b93f3f0682250b6cf8331b7ee68fd8", "is_verified": false, - "line_number": 1285, + "line_number": 1286, "is_secret": false } ], @@ -384,5 +384,5 @@ } ] }, - "generated_at": "2025-01-23T21:46:22Z" + "generated_at": "2025-01-28T20:11:33Z" } diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 9ec572e8b..38d8263c7 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -1,3 +1,6 @@ +from datetime import datetime +from zoneinfo import ZoneInfo + from flask import current_app from sqlalchemy.orm.exc import NoResultFound @@ -31,9 +34,17 @@ def check_service_over_total_message_limit(key_type, service): # TODO # For now we are using calendar year # Switch to using service agreement dates when the Agreement model is ready + # If the service stat has never been set before, compute the remaining seconds for 2025 + # and set it (all services) to expire on 12/31/2025. if service_stats is None: + now_et = datetime.now(ZoneInfo("America/New_York")) + target_time = datetime( + 2025, 12, 31, 23, 59, 59, tzinfo=ZoneInfo("America/New_York") + ) + time_difference = target_time - now_et + seconds_difference = int(time_difference.total_seconds()) service_stats = 0 - redis_store.set(cache_key, service_stats, ex=365 * 24 * 60 * 60) + redis_store.set(cache_key, service_stats, ex=seconds_difference) return service_stats # TODO CHANGE THIS BACK TO SERVICE TOTAL MESSAGE LIMIT if int(service_stats) >= 5: diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 6dd488ab9..a88f8e21e 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1,7 +1,7 @@ import json import uuid from datetime import date, datetime, timedelta -from unittest.mock import ANY +from unittest.mock import ANY, MagicMock import pytest from flask import current_app, url_for @@ -38,6 +38,7 @@ from app.models import ( ServiceSmsSender, User, ) +from app.service.rest import check_request_args from app.utils import utc_now from tests import create_admin_authorization_header from tests.app.db import ( @@ -3712,3 +3713,24 @@ def test_get_service_notification_statistics_by_day( assert mock_get_service_statistics_for_specific_days.assert_called_once assert response == mock_data + + +def test_valid_request(): + request = MagicMock() + request.args = { + "service_id": "123", + "name": "Test Name", + "email_from": "test@example.com", + } + result = check_request_args(request) + assert result == ("123", "Test Name", "test@example.com") + + +def test_missing_service_id(): + request = MagicMock() + request.args = {"name": "Test Name", "email_from": "test@example.com"} + try: + check_request_args(request) + except Exception as e: + assert e.status_code == 400 + assert {"service_id": ["Can't be empty"] in e.errors} From ae499fd27823f8f5fff9f7ff4751c9735e1c1cc8 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 28 Jan 2025 12:19:04 -0800 Subject: [PATCH 39/48] add tests --- tests/app/service/test_rest.py | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index a88f8e21e..d1ae6c012 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -3715,22 +3715,22 @@ def test_get_service_notification_statistics_by_day( assert response == mock_data -def test_valid_request(): - request = MagicMock() - request.args = { - "service_id": "123", - "name": "Test Name", - "email_from": "test@example.com", - } - result = check_request_args(request) - assert result == ("123", "Test Name", "test@example.com") +# def test_valid_request(): +# request = MagicMock() +# request.args = { +# "service_id": "123", +# "name": "Test Name", +# "email_from": "test@example.com", +# } +# result = check_request_args(request) +# assert result == ("123", "Test Name", "test@example.com") -def test_missing_service_id(): - request = MagicMock() - request.args = {"name": "Test Name", "email_from": "test@example.com"} - try: - check_request_args(request) - except Exception as e: - assert e.status_code == 400 - assert {"service_id": ["Can't be empty"] in e.errors} +# def test_missing_service_id(): +# request = MagicMock() +# request.args = {"name": "Test Name", "email_from": "test@example.com"} +# try: +# check_request_args(request) +# except Exception as e: +# assert e.status_code == 400 +# assert {"service_id": ["Can't be empty"] in e.errors} From 8863400051b5776ee3802f44dd3a1ddecbebddc6 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 28 Jan 2025 12:24:52 -0800 Subject: [PATCH 40/48] add tests --- .ds.baseline | 4 ++-- tests/app/service/test_rest.py | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/.ds.baseline b/.ds.baseline index 1a39bc074..ad8d6eb1e 100644 --- a/.ds.baseline +++ b/.ds.baseline @@ -305,7 +305,7 @@ "filename": "tests/app/service/test_rest.py", "hashed_secret": "5baa61e4c9b93f3f0682250b6cf8331b7ee68fd8", "is_verified": false, - "line_number": 1286, + "line_number": 1285, "is_secret": false } ], @@ -384,5 +384,5 @@ } ] }, - "generated_at": "2025-01-28T20:11:33Z" + "generated_at": "2025-01-28T20:24:49Z" } diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index d1ae6c012..81341a0a0 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1,7 +1,7 @@ import json import uuid from datetime import date, datetime, timedelta -from unittest.mock import ANY, MagicMock +from unittest.mock import ANY import pytest from flask import current_app, url_for @@ -38,7 +38,6 @@ from app.models import ( ServiceSmsSender, User, ) -from app.service.rest import check_request_args from app.utils import utc_now from tests import create_admin_authorization_header from tests.app.db import ( From c1c7e7b9e6e5d583126ab194bf08ecf195f10f0e Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 28 Jan 2025 12:51:55 -0800 Subject: [PATCH 41/48] add tests --- .../notification_dao/test_notification_dao.py | 28 ++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index db369c5fe..aa0d68bc6 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -1,7 +1,7 @@ import uuid from datetime import date, datetime, timedelta from functools import partial -from unittest.mock import MagicMock, patch +from unittest.mock import ANY, MagicMock, patch import pytest from freezegun import freeze_time @@ -30,6 +30,7 @@ from app.dao.notifications_dao import ( get_notifications_for_service, get_service_ids_with_notifications_on_date, notifications_not_yet_sent, + sanitize_successful_notification_by_id, update_notification_status_by_id, update_notification_status_by_reference, ) @@ -2094,3 +2095,28 @@ def test_get_service_ids_with_notifications_on_date_checks_ft_status( ) == 1 ) + + +def test_sanitize_successful_notification_by_id(): + notification_id = "12345" + carrier = "CarrierX" + provider_response = "Success" + + mock_session = MagicMock() + mock_text = MagicMock() + with patch("app.dao.notification_dao.db.session", mock_session), patch( + "app.dao.notification_dao.text", mock_text + ): + sanitize_successful_notification_by_id( + notification_id, carrier, provider_response + ) + mock_text.assert_called_once_with("x") + mock_session.execute.assert_called_once_with( + mock_text.return_value, + { + "notification_id": notification_id, + "carrier": carrier, + "response": provider_response, + "sent_at": ANY, + }, + ) From d17a4ede666885b2bcbc793d6ccfb7553e7b6141 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 28 Jan 2025 13:00:30 -0800 Subject: [PATCH 42/48] add tests --- tests/app/dao/notification_dao/test_notification_dao.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index aa0d68bc6..19db72192 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -2104,8 +2104,8 @@ def test_sanitize_successful_notification_by_id(): mock_session = MagicMock() mock_text = MagicMock() - with patch("app.dao.notification_dao.db.session", mock_session), patch( - "app.dao.notification_dao.text", mock_text + with patch("app.dao.notifications_dao.db.session", mock_session), patch( + "app.dao.notifications_dao.text", mock_text ): sanitize_successful_notification_by_id( notification_id, carrier, provider_response From 0381768e587a6498e0c078b89bf881350ee90bcc Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 28 Jan 2025 13:18:59 -0800 Subject: [PATCH 43/48] add tests --- tests/app/dao/notification_dao/test_notification_dao.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 19db72192..1a145538a 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -2110,7 +2110,9 @@ def test_sanitize_successful_notification_by_id(): sanitize_successful_notification_by_id( notification_id, carrier, provider_response ) - mock_text.assert_called_once_with("x") + mock_text.assert_called_once_with( + "\n update notifications set provider_response=:response, carrier=:carrier,\n notification_status='delivered', sent_at=:sent_at, \"to\"='1', normalised_to='1'\n where id=:notification_id\n " # noqa + ) mock_session.execute.assert_called_once_with( mock_text.return_value, { From 91585a8b9e38eb8447fb1b2de38a2bbd3c2ae6cd Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 29 Jan 2025 07:50:23 -0800 Subject: [PATCH 44/48] cleanup --- app/celery/tasks.py | 2 +- app/delivery/send_to_providers.py | 6 +++++- app/notifications/validators.py | 12 ++---------- app/service/rest.py | 2 +- 4 files changed, 9 insertions(+), 13 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 8b3d9a353..92795c44a 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -179,7 +179,7 @@ def __total_sending_limits_for_job_exceeded(service, job, job_id): dao_update_job(job) current_app.logger.exception( "Job {} size {} error. Total sending limits {} exceeded".format( - job_id, job.notification_count, service.message_limit + job_id, job.notification_count, service.total_message_limit ), ) return True diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index e34847397..89d84b793 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -136,11 +136,15 @@ def send_sms_to_provider(notification): msg = f"Send to AWS!!! for job_id {n.job_id} row_number {n.job_row_number} message_id {message_id}" current_app.logger.info(hilite(msg)) notification.billable_units = template.fragment_count - current_app.logger.info("GOING TO UPDATE NOTI TO SENDING") update_notification_to_sending(notification, provider) cache_key = total_limit_cache_key(service.id) redis_store.incr(cache_key) + current_app.logger.info( + hilite( + f"message count for service {n.service_id} now {redis_store.get(cache_key)}" + ) + ) return message_id diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 38d8263c7..32cefcf2a 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -14,7 +14,7 @@ from app.models import ServicePermission from app.notifications.process_notifications import create_content_for_notification from app.serialised_models import SerialisedTemplate from app.service.utils import service_allowed_to_send_to -from app.utils import get_public_notify_type_text, hilite +from app.utils import get_public_notify_type_text from notifications_utils import SMS_CHAR_COUNT_LIMIT from notifications_utils.clients.redis import total_limit_cache_key from notifications_utils.recipients import ( @@ -46,21 +46,13 @@ def check_service_over_total_message_limit(key_type, service): service_stats = 0 redis_store.set(cache_key, service_stats, ex=seconds_difference) return service_stats - # TODO CHANGE THIS BACK TO SERVICE TOTAL MESSAGE LIMIT - if int(service_stats) >= 5: - # if int(service_stats) >= service.total_message_limit: + if int(service_stats) >= service.total_message_limit: current_app.logger.warning( "service {} has been rate limited for total use sent {} limit {}".format( service.id, int(service_stats), service.total_message_limit ) ) raise TotalRequestsError(service.total_message_limit) - else: - print( - hilite( - f"TOTAL MESSAGE LIMIT {service.total_message_limit} CURRENT {service_stats}" - ) - ) return int(service_stats) diff --git a/app/service/rest.py b/app/service/rest.py index 083ea23c4..84179e150 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -1140,7 +1140,7 @@ def get_service_message_ratio(): return { "messages_sent": messages_sent, "total_message_limit": my_service.total_message_limit, - } + }, 200 @service_blueprint.route("/monthly-data-by-service") From 88e623cb1090d6899f81818f2170e543289eceec Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 29 Jan 2025 08:04:33 -0800 Subject: [PATCH 45/48] cleanup --- app/delivery/send_to_providers.py | 6 +----- app/notifications/validators.py | 1 + 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 89d84b793..515d418e7 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -140,11 +140,7 @@ def send_sms_to_provider(notification): cache_key = total_limit_cache_key(service.id) redis_store.incr(cache_key) - current_app.logger.info( - hilite( - f"message count for service {n.service_id} now {redis_store.get(cache_key)}" - ) - ) + return message_id diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 32cefcf2a..8358b3c8a 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -53,6 +53,7 @@ def check_service_over_total_message_limit(key_type, service): ) ) raise TotalRequestsError(service.total_message_limit) + return int(service_stats) From 798cfbca0a5f5500cef97a57c80249bfb3c695de Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 29 Jan 2025 12:15:12 -0800 Subject: [PATCH 46/48] make command to see sms sender phones --- app/commands.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/app/commands.py b/app/commands.py index bbcdd2cd9..b865a5363 100644 --- a/app/commands.py +++ b/app/commands.py @@ -846,6 +846,19 @@ def create_new_service(name, message_limit, restricted, email_from, created_by_i db.session.rollback() +@notify_command(name="get-service-sender-phones") +@click.option("-s", "--service_id", required=True, prompt=True) +def get_service_sender_phones(service_id): + sender_phone_numbers = """ + select sms_sender, is_default + from service_sms_senders + where service_id = :service_id + """ + rows = db.session.execute(text(sender_phone_numbers), {"service_id": service_id}) + for row in rows: + print(row) + + @notify_command(name="promote-user-to-platform-admin") @click.option("-u", "--user-email-address", required=True, prompt=True) def promote_user_to_platform_admin(user_email_address): From b119457a47b7e734e207678ac1f3fece7ab465d3 Mon Sep 17 00:00:00 2001 From: Carlo Costino Date: Fri, 31 Jan 2025 10:55:33 -0500 Subject: [PATCH 47/48] Update zaproxy-api-scan reference This changeset updates our GitHub Action for dynamic scans to use the latest release of the zaproxy-api-scan. Signed-off-by: Carlo Costino --- .github/workflows/checks.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index 0de22c0fd..2d7311e1d 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -137,7 +137,7 @@ jobs: env: SQLALCHEMY_DATABASE_TEST_URI: postgresql://user:password@localhost:5432/test_notification_api - name: Run OWASP API Scan - uses: zaproxy/action-api-scan@v0.5.0 + uses: zaproxy/action-api-scan@v0.9.0 with: docker_name: 'ghcr.io/zaproxy/zaproxy:weekly' target: 'http://localhost:6011/docs/openapi.yml' From 0e3e305bfe0cbef93e7433538cbc4b51c06fdf8d Mon Sep 17 00:00:00 2001 From: Carlo Costino Date: Fri, 31 Jan 2025 11:07:26 -0500 Subject: [PATCH 48/48] Update daily checks reference as well. Signed-off-by: Carlo Costino --- .github/workflows/daily_checks.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/daily_checks.yml b/.github/workflows/daily_checks.yml index d8e19de98..edd1f7369 100644 --- a/.github/workflows/daily_checks.yml +++ b/.github/workflows/daily_checks.yml @@ -84,7 +84,7 @@ jobs: env: SQLALCHEMY_DATABASE_TEST_URI: postgresql://user:password@localhost:5432/test_notification_api - name: Run OWASP API Scan - uses: zaproxy/action-api-scan@v0.5.0 + uses: zaproxy/action-api-scan@v0.9.0 with: docker_name: 'ghcr.io/zaproxy/zaproxy:weekly' target: 'http://localhost:6011/docs/openapi.yml'