The send_delivery_status_to_service task was refactor to take the details of the notification and service api callback such that the task no longer needed to go to the database to provide the status update.

This PR removes the code that is no longer used. This extra step was necessary to keep the tasks backward compatible.

app/celery/service_callback_tasks.py

@@ -1,119 +1,52 @@
 import json
 
+from flask import current_app
 from notifications_utils.statsd_decorators import statsd
-
-from app import (
-    db,
-    DATETIME_FORMAT,
-    notify_celery,
-    encryption
-)
-from app.dao.notifications_dao import (
-    get_notification_by_id,
-)
-
-from app.dao.service_callback_api_dao import get_service_callback_api_for_service
 from requests import (
     HTTPError,
     request,
     RequestException
 )
-from flask import current_app
+
+from app import (
+    notify_celery,
+    encryption
+)
 from app.config import QueueNames
 
 
 @notify_celery.task(bind=True, name="send-delivery-status", max_retries=5, default_retry_delay=300)
 @statsd(namespace="tasks")
 def send_delivery_status_to_service(self, notification_id,
-                                    encrypted_status_update=None
+                                    encrypted_status_update
                                     ):
-    if not encrypted_status_update:
-        process_update_with_notification_id(self, notification_id=notification_id)
-    else:
-        try:
-            status_update = encryption.decrypt(encrypted_status_update)
-
-            data = {
-                "id": str(notification_id),
-                "reference": status_update['notification_client_reference'],
-                "to": status_update['notification_to'],
-                "status": status_update['notification_status'],
-                "created_at": status_update['notification_created_at'],
-                "completed_at": status_update['notification_updated_at'],
-                "sent_at": status_update['notification_sent_at'],
-                "notification_type": status_update['notification_type']
-            }
-
-            response = request(
-                method="POST",
-                url=status_update['service_callback_api_url'],
-                data=json.dumps(data),
-                headers={
-                    'Content-Type': 'application/json',
-                    'Authorization': 'Bearer {}'.format(status_update['service_callback_api_bearer_token'])
-                },
-                timeout=60
-            )
-            current_app.logger.info('send_delivery_status_to_service sending {} to {}, response {}'.format(
-                notification_id,
-                status_update['service_callback_api_url'],
-                response.status_code
-            ))
-            response.raise_for_status()
-        except RequestException as e:
-            current_app.logger.warning(
-                "send_delivery_status_to_service request failed for notification_id: {} and url: {}. exc: {}".format(
-                    notification_id,
-                    status_update['service_callback_api_url'],
-                    e
-                )
-            )
-            if not isinstance(e, HTTPError) or e.response.status_code >= 500:
-                try:
-                    self.retry(queue=QueueNames.RETRY)
-                except self.MaxRetriesExceededError:
-                    current_app.logger.exception(
-                        """Retry: send_delivery_status_to_service has retried the max num of times
-                         for notification: {}""".format(notification_id)
-                    )
-
-
-def process_update_with_notification_id(self, notification_id):
-    retry = False
     try:
-        notification = get_notification_by_id(notification_id)
+        status_update = encryption.decrypt(encrypted_status_update)
-        service_callback_api = get_service_callback_api_for_service(service_id=notification.service_id)
-        if not service_callback_api:
-            # No delivery receipt API info set
-            return
-
-        # Release DB connection before performing an external HTTP request
-        db.session.close()
 
         data = {
             "id": str(notification_id),
-            "reference": str(notification.client_reference),
+            "reference": status_update['notification_client_reference'],
-            "to": notification.to,
+            "to": status_update['notification_to'],
-            "status": notification.status,
+            "status": status_update['notification_status'],
-            "created_at": notification.created_at.strftime(DATETIME_FORMAT),
+            "created_at": status_update['notification_created_at'],
-            "completed_at": notification.updated_at.strftime(DATETIME_FORMAT),
+            "completed_at": status_update['notification_updated_at'],
-            "sent_at": notification.sent_at.strftime(DATETIME_FORMAT),
+            "sent_at": status_update['notification_sent_at'],
-            "notification_type": notification.notification_type
+            "notification_type": status_update['notification_type']
         }
 
         response = request(
             method="POST",
-            url=service_callback_api.url,
+            url=status_update['service_callback_api_url'],
             data=json.dumps(data),
             headers={
                 'Content-Type': 'application/json',
-                'Authorization': 'Bearer {}'.format(service_callback_api.bearer_token)
+                'Authorization': 'Bearer {}'.format(status_update['service_callback_api_bearer_token'])
             },
             timeout=60
         )
         current_app.logger.info('send_delivery_status_to_service sending {} to {}, response {}'.format(
             notification_id,
-            service_callback_api.url,
+            status_update['service_callback_api_url'],
             response.status_code
         ))
         response.raise_for_status()
