Split checking the redis counter from incrementing it (#1970)

app/job/rest.py

@@ -30,8 +30,10 @@
 )
 from app.notifications.process_notifications import simulated_recipient
 from app.notifications.validators import (
-    check_email_limit_increment_redis_send_warnings_if_needed,
-    check_sms_limit_increment_redis_send_warnings_if_needed,
+    check_email_daily_limit,
+    check_sms_daily_limit,
+    increment_email_daily_count_send_warnings_if_needed,
+    increment_sms_daily_count_send_warnings_if_needed,
 )
 from app.schemas import (
     job_schema,
@@ -167,10 +169,10 @@ def create_job(service_id):
         is_test_notification = len(list(recipient_csv.get_rows())) == numberOfSimulated
 
         if not is_test_notification:
-            check_sms_limit_increment_redis_send_warnings_if_needed(service, recipient_csv.sms_fragment_count)
+            check_sms_daily_limit(service, recipient_csv.sms_fragment_count)
 
     if template.template_type == EMAIL_TYPE:
-        check_email_limit_increment_redis_send_warnings_if_needed(service, len(list(recipient_csv.get_rows())))
+        check_email_daily_limit(service, len(list(recipient_csv.get_rows())))
 
     if data.get("valid") != "True":
         raise InvalidRequest("File is not valid, can't create job", 400)
@@ -180,6 +182,11 @@ def create_job(service_id):
     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, recipient_csv.sms_fragment_count)
+    elif template.template_type == EMAIL_TYPE:
+        increment_email_daily_count_send_warnings_if_needed(service, len(list(recipient_csv.get_rows())))
+
     data.update({"template_version": template.version})
 
     job = job_schema.load(data)

app/notifications/validators.py

@@ -218,20 +218,18 @@ def time_until_end_of_day() -> timedelta:
     return datetime.combine(tomorrow, time.min) - dt
 
 
-def check_sms_limit_increment_redis_send_warnings_if_needed(service: Service, requested_sms=0) -> None:
+def increment_sms_daily_count_send_warnings_if_needed(service: Service, requested_sms=0) -> None:
     if not current_app.config["REDIS_ENABLED"]:
         return
 
-    check_sms_daily_limit(service, requested_sms)
     increment_todays_requested_sms_count(service.id, requested_sms)
     send_warning_sms_limit_emails_if_needed(service)
 
 
-def check_email_limit_increment_redis_send_warnings_if_needed(service: Service, requested_email=0) -> None:
+def increment_email_daily_count_send_warnings_if_needed(service: Service, requested_email=0) -> None:
     if not current_app.config["FF_EMAIL_DAILY_LIMIT"]:
         return
 
-    check_email_daily_limit(service, requested_email)
     increment_todays_email_count(service.id, requested_email)
     send_warning_email_limit_emails_if_needed(service)
 

app/service/send_notification.py

@@ -33,10 +33,12 @@
     simulated_recipient,
 )
 from app.notifications.validators import (
-    check_email_limit_increment_redis_send_warnings_if_needed,
+    check_email_daily_limit,
     check_service_has_permission,
     check_service_over_daily_message_limit,
-    check_sms_limit_increment_redis_send_warnings_if_needed,
+    check_sms_daily_limit,
+    increment_email_daily_count_send_warnings_if_needed,
+    increment_sms_daily_count_send_warnings_if_needed,
     validate_and_format_recipient,
     validate_template,
 )
@@ -70,9 +72,9 @@ def send_one_off_notification(service_id, post_data):
     if template.template_type == SMS_TYPE:
         is_test_notification = simulated_recipient(post_data["to"], template.template_type)
         if not is_test_notification:
-            check_sms_limit_increment_redis_send_warnings_if_needed(service, template_with_content.fragment_count)
+            check_sms_daily_limit(service, template_with_content.fragment_count)
     elif template.template_type == EMAIL_TYPE and current_app.config["FF_EMAIL_DAILY_LIMIT"]:
