From 9d1d445877c8a92adba6f18d134fe3aae259c7b8 Mon Sep 17 00:00:00 2001 From: William B <7444334+whabanks@users.noreply.github.com> Date: Thu, 22 Feb 2024 13:29:30 -0400 Subject: [PATCH] Fix daily limit counting when scheduling jobs (#2112) * Fix daily limit counting when scheduling jobs - When scheduling a job, the daily limits in Redis will only be incremented if the job was scheduled for the current day - When refreshing daily count keys from the DB the underlying query will now include counts from jobs scheduled for the current day - Experimenting with a new decorator for feature flags - Fixed an issue with the dev container where the poetry installation will sometimes not be detected by adding the installation dir to the $PATH * Bump utils version and update lock file * fix tests? * Fix tests * Bump waffles commit sha in test action --- .../scripts/notify-dev-entrypoint.sh | 6 +- .github/workflows/test.yaml | 2 +- app/dao/services_dao.py | 43 +++++--- app/email_limit_utils.py | 11 ++- app/job/rest.py | 35 ++++--- app/notifications/rest.py | 2 +- app/v2/notifications/post_notifications.py | 5 +- tests/app/dao/test_services_dao.py | 97 ++++++++++++++++++- tests/app/job/test_rest.py | 1 + .../rest/test_send_notification.py | 2 +- 10 files changed, 164 insertions(+), 40 deletions(-) diff --git a/.devcontainer/scripts/notify-dev-entrypoint.sh b/.devcontainer/scripts/notify-dev-entrypoint.sh index 22cb552d2d..e2f99ea29a 100755 --- a/.devcontainer/scripts/notify-dev-entrypoint.sh +++ b/.devcontainer/scripts/notify-dev-entrypoint.sh @@ -34,8 +34,10 @@ cd /workspace echo -e "fpath+=/.zfunc" >> ~/.zshrc echo -e "autoload -Uz compinit && compinit" -pip install poetry==${POETRY_VERSION} \ - && poetry --version +pip install poetry==${POETRY_VERSION} +export PATH=$PATH:/home/vscode/.local/bin/ +which poetry +poetry --version # Initialize poetry autocompletions mkdir ~/.zfunc diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 3169d77d64..8d5c023e27 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -67,7 +67,7 @@ jobs: run: | cp -f .env.example .env - name: Checks for new endpoints against AWS WAF rules - uses: cds-snc/notification-utils/.github/actions/waffles@2da74685e0ffb220f0403e1f2584e783be99bbad # 52.1.0 + uses: cds-snc/notification-utils/.github/actions/waffles@06a40db6286f525fe3551e029418458d33342592 # 52.1.0 with: app-loc: '/github/workspace' app-libs: '/github/workspace/env/site-packages' diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 80d1f25c35..082a2566a1 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -23,6 +23,7 @@ CROWN_ORGANISATION_TYPES, EMAIL_TYPE, INTERNATIONAL_SMS_TYPE, + JOB_STATUS_SCHEDULED, KEY_TYPE_TEST, NHS_ORGANISATION_TYPES, NON_CROWN_ORGANISATION_TYPES, @@ -424,20 +425,24 @@ def dao_fetch_todays_stats_for_service(service_id): def fetch_todays_total_message_count(service_id): midnight = get_midnight(datetime.now(tz=pytz.utc)) + scheduled = ( + db.session.query(func.coalesce(func.sum(Job.notification_count), 0).label("count")).filter( + Job.service_id == service_id, + Job.job_status == JOB_STATUS_SCHEDULED, + Job.scheduled_for >= midnight, + Job.scheduled_for < midnight + timedelta(days=1), + ) + ).first() + result = ( - db.session.query(func.count(Notification.id).label("count")) - .filter( + db.session.query(func.coalesce(func.count(Notification.id), 0).label("count")).filter( Notification.service_id == service_id, Notification.key_type != KEY_TYPE_TEST, - Notification.created_at > midnight, + Notification.created_at >= midnight, ) - .group_by( - Notification.notification_type, - Notification.status, - ) - .first() - ) - return 0 if result is None else result.count + ).first() + + return result.count + scheduled.count def fetch_todays_total_sms_count(service_id): @@ -461,17 +466,25 @@ def fetch_service_email_limit(service_id: uuid.UUID) -> int: def fetch_todays_total_email_count(service_id: uuid.UUID) -> int: midnight = get_midnight(datetime.now(tz=pytz.utc)) + scheduled = ( + db.session.query(func.coalesce(func.sum(Job.notification_count), 0).label("total_scheduled_notifications")).filter( + Job.service_id == service_id, + Job.job_status == JOB_STATUS_SCHEDULED, + Job.scheduled_for > midnight, + Job.scheduled_for < midnight + timedelta(hours=23, minutes=59, seconds=59), + ) + ).first() + result = ( - db.session.query(func.count(Notification.id).label("total_email_notifications")) - .filter( + db.session.query(func.coalesce(func.count(Notification.id), 0).label("total_email_notifications")).filter( Notification.service_id == service_id, Notification.key_type != KEY_TYPE_TEST, Notification.created_at > midnight, Notification.notification_type == "email", ) - .first() - ) - return 0 if result is None else result.total_email_notifications + ).first() + + return result.total_email_notifications + scheduled.total_scheduled_notifications def _stats_for_service_query(service_id): diff --git a/app/email_limit_utils.py b/app/email_limit_utils.py index 7086d5c6f0..0fb8bf3364 100644 --- a/app/email_limit_utils.py +++ b/app/email_limit_utils.py @@ -3,6 +3,7 @@ from flask import current_app from notifications_utils.clients.redis import email_daily_count_cache_key +from notifications_utils.decorators import requires_feature from app import redis_store from app.dao.services_dao import fetch_todays_total_email_count @@ -20,9 +21,15 @@ def fetch_todays_email_count(service_id: UUID) -> int: return int(total_email_count) +@requires_feature("REDIS_ENABLED") def increment_todays_email_count(service_id: UUID, increment_by: int) -> None: - if not current_app.config["REDIS_ENABLED"]: - return fetch_todays_email_count(service_id) # to make sure it's set in redis cache_key = email_daily_count_cache_key(service_id) redis_store.incrby(cache_key, increment_by) + + +@requires_feature("REDIS_ENABLED") +def decrement_todays_email_count(service_id: UUID, decrement_by: int) -> None: + fetch_todays_email_count(service_id) + cache_key = email_daily_count_cache_key(service_id) + redis_store.decrby(cache_key, decrement_by) diff --git a/app/job/rest.py b/app/job/rest.py index 3c443ec4ba..49990f2961 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -1,3 +1,5 @@ +from datetime import datetime + import dateutil from flask import Blueprint, current_app, jsonify, request from notifications_utils.recipients import RecipientCSV @@ -20,6 +22,7 @@ from app.dao.notifications_dao import get_notifications_for_job from app.dao.services_dao import dao_fetch_service_by_id from app.dao.templates_dao import dao_get_template_by_id +from app.email_limit_utils import decrement_todays_email_count from app.errors import InvalidRequest, register_errors from app.models import ( EMAIL_TYPE, @@ -67,7 +70,7 @@ def cancel_job(service_id, job_id): job = dao_get_future_scheduled_job_by_id_and_service_id(job_id, service_id) job.job_status = JOB_STATUS_CANCELLED dao_update_job(job) - + decrement_todays_email_count(service_id, job.notification_count) return get_job_by_service_and_job_id(service_id, job_id) @@ -137,15 +140,23 @@ def create_job(service_id): raise InvalidRequest("Create job is not allowed: service is inactive ", 403) data = request.get_json() - data.update({"service": service_id}) + try: data.update(**get_job_metadata_from_s3(service_id, data["id"])) except KeyError: raise InvalidRequest({"id": ["Missing data for required field."]}, status_code=400) + if data.get("valid") != "True": + raise InvalidRequest("File is not valid, can't create job", 400) + data["template"] = data.pop("template_id") + template = dao_get_template_by_id(data["template"]) + template_errors = unarchived_template_schema.validate({"archived": template.archived}) + + if template_errors: + raise InvalidRequest(template_errors, status_code=400) job = get_job_from_s3(service_id, data["id"]) recipient_csv = RecipientCSV( @@ -170,22 +181,16 @@ def create_job(service_id): if not is_test_notification: check_sms_daily_limit(service, len(recipient_csv)) + increment_sms_daily_count_send_warnings_if_needed(service, len(recipient_csv)) - if template.template_type == EMAIL_TYPE: + elif template.template_type == EMAIL_TYPE: check_email_daily_limit(service, len(list(recipient_csv.get_rows()))) + scheduled_for = datetime.fromisoformat(form.get("scheduled_for")) if form.get("scheduled_for") else None # noqa: F821 - if data.get("valid") != "True": - raise InvalidRequest("File is not valid, can't create job", 400) - - errors = unarchived_template_schema.validate({"archived": template.archived}) - - if errors: - raise InvalidRequest(errors, status_code=400) - - if template.template_type == SMS_TYPE and not is_test_notification: - increment_sms_daily_count_send_warnings_if_needed(service, len(recipient_csv)) - elif template.template_type == EMAIL_TYPE: - increment_email_daily_count_send_warnings_if_needed(service, len(list(recipient_csv.get_rows()))) + if scheduled_for is None or not scheduled_for.date() > datetime.today().date(): + increment_email_daily_count_send_warnings_if_needed( + authenticated_service, len(list(recipient_csv.get_rows())) # noqa: F821 + ) data.update({"template_version": template.version}) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 70b09228f8..e3ec327df6 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -112,7 +112,7 @@ def send_notification(notification_type: NotificationType): ) simulated = simulated_recipient(notification_form["to"], notification_type) - if not simulated != api_user.key_type == KEY_TYPE_TEST: + if not simulated != api_user.key_type == KEY_TYPE_TEST and notification_type == EMAIL_TYPE: check_email_daily_limit(authenticated_service, 1) check_template_is_for_notification_type(notification_type, template.template_type) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index d9d333c0e0..9a1640b9a2 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -223,7 +223,10 @@ def post_bulk(): if template.template_type == EMAIL_TYPE and api_user.key_type != KEY_TYPE_TEST: check_email_daily_limit(authenticated_service, len(list(recipient_csv.get_rows()))) - increment_email_daily_count_send_warnings_if_needed(authenticated_service, len(list(recipient_csv.get_rows()))) + scheduled_for = datetime.fromisoformat(form.get("scheduled_for")) if form.get("scheduled_for") else None + + if scheduled_for is None or not scheduled_for.date() > datetime.today().date(): + increment_email_daily_count_send_warnings_if_needed(authenticated_service, len(list(recipient_csv.get_rows()))) if template.template_type == SMS_TYPE: # calculate the number of simulated recipients diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 4a81ea4f86..46941c9887 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -46,6 +46,7 @@ from app.models import ( EMAIL_TYPE, INTERNATIONAL_SMS_TYPE, + JOB_STATUS_SCHEDULED, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST, @@ -68,6 +69,7 @@ user_folder_permissions, ) from app.schemas import service_schema +from tests.app.conftest import create_sample_job from tests.app.db import ( create_annual_billing, create_api_key, @@ -1396,7 +1398,7 @@ def create_email_sms_letter_template(): class TestServiceEmailLimits: - def test_get_email_count_for_service(self, notify_db_session): + def test_get_email_count_for_service(self): active_user_1 = create_user(email="active1@foo.com", state="active") service = Service( name="service_name", @@ -1432,7 +1434,98 @@ def test_dao_fetch_todays_total_message_count_returns_0_with_yesterday_messages( notification = save_notification( create_notification( created_at=yesterday, - template=create_template(service=create_service(service_name="tester"), template_type="email"), + template=create_template(service=create_service(service_name="tester123"), template_type="email"), ) ) assert fetch_todays_total_message_count(notification.service.id) == 0 + + def test_dao_fetch_todays_total_message_count_counts_notifications_in_jobs_scheduled_for_today( + self, notify_db, notify_db_session + ): + service = create_service(service_name="tester12") + template = create_template(service=service, template_type="email") + today = datetime.utcnow().date() + + create_sample_job( + notify_db, + notify_db_session, + service=service, + template=template, + scheduled_for=today, + job_status=JOB_STATUS_SCHEDULED, + notification_count=10, + ) + save_notification( + create_notification( + created_at=today, + template=template, + ) + ) + assert fetch_todays_total_message_count(service.id) == 11 + + def test_dao_fetch_todays_total_message_count_counts_notifications_in_jobs_scheduled_for_today_but_not_after_today( + self, notify_db, notify_db_session + ): + service = create_service() + template = create_template(service=service, template_type="email") + today = datetime.utcnow().date() + + create_sample_job( + notify_db, + notify_db_session, + service=service, + template=template, + scheduled_for=today, + job_status=JOB_STATUS_SCHEDULED, + notification_count=10, + ) + save_notification( + create_notification( + created_at=today, + template=template, + ) + ) + create_sample_job( + notify_db, + notify_db_session, + service=service, + template=template, + scheduled_for=today + timedelta(days=1), + job_status=JOB_STATUS_SCHEDULED, + notification_count=10, + ) + + assert fetch_todays_total_message_count(service.id) == 11 + + def test_dao_fetch_todays_total_message_count_counts_notifications_in_jobs_scheduled_for_today_but_not_before_today( + self, notify_db, notify_db_session + ): + service = create_service() + template = create_template(service=service, template_type="email") + today = datetime.utcnow().date() + + create_sample_job( + notify_db, + notify_db_session, + service=service, + template=template, + scheduled_for=today, + job_status=JOB_STATUS_SCHEDULED, + notification_count=10, + ) + create_sample_job( + notify_db, + notify_db_session, + service=service, + template=template, + scheduled_for=today - timedelta(days=1), + job_status=JOB_STATUS_SCHEDULED, + notification_count=10, + ) + save_notification( + create_notification( + created_at=today, + template=template, + ) + ) + assert fetch_todays_total_message_count(service.id) == 11 diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index e184da8199..932e9c7834 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -451,6 +451,7 @@ def test_create_job_returns_404_if_template_does_not_exist(client, sample_servic "app.job.rest.get_job_metadata_from_s3", return_value={ "template_id": str(sample_service.id), + "valid": "True", }, ) data = { diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index 71f8c63ec6..a1cf1f0d52 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -466,7 +466,7 @@ def test_should_allow_api_call_if_under_day_limit_regardless_of_type( sms_template = create_sample_template(notify_db, notify_db_session, service=service) create_sample_notification(notify_db, notify_db_session, template=email_template, service=service) - data = {"to": sample_user.mobile_number, "template": str(sms_template.id)} + data = {"to": sample_user.mobile_number, "template": str(sms_template.id), "valid": "True"} auth_header = create_authorization_header(service_id=service.id)