Implemented deleted notification if SQS write fails

app/dao/notifications_dao.py

@@ -106,7 +106,7 @@ def dao_get_template_usage(service_id, limit_days=None):
 
 @statsd(namespace="dao")
 def dao_get_last_template_usage(template_id):
-    return NotificationHistory.query.filter(NotificationHistory.template_id == template_id)\
+    return NotificationHistory.query.filter(NotificationHistory.template_id == template_id) \
         .join(Template) \
         .order_by(desc(NotificationHistory.created_at)) \
         .first()
@@ -275,3 +275,14 @@ def delete_notifications_created_more_than_a_week_ago(status):
     ).delete(synchronize_session='fetch')
     db.session.commit()
     return deleted
+
+
+@statsd(namespace="dao")
+@transactional
+def dao_delete_notifications_and_history_by_id(notification_id):
+    db.session.query(Notification).filter(
+        Notification.id == notification_id
+    ).delete(synchronize_session='fetch')
+    db.session.query(NotificationHistory).filter(
+        NotificationHistory.id == notification_id
+    ).delete(synchronize_session='fetch')

app/notifications/rest.py

@@ -13,8 +13,8 @@ from notifications_utils.recipients import allowed_to_send_to, first_column_head
 from notifications_utils.template import Template
 from notifications_utils.renderers import PassThrough
 from app.clients.email.aws_ses import get_aws_responses
-from app import api_user, encryption, create_uuid, DATETIME_FORMAT, DATE_FORMAT, statsd_client
-from app.dao.notifications_dao import dao_create_notification
+from app import api_user, create_uuid, DATETIME_FORMAT, statsd_client
+from app.dao.notifications_dao import dao_create_notification, dao_delete_notifications_and_history_by_id
 from app.dao.services_dao import dao_fetch_todays_stats_for_service
 from app.models import KEY_TYPE_TEAM, KEY_TYPE_TEST, Notification, KEY_TYPE_NORMAL, EMAIL_TYPE
 from app.dao import (
@@ -36,7 +36,6 @@ from app.schemas import (
     day_schema,
     unarchived_template_schema
 )
-from app.celery.tasks import send_sms, send_email
 from app.utils import pagination_links
 
 notifications = Blueprint('notifications', __name__)
@@ -320,10 +319,15 @@ def persist_notification(
         )
     )
 
-    if notification_type == SMS_TYPE:
-        send_sms_to_provider.apply_async((str(service.id), str(notification_id)), queue='send-sms')
-    if notification_type == EMAIL_TYPE:
-        send_email_to_provider.apply_async((str(service.id), str(notification_id)), queue='send-email')
+    try:
+        if notification_type == SMS_TYPE:
+            send_sms_to_provider.apply_async((str(service.id), str(notification_id)), queue='send-sms')
+        if notification_type == EMAIL_TYPE:
+            send_email_to_provider.apply_async((str(service.id), str(notification_id)), queue='send-email')
+    except Exception as e:
+        current_app.logger.exception("Failed to send to SQS exception", e)
+        dao_delete_notifications_and_history_by_id(notification_id)
+        raise InvalidRequest(message="Internal server error", status_code=500)
 
     current_app.logger.info(
         "{} {} created at {}".format(notification_type, notification_id, created_at)

tests/app/dao/test_notification_dao.py

@@ -33,7 +33,8 @@ from app.dao.notifications_dao import (
     get_notifications_for_job,
     get_notifications_for_service,
     update_notification_status_by_id,
-    update_notification_status_by_reference
+    update_notification_status_by_reference,
+    dao_delete_notifications_and_history_by_id
 )
 
 from notifications_utils.template import get_sms_fragment_count
@@ -55,6 +56,7 @@ def test_should_have_decorated_notifications_dao_functions():
     assert get_notifications_for_service.__wrapped__.__name__ == 'get_notifications_for_service'  # noqa
     assert get_notification_by_id.__wrapped__.__name__ == 'get_notification_by_id'  # noqa
     assert delete_notifications_created_more_than_a_week_ago.__wrapped__.__name__ == 'delete_notifications_created_more_than_a_week_ago'  # noqa
+    assert dao_delete_notifications_and_history_by_id.__wrapped__.__name__ == 'dao_delete_notifications_and_history_by_id'  # noqa
 
 
 def test_should_be_able_to_get_template_usage_history(notify_db, notify_db_session, sample_service):
@@ -73,7 +75,6 @@ def test_should_be_able_to_get_all_template_usage_history_order_by_notification_
         notify_db,
         notify_db_session,
         sample_service):
-
     sms = sample_template(notify_db, notify_db_session)
 
     sample_notification(notify_db, notify_db_session, service=sample_service, template=sms)
@@ -88,7 +89,6 @@ def test_should_be_able_to_get_all_template_usage_history_order_by_notification_
 def test_should_be_able_to_get_no_template_usage_history_if_no_notifications_using_template(
         notify_db,
         notify_db_session):
-
     sms = sample_template(notify_db, notify_db_session)
 
     results = dao_get_last_template_usage(sms.id)
@@ -729,6 +729,67 @@ def test_updating_notification_updates_notification_history(sample_notification)
     assert hist.status == 'sending'
 
 