-        check_email_limit_increment_redis_send_warnings_if_needed(service, 1)  # 1 email
+        check_email_daily_limit(service, 1)  # 1 email
 
     validate_and_format_recipient(
         send_to=post_data["to"],
@@ -84,6 +86,15 @@ def send_one_off_notification(service_id, post_data):
 
     validate_created_by(service, post_data["created_by"])
 
+    if not current_app.config["FF_EMAIL_DAILY_LIMIT"]:
+        pass  # will remove this soon, don't bother refactoring
+    if template.template_type == SMS_TYPE:
+        is_test_notification = simulated_recipient(post_data["to"], template.template_type)
+        if not is_test_notification:
+            increment_sms_daily_count_send_warnings_if_needed(service, template_with_content.fragment_count)
+    elif template.template_type == EMAIL_TYPE and current_app.config["FF_EMAIL_DAILY_LIMIT"]:
+        increment_email_daily_count_send_warnings_if_needed(service, 1)  # 1 email
+
     sender_id = post_data.get("sender_id", None)
     reply_to = get_reply_to_text(
         notification_type=template.template_type,

app/v2/notifications/post_notifications.py

@@ -72,13 +72,15 @@
     transform_notification,
 )
 from app.notifications.validators import (
-    check_email_limit_increment_redis_send_warnings_if_needed,
+    check_email_daily_limit,
     check_rate_limiting,
     check_service_can_schedule_notification,
     check_service_email_reply_to_id,
     check_service_has_permission,
     check_service_sms_sender_id,
-    check_sms_limit_increment_redis_send_warnings_if_needed,
+    check_sms_daily_limit,
+    increment_email_daily_count_send_warnings_if_needed,
+    increment_sms_daily_count_send_warnings_if_needed,
     validate_and_format_recipient,
     validate_template,
     validate_template_exists,
@@ -215,7 +217,8 @@ def post_bulk():
             raise BadRequestError(message=message)
 
     if template.template_type == EMAIL_TYPE and api_user.key_type != KEY_TYPE_TEST:
-        check_email_limit_increment_redis_send_warnings_if_needed(authenticated_service, len(list(recipient_csv.get_rows())))
+        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())))
 
     if template.template_type == SMS_TYPE:
         # calculate the number of simulated recipients
@@ -231,7 +234,8 @@ def post_bulk():
         is_test_notification = api_user.key_type == KEY_TYPE_TEST or len(list(recipient_csv.get_rows())) == numberOfSimulated
 
         if not is_test_notification:
-            check_sms_limit_increment_redis_send_warnings_if_needed(authenticated_service, recipient_csv.sms_fragment_count)
+            check_sms_daily_limit(authenticated_service, recipient_csv.sms_fragment_count)
+            increment_sms_daily_count_send_warnings_if_needed(authenticated_service, recipient_csv.sms_fragment_count)
 
     job = create_bulk_job(authenticated_service, api_user, template, form, recipient_csv)
 
@@ -279,12 +283,12 @@ def post_notification(notification_type: NotificationType):
     )
 
     if template.template_type == EMAIL_TYPE and api_user.key_type != KEY_TYPE_TEST:
-        check_email_limit_increment_redis_send_warnings_if_needed(authenticated_service, 1)  # 1 email
+        check_email_daily_limit(authenticated_service, 1)  # 1 email
 
     if template.template_type == SMS_TYPE:
         is_test_notification = api_user.key_type == KEY_TYPE_TEST or simulated_recipient(form["phone_number"], notification_type)
         if not is_test_notification:
-            check_sms_limit_increment_redis_send_warnings_if_needed(authenticated_service, template_with_content.fragment_count)
+            check_sms_daily_limit(authenticated_service, template_with_content.fragment_count)
 
     current_app.logger.info(f"Trying to send notification for Template ID: {template.id}")
 
@@ -309,6 +313,14 @@ def post_notification(notification_type: NotificationType):
 
         template_with_content.values = notification.personalisation
 
