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

@@ -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
 

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
 

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)

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
         )