@@ -121,26 +54,18 @@ def process_update_with_notification_id(self, notification_id):
         current_app.logger.warning(
             "send_delivery_status_to_service request failed for notification_id: {} and url: {}. exc: {}".format(
                 notification_id,
-                service_callback_api.url,
+                status_update['service_callback_api_url'],
                 e
             )
         )
         if not isinstance(e, HTTPError) or e.response.status_code >= 500:
-            retry = True
+            try:
-    except Exception as e:
+                self.retry(queue=QueueNames.RETRY)
-        current_app.logger.exception(
+            except self.MaxRetriesExceededError:
-            'Unhandled exception when sending callback for notification {}'.format(notification_id)
+                current_app.logger.exception(
-        )
+                    """Retry: send_delivery_status_to_service has retried the max num of times
-        retry = True
+                     for notification: {}""".format(notification_id)
-
+                )
-    if retry:
-        try:
-            self.retry(queue=QueueNames.RETRY)
-        except self.MaxRetriesExceededError:
-            current_app.logger.exception(
-                """Retry: send_delivery_status_to_service has retried the max num of times
-                 for notification: {}""".format(notification_id)
-            )
 
 
 def create_encrypted_callback_data(notification, service_callback_api):

tests/app/celery/test_service_callback_tasks.py

@@ -1,22 +1,17 @@
-import uuid
 import json
 from datetime import datetime
 
-from requests import RequestException
 import pytest
 import requests_mock
-from sqlalchemy.exc import SQLAlchemyError
 
 from app import (DATETIME_FORMAT, encryption)
-
+from app.celery.service_callback_tasks import send_delivery_status_to_service
 from tests.app.db import (
     create_notification,
     create_service_callback_api,
     create_service,
     create_template
 )
-from app.celery.service_callback_tasks import send_delivery_status_to_service
-from app.config import QueueNames
 
 
 @pytest.mark.parametrize("notification_type",
@@ -157,148 +152,3 @@ def _set_up_encrypted_data(callback_api, notification):
     }
     encrypted_status_update = encryption.encrypt(data)
     return encrypted_status_update
