set delete(synchronize_session=False) on bulk deletes

When we're bulk updating, make sure we call `synchronize_session=False`,
and make sure that we then commit before we try and access any ORM
objects in the session that might be deleted.

Tutorial time:

when you delete, sqlalchemy needs to work out what to do with objects in
the session. "evaluate", "fetch", or False (do nothing). It wants to
remove items from the session if you're about to delete them, so that
you don't get confused about the state of objects

* evaluate compares the delete query to each item in the session in
  turn. this is the default. if you have lots in the session this could
  be super slow I guess but that's rarely the case for us. This can lead
  to errors if the column names are different to the model names, like
  on our notification and history models.
* fetch runs the delete query as if it's a select, and then checks the
  results of that vs the session. This could be slow.
* False doesn't do anything. This means the session will be stale and
  potentially will contain now-deleted items, until we call `commit` or
  `expire_all` on the session.

https://docs.sqlalchemy.org/en/13/orm/query.html?highlight=query.update#sqlalchemy.orm.query.Query.delete

app/celery/process_sms_client_response_tasks.py

@@ -1,20 +1,17 @@
 import uuid
-
 from datetime import datetime
+
 from flask import current_app
+from notifications_utils.statsd_decorators import statsd
 from notifications_utils.template import SMSMessageTemplate
 
-from app import statsd_client
+from app import notify_celery, statsd_client
 from app.clients import ClientException
-from app.dao import notifications_dao
 from app.clients.sms.firetext import get_firetext_responses
 from app.clients.sms.mmg import get_mmg_responses
-from app.celery.service_callback_tasks import (
-    send_delivery_status_to_service,
-    create_delivery_status_callback_data,
-)
+from app.celery.service_callback_tasks import send_delivery_status_to_service, create_delivery_status_callback_data
 from app.config import QueueNames
-from app.dao.notifications_dao import dao_update_notification
+from app.dao import notifications_dao
 from app.dao.service_callback_api_dao import get_service_delivery_status_callback_api_for_service
 from app.dao.templates_dao import dao_get_template_by_id
 from app.models import NOTIFICATION_PENDING
@@ -25,39 +22,23 @@ sms_response_mapper = {
 }
 
 
-def validate_callback_data(data, fields, client_name):
-    errors = []
-    for f in fields:
-        if not str(data.get(f, '')):
-            error = "{} callback failed: {} missing".format(client_name, f)
-            errors.append(error)
-    return errors if len(errors) > 0 else None
-
-
-def process_sms_client_response(status, provider_reference, client_name):
-    success = None
-    errors = None
+@notify_celery.task(bind=True, name="process-sms-client-response", max_retries=5, default_retry_delay=300)
+@statsd(namespace="tasks")
+def process_sms_client_response(self, status, provider_reference, client_name):
     # validate reference
-    if provider_reference == 'send-sms-code':
-        success = "{} callback succeeded: send-sms-code".format(client_name)
-        return success, errors
-
     try:
         uuid.UUID(provider_reference, version=4)
-    except ValueError:
-        errors = "{} callback with invalid reference {}".format(client_name, provider_reference)
-        return success, errors
+    except ValueError as e:
+        current_app.logger.exception(f'{client_name} callback with invalid reference {provider_reference}')
+        raise e
 
-    try:
-        response_parser = sms_response_mapper[client_name]
-    except KeyError:
-        return success, 'unknown sms client: {}'.format(client_name)
+    response_parser = sms_response_mapper[client_name]
 
-    # validate  status
+    # validate status
     try:
         notification_status = response_parser(status)
