Added new tests/code to ensure failure stats recorded

app/celery/provider_tasks.py

@@ -1,8 +1,5 @@
 import json
 
-from celery.exceptions import MaxRetriesExceededError
-
-from sqlalchemy.exc import SQLAlchemyError
 from datetime import datetime
 from monotonic import monotonic
 from flask import current_app
@@ -11,10 +8,10 @@ from app.clients.sms import SmsClientException
 from app.dao.notifications_dao import (
     update_provider_stats,
     get_notification_by_id,
-    dao_update_notification)
+    dao_update_notification, update_notification_status_by_id)
 from app.dao.provider_details_dao import get_provider_details_by_notification_type
 from app.dao.services_dao import dao_fetch_service_by_id
-from app.celery.research_mode_tasks import send_email_response, send_sms_response
+from app.celery.research_mode_tasks import send_sms_response
 
 from notifications_utils.recipients import (
     validate_and_format_phone_number
@@ -26,13 +23,20 @@ from notifications_utils.template import (
     unlink_govuk_escaped
 )
 
-retry_iteration_to_delay = {
+
-    0: 5,  # 5 seconds
+def retry_iteration_to_delay(retry):
-    1: 30,  # 30 seconds
+    """
-    2: 60 * 5,  # 5 minutes
+    Given current retry calculate some delay before retrying
-    3: 60 * 15,  # 15 minutes
+    Delay calculated as (1 + retry number) squared * 60
-    4: 60 * 30  # 30 minutes
+    0: 60 seconds (1 minute)
-}
+    1: 240 seconds (4 minutes)
+    2: 540 seconds (9 minutes)
+    3: 960 seconds (16 minutes)
+    4: 1500 seconds (25 minutes)
+    :param retry (zero indexed):
+    :return length to retry in seconds:
+    """
+    return ((retry + 1) ** 2) * 60
 
 
 @notify_celery.task(bind=True, name="send-sms-to-provider", max_retries=5, default_retry_delay=5)
@@ -73,23 +77,22 @@ def send_sms_to_provider(self, service_id, notification_id, encrypted_notificati
         notification.sent_by = provider.get_name(),
         notification.content_char_count = template.replaced_content_count
         dao_update_notification(notification)
-
     except SmsClientException as e:
         try:
             current_app.logger.error(
                 "SMS notification {} failed".format(notification_id)
             )
             current_app.logger.exception(e)
-            raise self.retry(queue="retry", countdown=retry_iteration_to_delay[self.request.retries])
+            self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries))
         except self.MaxRetriesExceededError:
-            notification.status = 'technical-failure'
+            update_notification_status_by_id(notification.id, 'technical-failure', 'failure')
 
     current_app.logger.info(
         "SMS {} created at {} sent at {}".format(notification_id, notification.created_at, notification.sent_at)
     )
     statsd_client.incr("notifications.tasks.send-sms-to-provider")
     statsd_client.timing("notifications.tasks.send-sms-to-provider.task-time", monotonic() - task_start)
-    statsd_client.timing("notifications.sms.total-time", notification.sent_at - notification.created_at)
+    statsd_client.timing("notifications.sms.total-time", datetime.utcnow() - notification.created_at)
 
 
 def provider_to_use(notification_type, notification_id):

app/celery/tasks.py

@@ -8,7 +8,6 @@ from sqlalchemy.exc import SQLAlchemyError
 from app import clients, statsd_client
 from app.clients import STATISTICS_FAILURE
 from app.clients.email import EmailClientException
-from app.clients.sms import SmsClientException
 from app.dao.services_dao import dao_fetch_service_by_id
 from app.dao.templates_dao import dao_get_template_by_id
 from app.dao.provider_details_dao import get_provider_details_by_notification_type

tests/app/celery/test_provider_tasks.py

@@ -1,16 +1,19 @@
-import uuid
+from celery.exceptions import MaxRetriesExceededError
 
 from mock import ANY, call
 
 from app import statsd_client, mmg_client
+from app.celery import provider_tasks
 from app.celery.provider_tasks import send_sms_to_provider
 from app.celery.tasks import provider_to_use
+from app.clients.sms import SmsClientException
 from app.dao import provider_statistics_dao
 from datetime import datetime
 from freezegun import freeze_time
 from app.dao import notifications_dao, provider_details_dao
 from notifications_utils.recipients import validate_phone_number, format_phone_number
 from app.celery.research_mode_tasks import send_sms_response
+from app.models import Notification, NotificationStatistics, Job
 
 from tests.app.conftest import (
     sample_notification
@@ -280,7 +283,6 @@ def test_statsd_updates(notify_db, notify_db_session, sample_service, sample_not
     mocker.patch('app.statsd_client.incr')
     mocker.patch('app.statsd_client.timing')
     mocker.patch('app.encryption.decrypt', return_value=notification)
-    mocker.patch('app.encryption.decrypt', return_value=notification)
     mocker.patch('app.mmg_client.send_sms')
     mocker.patch('app.mmg_client.get_name', return_value="mmg")
     mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async')
@@ -298,6 +300,44 @@ def test_statsd_updates(notify_db, notify_db_session, sample_service, sample_not
     ])
 
 
+def test_should_go_into_technical_error_if_exceeds_retries(
+        notify_db,
+        notify_db_session,
+        sample_service,
+        sample_notification,
+        mocker):
+
+    notification = _notification_json(
+        sample_notification.template,
+        to=sample_notification.to
+    )
+
+    mocker.patch('app.statsd_client.incr')
+    mocker.patch('app.statsd_client.timing')
+    mocker.patch('app.encryption.decrypt', return_value=notification)
+    mocker.patch('app.mmg_client.send_sms', side_effect=SmsClientException("EXPECTED"))
+    mocker.patch('app.celery.provider_tasks.send_sms_to_provider.retry', side_effect=MaxRetriesExceededError())
+
+    send_sms_to_provider(
+        sample_notification.service_id,
+        sample_notification.id,
+        "encrypted-in-reality"
+    )
+
+    provider_tasks.send_sms_to_provider.retry.assert_called_with(queue='retry', countdown=60)
+    assert statsd_client.incr.assert_not_called
+    assert statsd_client.timing.assert_not_called
+
+    db_notification = Notification.query.get(sample_notification.id)
+    assert db_notification.status == 'technical-failure'
+    notification_stats = NotificationStatistics.query.filter_by(service_id=sample_notification.service.id).first()
+    assert notification_stats.sms_requested == 1
+    assert notification_stats.sms_failed == 1
+    job = Job.query.get(sample_notification.job.id)
+    assert job.notification_count == 1
+    assert job.notifications_failed == 1
+
+
 def _notification_json(template, to, personalisation=None, job_id=None, row_number=None):
     notification = {
         "template": template.id,

tests/app/celery/test_tasks.py

@@ -2,7 +2,6 @@ import uuid
 import pytest
 from flask import current_app
 from mock import ANY
-from notifications_utils.recipients import validate_phone_number, format_phone_number
 
 from app.celery import provider_tasks
 from app.celery.tasks import (
@@ -18,13 +17,9 @@ from app.celery.tasks import (
     provider_to_use,
     timeout_notifications
 )
-from app.celery.research_mode_tasks import (
+from app.celery.research_mode_tasks import send_email_response
-    send_email_response,
+from app import (aws_ses_client, encryption, DATETIME_FORMAT, statsd_client)
-    send_sms_response
-)
-from app import (aws_ses_client, encryption, DATETIME_FORMAT, mmg_client, statsd_client)
 from app.clients.email.aws_ses import AwsSesClientException
-from app.clients.sms.mmg import MMGClientException
 from app.dao import notifications_dao, jobs_dao, provider_details_dao
 from sqlalchemy.exc import SQLAlchemyError
 from sqlalchemy.orm.exc import NoResultFound