Adjust the logic of the task to check for high failure rates

The
"check-for-services-with-high-failure-rates-or-sending-to-tv-numbers"
task had logic which meant that it checks for services with a high
percentage of permanent failures OR services that have sent more than
500 texts to TV numbers. If service(s) with high failure rates are
found, the check for sending to TV numbers didn't run.

This might have been deliberate when creating the task - services which
are sending to TV numbers are also likely to have high permanent failure
rates. However, it's possible we were sometimes missing services which
were sending to TV numbers because the code in the "elif" wasn't
reached.

This changes the check to check for both high failure rates and services
sending to TV numbers. Services might now appear in both categories, but
this alert very rarely triggers and having a service appear in both
should not be an issue.

Author: klssmith

app/celery/scheduled_tasks.py

@@ -473,10 +473,7 @@ def check_for_services_with_high_failure_rates_or_sending_to_tv_numbers():
             f"for sms messages in last 24 hours:\n"
         )
         for service in services_with_failures:
-            service_dashboard = "{}/services/{}".format(
-                current_app.config["ADMIN_BASE_URL"],
-                str(service.service_id),
-            )
+            service_dashboard = f"{current_app.config['ADMIN_BASE_URL']}/services/{service.service_id}"
             message += f"service: {service_dashboard} failure rate: {service.permanent_failure_rate},\n"
 
             current_app.logger.warning(
@@ -486,16 +483,13 @@ def check_for_services_with_high_failure_rates_or_sending_to_tv_numbers():
                 extra={"service_id": service.service_id, "permanent_failure_rate": service.permanent_failure_rate},
             )
 
-    elif services_sending_to_tv_numbers:
+    if services_sending_to_tv_numbers:
         message += (
             f"{len(services_sending_to_tv_numbers)} service(s) have sent over 500 sms messages to "
             f"tv numbers in last 24 hours:\n"
         )
         for service in services_sending_to_tv_numbers:
-            service_dashboard = "{}/services/{}".format(
-                current_app.config["ADMIN_BASE_URL"],
-                str(service.service_id),
-            )
+            service_dashboard = f"{current_app.config['ADMIN_BASE_URL']}/services/{service.service_id}"
             message += f"service: {service_dashboard} count of sms to tv numbers: {service.notification_count},\n"
 
             current_app.logger.warning(

tests/app/celery/test_scheduled_tasks.py

@@ -802,26 +802,42 @@ def test_update_status_of_fully_processed_jobs(mocker, sample_email_template, mo
 
 
 @pytest.mark.parametrize(
-    "failure_rates, sms_to_tv_numbers, expected_log, expected_message",
+    "failure_rates, sms_to_tv_numbers, expected_logs, expected_message",
     [
         [
-            [MockServicesWithHighFailureRate("123", 0.3)],
+            [MockServicesWithHighFailureRate("123", 0.3), MockServicesWithHighFailureRate("456", 0.7)],
             [],
-            "Service 123 has had a high permanent-failure rate (0.3) for text messages in the last 24 hours",
-            "1 service(s) have had high permanent-failure rates for sms messages in last "
-            "24 hours:\nservice: {}/services/{} failure rate: 0.3,\n".format(Config.ADMIN_BASE_URL, "123"),
+            [
+                "Service 123 has had a high permanent-failure rate (0.3) for text messages in the last 24 hours",
+                "Service 456 has had a high permanent-failure rate (0.7) for text messages in the last 24 hours",
+            ],
+            "2 service(s) have had high permanent-failure rates for sms messages in last 24 hours:\n"
+            f"service: {Config.ADMIN_BASE_URL}/services/123 failure rate: 0.3,\n"
+            f"service: {Config.ADMIN_BASE_URL}/services/456 failure rate: 0.7,\n",
         ],
         [
             [],
             [MockServicesSendingToTVNumbers("123", 567)],
-            "Service 123 has sent 567 text messages to tv numbers in the last 24 hours",
+            ["Service 123 has sent 567 text messages to tv numbers in the last 24 hours"],
+            "1 service(s) have sent over 500 sms messages to tv numbers in last 24 hours:\n"
+            f"service: {Config.ADMIN_BASE_URL}/services/123 count of sms to tv numbers: 567,\n",
+        ],
+        [
+            [MockServicesWithHighFailureRate("123", 0.3)],
+            [MockServicesSendingToTVNumbers("456", 567)],
+            [
+                "Service 123 has had a high permanent-failure rate (0.3) for text messages in the last 24 hours",
+                "Service 456 has sent 567 text messages to tv numbers in the last 24 hours",
+            ],
+            "1 service(s) have had high permanent-failure rates for sms messages in last 24 hours:\n"
+            f"service: {Config.ADMIN_BASE_URL}/services/123 failure rate: 0.3,\n"
             "1 service(s) have sent over 500 sms messages to tv numbers in last 24 hours:\n"
-            "service: {}/services/{} count of sms to tv numbers: 567,\n".format(Config.ADMIN_BASE_URL, "123"),
+            f"service: {Config.ADMIN_BASE_URL}/services/456 count of sms to tv numbers: 567,\n",
         ],
     ],
 )
 def test_check_for_services_with_high_failure_rates_or_sending_to_tv_numbers(
-    notify_db_session, failure_rates, sms_to_tv_numbers, expected_log, expected_message, caplog, mocker
+    notify_db_session, failure_rates, sms_to_tv_numbers, expected_logs, expected_message, caplog, mocker
 ):
     mock_create_ticket = mocker.spy(NotifySupportTicket, "__init__")
     mock_send_ticket_to_zendesk = mocker.patch(
@@ -842,7 +858,7 @@ def test_check_for_services_with_high_failure_rates_or_sending_to_tv_numbers(
 
     assert mock_failure_rates.called
     assert mock_sms_to_tv_numbers.called
-    assert expected_log in caplog.messages
+    assert set(expected_logs) == set(caplog.messages)
     mock_create_ticket.assert_called_with(
         ANY,
         message=expected_message + zendesk_actions,