Fix several annual limit bugs: (#2385)

* Fix several annual limit bugs:

- Fixed warning and reached email sent keys not being deleted when a services annual limits are changed
- Fixed logic issues in check_[email|sms]_annual_limit where some checks were being made against the incorrect notification type and emails for email limits were being sent when they should have been sms emails and vise versa
- Fixed an issue where the daily limit error message was taking precedence over the annual limit message when posting bulk jobs.

* Make annual limit error higher precedence

* fix(send_annual_limit_reached_email): pass in service name

* Fix wrong notification types being passed into checks

---------

Co-authored-by: Andrew Leith <[email protected]>

app/notifications/validators.py

@@ -198,8 +198,8 @@ def check_email_annual_limit(service: Service, requested_emails=0):
 
     if not send_exceeds_annual_limit:
         # Will this send put the service right at their limit?
-        if send_reaches_annual_limit and not annual_limit_client.check_has_over_limit_been_sent(service.id, SMS_TYPE):
-            annual_limit_client.set_over_sms_limit(service.id)
+        if send_reaches_annual_limit and not annual_limit_client.check_has_over_limit_been_sent(service.id, EMAIL_TYPE):
+            annual_limit_client.set_over_email_limit(service.id)
             current_app.logger.info(
                 f"Service {service.id} reached their annual email limit of {service.email_annual_limit} when sending {requested_emails} messages. Sending reached annual limit email."
             )
@@ -257,16 +257,16 @@ def check_sms_annual_limit(service: Service, requested_sms=0):
             current_app.logger.info(
                 f"Service {service.id} reached their annual SMS limit of {service.sms_annual_limit} messages. Sending reached annual limit email."
             )
-            send_annual_limit_reached_email(service, "email", current_fiscal_year + 1)
+            send_annual_limit_reached_email(service, "sms", current_fiscal_year + 1)
 
         # Will this send put annual usage within 80% of the limit?
-        if is_near_annual_limit and not annual_limit_client.check_has_warning_been_sent(service.id, EMAIL_TYPE):
-            annual_limit_client.set_nearing_email_limit(service.id)
+        if is_near_annual_limit and not annual_limit_client.check_has_warning_been_sent(service.id, SMS_TYPE):
+            annual_limit_client.set_nearing_sms_limit(service.id)
             current_app.logger.info(
                 f"Service {service.id} reached 80% of their annual SMS limit of {service.sms_annual_limit} messages. Sending annual limit usage warning email."
             )
             send_near_annual_limit_warning_email(
-                service, "email", int(sms_sent_today + sms_sent_this_fiscal), current_fiscal_year + 1
+                service, "sms", int(sms_sent_today + sms_sent_this_fiscal), current_fiscal_year + 1
             )
 
         return
@@ -497,6 +497,7 @@ def send_annual_limit_reached_email(service: Service, notification_type: Notific
         service_id=service.id,
         template_id=current_app.config["REACHED_ANNUAL_LIMIT_TEMPLATE_ID"],
         personalisation={
+            "service_name": service.name,
             "message_type_en": notification_type,
             "message_type_fr": "Courriel" if notification_type == EMAIL_TYPE else "SMS",
             "fiscal_end": fiscal_end,

app/service/rest.py

@@ -133,6 +133,10 @@
 
 register_errors(service_blueprint)
 
+# TODO: FF_ANNUAL_LIMIT - Remove once logic is consolidated in the annual_limit_client
+ANNUAL_LIMIT_SMS_OVER_NEAR_STATUS_FIELDS = ["near_sms_limit", "near_email_limit"]
+ANNUAL_LIMIT_EMAIL_OVER_NEAR_STATUS_FIELDS = ["over_email_limit", "near_email_limit"]
+
 
 @service_blueprint.errorhandler(IntegrityError)
 def handle_integrity_error(exc):
@@ -317,13 +321,11 @@ def update_service(service_id):
     if sms_annual_limit_changed:
         _warn_service_users_about_annual_limit_change(service, SMS_TYPE)
         # TODO: abstract this in the annual_limits_client
-        redis_store.delete(f"annual-limit:{service_id}:status:near_sms_limit")
-        redis_store.delete(f"annual-limit:{service_id}:status:over_sms_limit")
+        redis_store.delete_hash_fields(f"annual-limit:{service_id}:status", ANNUAL_LIMIT_SMS_OVER_NEAR_STATUS_FIELDS)
     if email_annual_limit_changed:
         _warn_service_users_about_annual_limit_change(service, EMAIL_TYPE)
         # TODO: abstract this in the annual_limits_client
-        redis_store.delete(f"annual-limit:{service_id}:status:near_email_limit")
-        redis_store.delete(f"annual-limit:{service_id}:status:over_email_limit")
+        redis_store.delete_hash_fields(f"annual-limit:{service_id}:status", ANNUAL_LIMIT_EMAIL_OVER_NEAR_STATUS_FIELDS)
 
     if service_going_live:
         _warn_services_users_about_going_live(service_id, current_data)

app/v2/notifications/post_notifications.py

@@ -701,27 +701,27 @@ def check_for_csv_errors(recipient_csv, max_rows, remaining_daily_messages, rema
                 message=f"Duplicate column headers: {', '.join(sorted(recipient_csv.duplicate_recipient_column_headers))}",
                 status_code=400,
             )
-        ## TODO: FF_ANNUAL_LIMIT - remove this if block in favour of more_rows_than_can_send_today found below
-        if recipient_csv.more_rows_than_can_send:
+        if recipient_csv.more_rows_than_can_send_this_year:
             if recipient_csv.template_type == SMS_TYPE:
                 raise BadRequestError(
-                    message=f"You only have {remaining_daily_messages} remaining sms messages before you reach your daily limit. You've tried to send {len(recipient_csv)} sms messages.",
+                    message=f"You only have {remaining_annual_messages} remaining sms messages before you reach your annual limit. You've tried to send {len(recipient_csv)} sms messages.",
                     status_code=400,
                 )
             else:
                 raise BadRequestError(
-                    message=f"You only have {remaining_daily_messages} remaining messages before you reach your daily limit. You've tried to send {nb_rows} messages.",
+                    message=f"You only have {remaining_annual_messages} remaining messages before you reach your annual limit. You've tried to send {nb_rows} messages.",
                     status_code=400,
                 )
-        if recipient_csv.more_rows_than_can_send_this_year:
+        ## TODO: FF_ANNUAL_LIMIT - remove this if block in favour of more_rows_than_can_send_today found below
+        if recipient_csv.more_rows_than_can_send:
             if recipient_csv.template_type == SMS_TYPE:
                 raise BadRequestError(
-                    message=f"You only have {remaining_annual_messages} remaining sms messages before you reach your annual limit. You've tried to send {len(recipient_csv)} sms messages.",
+                    message=f"You only have {remaining_daily_messages} remaining sms messages before you reach your daily limit. You've tried to send {len(recipient_csv)} sms messages.",
                     status_code=400,
                 )
             else:
                 raise BadRequestError(
-                    message=f"You only have {remaining_annual_messages} remaining messages before you reach your annual limit. You've tried to send {nb_rows} messages.",
+                    message=f"You only have {remaining_daily_messages} remaining messages before you reach your daily limit. You've tried to send {nb_rows} messages.",
                     status_code=400,
                 )
         if recipient_csv.more_rows_than_can_send_today:

tests/app/notifications/test_validators.py

@@ -893,3 +893,40 @@ def test_check_sms_annual_limit(
                     mock_redis_set.assert_called_with(service.id)
                     if log_msg:
                         assert log_msg in mock_logger.call_args[0][0]
+
+    def test_check_sms_annual_limit_only_sends_warning_email_once(
+        self,
+        notify_api,
+        notify_db,
+        notify_db_session,
+        mocker,
+    ):
+        mocker.patch("app.redis_store.set_hash_value")
+        mocker.patch("app.redis_store.get", return_value=45)
+        mocker.patch("app.annual_limit_client.check_has_warning_been_sent", return_value=True)
+        mock_send_email = mocker.patch("app.notifications.validators.send_notification_to_service_users")
+
+        service = create_sample_service(notify_db, notify_db_session, sms_annual_limit=49)
+
+        with set_config(notify_api, "FF_ANNUAL_LIMIT", True):
+            check_sms_annual_limit(service, 2)
+            mock_send_email.assert_not_called()
+
+    def test_check_sms_annual_limit_only_sends_reached_limit_email_once(
+        self,
+        notify_api,
+        notify_db,
+        notify_db_session,
+        mocker,
+    ):
+        mocker.patch("app.redis_store.set_hash_value")
+        mocker.patch("app.redis_store.get", return_value=45)
+        mocker.patch("app.annual_limit_client.check_has_over_limit_been_sent", return_value=True)
+        mocker.patch("app.annual_limit_client.check_has_warning_been_sent", return_value=True)
+        mock_send_email = mocker.patch("app.notifications.validators.send_notification_to_service_users")
+
+        service = create_sample_service(notify_db, notify_db_session, sms_annual_limit=49)
+
+        with set_config(notify_api, "FF_ANNUAL_LIMIT", True):
+            check_sms_annual_limit(service, 4)
+            mock_send_email.assert_not_called()

tests/app/service/test_rest.py

@@ -1028,7 +1028,7 @@ def test_update_service_annual_limits(
     mocker,
 ):
     mocker.patch("app.service.rest.send_notification_to_service_users")
-    redis_delete = mocker.patch("app.service.rest.redis_store.delete")
+    redis_delete = mocker.patch("app.service.rest.redis_store.delete_hash_fields")
     admin_request.post(
         "service.update_service",
         service_id=sample_service.id,
@@ -1040,13 +1040,11 @@ def test_update_service_annual_limits(
     assert getattr(sample_service, limit_field) == limit_value
     if limit_field == "sms_annual_limit":
         assert redis_delete.call_args_list == [
-            call(f"annual-limit:{sample_service.id}:status:near_sms_limit"),
-            call(f"annual-limit:{sample_service.id}:status:over_sms_limit"),
+            call(f"annual-limit:{sample_service.id}:status", ["near_sms_limit", "near_email_limit"]),
         ]
     if limit_field == "email_annual_limit":
         assert redis_delete.call_args_list == [
-            call(f"annual-limit:{sample_service.id}:status:near_email_limit"),
-            call(f"annual-limit:{sample_service.id}:status:over_email_limit"),
+            call(f"annual-limit:{sample_service.id}:status", ["over_email_limit", "near_email_limit"]),
         ]
 
 

tests/app/v2/notifications/test_post_notifications.py

@@ -2773,6 +2773,7 @@ def __send_notification():
 def test_post_bulk_validates_annual_limit(
     notify_api, notify_db, notify_db_session, client, template_type, data_type, expected_msg, mocker
 ):
+    mock_check_daily_limit = mocker.patch(f"app.v2.notifications.post_notifications.check_{template_type}_daily_limit")
     service = create_service(email_annual_limit=1, sms_annual_limit=1)
     template = create_template(service=service, template_type=template_type)
     data = {
@@ -2799,6 +2800,8 @@ def test_post_bulk_validates_annual_limit(
         assert response.status_code == 400
         assert resp_json["errors"][0]["message"] == expected_msg
 
+        mock_check_daily_limit.assert_not_called()
+
 
 class TestSeedingBounceRateData:
     @freeze_time("2019-01-01 12:00:00.000000")