Function to delete high volume notifications for a service

This command will allow us to explore the timing of deleting notifications, which is why it's written as a command. The intention is to try this strategy out then lift and shift the code into a task.

The next commit will do the insert into NotificationHistory and drop the back up table.

app/celery/letters_pdf_tasks.py

@@ -52,7 +52,7 @@ from app.models import (
     NOTIFICATION_TECHNICAL_FAILURE,
     NOTIFICATION_VALIDATION_FAILED,
     NOTIFICATION_VIRUS_SCAN_FAILED,
-    POSTAGE_TYPES)
+)
 from app.cronitor import cronitor
 
 
@@ -136,40 +136,39 @@ def collate_letter_pdfs_to_be_sent():
         hour=17, minute=30, second=0, microsecond=0
     )
 
-    for postage in POSTAGE_TYPES:
-        letters_to_print = get_key_and_size_of_letters_to_be_sent_to_print(print_run_deadline, postage)
+    letters_to_print = get_key_and_size_of_letters_to_be_sent_to_print(print_run_deadline)
 
-        for i, letters in enumerate(group_letters(letters_to_print)):
-            filenames = [letter['Key'] for letter in letters]
+    for i, letters in enumerate(group_letters(letters_to_print)):
+        filenames = [letter['Key'] for letter in letters]
 
-            hash = urlsafe_b64encode(sha512(''.join(filenames).encode()).digest())[:20].decode()
-            # eg NOTIFY.2018-12-31.001.Wjrui5nAvObjPd-3GEL-.ZIP
-            dvla_filename = 'NOTIFY.{date}.{num:03}.{hash}.ZIP'.format(
-                date=print_run_deadline.strftime("%Y-%m-%d"),
-                num=i + 1,
-                hash=hash
-            )
-
-            current_app.logger.info(
-                'Calling task zip-and-send-letter-pdfs for {} pdfs to upload {} with total size {:,} bytes'.format(
-                    len(filenames),
-                    dvla_filename,
-                    sum(letter['Size'] for letter in letters)
-                )
-            )
-            notify_celery.send_task(
-                name=TaskNames.ZIP_AND_SEND_LETTER_PDFS,
-                kwargs={
-                    'filenames_to_zip': filenames,
-                    'upload_filename': dvla_filename
-                },
-                queue=QueueNames.PROCESS_FTP,
-                compression='zlib'
+        hash = urlsafe_b64encode(sha512(''.join(filenames).encode()).digest())[:20].decode()
+        # eg NOTIFY.2018-12-31.001.Wjrui5nAvObjPd-3GEL-.ZIP
+        dvla_filename = 'NOTIFY.{date}.{num:03}.{hash}.ZIP'.format(
+            date=print_run_deadline.strftime("%Y-%m-%d"),
+            num=i + 1,
+            hash=hash
+        )
+
+        current_app.logger.info(
+            'Calling task zip-and-send-letter-pdfs for {} pdfs to upload {} with total size {:,} bytes'.format(
+                len(filenames),
+                dvla_filename,
+                sum(letter['Size'] for letter in letters)
             )
+        )
+        notify_celery.send_task(
+            name=TaskNames.ZIP_AND_SEND_LETTER_PDFS,
+            kwargs={
+                'filenames_to_zip': filenames,
+                'upload_filename': dvla_filename
+            },
+            queue=QueueNames.PROCESS_FTP,
+            compression='zlib'
+        )
 
 
-def get_key_and_size_of_letters_to_be_sent_to_print(print_run_deadline, postage):
-    letters_awaiting_sending = dao_get_letters_to_be_printed(print_run_deadline, postage)
+def get_key_and_size_of_letters_to_be_sent_to_print(print_run_deadline):
+    letters_awaiting_sending = dao_get_letters_to_be_printed(print_run_deadline)
 
     letter_pdfs = []
     for letter in letters_awaiting_sending:
@@ -184,7 +183,7 @@ def get_key_and_size_of_letters_to_be_sent_to_print(print_run_deadline, postage)
             letter_pdfs.append({"Key": letter_file_name, "Size": letter_head['ContentLength']})
         except BotoClientError as e:
             current_app.logger.exception(
-                f"Error getting letter from bucket for notification: {letter.id} with reference: {letter.reference}")
+                f"Error getting letter from bucket for notification: {letter.id} with reference: {letter.reference}", e)
 
     return letter_pdfs
 

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/commands.py

