Merge pull request #1781 from alphagov/only-use-data-on-queue-not-from-db

Remove unused code in send_delivery_status_to_service task

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)