From 1fd90362046448fecaa90ef3b0494a1c004800f3 Mon Sep 17 00:00:00 2001 From: William B <7444334+whabanks@users.noreply.github.com> Date: Mon, 9 Dec 2024 16:06:03 -0400 Subject: [PATCH] Fix post_bulk annual limit validation (#2384) * Fix validation - Add more specific logging messages when validating a job * Fix typo causing test failures --- app/celery/tasks.py | 38 ++++++++--- app/v2/notifications/post_notifications.py | 4 +- tests/app/celery/test_tasks.py | 63 ++++++++++++++++++- .../notifications/test_post_notifications.py | 6 +- 4 files changed, 93 insertions(+), 18 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index a4b554fece..432a492fe2 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -52,11 +52,9 @@ from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_id from app.dao.service_inbound_api_dao import get_service_inbound_api_for_service from app.dao.service_sms_sender_dao import dao_get_service_sms_senders_by_id -from app.dao.services_dao import ( - dao_fetch_service_by_id, - fetch_todays_total_message_count, -) +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 fetch_todays_email_count from app.encryption import SignedNotification from app.exceptions import DVLAException from app.models import ( @@ -81,6 +79,7 @@ persist_notifications, send_notification_to_queue, ) +from app.sms_fragment_utils import fetch_todays_requested_sms_count from app.types import VerifiedNotification from app.utils import get_csv_max_rows, get_delivery_queue_for_template from app.v2.errors import ( @@ -205,15 +204,36 @@ def process_rows(rows: List, template: Template, job: Job, service: Service): def __sending_limits_for_job_exceeded(service, job: Job, job_id): - total_sent = fetch_todays_total_message_count(service.id) + error_message = None + + if job.template.template_type == SMS_TYPE: + total_post_send = fetch_todays_requested_sms_count(service.id) + job.notification_count + + if total_post_send > service.sms_annual_limit: + error_message = ( + f"Job {job_id} size {job.notification_count} error. SMS annual limit {service.sms_annual_limit} exceeded." + ) + elif total_post_send > service.sms_daily_limit: + error_message = ( + f"Job {job_id} size {job.notification_count} error. SMS daily limit {service.sms_daily_limit} exceeded." + ) + else: + total_post_send = fetch_todays_email_count(service.id) + job.notification_count - if total_sent + job.notification_count > service.message_limit: + if total_post_send > service.email_annual_limit: + error_message = ( + f"Job {job_id} size {job.notification_count} error. Email annual limit {service.sms_annual_limit} exceeded." + ) + elif total_post_send > service.message_limit: + error_message = ( + f"Job {job_id} size {job.notification_count} error. SMS daily limit {service.sms_daily_limit} exceeded." + ) + + if error_message: job.job_status = JOB_STATUS_SENDING_LIMITS_EXCEEDED job.processing_finished = datetime.utcnow() dao_update_job(job) - current_app.logger.info( - "Job {} size {} error. Sending limits {} exceeded".format(job_id, job.notification_count, service.message_limit) - ) + current_app.logger.info(error_message) return True return False diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 537bc2849a..20fd0a0949 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -38,8 +38,8 @@ from app.config import QueueNames, TaskNames from app.dao.jobs_dao import dao_create_job from app.dao.notifications_dao import update_notification_status_by_reference -from app.dao.services_dao import fetch_todays_total_message_count from app.dao.templates_dao import get_precompiled_letter_template +from app.email_limit_utils import fetch_todays_email_count from app.encryption import NotificationDictToSign from app.letters.utils import upload_letter_pdf from app.models import ( @@ -189,7 +189,7 @@ def post_bulk(): else: current_app.logger.info(f"[post_notifications.post_bulk()] Checking bounce rate for service: {authenticated_service.id}") - emails_sent = fetch_todays_total_message_count(authenticated_service.id) + emails_sent = fetch_todays_email_count(authenticated_service.id) remaining_daily_messages = authenticated_service.message_limit - emails_sent remaining_annual_messages = authenticated_service.email_annual_limit - emails_sent diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index d8675db0be..713bb0e906 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -547,7 +547,7 @@ def test_should_process_sms_job_with_sender_id(self, sample_template, mocker, fa @freeze_time("2016-01-01 11:09:00.061258") def test_should_not_process_sms_job_if_would_exceed_send_limits(self, notify_db_session, mocker): - service = create_service(message_limit=9) + service = create_service(sms_daily_limit=9) template = create_template(service=service) job = create_job(template=template, notification_count=10, original_file_name="multiple_sms.csv") mocker.patch( @@ -564,7 +564,7 @@ def test_should_not_process_sms_job_if_would_exceed_send_limits(self, notify_db_ assert tasks.process_rows.called is False def test_should_not_process_sms_job_if_would_exceed_send_limits_inc_today(self, notify_db_session, mocker): - service = create_service(message_limit=1) + service = create_service(sms_daily_limit=1) template = create_template(service=service) job = create_job(template=template) @@ -580,9 +580,27 @@ def test_should_not_process_sms_job_if_would_exceed_send_limits_inc_today(self, assert s3.get_job_from_s3.called is False assert tasks.process_rows.called is False + @pytest.mark.parametrize("template_type", ["sms", "email"]) + def test_should_not_process_job_if_would_exceed_annual_limit(self, notify_db_session, template_type, mocker): + service = create_service(email_annual_limit=1, sms_annual_limit=1) + template = create_template(service=service, template_type=template_type) + job = create_job(template=template) + + save_notification(create_notification(template=template, job=job)) + + mocker.patch("app.celery.tasks.s3.get_job_from_s3") + mocker.patch("app.celery.tasks.process_rows") + + process_job(job.id) + + job = jobs_dao.dao_get_job_by_id(job.id) + assert job.job_status == "sending limits exceeded" + assert s3.get_job_from_s3.called is False + assert tasks.process_rows.called is False + @pytest.mark.parametrize("template_type", ["sms", "email"]) def test_should_not_process_email_job_if_would_exceed_send_limits_inc_today(self, notify_db_session, template_type, mocker): - service = create_service(message_limit=1) + service = create_service(message_limit=1, sms_daily_limit=1) template = create_template(service=service, template_type=template_type) job = create_job(template=template) @@ -647,6 +665,45 @@ def test_should_process_email_job_if_exactly_on_send_limits(self, notify_db_sess queue="-normal-database-tasks", ) + @pytest.mark.parametrize("template_type, save_task", [("sms", save_smss), ("email", save_emails)]) + def test_should_process_job_if_exactly_on_send_limits(self, notify_db_session, template_type, save_task, mocker): + service = create_service(message_limit=10) + template = create_template(service=service, template_type=template_type) + job = create_job(template=template, notification_count=10) + + mocker.patch( + "app.celery.tasks.s3.get_job_from_s3", + return_value=load_example_csv("multiple_email"), + ) + save_task_mock = mocker.patch(f"app.celery.tasks.{save_task.__name__}.apply_async") + mocker.patch("app.signer_notification.sign", return_value="something_encrypted") + mocker.patch("app.celery.tasks.create_uuid", return_value="uuid") + + process_job(job.id) + + s3.get_job_from_s3.assert_called_once_with(str(job.service.id), str(job.id)) + job = jobs_dao.dao_get_job_by_id(job.id) + assert job.job_status == "in progress" + save_task_mock.assert_called_with( + ( + str(job.service_id), + [ + "something_encrypted", + "something_encrypted", + "something_encrypted", + "something_encrypted", + "something_encrypted", + "something_encrypted", + "something_encrypted", + "something_encrypted", + "something_encrypted", + "something_encrypted", + ], + None, + ), + queue="-normal-database-tasks", + ) + def test_should_process_smss_job(self, notify_db_session, mocker): service = create_service(message_limit=20) template = create_template(service=service) diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index bbd2b100c6..a41129cd06 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -2359,9 +2359,7 @@ def test_post_bulk_flags_recipient_not_in_safelist_with_restricted_service(self, def test_post_bulk_flags_not_enough_remaining_messages(self, client, notify_api, notify_db, notify_db_session, mocker): service = create_service(message_limit=10) template = create_sample_template(notify_db, notify_db_session, service=service, template_type="email") - messages_count_mock = mocker.patch( - "app.v2.notifications.post_notifications.fetch_todays_total_message_count", return_value=9 - ) + messages_count_mock = mocker.patch("app.v2.notifications.post_notifications.fetch_todays_email_count", return_value=9) data = { "name": "job_name", "template_id": template.id, @@ -2389,7 +2387,7 @@ def test_post_bulk_flags_not_enough_remaining_messages(self, client, notify_api, def test_post_bulk_flags_not_enough_remaining_sms_messages(self, notify_api, client, notify_db, notify_db_session, mocker): service = create_service(sms_daily_limit=10, message_limit=100) template = create_sample_template(notify_db, notify_db_session, service=service, template_type="sms") - mocker.patch("app.v2.notifications.post_notifications.fetch_todays_total_message_count", return_value=9) + mocker.patch("app.v2.notifications.post_notifications.fetch_todays_email_count", return_value=9) messages_count_mock = mocker.patch( "app.v2.notifications.post_notifications.fetch_todays_requested_sms_count", return_value=9 )