-
-
-# We are updating the task to take everything it needs so that there are no db calls.
-# The following tests will be deleted once that is complete.
-@pytest.mark.parametrize("notification_type",
-                         ["email", "letter", "sms"])
-def test_send_delivery_status_to_service_post_https_request_to_service(
-        notify_db_session, notification_type):
-    callback_api, template = _set_up_test_data(notification_type)
-    datestr = datetime(2017, 6, 20)
-
-    notification = create_notification(template=template,
-                                       created_at=datestr,
-                                       updated_at=datestr,
-                                       sent_at=datestr,
-                                       status='sent'
-                                       )
-
-    with requests_mock.Mocker() as request_mock:
-        request_mock.post(callback_api.url,
-                          json={},
-                          status_code=200)
-        send_delivery_status_to_service(notification.id)
-
-    mock_data = {
-        "id": str(notification.id),
-        "reference": str(notification.client_reference),
-        "to": notification.to,
-        "status": notification.status,
-        "created_at": datestr.strftime(DATETIME_FORMAT),  # the time GOV.UK email sent the request
-        "completed_at": datestr.strftime(DATETIME_FORMAT),  # the last time the status was updated
-        "sent_at": datestr.strftime(DATETIME_FORMAT),  # the time the email was sent
-        "notification_type": notification_type
-    }
-
-    assert request_mock.call_count == 1
-    assert request_mock.request_history[0].url == callback_api.url
-    assert request_mock.request_history[0].method == 'POST'
-    assert request_mock.request_history[0].text == json.dumps(mock_data)
-    assert request_mock.request_history[0].headers["Content-type"] == "application/json"
-    assert request_mock.request_history[0].headers["Authorization"] == "Bearer {}".format(callback_api.bearer_token)
-
-
-@pytest.mark.parametrize("notification_type",
-                         ["email", "letter", "sms"])
-def test_send_delivery_status_to_service_does_not_sent_request_when_service_callback_api_does_not_exist(
-        notify_db_session, mocker, notification_type):
-    service = create_service(restricted=True)
-    template = create_template(service=service, template_type=notification_type, subject='Hello')
-    datestr = datetime(2017, 6, 20)
-
-    notification = create_notification(template=template,
-                                       created_at=datestr,
-                                       updated_at=datestr,
-                                       sent_at=datestr,
-                                       status='sent'
-                                       )
-    mocked = mocker.patch("requests.request")
-    send_delivery_status_to_service(notification.id)
-
-    assert mocked.call_count == 0
-
-
-@pytest.mark.parametrize("notification_type",
-                         ["email", "letter", "sms"])
-def test_send_delivery_status_to_service_retries_if_request_returns_500(notify_db_session,
-                                                                        mocker,
-                                                                        notification_type):
-    callback_api, template = _set_up_test_data(notification_type)
-    datestr = datetime(2017, 6, 20)
-    notification = create_notification(template=template,
-                                       created_at=datestr,
-                                       updated_at=datestr,
-                                       sent_at=datestr,
-                                       status='sent'
-                                       )
-    mocked = mocker.patch('app.celery.service_callback_tasks.send_delivery_status_to_service.retry')
-    with requests_mock.Mocker() as request_mock:
-        request_mock.post(callback_api.url,
-                          json={},
-                          status_code=500)
-        send_delivery_status_to_service(notification.id)
-
-    assert mocked.call_count == 1
-    assert mocked.call_args[1]['queue'] == 'retry-tasks'
-
-
-@pytest.mark.parametrize("notification_type",
-                         ["email", "letter", "sms"])
-def test_send_delivery_status_to_service_retries_if_request_throws_unknown(notify_db_session,
-                                                                           mocker,
-                                                                           notification_type):
-
-    callback_api, template = _set_up_test_data(notification_type)
-    datestr = datetime(2017, 6, 20)
-    notification = create_notification(template=template,
-                                       created_at=datestr,
-                                       updated_at=datestr,
-                                       sent_at=datestr,
-                                       status='sent'
-                                       )
-
-    mocked = mocker.patch('app.celery.service_callback_tasks.send_delivery_status_to_service.retry')
-    mocker.patch("app.celery.tasks.request", side_effect=RequestException())
-
-    send_delivery_status_to_service(notification.id)
-
-    assert mocked.call_count == 1
-    assert mocked.call_args[1]['queue'] == 'retry-tasks'
-
-
-@pytest.mark.parametrize("notification_type",
-                         ["email", "letter", "sms"])
-def test_send_delivery_status_to_service_does_not_retries_if_request_returns_404(
-        notify_db_session,
-        mocker,
-        notification_type
-):
-    callback_api, template = _set_up_test_data(notification_type)
-    datestr = datetime(2017, 6, 20)
-    notification = create_notification(template=template,
-                                       created_at=datestr,
-                                       updated_at=datestr,
-                                       sent_at=datestr,
-                                       status='sent'
-                                       )
-    mocked = mocker.patch('app.celery.service_callback_tasks.send_delivery_status_to_service.retry')
-    with requests_mock.Mocker() as request_mock:
-        request_mock.post(callback_api.url,
-                          json={},
-                          status_code=404)
-        send_delivery_status_to_service(notification.id)
-
-    assert mocked.call_count == 0
-
-
-def test_send_delivery_status_to_service_retries_if_database_error(client, mocker):
-    notification_id = uuid.uuid4()
-    db_call = mocker.patch('app.celery.service_callback_tasks.get_notification_by_id', side_effect=SQLAlchemyError)
-    retry = mocker.patch('app.celery.service_callback_tasks.send_delivery_status_to_service.retry')
-
-    send_delivery_status_to_service(notification_id)
-
-    db_call.assert_called_once_with(notification_id)
-    retry.assert_called_once_with(queue=QueueNames.RETRY)