@@ -11,7 +11,7 @@ from click_datetime import Datetime as click_dt
 from flask import current_app, json
 from notifications_utils.recipients import RecipientCSV
 from notifications_utils.template import SMSMessageTemplate
-from sqlalchemy.exc import IntegrityError
+from sqlalchemy.exc import IntegrityError, SQLAlchemyError
 from sqlalchemy.orm.exc import NoResultFound
 from notifications_utils.statsd_decorators import statsd
 
@@ -45,7 +45,6 @@ from app.dao.templates_dao import dao_get_template_by_id
 from app.dao.users_dao import delete_model_user, delete_user_verify_codes, get_user_by_email
 from app.models import (
     PROVIDERS,
-    NOTIFICATION_CREATED,
     KEY_TYPE_TEST,
     SMS_TYPE,
     EMAIL_TYPE,
@@ -57,6 +56,10 @@ from app.models import (
     Service,
     EmailBranding,
     LetterBranding,
+    NOTIFICATION_CREATED,
+    NOTIFICATION_DELIVERED,
+    NOTIFICATION_PERMANENT_FAILURE,
+    NOTIFICATION_TEMPORARY_FAILURE,
 )
 from app.performance_platform.processing_time import send_processing_time_for_start_and_end
 from app.utils import get_london_midnight_in_utc, get_midnight_for_day_before
@@ -901,3 +904,105 @@ def process_row_from_job(job_id, job_row_number):
             notification_id = process_row(row, template, job, job.service)
             current_app.logger.info("Process row {} for job {} created notification_id: {}".format(
                 job_row_number, job_id, notification_id))
+
+
+@notify_command(name='delete-high-volume-service-data')
+@click.option('-i', '--service_id', required=True, help='Service id of the high volume service')
+@click.option('-s', '--start_date', required=True, type=click_dt(format='%Y-%m-%d %H'),
+              help='Start date of process YYYY-MM-DD HH:mm')
+@click.option('-e', '--end_date', required=True, type=click_dt(format='%Y-%m-%d %H'),
+              help='End date of process YYYY-MM-DD HH:MM')
+@click.option('-t', '--notification_type', required=False, default='email',
+              help='Notification type of the data to delete')
+def delete_high_volume_service_data(service_id, start_date, end_date, notification_type):
+    str_date = start_date.strftime('%Y_%m_%d_%H')
+    str_end_date = end_date.strftime('%Y_%m_%d_%H')
+
+    bkup_tble_name = f'back_up_notifications_{str_date}_to_{str_end_date}'
+
+    print(f"""Creating back up {bkup_tble_name} starting at: {datetime.utcnow()}
+          for {notification_type} notifications for service: {service_id}, starting at {start_date} and {end_date}
+         """)
+
+    _create_notification_bkup_table(bkup_tble_name, end_date, notification_type, service_id, start_date)
+
+    hour_start = start_date
+    hour_end = hour_start + timedelta(hours=1)
+
+    terminate_statuses = [NOTIFICATION_DELIVERED, NOTIFICATION_TEMPORARY_FAILURE, NOTIFICATION_PERMANENT_FAILURE]
+    delete_query = Notification.query.filter(
+        Notification.notification_type == notification_type,
+        Notification.service_id == service_id,
+        Notification.created_at >= hour_start,
+        Notification.created_at <= hour_end,
+        Notification.status.in_(terminate_statuses)
+    )
+    # Iterate hour by hour
+    del_count = 0
+    while hour_start < end_date:
+        del_count += delete_query.delete(synchronize_session=False)
+        db.session.commit()
+        # print(hour_start, hour_end)
+        # increment hour
+        hour_end = hour_end + timedelta(hours=1)
+        hour_start = hour_start + timedelta(hours=1)
+        delete_query = Notification.query.filter(
+            Notification.notification_type == notification_type,
+            Notification.service_id == service_id,
+            Notification.created_at >= hour_start,
+            Notification.created_at <= hour_end,
+            Notification.status.in_(terminate_statuses)
+        )
+
+    print(f"""Completed deleting {del_count} from notifications
+              for {notification_type} notifications for
+              service: {service_id}, starting at {start_date} and {end_date}
+             """)
+
+
+def _create_notification_bkup_table(bkup_tble_name, end_date, notification_type, service_id, start_date):
+    try:
+        create_tbl_sql = f"""
+            CREATE TABLE {bkup_tble_name} AS 
+                SELECT *
+                  FROM notifications
+                 WHERE service_id = :service_id
+                   AND notification_type = :notification_type
+                   AND created_at >= :start_date
+                   AND created_at <= :end_date
+                   AND key_type = 'normal'
+                   AND notification_status in ('delivered', 'permanent-failure', 'temporary-failure')
+        """
+        input_params = {
+            "service_id": service_id,
+            "notification_type": notification_type,
+            "start_date": start_date,
+            "end_date": end_date
+        }
+        db.session.execute(create_tbl_sql, input_params)
+        db.session.commit()
+    except SQLAlchemyError as e:
+        db.session.commit()  # terminate previous transaction
+        # This query isn't quite right yet, if the notifications are already deleted still get 0.
+        # however it doesn't cause any harm, because there is nothing to delete.
+        # But it will also return 0 if the rows exist in notifications
+        qry = f""" Select count(*) from notifications 
+          WHERE service_id = :service_id
+          AND notification_type = :notification_type
+          AND created_at >= :start_date
+          AND created_at <= :end_date
+          AND key_type = 'normal'
+          UNION 
+          SELECT count(*) 
+            FROM {bkup_tble_name} 
+        """
+        result = db.session.execute(qry, input_params).fetchall()
+        if result[0][0] == result[1][0]:
+            print("Table and data already exists, keep going")
+            return
+        else:
+            # This will throw the exception if the data is already deleted or partically deleted...
+            # but gives us a chance to see what's happending.
+            print(f"Table already exists but row counts are inconsistent is missing bail out. "
+                  f"There are {result[0][0]} rows in notifications and {result[1][0]} rows in {bkup_tble_name}")
+            raise e

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/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,18 +324,54 @@ 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 _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
+        )
+
+    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)
+
+        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
+                )
+            )
+
+        insert_update_notification_history(notification_type, timestamp_to_delete_backwards_from, service_id, qry_limit)
+
+        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, qry_limit
+        )
+
+    return deleted
+
+
 def _delete_notifications(notification_type, date_to_delete_from, service_id, query_limit):
     subquery = db.session.query(
         Notification.id
@@ -389,6 +420,8 @@ def insert_update_notification_history(notification_type, date_to_delete_from, s
         Notification.service_id == service_id,
         Notification.created_at < date_to_delete_from,
         Notification.key_type != KEY_TYPE_TEST
+    ).order_by(
+        Notification.created_at
     )
     notifications_count = notification_query.count()
 
@@ -731,7 +764,7 @@ def notifications_not_yet_sent(should_be_sending_after_seconds, notification_typ
     return notifications
 
 
-def dao_get_letters_to_be_printed(print_run_deadline, postage):
+def dao_get_letters_to_be_printed(print_run_deadline):
     """
     Return all letters created before the print run deadline that have not yet been sent
     """
@@ -739,8 +772,7 @@ def dao_get_letters_to_be_printed(print_run_deadline, postage):
         Notification.created_at < convert_bst_to_utc(print_run_deadline),
         Notification.notification_type == LETTER_TYPE,
         Notification.status == NOTIFICATION_CREATED,
-        Notification.key_type == KEY_TYPE_NORMAL,
-        Notification.postage == postage
+        Notification.key_type == KEY_TYPE_NORMAL
     ).order_by(
         Notification.created_at
     ).all()

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/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)