diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index 14c3ba937..d2a9fb209 100644 --- a/app/celery/provider_tasks.py +++ b/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): diff --git a/app/delivery/rest.py b/app/delivery/rest.py index 67cca534b..5f534f631 100644 --- a/app/delivery/rest.py +++ b/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 ( - register_errors, - InvalidRequest -) +from app.errors import register_errors register_errors(delivery) @delivery.route('/deliver/notification/', 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) - except NoResultFound as e: - raise InvalidRequest(e, status_code=404) + send_call(notification) except Exception as e: - task_call.apply_async((str(notification_id)), queue=queue) + task_call.apply_async((str(notification.id)), queue=queue) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 8b43ecec0..60bca95c4 100644 --- a/app/delivery/send_to_providers.py +++ b/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): - notification = get_notification_by_id(notification_id) +def send_email_to_provider(notification): 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) diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index af49d64f5..94e0ff6f5 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/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)