-        current_app.logger.info('{} callback return status of {} for reference: {}'.format(
-            client_name, status, provider_reference)
+        current_app.logger.info(
+            f'{client_name} callback returned status of {status} for reference: {provider_reference}'
         )
     except KeyError:
         _process_for_status(
@@ -65,14 +46,13 @@ def process_sms_client_response(status, provider_reference, client_name):
             client_name=client_name,
             provider_reference=provider_reference
         )
-        raise ClientException("{} callback failed: status {} not found.".format(client_name, status))
+        raise ClientException(f'{client_name} callback failed: status {status} not found.')
 
-    success = _process_for_status(
+    _process_for_status(
         notification_status=notification_status,
         client_name=client_name,
         provider_reference=provider_reference
     )
-    return success, errors
 
 
 def _process_for_status(notification_status, client_name, provider_reference):
@@ -114,11 +94,3 @@ def _process_for_status(notification_status, client_name, provider_reference):
             encrypted_notification = create_delivery_status_callback_data(notification, service_callback_api)
             send_delivery_status_to_service.apply_async([str(notification.id), encrypted_notification],
                                                         queue=QueueNames.CALLBACKS)
-
-    success = "{} callback succeeded. reference {} updated".format(client_name, provider_reference)
-    return success
-
-
-def set_notification_sent_by(notification, client_name):
-    notification.sent_by = client_name
-    dao_update_notification(notification)

app/celery/tasks.py

@@ -4,6 +4,7 @@ from collections import namedtuple, defaultdict
 
 from flask import current_app
 from notifications_utils.recipients import (
+    format_postcode_for_printing,
     RecipientCSV
 )
 from notifications_utils.statsd_decorators import statsd
@@ -305,6 +306,10 @@ def save_letter(
     # we store the recipient as just the first item of the person's address
     recipient = notification['personalisation']['addressline1']
 
+    notification['personalisation']['postcode'] = format_postcode_for_printing(
+        notification['personalisation']['postcode']
+    )
+
     service = dao_fetch_service_by_id(service_id)
     template = dao_get_template_by_id(notification['template'], version=notification['template_version'])
 

app/config.py

@@ -28,6 +28,7 @@ class QueueNames(object):
     CREATE_LETTERS_PDF = 'create-letters-pdf-tasks'
     CALLBACKS = 'service-callbacks'
     LETTERS = 'letter-tasks'
+    SMS_CALLBACKS = 'sms-callbacks'
     ANTIVIRUS = 'antivirus-tasks'
     SANITISE_LETTERS = 'sanitise-letter-tasks'
 
@@ -47,6 +48,7 @@ class QueueNames(object):
             QueueNames.CREATE_LETTERS_PDF,
             QueueNames.CALLBACKS,
             QueueNames.LETTERS,
+            QueueNames.SMS_CALLBACKS,
         ]
 
 

app/dao/inbound_sms_dao.py

@@ -102,7 +102,7 @@ def _delete_inbound_sms(datetime_to_delete_from, query_filter):
     while number_deleted > 0:
         _insert_inbound_sms_history(subquery, query_limit=query_limit)
 
-        number_deleted = InboundSms.query.filter(InboundSms.id.in_(subquery)).delete(synchronize_session='fetch')
+        number_deleted = InboundSms.query.filter(InboundSms.id.in_(subquery)).delete(synchronize_session=False)
         deleted += number_deleted
 
     return deleted

app/dao/notifications_dao.py

@@ -307,19 +307,14 @@ def delete_notifications_older_than_retention_by_type(notification_type, qry_lim
     ).all()
     deleted = 0
     for f in flexible_data_retention:
-        days_of_retention = get_london_midnight_in_utc(
-            convert_utc_to_bst(datetime.utcnow()).date()) - timedelta(days=f.days_of_retention)
-
-        if notification_type == LETTER_TYPE:
-            _delete_letters_from_s3(
-                notification_type, f.service_id, days_of_retention, qry_limit
-            )
-
-        insert_update_notification_history(notification_type, days_of_retention, f.service_id)
-
         current_app.logger.info(
             "Deleting {} notifications for service id: {}".format(notification_type, f.service_id))
-        deleted += _delete_notifications(notification_type, days_of_retention, f.service_id, qry_limit)
+
+        day_to_delete_backwards_from = get_london_midnight_in_utc(
+            convert_utc_to_bst(datetime.utcnow()).date()) - timedelta(days=f.days_of_retention)
+
+        deleted += _move_notifications_to_notification_history(
+            notification_type, f.service_id, day_to_delete_backwards_from, qry_limit)
 
     current_app.logger.info(
         'Deleting {} notifications for services without flexible data retention'.format(notification_type))
@@ -329,59 +324,66 @@ def delete_notifications_older_than_retention_by_type(notification_type, qry_lim
     service_ids_to_purge = db.session.query(Service.id).filter(Service.id.notin_(services_with_data_retention)).all()
 
     for service_id in service_ids_to_purge:
-        if notification_type == LETTER_TYPE:
-            _delete_letters_from_s3(
-                notification_type, service_id, seven_days_ago, qry_limit
-            )
-        insert_update_notification_history(notification_type, seven_days_ago, service_id)
-        deleted += _delete_notifications(notification_type, seven_days_ago, service_id, qry_limit)
+        deleted += _move_notifications_to_notification_history(
+            notification_type, service_id, seven_days_ago, qry_limit)
 
     current_app.logger.info('Finished deleting {} notifications'.format(notification_type))
 
     return deleted
 
 
-def _delete_notifications(notification_type, date_to_delete_from, service_id, query_limit):
-    subquery = db.session.query(
-        Notification.id
-    ).join(NotificationHistory, NotificationHistory.id == Notification.id).filter(
-        Notification.notification_type == notification_type,
-        Notification.service_id == service_id,
-        Notification.created_at < date_to_delete_from,
-    ).limit(query_limit).subquery()
+def _move_notifications_to_notification_history(notification_type, service_id, day_to_delete_backwards_from, qry_limit):
+    deleted = 0
+    if notification_type == LETTER_TYPE:
+        _delete_letters_from_s3(
+            notification_type, service_id, day_to_delete_backwards_from, qry_limit
+        )
 
-    deleted = _delete_for_query(subquery)
+    stop = -1  # exclusive, we want to include 0
+    step = -1
+    for hour_delta in range(23, stop, step):
+        # We find the timestamp we want to delete all notifications backwards from
+        # We then start 23 hours ago, and do an insert notification history before deleting all notifications older
+        # We then look 22 hours ago, do an insert notifications history before deleting all notifications older
+        # We continue this until we reach the original timestamp we wanted to delete notifications backwardsfrom
+        # This enables us to break this into smaller database queries
+        timestamp_to_delete_backwards_from = day_to_delete_backwards_from - timedelta(hours=hour_delta)
 
-    subquery_for_test_keys = db.session.query(
-        Notification.id
-    ).filter(
-        Notification.notification_type == notification_type,
-        Notification.service_id == service_id,
-        Notification.created_at < date_to_delete_from,
-        Notification.key_type == KEY_TYPE_TEST
-    ).limit(query_limit).subquery()
+        if service_id == '539d63a1-701d-400d-ab11-f3ee2319d4d4':
+            current_app.logger.info(
+                "Beginning insert_update_notification_history for GOV.UK Email from {} backwards".format(
+                    timestamp_to_delete_backwards_from
+                )
+            )
 
-    deleted += _delete_for_query(subquery_for_test_keys)
+        insert_update_notification_history(notification_type, timestamp_to_delete_backwards_from, service_id)
+
+        if service_id == '539d63a1-701d-400d-ab11-f3ee2319d4d4':
+            current_app.logger.info(
+                "Beginning _delete_notifications for GOV.UK Email {} backwards".format(
+                    timestamp_to_delete_backwards_from
+                )
+            )
+
+        deleted += _delete_notifications(
+            notification_type, timestamp_to_delete_backwards_from, service_id
+        )
 
     return deleted
 
 
-def _delete_for_query(subquery):
-    number_deleted = db.session.query(Notification).filter(
-        Notification.id.in_(subquery)).delete(synchronize_session='fetch')
-    deleted = number_deleted
+def _delete_notifications(notification_type, date_to_delete_from, service_id):
+    deleted = Notification.query.filter(
+        Notification.notification_type == notification_type,
+        Notification.service_id == service_id,
+        Notification.created_at < date_to_delete_from,
+    ).delete(synchronize_session=False)
     db.session.commit()
-    while number_deleted > 0:
-        number_deleted = db.session.query(Notification).filter(
-            Notification.id.in_(subquery)).delete(synchronize_session='fetch')
-        deleted += number_deleted
-        db.session.commit()
+
     return deleted
 
 
-def insert_update_notification_history(notification_type, date_to_delete_from, service_id, query_limit=10000):
-    offset = 0
-
+def insert_update_notification_history(notification_type, date_to_delete_from, service_id):
     notification_query = db.session.query(
         *[x.name for x in NotificationHistory.__table__.c]
     ).filter(
@@ -390,29 +392,25 @@ def insert_update_notification_history(notification_type, date_to_delete_from, s
         Notification.created_at < date_to_delete_from,
         Notification.key_type != KEY_TYPE_TEST
     )
-    notifications_count = notification_query.count()
 
-    while offset < notifications_count:
-        stmt = insert(NotificationHistory).from_select(
-            NotificationHistory.__table__.c,
-            notification_query.limit(query_limit).offset(offset)
-        )
+    stmt = insert(NotificationHistory).from_select(
+        NotificationHistory.__table__.c,
+        notification_query
+    )
 
-        stmt = stmt.on_conflict_do_update(
-            constraint="notification_history_pkey",
-            set_={
-                "notification_status": stmt.excluded.status,
-                "reference": stmt.excluded.reference,
-                "billable_units": stmt.excluded.billable_units,
-                "updated_at": stmt.excluded.updated_at,
-                "sent_at": stmt.excluded.sent_at,
-                "sent_by": stmt.excluded.sent_by
-            }
-        )
-        db.session.connection().execute(stmt)
-        db.session.commit()
-
-        offset += query_limit
+    stmt = stmt.on_conflict_do_update(
+        constraint="notification_history_pkey",
+        set_={
+            "notification_status": stmt.excluded.status,
+            "reference": stmt.excluded.reference,
+            "billable_units": stmt.excluded.billable_units,
+            "updated_at": stmt.excluded.updated_at,
+            "sent_at": stmt.excluded.sent_at,
+            "sent_by": stmt.excluded.sent_by
+        }
+    )
+    db.session.connection().execute(stmt)
+    db.session.commit()
 
 
 def _delete_letters_from_s3(

app/notifications/notifications_sms_callback.py

@@ -3,8 +3,9 @@ from flask import current_app
 from flask import json
 from flask import request, jsonify
 
+from app.celery.process_sms_client_response_tasks import process_sms_client_response
+from app.config import QueueNames
 from app.errors import InvalidRequest, register_errors
-from app.notifications.process_client_response import validate_callback_data, process_sms_client_response
 
 sms_callback_blueprint = Blueprint("sms_callback", __name__, url_prefix="/notifications/sms")
 register_errors(sms_callback_blueprint)
@@ -20,19 +21,21 @@ def process_mmg_response():
     if errors:
         raise InvalidRequest(errors, status_code=400)
 
-    success, errors = process_sms_client_response(status=str(data.get('status')),
-                                                  provider_reference=data.get('CID'),
-                                                  client_name=client_name)
+    status = str(data.get('status'))
+    provider_reference = data.get('CID')
+
+    process_sms_client_response.apply_async(
+        [status, provider_reference, client_name],
+        queue=QueueNames.SMS_CALLBACKS,
+    )
 
     safe_to_log = data.copy()
     safe_to_log.pop("MSISDN")
     current_app.logger.debug(
-        "Full delivery response from {} for notification: {}\n{}".format(client_name, request.form.get('CID'),
-                                                                         safe_to_log))
-    if errors:
-        raise InvalidRequest(errors, status_code=400)
-    else:
-        return jsonify(result='success', message=success), 200
+        f"Full delivery response from {client_name} for notification: {provider_reference}\n{safe_to_log}"
+    )
+
+    return jsonify(result='success'), 200
 
 
 @sms_callback_blueprint.route('/firetext', methods=['POST'])
@@ -43,15 +46,28 @@ def process_firetext_response():
                                     client_name=client_name)
     if errors:
         raise InvalidRequest(errors, status_code=400)
+
+    status = request.form.get('status')
+    provider_reference = request.form.get('reference')
+
     safe_to_log = dict(request.form).copy()
     safe_to_log.pop('mobile')
     current_app.logger.debug(
-        "Full delivery response from {} for notification: {}\n{}".format(client_name, request.form.get('reference'),
-                                                                         safe_to_log))
-    success, errors = process_sms_client_response(status=request.form.get('status'),
-                                                  provider_reference=request.form.get('reference'),
-                                                  client_name=client_name)
-    if errors:
-        raise InvalidRequest(errors, status_code=400)
-    else:
-        return jsonify(result='success', message=success), 200
+        f"Full delivery response from {client_name} for notification: {provider_reference}\n{safe_to_log}"
+    )
+
+    process_sms_client_response.apply_async(
+        [status, provider_reference, client_name],
+        queue=QueueNames.SMS_CALLBACKS,
+    )
+
+    return jsonify(result='success'), 200
+
+
+def validate_callback_data(data, fields, client_name):
+    errors = []
+    for f in fields:
+        if not str(data.get(f, '')):
+            error = "{} callback failed: {} missing".format(client_name, f)
+            errors.append(error)
+    return errors if len(errors) > 0 else None

manifest.yml.j2

@@ -2,7 +2,7 @@
   'notify-api': {
     'NOTIFY_APP_NAME': 'api',
     'disk_quota': '2G',
-    'sqlalchemy_pool_size': 20,
+    'sqlalchemy_pool_size': 30,
     'routes': {
       'preview': ['api.notify.works'],
       'staging': ['api.staging-notify.works'],

requirements-app.txt

@@ -27,4 +27,4 @@ notifications-python-client==5.5.1
 awscli-cwlogs>=1.4,<1.5
 
 
-git+https://github.com/alphagov/notifications-utils.git@36.6.2#egg=notifications-utils==36.6.2
+git+https://github.com/alphagov/notifications-utils.git@36.9.0#egg=notifications-utils==36.9.0

requirements.txt

@@ -29,23 +29,23 @@ notifications-python-client==5.5.1
 awscli-cwlogs>=1.4,<1.5
 
 
-git+https://github.com/alphagov/notifications-utils.git@36.6.2#egg=notifications-utils==36.6.2
+git+https://github.com/alphagov/notifications-utils.git@36.9.0#egg=notifications-utils==36.9.0
 
 ## The following requirements were added by pip freeze:
 alembic==1.4.1
 amqp==1.4.9
 anyjson==0.3.3
 attrs==19.3.0
-awscli==1.18.16
+awscli==1.18.20
 bcrypt==3.1.7
 billiard==3.3.0.23
 bleach==3.1.1
 boto==2.49.0
 boto3==1.10.38
-botocore==1.15.16
+botocore==1.15.20
 certifi==2019.11.28
 chardet==3.0.4
-click==7.1
+click==7.1.1
 colorama==0.4.3
 dnspython==1.16.0
 docutils==0.15.2

scripts/paas_app_wrapper.sh

@@ -43,7 +43,7 @@ case $NOTIFY_APP_NAME in
     ;;
   delivery-worker-receipts)
     exec scripts/run_app_paas.sh celery -A run_celery.notify_celery worker --loglevel=INFO --concurrency=11 \
-    -Q ses-callbacks 2> /dev/null
+    -Q ses-callbacks,sms-callbacks 2> /dev/null
     ;;
   delivery-worker-service-callbacks)
     exec scripts/run_app_paas.sh celery -A run_celery.notify_celery worker --loglevel=INFO --concurrency=11 \

tests/app/celery/test_tasks.py

@@ -961,7 +961,7 @@ def test_save_letter_saves_letter_to_database(mocker, notify_db_session):
         'addressline4': 'Wibble',
         'addressline5': 'Wobble',
         'addressline6': 'Wubble',
-        'postcode': 'Flob',
+        'postcode': 'SE1 2SA',
     }
     notification_json = _notification_json(
         template=job.template,
@@ -1021,6 +1021,31 @@ def test_save_letter_saves_letter_to_database_with_correct_postage(mocker, notif
     assert notification_db.postage == postage
 
 
+def test_save_letter_saves_letter_to_database_with_formatted_postcode(mocker, notify_db_session):
+    service = create_service(service_permissions=[LETTER_TYPE])
+    template = create_template(service=service, template_type=LETTER_TYPE)
+    letter_job = create_job(template=template)
+
+    mocker.patch('app.celery.tasks.letters_pdf_tasks.create_letters_pdf.apply_async')
+    notification_json = _notification_json(
+        template=letter_job.template,
+        to='Foo',
+        personalisation={'addressline1': 'Foo', 'addressline2': 'Bar', 'postcode': 'se1 64sa'},
+        job_id=letter_job.id,
+        row_number=1
+    )
+    notification_id = uuid.uuid4()
+    save_letter(
+        letter_job.service_id,
+        notification_id,
+        encryption.encrypt(notification_json),
+    )
+
+    notification_db = Notification.query.one()
+    assert notification_db.id == notification_id
+    assert notification_db.personalisation["postcode"] == "SE16 4SA"
+
+
 def test_save_letter_saves_letter_to_database_right_reply_to(mocker, notify_db_session):
     service = create_service()
     create_letter_contact(service=service, contact_block="Address contact", is_default=True)
@@ -1037,7 +1062,7 @@ def test_save_letter_saves_letter_to_database_right_reply_to(mocker, notify_db_s
         'addressline4': 'Wibble',
         'addressline5': 'Wobble',
         'addressline6': 'Wubble',
-        'postcode': 'Flob',
+        'postcode': 'SE1 3WS',
     }
     notification_json = _notification_json(
         template=job.template,

tests/app/dao/notification_dao/test_notification_dao_delete_notifications.py

@@ -9,13 +9,13 @@ from freezegun import freeze_time
 
 from app.dao.notifications_dao import (
     delete_notifications_older_than_retention_by_type,
-    db,
     insert_update_notification_history
 )
 from app.models import Notification, NotificationHistory
 from tests.app.db import (
     create_template,
     create_notification,
+    create_notification_history,
     create_service_data_retention,
     create_service
 )
@@ -75,7 +75,7 @@ def test_should_delete_notifications_by_type_after_seven_days(
 ):
     mocker.patch("app.dao.notifications_dao.get_s3_bucket_objects")
     email_template, letter_template, sms_template = _create_templates(sample_service)
-    # create one notification a day between 1st and 10th from 11:00 to 19:00 of each type
+    # create one notification a day between 1st and 10th from 01:00 to 11:00 of each type
     for i in range(1, 11):
         past_date = '2016-0{0}-{1:02d}  {1:02d}:00:00.000000'.format(month, i)
         with freeze_time(past_date):
@@ -157,17 +157,24 @@ def test_delete_notifications_inserts_notification_history(sample_service):
     assert NotificationHistory.query.count() == 2
 
 
-def test_delete_notifications_updates_notification_history(sample_email_template, mocker):
+def test_delete_notifications_updates_notification_history(notify_db, sample_email_template, mocker):
     mocker.patch("app.dao.notifications_dao.get_s3_bucket_objects")
-    notification = create_notification(template=sample_email_template, created_at=datetime.utcnow() - timedelta(days=8))
-    Notification.query.filter_by(id=notification.id).update(
-        {"status": "delivered",
-         "reference": "ses_reference",
-         "billable_units": 1,  # I know we don't update this for emails but this is a unit test
-         "updated_at": datetime.utcnow(),
-         "sent_at": datetime.utcnow(),
-         "sent_by": "ses"
-         }
+    now = datetime.utcnow()
+    notification = create_notification(
+        template=sample_email_template,
+        created_at=datetime.utcnow() - timedelta(days=8),
+        reference="ses_reference",
+        billable_units=1,
+        updated_at=now,
+        sent_by="ses",
+        status='delivered'
+    )
+
+    create_notification_history(
+        id=notification.id,
+        template=sample_email_template,
+        created_at=datetime.utcnow() - timedelta(days=8),
+        status='sending',
     )
 
     delete_notifications_older_than_retention_by_type("email")
@@ -177,7 +184,7 @@ def test_delete_notifications_updates_notification_history(sample_email_template
     assert history[0].status == 'delivered'
     assert history[0].reference == 'ses_reference'
     assert history[0].billable_units == 1
-    assert history[0].updated_at
+    assert history[0].updated_at == now
     assert history[0].sent_by == 'ses'
 
 
@@ -227,21 +234,6 @@ def test_delete_notifications_does_try_to_delete_from_s3_when_letter_has_not_bee
     mock_get_s3.assert_not_called()
 
 
-@freeze_time("2016-01-10 12:00:00.000000")
-def test_should_not_delete_notification_if_history_does_not_exist(sample_service, mocker):
-    mocker.patch("app.dao.notifications_dao.get_s3_bucket_objects")
-    mocker.patch("app.dao.notifications_dao.insert_update_notification_history")
-    with freeze_time('2016-01-01 12:00'):
-        email_template, letter_template, sms_template = _create_templates(sample_service)
-        create_notification(template=email_template, status='permanent-failure')
-        create_notification(template=sms_template, status='delivered')
-        create_notification(template=letter_template, status='temporary-failure')
-    assert Notification.query.count() == 3
-    delete_notifications_older_than_retention_by_type('sms')
-    assert Notification.query.count() == 3
-    assert NotificationHistory.query.count() == 0
-
-
 def test_delete_notifications_calls_subquery_multiple_times(sample_template):
     create_notification(template=sample_template, created_at=datetime.now() - timedelta(days=8))
     create_notification(template=sample_template, created_at=datetime.now() - timedelta(days=8))
@@ -288,35 +280,6 @@ def test_insert_update_notification_history(sample_service):
     assert notification_3.id in history_ids
 
 
-def test_insert_update_notification_history_with_more_notifications_than_query_limit(mocker, sample_service):
-    template = create_template(sample_service, template_type='sms')
-    notification_1 = create_notification(template=template, created_at=datetime.utcnow() - timedelta(days=3))
-    notification_2 = create_notification(template=template, created_at=datetime.utcnow() - timedelta(days=8))
-    notification_3 = create_notification(template=template, created_at=datetime.utcnow() - timedelta(days=9))
-    other_types = ['email', 'letter']
-    for template_type in other_types:
-        t = create_template(service=sample_service, template_type=template_type)
-        create_notification(template=t, created_at=datetime.utcnow() - timedelta(days=3))
-        create_notification(template=t, created_at=datetime.utcnow() - timedelta(days=8))
-
-    db_connection_spy = mocker.spy(db.session, 'connection')
-    db_commit_spy = mocker.spy(db.session, 'commit')
-
-    insert_update_notification_history(
-        notification_type='sms', date_to_delete_from=datetime.utcnow() - timedelta(days=7),
-        service_id=sample_service.id, query_limit=1)
-    history = NotificationHistory.query.all()
-    assert len(history) == 2
-
-    history_ids = [x.id for x in history]
-    assert notification_1.id not in history_ids
-    assert notification_2.id in history_ids
-    assert notification_3.id in history_ids
-
-    assert db_connection_spy.call_count == 2
-    assert db_commit_spy.call_count == 2
-
-
 def test_insert_update_notification_history_only_insert_update_given_service(sample_service):
     other_service = create_service(service_name='another service')
     other_template = create_template(service=other_service)

tests/app/db.py

@@ -324,7 +324,8 @@ def create_notification_history(
         international=False,
         phone_prefix=None,
         created_by_id=None,
-        postage=None
+        postage=None,
+        id=None
 ):
     assert job or template
     if job:
@@ -341,7 +342,7 @@ def create_notification_history(
         postage = 'second'
 
     data = {
-        'id': uuid.uuid4(),
+        'id': id or uuid.uuid4(),
         'job_id': job and job.id,
         'job': job,
         'service_id': template.service.id,

tests/app/notifications/rest/test_callbacks.py

@@ -1,17 +1,7 @@
-import uuid
-
-from datetime import datetime
-
 import pytest
 from flask import json
-from freezegun import freeze_time
 
-import app.celery.tasks
-from app.clients import ClientException
-from app.dao.notifications_dao import (
-    get_notification_by_id
-)
-from tests.app.db import create_notification, create_service_callback_api
+from app.notifications.notifications_sms_callback import validate_callback_data
 
 
 def firetext_post(client, data):
@@ -111,15 +101,14 @@ def test_dvla_ack_calls_does_not_call_letter_notifications_task(client, mocker):
 
 
 def test_firetext_callback_should_not_need_auth(client, mocker):
-    mocker.patch('app.statsd_client.incr')
-    data = 'mobile=441234123123&status=0&reference=send-sms-code&time=2016-03-10 14:17:00'
+    mocker.patch('app.notifications.notifications_sms_callback.process_sms_client_response')
+    data = 'mobile=441234123123&status=0&reference=notification_id&time=2016-03-10 14:17:00'
 
     response = firetext_post(client, data)
     assert response.status_code == 200
 
 
 def test_firetext_callback_should_return_400_if_empty_reference(client, mocker):
-    mocker.patch('app.statsd_client.incr')
     data = 'mobile=441234123123&status=0&reference=&time=2016-03-10 14:17:00'
     response = firetext_post(client, data)
 
@@ -130,7 +119,6 @@ def test_firetext_callback_should_return_400_if_empty_reference(client, mocker):
 
 
 def test_firetext_callback_should_return_400_if_no_reference(client, mocker):
-    mocker.patch('app.statsd_client.incr')
     data = 'mobile=441234123123&status=0&time=2016-03-10 14:17:00'
     response = firetext_post(client, data)
     json_resp = json.loads(response.get_data(as_text=True))
@@ -139,19 +127,8 @@ def test_firetext_callback_should_return_400_if_no_reference(client, mocker):
     assert json_resp['message'] == ['Firetext callback failed: reference missing']
 
 
-def test_firetext_callback_should_return_200_if_send_sms_reference(client, mocker):
-    mocker.patch('app.statsd_client.incr')
-    data = 'mobile=441234123123&status=0&time=2016-03-10 14:17:00&reference=send-sms-code'
-    response = firetext_post(client, data)
-    json_resp = json.loads(response.get_data(as_text=True))
-    assert response.status_code == 200
-    assert json_resp['result'] == 'success'
-    assert json_resp['message'] == 'Firetext callback succeeded: send-sms-code'
-
-
 def test_firetext_callback_should_return_400_if_no_status(client, mocker):
-    mocker.patch('app.statsd_client.incr')
-    data = 'mobile=441234123123&time=2016-03-10 14:17:00&reference=send-sms-code'
+    data = 'mobile=441234123123&time=2016-03-10 14:17:00&reference=notification_id'
     response = firetext_post(client, data)
     json_resp = json.loads(response.get_data(as_text=True))
     assert response.status_code == 400
@@ -159,136 +136,24 @@ def test_firetext_callback_should_return_400_if_no_status(client, mocker):
     assert json_resp['message'] == ['Firetext callback failed: status missing']
 
 
-def test_firetext_callback_should_set_status_technical_failure_if_status_unknown(
-        client, mocker, sample_notification):
-    sample_notification.status = 'sending'
-    # mocker.patch('app.statsd_client.incr')
-    data = 'mobile=441234123123&status=99&time=2016-03-10 14:17:00&reference={}'.format(sample_notification.id)
-    with pytest.raises(ClientException) as e:
-        firetext_post(client, data)
-    assert get_notification_by_id(sample_notification.id).status == 'technical-failure'
-    assert 'Firetext callback failed: status 99 not found.' in str(e.value)
+def test_firetext_callback_should_return_200_and_call_task_with_valid_data(client, mocker):
+    mock_celery = mocker.patch(
+        'app.notifications.notifications_sms_callback.process_sms_client_response.apply_async')
 
-
-def test_firetext_callback_returns_200_when_notification_id_is_not_a_valid_uuid(client, mocker):
-    mocker.patch('app.statsd_client.incr')
-    data = 'mobile=441234123123&status=0&time=2016-03-10 14:17:00&reference=1234'
+    data = 'mobile=441234123123&status=0&time=2016-03-10 14:17:00&reference=notification_id'
     response = firetext_post(client, data)
-    json_resp = json.loads(response.get_data(as_text=True))
-    assert response.status_code == 400
-    assert json_resp['result'] == 'error'
-    assert json_resp['message'] == 'Firetext callback with invalid reference 1234'
-
-
-def test_callback_should_return_200_if_cannot_find_notification_id(
-    notify_db,
-    notify_db_session,
-    client,
-    mocker
-):
-    mocker.patch('app.statsd_client.incr')
-    missing_notification_id = uuid.uuid4()
-    data = 'mobile=441234123123&status=0&time=2016-03-10 14:17:00&reference={}'.format(
-        missing_notification_id)
-    response = firetext_post(client, data)
-
     json_resp = json.loads(response.get_data(as_text=True))
     assert response.status_code == 200
     assert json_resp['result'] == 'success'
 
-
-def test_firetext_callback_should_update_notification_status(
-        client, mocker, sample_notification
-):
-    mocker.patch('app.statsd_client.incr')
-    send_mock = mocker.patch(
-        'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async'
+    mock_celery.assert_called_once_with(
+        ['0', 'notification_id', 'Firetext'],
+        queue='sms-callbacks',
     )
-    sample_notification.status = 'sending'
-
-    original = get_notification_by_id(sample_notification.id)
-    assert original.status == 'sending'
-    data = 'mobile=441234123123&status=0&time=2016-03-10 14:17:00&reference={}'.format(
-        sample_notification.id)
-    response = firetext_post(client, data)
-
-    json_resp = json.loads(response.get_data(as_text=True))
-    assert response.status_code == 200
-    assert json_resp['result'] == 'success'
-    assert json_resp['message'] == 'Firetext callback succeeded. reference {} updated'.format(
-        sample_notification.id
-    )
-    updated = get_notification_by_id(sample_notification.id)
-    assert updated.status == 'delivered'
-    assert get_notification_by_id(sample_notification.id).status == 'delivered'
-    assert send_mock.called_once_with([sample_notification.id], queue="notify-internal-tasks")
 
 
-def test_firetext_callback_should_update_notification_status_failed(
-        client, mocker, sample_template
-):
-    mocker.patch('app.statsd_client.incr')
-    mocker.patch(
-        'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async'
-    )
-    notification = create_notification(template=sample_template, status='sending')
-
-    original = get_notification_by_id(notification.id)
-    assert original.status == 'sending'
-
-    data = 'mobile=441234123123&status=1&time=2016-03-10 14:17:00&reference={}'.format(
-        notification.id)
-    response = firetext_post(client, data)
-
-    json_resp = json.loads(response.get_data(as_text=True))
-    assert response.status_code == 200
-    assert json_resp['result'] == 'success'
-    assert json_resp['message'] == 'Firetext callback succeeded. reference {} updated'.format(
-        notification.id
-    )
-    assert get_notification_by_id(notification.id).status == 'permanent-failure'
-
-
-def test_firetext_callback_should_update_notification_status_pending(client, sample_template, mocker):
-    mocker.patch('app.statsd_client.incr')
-    mocker.patch(
-        'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async'
-    )
-    notification = create_notification(template=sample_template, status='sending')
-
-    original = get_notification_by_id(notification.id)
-    assert original.status == 'sending'
-    data = 'mobile=441234123123&status=2&time=2016-03-10 14:17:00&reference={}'.format(
-        notification.id)
-    response = firetext_post(client, data)
-
-    json_resp = json.loads(response.get_data(as_text=True))
-    assert response.status_code == 200
-    assert json_resp['result'] == 'success'
-    assert json_resp['message'] == 'Firetext callback succeeded. reference {} updated'.format(
-        notification.id
-    )
-    assert get_notification_by_id(notification.id).status == 'pending'
-
-
-def test_process_mmg_response_return_200_when_cid_is_send_sms_code(client):
-    data = '{"reference": "10100164", "CID": "send-sms-code", "MSISDN": "447775349060", "status": "3", \
-        "deliverytime": "2016-04-05 16:01:07"}'
-
-    response = mmg_post(client, data)
-    assert response.status_code == 200
-    json_data = json.loads(response.data)
-    assert json_data['result'] == 'success'
-    assert json_data['message'] == 'MMG callback succeeded: send-sms-code'
-
-
-def test_process_mmg_response_returns_200_when_cid_is_valid_notification_id(
-        sample_notification, client, mocker
-):
-    mocker.patch(
-        'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async'
-    )
-    sample_notification.status = 'sending'
+def test_mmg_callback_should_not_need_auth(client, mocker, sample_notification):
+    mocker.patch('app.notifications.notifications_sms_callback.process_sms_client_response')
     data = json.dumps({"reference": "mmg_reference",
                        "CID": str(sample_notification.id),
                        "MSISDN": "447777349060",
@@ -296,93 +161,7 @@ def test_process_mmg_response_returns_200_when_cid_is_valid_notification_id(
                        "deliverytime": "2016-04-05 16:01:07"})
 
     response = mmg_post(client, data)
-
     assert response.status_code == 200
-    json_data = json.loads(response.data)
-    assert json_data['result'] == 'success'
-    assert json_data['message'] == 'MMG callback succeeded. reference {} updated'.format(sample_notification.id)
-    assert get_notification_by_id(sample_notification.id).status == 'delivered'
-
-
-def test_process_mmg_response_status_5_updates_notification_with_permanently_failed(
-    sample_notification, client, mocker
-):
-    mocker.patch(
-        'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async'
-    )
-    sample_notification.status = 'sending'
-
-    data = json.dumps({"reference": "mmg_reference",
-                       "CID": str(sample_notification.id),
-                       "MSISDN": "447777349060",
-                       "status": 5})
-
-    response = mmg_post(client, data)
-    assert response.status_code == 200
-    json_data = json.loads(response.data)
-    assert json_data['result'] == 'success'
-    assert json_data['message'] == 'MMG callback succeeded. reference {} updated'.format(sample_notification.id)
-    assert get_notification_by_id(sample_notification.id).status == 'permanent-failure'
-
-
-def test_process_mmg_response_status_2_updates_notification_with_permanently_failed(
-    sample_notification, client, mocker
-):
-    mocker.patch(
-        'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async'
-    )
-    sample_notification.status = 'sending'
-    data = json.dumps({"reference": "mmg_reference",
-                       "CID": str(sample_notification.id),
-                       "MSISDN": "447777349060",
-                       "status": 2})
-
-    response = mmg_post(client, data)
-    assert response.status_code == 200
-    json_data = json.loads(response.data)
-    assert json_data['result'] == 'success'
-    assert json_data['message'] == 'MMG callback succeeded. reference {} updated'.format(sample_notification.id)
-    assert get_notification_by_id(sample_notification.id).status == 'permanent-failure'
-
-
-def test_process_mmg_response_status_4_updates_notification_with_temporary_failed(
-        sample_notification, client, mocker
-):
-    mocker.patch(
-        'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async'
-    )
-    sample_notification.status = 'sending'
-
-    data = json.dumps({"reference": "mmg_reference",
-                       "CID": str(sample_notification.id),
-                       "MSISDN": "447777349060",
-                       "status": 4})
-
-    response = mmg_post(client, data)
-    assert response.status_code == 200
-    json_data = json.loads(response.data)
-    assert json_data['result'] == 'success'
-    assert json_data['message'] == 'MMG callback succeeded. reference {} updated'.format(sample_notification.id)
-    assert get_notification_by_id(sample_notification.id).status == 'temporary-failure'
-
-
-def test_process_mmg_response_unknown_status_updates_notification_with_technical_failure(
-        sample_notification, client, mocker
-):
-    send_mock = mocker.patch(
-        'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async'
-    )
-    sample_notification.status = 'sending'
-    data = json.dumps({"reference": "mmg_reference",
-                       "CID": str(sample_notification.id),
-                       "MSISDN": "447777349060",
-                       "status": 10})
-    create_service_callback_api(service=sample_notification.service, url="https://original_url.com")
-    with pytest.raises(ClientException) as e:
-        mmg_post(client, data)
-    assert 'MMG callback failed: status 10 not found.' in str(e.value)
-    assert get_notification_by_id(sample_notification.id).status == 'technical-failure'
-    assert send_mock.called
 
 
 def test_process_mmg_response_returns_400_for_malformed_data(client):
@@ -401,68 +180,64 @@ def test_process_mmg_response_returns_400_for_malformed_data(client):
     assert "{} callback failed: {} missing".format('MMG', 'CID') in json_data['message']
 
 
-def test_mmg_callback_returns_200_when_notification_id_not_found_or_already_updated(client):
-    data = '{"reference": "10100164", "CID": "send-sms-code", "MSISDN": "447775349060", "status": "3", \
-             "deliverytime": "2016-04-05 16:01:07"}'
+def test_mmg_callback_should_return_200_and_call_task_with_valid_data(client, mocker):
+    mock_celery = mocker.patch(
+        'app.notifications.notifications_sms_callback.process_sms_client_response.apply_async')
+    data = json.dumps({"reference": "mmg_reference",
+                       "CID": "notification_id",
+                       "MSISDN": "447777349060",
+                       "status": "3",
+                       "deliverytime": "2016-04-05 16:01:07"})
 
     response = mmg_post(client, data)
+
     assert response.status_code == 200
+    json_data = json.loads(response.data)
+    assert json_data['result'] == 'success'
+
+    mock_celery.assert_called_once_with(
+        ['3', 'notification_id', 'MMG'],
+        queue='sms-callbacks',
+    )
 
 
-def test_mmg_callback_returns_400_when_notification_id_is_not_a_valid_uuid(client):
-    data = '{"reference": "10100164", "CID": "1234", "MSISDN": "447775349060", "status": "3", \
-             "deliverytime": "2016-04-05 16:01:07"}'
+def test_validate_callback_data_returns_none_when_valid():
+    form = {'status': 'good',
+            'reference': 'send-sms-code'}
+    fields = ['status', 'reference']
+    client_name = 'sms client'
 
-    response = mmg_post(client, data)
-    json_resp = json.loads(response.get_data(as_text=True))
-    assert response.status_code == 400
-    assert json_resp['message'] == 'MMG callback with invalid reference 1234'
+    assert validate_callback_data(form, fields, client_name) is None
 
 
-def test_process_mmg_response_records_statsd(sample_notification, client, mocker):
-    with freeze_time('2001-01-01T12:00:00'):
+def test_validate_callback_data_return_errors_when_fields_are_empty():
+    form = {'monkey': 'good'}
+    fields = ['status', 'cid']
+    client_name = 'sms client'
 
-        mocker.patch('app.statsd_client.incr')
-        mocker.patch('app.statsd_client.timing_with_dates')
-        mocker.patch(
-            'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async'
-        )
-        sample_notification.status = 'sending'
-        sample_notification.sent_at = datetime.now()
-
-        data = json.dumps({"reference": "mmg_reference",
-                           "CID": str(sample_notification.id),
-                           "MSISDN": "447777349060",
-                           "status": "3",
-                           "deliverytime": "2016-04-05 16:01:07"})
-
-        mmg_post(client, data)
-
-        app.statsd_client.incr.assert_any_call("callback.mmg.delivered")
-        app.statsd_client.timing_with_dates.assert_any_call(
-            "callback.mmg.elapsed-time", datetime.utcnow(), sample_notification.sent_at
-        )
+    errors = validate_callback_data(form, fields, client_name)
+    assert len(errors) == 2
+    assert "{} callback failed: {} missing".format(client_name, 'status') in errors
+    assert "{} callback failed: {} missing".format(client_name, 'cid') in errors
 
 
-def test_firetext_callback_should_record_statsd(client, sample_notification, mocker):
-    with freeze_time('2001-01-01T12:00:00'):
+def test_validate_callback_data_can_handle_integers():
+    form = {'status': 00, 'cid': 'fsdfadfsdfas'}
+    fields = ['status', 'cid']
+    client_name = 'sms client'
 
-        mocker.patch('app.statsd_client.incr')
-        mocker.patch('app.statsd_client.timing_with_dates')
-        mocker.patch(
-            'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async'
-        )
-        sample_notification.status = 'sending'
-        sample_notification.sent_at = datetime.now()
+    result = validate_callback_data(form, fields, client_name)
+    assert result is None
 
-        data = 'mobile=441234123123&status=0&time=2016-03-10 14:17:00&code=101&reference={}'.format(
-            sample_notification.id)
-        firetext_post(client, data)
 
-        app.statsd_client.timing_with_dates.assert_any_call(
-            "callback.firetext.elapsed-time", datetime.utcnow(), sample_notification.sent_at
-        )
-        app.statsd_client.incr.assert_any_call("callback.firetext.delivered")
+def test_validate_callback_data_returns_error_for_empty_string():
+    form = {'status': '', 'cid': 'fsdfadfsdfas'}
+    fields = ['status', 'cid']
+    client_name = 'sms client'
+
+    result = validate_callback_data(form, fields, client_name)
+    assert result is not None
+    assert "{} callback failed: {} missing".format(client_name, 'status') in result
 
 
 def _sample_sns_s3_callback(filename):

tests/app/notifications/test_process_client_response.py

@@ -1,79 +1,66 @@
 import uuid
+from datetime import datetime
 
 import pytest
+from freezegun import freeze_time
 
+from app import statsd_client
 from app.clients import ClientException
-from app.notifications.process_client_response import (
-    validate_callback_data,
-    process_sms_client_response
-)
+from app.celery.process_sms_client_response_tasks import process_sms_client_response
 from app.celery.service_callback_tasks import create_delivery_status_callback_data
+from app.models import NOTIFICATION_TECHNICAL_FAILURE
 from tests.app.db import create_service_callback_api
 
 
-def test_validate_callback_data_returns_none_when_valid():
-    form = {'status': 'good',
-            'reference': 'send-sms-code'}
-    fields = ['status', 'reference']
-    client_name = 'sms client'
-
-    assert validate_callback_data(form, fields, client_name) is None
+def test_process_sms_client_response_raises_error_if_reference_is_not_a_valid_uuid(client):
+    with pytest.raises(ValueError):
+        process_sms_client_response(
+            status='000', provider_reference='something-bad', client_name='sms-client')
 
 
-def test_validate_callback_data_return_errors_when_fields_are_empty():
-    form = {'monkey': 'good'}
-    fields = ['status', 'cid']
-    client_name = 'sms client'
+@pytest.mark.parametrize('client_name', ('Firetext', 'MMG'))
+def test_process_sms_response_raises_client_exception_for_unknown_status(
+    sample_notification,
+    mocker,
+    client_name,
+):
+    with pytest.raises(ClientException) as e:
+        process_sms_client_response(
+            status='000',
+            provider_reference=str(sample_notification.id),
+            client_name=client_name,
+        )
 
-    errors = validate_callback_data(form, fields, client_name)
-    assert len(errors) == 2
-    assert "{} callback failed: {} missing".format(client_name, 'status') in errors
-    assert "{} callback failed: {} missing".format(client_name, 'cid') in errors
+    assert f"{client_name} callback failed: status {'000'} not found." in str(e.value)
+    assert sample_notification.status == NOTIFICATION_TECHNICAL_FAILURE
 
 
-def test_validate_callback_data_can_handle_integers():
-    form = {'status': 00, 'cid': 'fsdfadfsdfas'}
-    fields = ['status', 'cid']
-    client_name = 'sms client'
+@pytest.mark.parametrize('status, sms_provider, expected_notification_status', [
+    ('0', 'Firetext', 'delivered'),
+    ('1', 'Firetext', 'permanent-failure'),
+    ('2', 'Firetext', 'pending'),
+    ('2', 'MMG', 'permanent-failure'),
+    ('3', 'MMG', 'delivered'),
+    ('4', 'MMG', 'temporary-failure'),
+    ('5', 'MMG', 'permanent-failure'),
+])
+def test_process_sms_client_response_updates_notification_status(
+    sample_notification,
+    mocker,
+    status,
+    sms_provider,
+    expected_notification_status,
+):
+    sample_notification.status = 'sending'
+    process_sms_client_response(status, str(sample_notification.id), sms_provider)
 
-    result = validate_callback_data(form, fields, client_name)
-    assert result is None
+    assert sample_notification.status == expected_notification_status
 
 
-def test_validate_callback_data_returns_error_for_empty_string():
-    form = {'status': '', 'cid': 'fsdfadfsdfas'}
-    fields = ['status', 'cid']
-    client_name = 'sms client'
-
-    result = validate_callback_data(form, fields, client_name)
-    assert result is not None
-    assert "{} callback failed: {} missing".format(client_name, 'status') in result
-
-
-def test_outcome_statistics_called_for_successful_callback(sample_notification, mocker):
+def test_sms_response_does_not_send_callback_if_notification_is_not_in_the_db(sample_service, mocker):
     mocker.patch(
-        'app.notifications.process_client_response.notifications_dao.update_notification_status_by_id',
-        return_value=sample_notification
-    )
-    send_mock = mocker.patch(
-        'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async'
-    )
-    callback_api = create_service_callback_api(service=sample_notification.service, url="https://original_url.com")
-    reference = str(uuid.uuid4())
-
-    success, error = process_sms_client_response(status='3', provider_reference=reference, client_name='MMG')
-    assert success == "MMG callback succeeded. reference {} updated".format(str(reference))
-    assert error is None
-    encrypted_data = create_delivery_status_callback_data(sample_notification, callback_api)
-    send_mock.assert_called_once_with([str(sample_notification.id), encrypted_data],
-                                      queue="service-callbacks")
-
-
-def test_sms_resonse_does_not_call_send_callback_if_no_db_entry(sample_notification, mocker):
-    mocker.patch(
-        'app.notifications.process_client_response.notifications_dao.update_notification_status_by_id',
-        return_value=sample_notification
-    )
+        'app.celery.process_sms_client_response_tasks.get_service_delivery_status_callback_api_for_service',
+        return_value='mock-delivery-callback-for-service')
     send_mock = mocker.patch(
         'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async'
     )
@@ -82,63 +69,54 @@ def test_sms_resonse_does_not_call_send_callback_if_no_db_entry(sample_notificat
     send_mock.assert_not_called()
 
 
-def test_process_sms_response_return_success_for_send_sms_code_reference(mocker):
-    success, error = process_sms_client_response(
-        status='000', provider_reference='send-sms-code', client_name='sms-client')
-    assert success == "{} callback succeeded: send-sms-code".format('sms-client')
-    assert error is None
+@freeze_time('2001-01-01T12:00:00')
+def test_process_sms_client_response_records_statsd_metrics(sample_notification, client, mocker):
+    mocker.patch('app.statsd_client.incr')
+    mocker.patch('app.statsd_client.timing_with_dates')
 
+    sample_notification.status = 'sending'
+    sample_notification.sent_at = datetime.utcnow()
 
-def test_process_sms_response_does_not_send_status_update_for_pending(sample_notification, mocker):
-    send_mock = mocker.patch('app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async')
-    process_sms_client_response(
-        status='2', provider_reference=str(sample_notification.id), client_name='firetext')
-    send_mock.assert_not_called()
+    process_sms_client_response('0', str(sample_notification.id), 'Firetext')
 
-
-def test_process_sms_updates_sent_by_with_client_name_if_not_in_noti(sample_notification):
-    sample_notification.sent_by = None
-    success, error = process_sms_client_response(
-        status='3', provider_reference=str(sample_notification.id), client_name='MMG')
-    assert error is None
-    assert success == 'MMG callback succeeded. reference {} updated'.format(sample_notification.id)
-    assert sample_notification.sent_by == 'mmg'
+    statsd_client.incr.assert_any_call("callback.firetext.delivered")
+    statsd_client.timing_with_dates.assert_any_call(
+        "callback.firetext.elapsed-time", datetime.utcnow(), sample_notification.sent_at
+    )
 
 
 def test_process_sms_updates_billable_units_if_zero(sample_notification):
     sample_notification.billable_units = 0
-    success, error = process_sms_client_response(
-        status='3', provider_reference=str(sample_notification.id), client_name='MMG')
-    assert error is None
-    assert success == 'MMG callback succeeded. reference {} updated'.format(sample_notification.id)
+    process_sms_client_response('3', str(sample_notification.id), 'MMG')
+
     assert sample_notification.billable_units == 1
 
 
-def test_process_sms_does_not_update_sent_by_if_already_set(mocker, sample_notification):
-    mock_update = mocker.patch('app.notifications.process_client_response.set_notification_sent_by')
-    sample_notification.sent_by = 'MMG'
-    process_sms_client_response(
-        status='3', provider_reference=str(sample_notification.id), client_name='MMG')
-    assert not mock_update.called
+def test_process_sms_response_does_not_send_service_callback_for_pending_notifications(sample_notification, mocker):
+    mocker.patch(
+        'app.celery.process_sms_client_response_tasks.get_service_delivery_status_callback_api_for_service',
+        return_value='fake-callback')
+    send_mock = mocker.patch('app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async')
+    process_sms_client_response('2', str(sample_notification.id), 'Firetext')
+    send_mock.assert_not_called()
 
 
-def test_process_sms_response_returns_error_bad_reference(mocker):
-    success, error = process_sms_client_response(
-        status='000', provider_reference='something-bad', client_name='sms-client')
-    assert success is None
-    assert error == "{} callback with invalid reference {}".format('sms-client', 'something-bad')
+def test_outcome_statistics_called_for_successful_callback(sample_notification, mocker):
+    send_mock = mocker.patch(
+        'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async'
+    )
+    callback_api = create_service_callback_api(service=sample_notification.service, url="https://original_url.com")
+    reference = str(sample_notification.id)
+
+    process_sms_client_response('3', reference, 'MMG')
+
+    encrypted_data = create_delivery_status_callback_data(sample_notification, callback_api)
+    send_mock.assert_called_once_with([reference, encrypted_data],
+                                      queue="service-callbacks")
 
 
-def test_process_sms_response_raises_client_exception_for_unknown_sms_client(mocker):
-    success, error = process_sms_client_response(
-        status='000', provider_reference=str(uuid.uuid4()), client_name='sms-client')
+def test_process_sms_updates_sent_by_with_client_name_if_not_in_noti(sample_notification):
+    sample_notification.sent_by = None
+    process_sms_client_response('3', str(sample_notification.id), 'MMG')
 
-    assert success is None
-    assert error == 'unknown sms client: {}'.format('sms-client')
-
-
-def test_process_sms_response_raises_client_exception_for_unknown_status(mocker):
-    with pytest.raises(ClientException) as e:
-        process_sms_client_response(status='000', provider_reference=str(uuid.uuid4()), client_name='Firetext')
-
-    assert "{} callback failed: status {} not found.".format('Firetext', '000') in str(e.value)
+    assert sample_notification.sent_by == 'mmg'

tests/app/test_config.py

@@ -65,7 +65,7 @@ def test_cloudfoundry_config_has_different_defaults():
 def test_queue_names_all_queues_correct():
     # Need to ensure that all_queues() only returns queue names used in API
     queues = QueueNames.all_queues()
-    assert len(queues) == 13
+    assert len(queues) == 14
     assert set([
         QueueNames.PRIORITY,
         QueueNames.PERIODIC,
@@ -80,4 +80,5 @@ def test_queue_names_all_queues_correct():
         QueueNames.CREATE_LETTERS_PDF,
         QueueNames.CALLBACKS,
         QueueNames.LETTERS,
+        QueueNames.SMS_CALLBACKS,
     ]) == set(queues)