Refactor to make the new send_to_provider methods take a notification not a notification ID.

- Driven by the fact we won't know the type in the API call
- hence we need to load notification earlier , so pass it not the id through to the send task to avoid loading it twice.

app/celery/provider_tasks.py

@@ -1,6 +1,7 @@
 from flask import current_app
 
 from app import notify_celery
+from app.dao import notifications_dao
 from app.dao.notifications_dao import update_notification_status_by_id
 from app.statsd_decorators import statsd
 
@@ -35,7 +36,8 @@ def retry_iteration_to_delay(retry=0):
 @statsd(namespace="tasks")
 def deliver_sms(self, notification_id):
     try:
-        send_to_providers.send_sms_to_provider(notification_id)
+        notification = notifications_dao.get_notification_by_id(notification_id)
+        send_to_providers.send_sms_to_provider(notification)
     except Exception as e:
         try:
             current_app.logger.error(
@@ -55,7 +57,8 @@ def deliver_sms(self, notification_id):
 @statsd(namespace="tasks")
 def deliver_email(self, notification_id):
     try:
-        send_to_providers.send_email_response(notification_id)
+        notification = notifications_dao.get_notification_by_id(notification_id)
+        send_to_providers.send_email_to_provider(notification)
     except Exception as e:
         try:
             current_app.logger.error(
@@ -71,7 +74,6 @@ def deliver_email(self, notification_id):
             update_notification_status_by_id(notification_id, 'technical-failure')
 
 
-
 @notify_celery.task(bind=True, name="send-sms-to-provider", max_retries=5, default_retry_delay=5)
 @statsd(namespace="tasks")
 def send_sms_to_provider(self, service_id, notification_id):

app/delivery/rest.py

@@ -3,32 +3,31 @@ from flask import Blueprint, jsonify
 from app.delivery import send_to_providers
 from app.models import EMAIL_TYPE
 from app.celery import provider_tasks
-from sqlalchemy.orm.exc import NoResultFound
+from app.dao import notifications_dao
 
 delivery = Blueprint('delivery', __name__)
 
-from app.errors import (
+from app.errors import register_errors
-    register_errors,
-    InvalidRequest
-)
 
 register_errors(delivery)
 
 
 @delivery.route('/deliver/notification/<uuid:notification_id>',  methods=['POST'])
-def send_notification_to_provider(notification_type, notification_id):
+def send_notification_to_provider(notification_id):
 
-    if notification_type == EMAIL_TYPE:
+    notification = notifications_dao.get_notification_by_id(notification_id)
+    if not notification:
+        return jsonify(notification={"result": "error", "message": "No result found"}), 404
+
+    if notification.notification_type == EMAIL_TYPE:
         send_response(send_to_providers.send_email_response, provider_tasks.deliver_email, notification_id, 'send-email')
     else:
         send_response(send_to_providers.send_sms_response, provider_tasks.deliver_sms, notification_id, 'send-sms')
     return jsonify({}), 204
 
 
-def send_response(send_call, task_call, notification_id, queue):
+def send_response(send_call, task_call, notification, queue):
     try:
-        send_call(notification_id)
+        send_call(notification)
-    except NoResultFound as e:
-        raise InvalidRequest(e, status_code=404)
     except Exception as e:
-        task_call.apply_async((str(notification_id)), queue=queue)
+        task_call.apply_async((str(notification.id)), queue=queue)

app/delivery/send_to_providers.py

@@ -39,7 +39,7 @@ def send_sms_to_provider(notification):
             provider.send_sms(
                 to=validate_and_format_phone_number(notification.to),
                 content=template.replaced,
-                reference=str(notification_id),
+                reference=str(notification.id),
                 sender=service.sms_sender
             )
             notification.billable_units = get_sms_fragment_count(template.replaced_content_count)
@@ -50,16 +50,15 @@ def send_sms_to_provider(notification):
         dao_update_notification(notification)
 
         current_app.logger.info(
-            "SMS {} sent to provider at {}".format(notification_id, notification.sent_at)
+            "SMS {} sent to provider at {}".format(notification.id, notification.sent_at)
         )
         delta_milliseconds = (datetime.utcnow() - notification.created_at).total_seconds() * 1000
         statsd_client.timing("sms.total-time", delta_milliseconds)
 
 
-def send_email_to_provider(notification_id):
+def send_email_to_provider(notification):
-    notification = get_notification_by_id(notification_id)
     service = dao_fetch_service_by_id(notification.service_id)
-    provider = provider_to_use(EMAIL_TYPE, notification_id)
+    provider = provider_to_use(EMAIL_TYPE, notification.id)
     if notification.status == 'created':
         template_dict = dao_get_template_by_id(notification.template_id, notification.template_version).__dict__
 
@@ -99,7 +98,7 @@ def send_email_to_provider(notification_id):
         dao_update_notification(notification)
 
         current_app.logger.info(
-            "Email {} sent to provider at {}".format(notification_id, notification.sent_at)
+            "Email {} sent to provider at {}".format(notification.id, notification.sent_at)
         )
         delta_milliseconds = (datetime.utcnow() - notification.created_at).total_seconds() * 1000
         statsd_client.timing("email.total-time", delta_milliseconds)

tests/app/delivery/test_send_to_providers.py

@@ -47,18 +47,6 @@ def test_should_return_highest_priority_active_provider(notify_db, notify_db_ses
     assert send_to_providers.provider_to_use('sms', '1234').name == first.identifier
 
 
-def test_raises_not_found_exception_if_no_notification_for_id(notify_db, notify_db_session, mocker):
-    mocker.patch('app.mmg_client.send_sms')
-    mocker.patch('app.mmg_client.get_name', return_value="mmg")
-    notification_id = uuid.uuid4()
-
-    with pytest.raises(NoResultFound) as exc:
-        send_to_providers.send_sms_to_provider(notification_id)
-
-    assert str(exc.value) == "No notification for {}".format(str(notification_id))
-    app.mmg_client.send_sms.assert_not_called()
-
-
 def test_should_send_personalised_template_to_correct_sms_provider_and_persist(
     notify_db,
     notify_db_session,
@@ -73,7 +61,7 @@ def test_should_send_personalised_template_to_correct_sms_provider_and_persist(
     mocker.patch('app.mmg_client.get_name', return_value="mmg")
 
     send_to_providers.send_sms_to_provider(
-        db_notification.id
+        db_notification
     )
 
     mmg_client.send_sms.assert_called_once_with(
@@ -108,7 +96,7 @@ def test_should_send_personalised_template_to_correct_email_provider_and_persist
     mocker.patch('app.aws_ses_client.get_name', return_value="ses")
 
     send_to_providers.send_email_to_provider(
-        db_notification.id
+        db_notification
     )
 
     app.aws_ses_client.send_email.assert_called_once_with(
@@ -150,7 +138,7 @@ def test_send_sms_should_use_template_version_from_notification_not_latest(
     assert t.version > version_on_notification
 
     send_to_providers.send_sms_to_provider(
-        db_notification.id
+        db_notification
     )
 
     mmg_client.send_sms.assert_called_once_with(
@@ -187,7 +175,7 @@ def test_should_call_send_sms_response_task_if_research_mode(notify_db, sample_s
     sample_notification.key_type = key_type
 
     send_to_providers.send_sms_to_provider(
-        sample_notification.id
+        sample_notification
     )
     assert not mmg_client.send_sms.called
     send_to_providers.send_sms_response.apply_async.assert_called_once_with(
@@ -222,7 +210,7 @@ def test_should_set_billable_units_to_zero_in_research_mode_or_test_key(
     sample_notification.key_type = key_type
 
     send_to_providers.send_sms_to_provider(
-        sample_notification.id
+        sample_notification
     )
 
     assert notifications_dao.get_notification_by_id(sample_notification.id).billable_units == 0
@@ -239,7 +227,7 @@ def test_should_not_send_to_provider_when_status_is_not_created(notify_db, notif
     mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async')
 
     send_to_providers.send_sms_to_provider(
-        notification.id
+        notification
     )
 
     app.mmg_client.send_sms.assert_not_called()
@@ -264,7 +252,7 @@ def test_should_send_sms_sender_from_service_if_present(
     mocker.patch('app.mmg_client.get_name', return_value="mmg")
 
     send_to_providers.send_sms_to_provider(
-        db_notification.id
+        db_notification
     )
 
     mmg_client.send_sms.assert_called_once_with(
@@ -307,8 +295,9 @@ def test_send_email_to_provider_should_call_research_mode_task_response_task_if_
     assert not get_provider_statistics(
         sample_email_template.service,
         providers=[ses_provider.identifier]).first()
+
     send_to_providers.send_email_to_provider(
-        notification.id
+        notification
     )
     assert not app.aws_ses_client.send_email.called
     send_to_providers.send_email_response.apply_async.assert_called_once_with(
@@ -341,7 +330,7 @@ def test_send_email_to_provider_should_not_send_to_provider_when_status_is_not_c
     mocker.patch('app.celery.research_mode_tasks.send_email_response.apply_async')
 
     send_to_providers.send_sms_to_provider(
-        notification.id
+        notification
     )
 
     app.aws_ses_client.send_email.assert_not_called()
@@ -360,7 +349,7 @@ def test_send_email_should_use_service_reply_to_email(
     sample_service.reply_to_email_address = 'foo@bar.com'
 
     send_to_providers.send_email_to_provider(
-        db_notification.id,
+        db_notification,
     )
 
     app.aws_ses_client.send_email.assert_called_once_with(
@@ -421,7 +410,7 @@ def test_should_not_set_billable_units_if_research_mode(notify_db, sample_servic
     notify_db.session.commit()
 
     send_to_providers.send_sms_to_provider(
-        sample_notification.id
+        sample_notification
     )
 
     persisted_notification = notifications_dao.get_notification_by_id(sample_notification.id)