+def test_should_delete_notification_and_notification_history_for_id(notify_db, notify_db_session, sample_template):
+    data = _notification_json(sample_template)
+    notification = Notification(**data)
+
+    dao_create_notification(notification)
+
+    assert Notification.query.count() == 1
+    assert NotificationHistory.query.count() == 1
+
+    dao_delete_notifications_and_history_by_id(notification.id)
+
+    assert Notification.query.count() == 0
+    assert NotificationHistory.query.count() == 0
+
+
+def test_should_delete_only_notification_and_notification_history_with_id(notify_db, notify_db_session,
+                                                                          sample_template):
+    id_1 = uuid.uuid4()
+    id_2 = uuid.uuid4()
+    data_1 = _notification_json(sample_template, id=id_1)
+    data_2 = _notification_json(sample_template, id=id_2)
+
+    notification_1 = Notification(**data_1)
+    notification_2 = Notification(**data_2)
+
+    dao_create_notification(notification_1)
+    dao_create_notification(notification_2)
+
+    assert Notification.query.count() == 2
+    assert NotificationHistory.query.count() == 2
+
+    dao_delete_notifications_and_history_by_id(notification_1.id)
+
+    assert Notification.query.count() == 1
+    assert NotificationHistory.query.count() == 1
+    assert Notification.query.first().id == notification_2.id
+    assert NotificationHistory.query.first().id == notification_2.id
+
+
+def test_should_delete_no_notifications_or_notification_historys_if_no_matching_ids(
+        notify_db,
+        notify_db_session,
+        sample_template
+):
+    id_1 = uuid.uuid4()
+    id_2 = uuid.uuid4()
+    data_1 = _notification_json(sample_template, id=id_1)
+
+    notification_1 = Notification(**data_1)
+
+    dao_create_notification(notification_1)
+
+    assert Notification.query.count() == 1
+    assert NotificationHistory.query.count() == 1
+
+    dao_delete_notifications_and_history_by_id(id_2)
+
+    assert Notification.query.count() == 1
+    assert NotificationHistory.query.count() == 1
+
+
 def _notification_json(sample_template, job_id=None, id=None, status=None):
     data = {
         'to': '+44709123456',

tests/app/notifications/rest/test_send_notification.py

@@ -12,7 +12,7 @@ from notifications_python_client.authentication import create_jwt_token
 import app
 from app import encryption
 from app.dao import notifications_dao
-from app.models import ApiKey, KEY_TYPE_TEAM, KEY_TYPE_TEST, Notification
+from app.models import ApiKey, KEY_TYPE_TEAM, KEY_TYPE_TEST, Notification, NotificationHistory
 from app.dao.templates_dao import dao_get_all_templates_for_service, dao_update_template
 from app.dao.services_dao import dao_update_service
 from app.dao.api_key_dao import save_model_api_key
@@ -874,3 +874,73 @@ def test_should_persist_email_notification(notify_api, sample_email_template, fa
         assert notification.to == sample_email_template.service.created_by.email_address
         assert notification.template_id == sample_email_template.id
         assert notification.notification_type == 'email'
+
+
+def test_should_delete_email_notification_and_return_error_if_sqs_fails(
+        notify_api,
+        sample_email_template,
+        fake_uuid,
+        mocker):
+    with notify_api.test_request_context(), notify_api.test_client() as client:
+        mocker.patch(
+            'app.celery.provider_tasks.send_email_to_provider.apply_async',
+            side_effect=Exception("failed to talk to SQS")
+        )
+        mocker.patch('app.notifications.rest.create_uuid', return_value=fake_uuid)
+
+        data = {
+            'to': sample_email_template.service.created_by.email_address,
+            'template': sample_email_template.id
+        }
+        api_key = ApiKey(
+            service=sample_email_template.service,
+            name='team_key',
+            created_by=sample_email_template.created_by,
+            key_type=KEY_TYPE_TEAM)
+        save_model_api_key(api_key)
+        auth_header = create_jwt_token(secret=api_key.unsigned_secret, client_id=str(api_key.service_id))
+
+        response = client.post(
+            path='/notifications/email',
+            data=json.dumps(data),
+            headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))])
+
+        app.celery.provider_tasks.send_email_to_provider.apply_async.assert_called_once_with(
+            (str(sample_email_template.service.id), fake_uuid), queue='send-email')
+
+        assert response.status_code == 500
+        assert not notifications_dao.get_notification_by_id(fake_uuid)
+        assert not NotificationHistory.query.get(fake_uuid)
+
+
+def test_should_delete_sms_notification_and_return_error_if_sqs_fails(notify_api, sample_template, fake_uuid, mocker):
+    with notify_api.test_request_context(), notify_api.test_client() as client:
+        mocker.patch(
+            'app.celery.provider_tasks.send_sms_to_provider.apply_async',
+            side_effect=Exception("failed to talk to SQS")
+        )
+        mocker.patch('app.notifications.rest.create_uuid', return_value=fake_uuid)
+
+        data = {
+            'to': sample_template.service.created_by.mobile_number,
+            'template': sample_template.id
+        }
+        api_key = ApiKey(
+            service=sample_template.service,
+            name='team_key',
+            created_by=sample_template.created_by,
+            key_type=KEY_TYPE_TEAM)
+        save_model_api_key(api_key)
+        auth_header = create_jwt_token(secret=api_key.unsigned_secret, client_id=str(api_key.service_id))
+
+        response = client.post(
+            path='/notifications/sms',
+            data=json.dumps(data),
+            headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))])
+
+        app.celery.provider_tasks.send_sms_to_provider.apply_async.assert_called_once_with(
+            (str(sample_template.service.id), fake_uuid), queue='send-sms')
+
+        assert response.status_code == 500
+        assert not notifications_dao.get_notification_by_id(fake_uuid)
+        assert not NotificationHistory.query.get(fake_uuid)