+    if template.template_type == EMAIL_TYPE and api_user.key_type != KEY_TYPE_TEST:
+        increment_email_daily_count_send_warnings_if_needed(authenticated_service, 1)  # 1 email
+
+    if template.template_type == SMS_TYPE:
+        is_test_notification = api_user.key_type == KEY_TYPE_TEST or simulated_recipient(form["phone_number"], notification_type)
+        if not is_test_notification:
+            increment_sms_daily_count_send_warnings_if_needed(authenticated_service, template_with_content.fragment_count)
+
     if notification_type == SMS_TYPE:
         create_resp_partial = functools.partial(create_post_sms_response_from_notification, from_number=reply_to)
     elif notification_type == EMAIL_TYPE:

tests/app/notifications/test_validators.py

@@ -17,7 +17,6 @@
 )
 from app.notifications.validators import (
     check_email_daily_limit,
-    check_email_limit_increment_redis_send_warnings_if_needed,
     check_reply_to,
     check_service_email_reply_to_id,
     check_service_letter_contact_id,
@@ -26,9 +25,10 @@
     check_service_sms_sender_id,
     check_sms_content_char_count,
     check_sms_daily_limit,
-    check_sms_limit_increment_redis_send_warnings_if_needed,
     check_template_is_active,
     check_template_is_for_notification_type,
+    increment_email_daily_count_send_warnings_if_needed,
+    increment_sms_daily_count_send_warnings_if_needed,
     service_can_send_to_recipient,
     validate_and_format_recipient,
 )
@@ -206,7 +206,7 @@ def test_check_service_message_limit_records_nearing_daily_limit(
         self, notify_api, limit_type, template_name, notify_db, notify_db_session, mocker
     ):
         with freeze_time("2016-01-01 12:00:00.000000"):
-            redis_get = mocker.patch("app.redis_store.get", side_effect=[4, 4, 4, None])
+            redis_get = mocker.patch("app.redis_store.get", side_effect=[4, 4, None])
             send_notification = mocker.patch("app.notifications.validators.send_notification_to_service_users")
 
             service = create_sample_service(notify_db, notify_db_session, restricted=True, limit=5, sms_limit=5)
@@ -215,13 +215,12 @@ def test_check_service_message_limit_records_nearing_daily_limit(
                 create_sample_notification(notify_db, notify_db_session, service=service, template=template)
 
             if limit_type == "sms":
-                check_sms_limit_increment_redis_send_warnings_if_needed(service)
+                increment_sms_daily_count_send_warnings_if_needed(service)
             else:
                 with set_config(notify_api, "FF_EMAIL_DAILY_LIMIT", True):
-                    check_email_limit_increment_redis_send_warnings_if_needed(service)
+                    increment_email_daily_count_send_warnings_if_needed(service)
 
             assert redis_get.call_args_list == [
-                call(count_key(limit_type, service.id)),
                 call(count_key(limit_type, service.id)),
                 call(count_key(limit_type, service.id)),
                 call(near_key(limit_type, service.id)),

tests/app/service/test_send_one_off_notification.py

@@ -304,7 +304,7 @@ def test_send_one_off_notification_raises_if_over_email_limit(notify_db_session,
     service = create_service(message_limit=0)
     template = create_template(service=service, template_type=EMAIL_TYPE)
     mocker.patch(
-        "app.service.send_notification.check_email_limit_increment_redis_send_warnings_if_needed",
+        "app.service.send_notification.check_email_daily_limit",
         side_effect=LiveServiceTooManyEmailRequestsError(1),
     )
 
@@ -323,7 +323,7 @@ def test_send_one_off_notification_raises_if_over_sms_daily_limit(notify_db_sess
     service = create_service(sms_daily_limit=0)
     template = create_template(service=service)
     mocker.patch(
-        "app.service.send_notification.check_sms_limit_increment_redis_send_warnings_if_needed",
+        "app.service.send_notification.check_sms_daily_limit",
         side_effect=LiveServiceTooManySMSRequestsError(1),
     )