diff --git a/.ds.baseline b/.ds.baseline index 2baf278e1..ad8d6eb1e 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-28T20:24:49Z" } diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index 5244276bd..2d7311e1d 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -47,10 +47,11 @@ 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: Run style checks - run: poetry run flake8 . + - name: Check imports alphabetized run: poetry run isort --check-only ./app ./tests + - name: Run style checks + run: poetry run flake8 . - name: Check for dead code run: make dead-code - name: Run tests with coverage @@ -136,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' 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' diff --git a/Makefile b/Makefile index 9666bfa5d..7d899c80a 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/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 331d95364..92795c44a 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): + 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: @@ -172,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/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): 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/app/dao/services_dao.py b/app/dao/services_dao.py index c6ad4cdf9..c0503a5cd 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -471,6 +471,35 @@ 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)) + 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, + ) + .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, @@ -490,7 +519,10 @@ def dao_fetch_stats_for_service_from_days(service_id, start_date, end_date): NotificationAllTimeView.created_at, ) ) - return db.session.execute(stmt).all() + + data = db.session.execute(stmt).all() + + return total_notifications, data def dao_fetch_stats_for_service_from_days_for_user( @@ -499,6 +531,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, @@ -506,7 +568,6 @@ def dao_fetch_stats_for_service_from_days_for_user( func.date_trunc("day", NotificationAllTimeView.created_at).label("timestamp"), func.count(NotificationAllTimeView.id).label("count"), ) - .select_from(NotificationAllTimeView) .where( NotificationAllTimeView.service_id == service_id, NotificationAllTimeView.key_type != KeyType.TEST, @@ -520,7 +581,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( @@ -736,7 +800,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, timezone="UTC"): +def get_specific_days_stats( + data, start_date, days=None, end_date=None, total_notifications=None +, timezone="UTC"): user_timezone = pytz.timezone(timezone if timezone else "UTC") if days is not None and end_date is not None: @@ -748,23 +814,20 @@ def get_specific_days_stats(data, start_date, days=None, end_date=None, timezone else: raise ValueError("Either days or end_date must be set.") - for item in data: - if not isinstance(item.timestamp, datetime.datetime): - print(f"ERROR: Found non-datetime value: {item.timestamp} ({type(item.timestamp)})") - - - # Group data by date, ensuring ALL dates in range are included - grouped_data = {date: [] for date in date_range} - for item in data: - local_datetime = item.timestamp.replace(tzinfo=pytz.utc).astimezone(user_timezone) - local_date = local_datetime.date() - - grouped_data[local_date].append(item) - - # Ensure all dates exist in the final output, even if empty - stats = { - day.strftime("%Y-%m-%d"): statistics.format_statistics(grouped_data.get(day, [])) - for day in date_range + 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} } + stats = { + day.strftime("%Y-%m-%d"): statistics.format_statistics( + grouped_data.get(day, []), + total_notifications=( + total_notifications.get(day, 0) + if total_notifications is not None + else None + ), + ) + for day in date_range + } return stats diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index e41062b41..515d418e7 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, @@ -119,7 +120,7 @@ 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 +133,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 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/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/notifications/rest.py b/app/notifications/rest.py index 43224f0e7..4e9adc2c6 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -12,7 +12,6 @@ from app.notifications.process_notifications import ( ) from app.notifications.validators import ( check_if_service_can_send_to_number, - check_rate_limiting, service_has_permission, validate_template, ) @@ -125,8 +124,6 @@ def send_notification(notification_type): else email_notification_schema ).load(request.get_json()) - check_rate_limiting(authenticated_service, api_user) - template, template_with_content = validate_template( template_id=notification_form["template"], personalisation=notification_form.get("personalisation", {}), diff --git a/app/notifications/validators.py b/app/notifications/validators.py index f0a7f2a8f..8358b3c8a 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 @@ -6,17 +9,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 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, @@ -24,31 +24,27 @@ 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): if key_type == KeyType.TEST or not current_app.config["REDIS_ENABLED"]: return 0 cache_key = total_limit_cache_key(service.id) service_stats = redis_store.get(cache_key) + + # 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: - # first message of the day, set the cache to 0 and the expiry to 24 hours + 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=86400) + redis_store.set(cache_key, service_stats, ex=seconds_difference) return service_stats if int(service_stats) >= service.total_message_limit: current_app.logger.warning( @@ -57,6 +53,7 @@ def check_service_over_total_message_limit(key_type, service): ) ) raise TotalRequestsError(service.total_message_limit) + return int(service_stats) @@ -77,11 +74,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/app/service/rest.py b/app/service/rest.py index 09f0638cc..1a0851afd 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -8,7 +8,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 @@ -110,6 +110,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__) @@ -235,12 +236,17 @@ def get_service_statistics_for_specific_days(service_id, start, days=7, timezone utc_end_date ) = build_local_and_utc_date_range(start, days, timezone) - results = dao_fetch_stats_for_service_from_days(service_id, utc_start_date, utc_end_date) + total_notifications, results = dao_fetch_stats_for_service_from_days( + service_id, + utc_start_date, + utc_end_date, + ) stats = get_specific_days_stats( data=results, start_date=local_start_date, days=days, + total_notifications=total_notifications, timezone=timezone ) return stats @@ -268,18 +274,10 @@ def get_service_statistics_for_specific_days_by_user( ) = build_local_and_utc_date_range(start_date_str=start, days=days, timezone=timezone) results = dao_fetch_stats_for_service_from_days_for_user( - service_id, - utc_start_date, - utc_end_date, - user_id + service_id, start_date, end_date, user_id ) - stats = get_specific_days_stats( - data=results, - start_date=local_start_date, - days=days, - timezone=timezone - ) + stats = get_specific_days_stats(results, start_date, days=days) return stats @@ -690,11 +688,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) @@ -714,7 +712,9 @@ 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) + __, 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) @@ -1134,6 +1134,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, + }, 200 + + @service_blueprint.route("/monthly-data-by-service") def get_monthly_notification_data_by_service(): start_date = request.args.get("start_date") diff --git a/app/service/statistics.py b/app/service/statistics.py index a6b58e067..d6d776539 100644 --- a/app/service/statistics.py +++ b/app/service/statistics.py @@ -2,10 +2,16 @@ 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): +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,11 +20,27 @@ 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, + ) + + 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] + 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/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/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 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..8a3d9b3e2 --- /dev/null +++ b/migrations/versions/0414_change_total_message_limit.py @@ -0,0 +1,26 @@ +""" + +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(): + # 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 where total_message_limit=100000" + ) 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/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index db369c5fe..1a145538a 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,30 @@ 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.notifications_dao.db.session", mock_session), patch( + "app.dao.notifications_dao.text", mock_text + ): + sanitize_successful_notification_by_id( + notification_id, carrier, provider_response + ) + 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, + { + "notification_id": notification_id, + "carrier": carrier, + "response": provider_response, + "sent_at": ANY, + }, + ) diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 2545aaeef..d319ffcaf 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -1625,3 +1625,216 @@ def test_get_live_services_with_organization(sample_organization): (live_service.name, sample_organization.name), (service_without_org.name, None), ] + + +_this_date = utc_now() - timedelta(days=4) + + +@pytest.mark.parametrize( + ["data", "start_date", "days", "end_date", "expected", "is_error"], + [ + [None, _this_date, None, None, None, True], + [None, _this_date, 4, _this_date - timedelta(4), None, True], + [ + [ + {"day": _this_date, "something": "else"}, + {"day": _this_date, "something": "new"}, + {"day": _this_date + timedelta(days=1), "something": "borrowed"}, + {"day": _this_date + timedelta(days=2), "something": "old"}, + {"day": _this_date + timedelta(days=4), "something": "blue"}, + ], + _this_date, + 4, + None, + { + _this_date.date().strftime("%Y-%m-%d"): { + TemplateType.EMAIL: { + 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"): { + TemplateType.EMAIL: { + 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"): { + TemplateType.EMAIL: { + 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"): { + TemplateType.EMAIL: { + 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"): { + TemplateType.EMAIL: { + 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, + }, + }, + }, + False, + ], + [ + [ + {"day": _this_date, "something": "else"}, + {"day": _this_date, "something": "new"}, + {"day": _this_date + timedelta(days=1), "something": "borrowed"}, + {"day": _this_date + timedelta(days=2), "something": "old"}, + {"day": _this_date + timedelta(days=4), "something": "blue"}, + ], + _this_date, + None, + _this_date + timedelta(4), + { + _this_date.date().strftime("%Y-%m-%d"): { + TemplateType.EMAIL: { + 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"): { + TemplateType.EMAIL: { + 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"): { + TemplateType.EMAIL: { + 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"): { + TemplateType.EMAIL: { + 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"): { + TemplateType.EMAIL: { + 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, + }, + }, + }, + False, + ], + ], +) +def test_get_specific_days(data, start_date, days, end_date, expected, is_error): + if is_error: + with pytest.raises(ValueError): + get_specific_days_stats(data, start_date, days, end_date) + else: + new_data = [] + for line in data: + new_line = Mock() + new_line.day = line["day"] + new_line.notification_type = NotificationType.SMS + new_line.count = 1 + new_line.something = line["something"] + new_data.append(new_line) + + 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/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/notifications/test_validators.py b/tests/app/notifications/test_validators.py index f9df6fb91..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, @@ -62,13 +52,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 == [] @@ -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" 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 ): diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 9aacf2c21..cd2ef8005 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", @@ -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["messages_sent"] == 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( @@ -2201,6 +2222,7 @@ def test_set_sms_prefixing_for_service_cant_be_none( StatisticsType.REQUESTED: 2, StatisticsType.DELIVERED: 1, StatisticsType.FAILURE: 0, + StatisticsType.PENDING: 0, }, ), ( @@ -2209,6 +2231,7 @@ def test_set_sms_prefixing_for_service_cant_be_none( StatisticsType.REQUESTED: 1, StatisticsType.DELIVERED: 0, StatisticsType.FAILURE: 0, + StatisticsType.PENDING: 0, }, ), ], @@ -2257,11 +2280,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, }, } @@ -2288,11 +2313,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, }, } @@ -2364,11 +2391,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, }, } @@ -2377,11 +2406,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, }, } @@ -2407,11 +2438,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, }, } @@ -2420,11 +2453,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, }, } @@ -2449,11 +2484,13 @@ def test_get_detailed_services_only_includes_todays_notifications(sample_templat 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, }, } @@ -2502,11 +2539,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, } @@ -3691,3 +3730,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} 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, }, }