From b2dfa59b1bc31cc67135253881ec05ad66d592a8 Mon Sep 17 00:00:00 2001
From: Rebecca Law <rebecca.law@digital.cabinet-office.gov.uk>
Date: Wed, 7 Feb 2018 17:49:15 +0000
Subject: [PATCH 01/11] We are not handling the case of an unknown status code
 sent by the SMS provider. This PR attempts to fix that.

- If the SMS client sends a status code that we do not recognize raise a ClientException and set the notification status to technical-failure
- Simplified the code in process_client_response, using a simple map.
---
 app/clients/sms/firetext.py                   | 22 ++---------
 app/clients/sms/mmg.py                        | 37 +++----------------
 app/notifications/process_client_response.py  | 27 ++++++--------
 tests/app/clients/test_firetext.py            | 18 ++-------
 tests/app/clients/test_mmg.py                 | 29 ++++++---------
 .../app/notifications/rest/test_callbacks.py  | 32 ++++++++--------
 .../test_process_client_response.py           | 15 ++++----
 7 files changed, 60 insertions(+), 120 deletions(-)

diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py
index e97091c0b..8f650ff79 100644
--- a/app/clients/sms/firetext.py
+++ b/app/clients/sms/firetext.py
@@ -5,7 +5,6 @@ from monotonic import monotonic
 from requests import request, RequestException
 
 from app.clients.sms import (SmsClient, SmsClientResponseException)
-from app.clients import STATISTICS_DELIVERED, STATISTICS_FAILURE
 
 logger = logging.getLogger(__name__)
 
@@ -15,24 +14,9 @@ logger = logging.getLogger(__name__)
 # the notification status to temporary-failure rather than permanent failure.
 #  See the code in the notification_dao.update_notifications_status_by_id
 firetext_responses = {
-    '0': {
-        "message": 'Delivered',
-        "notification_statistics_status": STATISTICS_DELIVERED,
-        "success": True,
-        "notification_status": 'delivered'
-    },
-    '1': {
-        "message": 'Declined',
-        "success": False,
-        "notification_statistics_status": STATISTICS_FAILURE,
-        "notification_status": 'permanent-failure'
-    },
-    '2': {
-        "message": 'Undelivered (Pending with Network)',
-        "success": True,
-        "notification_statistics_status": None,
-        "notification_status": 'pending'
-    }
+    '0': 'delivered',
+    '1': 'permanent-failure',
+    '2': 'pending'
 }
 
 
diff --git a/app/clients/sms/mmg.py b/app/clients/sms/mmg.py
index 3c8cf6338..55388119a 100644
--- a/app/clients/sms/mmg.py
+++ b/app/clients/sms/mmg.py
@@ -1,45 +1,18 @@
 import json
 from monotonic import monotonic
 from requests import (request, RequestException)
-from app.clients import (STATISTICS_DELIVERED, STATISTICS_FAILURE)
 from app.clients.sms import (SmsClient, SmsClientResponseException)
 
 mmg_response_map = {
-    '2': {
-        "message": ' Permanent failure',
-        "notification_statistics_status": STATISTICS_FAILURE,
-        "success": False,
-        "notification_status": 'permanent-failure'
-    },
-    '3': {
-        "message": 'Delivered',
-        "notification_statistics_status": STATISTICS_DELIVERED,
-        "success": True,
-        "notification_status": 'delivered'
-    },
-    '4': {
-        "message": ' Temporary failure',
-        "notification_statistics_status": STATISTICS_FAILURE,
-        "success": False,
-        "notification_status": 'temporary-failure'
-    },
-    '5': {
-        "message": 'Permanent failure',
-        "notification_statistics_status": STATISTICS_FAILURE,
-        "success": False,
-        "notification_status": 'permanent-failure'
-    },
-    'default': {
-        "message": 'Declined',
-        "success": False,
-        "notification_statistics_status": STATISTICS_FAILURE,
-        "notification_status": 'failed'
-    }
+    '2': 'permanent-failure',
+    '3': 'delivered',
+    '4': 'temporary-failure',
+    '5': 'permanent-failure'
 }
 
 
 def get_mmg_responses(status):
-    return mmg_response_map.get(status, mmg_response_map.get('default'))
+    return mmg_response_map[status]
 
 
 class MMGClientResponseException(SmsClientResponseException):
diff --git a/app/notifications/process_client_response.py b/app/notifications/process_client_response.py
index ad78c744d..8996f8a2b 100644
--- a/app/notifications/process_client_response.py
+++ b/app/notifications/process_client_response.py
@@ -4,6 +4,7 @@ from datetime import datetime
 from flask import current_app
 
 from app import 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
@@ -49,17 +50,19 @@ def process_sms_client_response(status, reference, client_name):
 
     # validate  status
     try:
-        response_dict = response_parser(status)
+        notification_status = response_parser(status)
         current_app.logger.info('{} callback return status of {} for reference: {}'.format(
             client_name, status, reference)
         )
     except KeyError:
-        msg = "{} callback failed: status {} not found.".format(client_name, status)
-        return success, msg
+        process_for_status(notification_status='technical-failure', client_name=client_name, reference=reference)
+        raise ClientException("{} callback failed: status {} not found.".format(client_name, status))
 
-    notification_status = response_dict['notification_status']
-    notification_status_message = response_dict['message']
-    notification_success = response_dict['success']
+    success = process_for_status(notification_status=notification_status, client_name=client_name, reference=reference)
+    return success, errors
+
+
+def process_for_status(notification_status, client_name, reference):
 
     # record stats
     notification = notifications_dao.update_notification_status_by_id(reference, notification_status)
@@ -67,14 +70,8 @@ def process_sms_client_response(status, reference, client_name):
         current_app.logger.warning("{} callback failed: notification {} either not found or already updated "
                                    "from sending. Status {}".format(client_name,
                                                                     reference,
-                                                                    notification_status_message))
-        return success, errors
-
-    if not notification_success:
-        current_app.logger.info(
-            "{} delivery failed: notification {} has error found. Status {}".format(client_name,
-                                                                                    reference,
-                                                                                    notification_status_message))
+                                                                    notification_status))
+        return
 
     statsd_client.incr('callback.{}.{}'.format(client_name.lower(), notification_status))
     if notification.sent_at:
@@ -92,4 +89,4 @@ def process_sms_client_response(status, reference, client_name):
         send_delivery_status_to_service.apply_async([str(notification.id)], queue=QueueNames.CALLBACKS)
 
     success = "{} callback succeeded. reference {} updated".format(client_name, reference)
-    return success, errors
+    return success
diff --git a/tests/app/clients/test_firetext.py b/tests/app/clients/test_firetext.py
index 1edcab564..94f7fa11d 100644
--- a/tests/app/clients/test_firetext.py
+++ b/tests/app/clients/test_firetext.py
@@ -9,27 +9,15 @@ from app.clients.sms.firetext import get_firetext_responses, SmsClientResponseEx
 
 
 def test_should_return_correct_details_for_delivery():
-    response_dict = get_firetext_responses('0')
-    assert response_dict['message'] == 'Delivered'
-    assert response_dict['notification_status'] == 'delivered'
-    assert response_dict['notification_statistics_status'] == 'delivered'
-    assert response_dict['success']
+    get_firetext_responses('0') == 'delivered'
 
 
 def test_should_return_correct_details_for_bounced():
-    response_dict = get_firetext_responses('1')
-    assert response_dict['message'] == 'Declined'
-    assert response_dict['notification_status'] == 'permanent-failure'
-    assert response_dict['notification_statistics_status'] == 'failure'
-    assert not response_dict['success']
+    get_firetext_responses('1') == 'permanent-failure'
 
 
 def test_should_return_correct_details_for_complaint():
-    response_dict = get_firetext_responses('2')
-    assert response_dict['message'] == 'Undelivered (Pending with Network)'
-    assert response_dict['notification_status'] == 'pending'
-    assert response_dict['notification_statistics_status'] is None
-    assert response_dict['success']
+    get_firetext_responses('2') == 'pending'
 
 
 def test_should_be_none_if_unrecognised_status_code():
diff --git a/tests/app/clients/test_mmg.py b/tests/app/clients/test_mmg.py
index c2947ca75..f5c875681 100644
--- a/tests/app/clients/test_mmg.py
+++ b/tests/app/clients/test_mmg.py
@@ -10,27 +10,22 @@ from app.clients.sms.mmg import get_mmg_responses, MMGClientResponseException
 
 
 def test_should_return_correct_details_for_delivery():
-    response_dict = get_mmg_responses('3')
-    assert response_dict['message'] == 'Delivered'
-    assert response_dict['notification_status'] == 'delivered'
-    assert response_dict['notification_statistics_status'] == 'delivered'
-    assert response_dict['success']
+    get_mmg_responses('3') == 'delivered'
 
 
-def test_should_return_correct_details_for_bounced():
-    response_dict = get_mmg_responses('50')
-    assert response_dict['message'] == 'Declined'
-    assert response_dict['notification_status'] == 'failed'
-    assert response_dict['notification_statistics_status'] == 'failure'
-    assert not response_dict['success']
+def test_should_return_correct_details_for_temporary_failure():
+    get_mmg_responses('4') == 'temporary-failure'
 
 
-def test_should_be_none_if_unrecognised_status_code():
-    response_dict = get_mmg_responses('blah')
-    assert response_dict['message'] == 'Declined'
-    assert response_dict['notification_status'] == 'failed'
-    assert response_dict['notification_statistics_status'] == 'failure'
-    assert not response_dict['success']
+@pytest.mark.parametrize('status', ['5', '2'])
+def test_should_return_correct_details_for_bounced(status):
+    get_mmg_responses(status) == 'permanent-failure'
+
+
+def test_should_be_raise_if_unrecognised_status_code():
+    with pytest.raises(KeyError) as e:
+        get_mmg_responses('99')
+    assert '99' in str(e.value)
 
 
 def test_send_sms_successful_returns_mmg_response(notify_api, mocker):
diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py
index 6db37085c..e3c3e7844 100644
--- a/tests/app/notifications/rest/test_callbacks.py
+++ b/tests/app/notifications/rest/test_callbacks.py
@@ -2,10 +2,12 @@ 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
 )
@@ -161,15 +163,17 @@ 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_return_400_if_unknown_status(client, mocker):
+def test_firetext_callback_should_set_status_technical_failure_if_status_unknown(
+        client, notify_db, notify_db_session, mocker):
+    notification = create_sample_notification(
+        notify_db, notify_db_session, status='sending', sent_at=datetime.utcnow()
+    )
     mocker.patch('app.statsd_client.incr')
-    data = 'mobile=441234123123&status=99&time=2016-03-10 14:17:00&reference={}'.format(uuid.uuid4())
-    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 failed: status 99 not found.'
+    data = 'mobile=441234123123&status=99&time=2016-03-10 14:17:00&reference={}'.format(notification.id)
+    with pytest.raises(ClientException) as e:
+        firetext_post(client, data)
+    assert get_notification_by_id(notification.id).status == 'technical-failure'
+    assert 'Firetext callback failed: status 99 not found.' in str(e.value)
 
 
 def test_firetext_callback_returns_200_when_notification_id_not_found_or_already_updated(client, mocker):
@@ -389,7 +393,7 @@ def test_process_mmg_response_status_4_updates_notification_with_temporary_faile
     assert get_notification_by_id(notification.id).status == 'temporary-failure'
 
 
-def test_process_mmg_response_unknown_status_updates_notification_with_failed(
+def test_process_mmg_response_unknown_status_updates_notification_with_technical_failure(
         notify_db, notify_db_session, client, mocker
 ):
     send_mock = mocker.patch(
@@ -403,12 +407,10 @@ def test_process_mmg_response_unknown_status_updates_notification_with_failed(
                        "MSISDN": "447777349060",
                        "status": 10})
     create_service_callback_api(service=notification.service, url="https://original_url.com")
-    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(notification.id)
-    assert get_notification_by_id(notification.id).status == 'failed'
+    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(notification.id).status == 'technical-failure'
     assert send_mock.called
 
 
diff --git a/tests/app/notifications/test_process_client_response.py b/tests/app/notifications/test_process_client_response.py
index 3f49901dd..64a3878ad 100644
--- a/tests/app/notifications/test_process_client_response.py
+++ b/tests/app/notifications/test_process_client_response.py
@@ -1,5 +1,8 @@
 import uuid
 
+import pytest
+
+from app.clients import ClientException
 from app.notifications.process_client_response import (
     validate_callback_data,
     process_sms_client_response
@@ -96,7 +99,7 @@ def test_process_sms_response_returns_error_bad_reference(mocker):
     stats_mock.assert_not_called()
 
 
-def test_process_sms_response_returns_error_for_unknown_sms_client(mocker):
+def test_process_sms_response_raises_client_exception_for_unknown_sms_client(mocker):
     stats_mock = mocker.patch('app.notifications.process_client_response.create_outcome_notification_statistic_tasks')
     success, error = process_sms_client_response(status='000', reference=str(uuid.uuid4()), client_name='sms-client')
 
@@ -105,10 +108,8 @@ def test_process_sms_response_returns_error_for_unknown_sms_client(mocker):
     stats_mock.assert_not_called()
 
 
-def test_process_sms_response_returns_error_for_unknown_status(mocker):
-    stats_mock = mocker.patch('app.notifications.process_client_response.create_outcome_notification_statistic_tasks')
+def test_process_sms_response_raises_client_exception_for_unknown_status(mocker):
+    with pytest.raises(ClientException) as e:
+        process_sms_client_response(status='000', reference=str(uuid.uuid4()), client_name='Firetext')
 
-    success, error = process_sms_client_response(status='000', reference=str(uuid.uuid4()), client_name='Firetext')
-    assert success is None
-    assert error == "{} callback failed: status {} not found.".format('Firetext', '000')
-    stats_mock.assert_not_called()
+    assert "{} callback failed: status {} not found.".format('Firetext', '000') in str(e.value)

From fa8cd780b0a98db9bda58f1c8fb32df093711128 Mon Sep 17 00:00:00 2001
From: Rebecca Law <rebecca.law@digital.cabinet-office.gov.uk>
Date: Mon, 12 Feb 2018 12:06:43 +0000
Subject: [PATCH 02/11] Make the errors in the process_sms_client_reponse more
 obvious by changing variable name.

Added a missing test if mmg sends a CID (or reference) that is not a UUID.

NB: All client specific tests are in test_callbacks.
---
 app/notifications/process_client_response.py   | 11 +++++------
 tests/app/notifications/rest/test_callbacks.py | 12 +++++++++++-
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/app/notifications/process_client_response.py b/app/notifications/process_client_response.py
index 8996f8a2b..19aa2d290 100644
--- a/app/notifications/process_client_response.py
+++ b/app/notifications/process_client_response.py
@@ -40,8 +40,8 @@ def process_sms_client_response(status, reference, client_name):
     try:
         uuid.UUID(reference, version=4)
     except ValueError:
-        message = "{} callback with invalid reference {}".format(client_name, reference)
-        return success, message
+        errors = "{} callback with invalid reference {}".format(client_name, reference)
+        return success, errors
 
     try:
         response_parser = sms_response_mapper[client_name]
@@ -55,15 +55,14 @@ def process_sms_client_response(status, reference, client_name):
             client_name, status, reference)
         )
     except KeyError:
-        process_for_status(notification_status='technical-failure', client_name=client_name, reference=reference)
+        _process_for_status(notification_status='technical-failure', client_name=client_name, reference=reference)
         raise ClientException("{} callback failed: status {} not found.".format(client_name, status))
 
-    success = process_for_status(notification_status=notification_status, client_name=client_name, reference=reference)
+    success = _process_for_status(notification_status=notification_status, client_name=client_name, reference=reference)
     return success, errors
 
 
-def process_for_status(notification_status, client_name, reference):
-
+def _process_for_status(notification_status, client_name, reference):
     # record stats
     notification = notifications_dao.update_notification_status_by_id(reference, notification_status)
     if not notification:
diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py
index e3c3e7844..1654b3b68 100644
--- a/tests/app/notifications/rest/test_callbacks.py
+++ b/tests/app/notifications/rest/test_callbacks.py
@@ -176,7 +176,7 @@ def test_firetext_callback_should_set_status_technical_failure_if_status_unknown
     assert 'Firetext callback failed: status 99 not found.' in str(e.value)
 
 
-def test_firetext_callback_returns_200_when_notification_id_not_found_or_already_updated(client, mocker):
+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'
     response = firetext_post(client, data)
@@ -438,6 +438,16 @@ def test_mmg_callback_returns_200_when_notification_id_not_found_or_already_upda
     assert response.status_code == 200
 
 
+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"}'
+
+    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'
+
+
 def test_process_mmg_response_records_statsd(notify_db, notify_db_session, client, mocker):
     with freeze_time('2001-01-01T12:00:00'):
 

From e736c90d002a90a25fb1534ac87458dd6df467d7 Mon Sep 17 00:00:00 2001
From: Rebecca Law <rebecca.law@digital.cabinet-office.gov.uk>
Date: Tue, 13 Feb 2018 18:38:32 +0000
Subject: [PATCH 03/11] Switch to using the pdf letter flow.

When sending letters always use the pdf letter flow regardless of service permissions.
---
 app/celery/tasks.py                           |  43 +++-----
 app/config.py                                 |  15 ---
 app/v2/notifications/post_notifications.py    |  27 +++--
 tests/app/celery/test_tasks.py                | 100 ++----------------
 .../test_post_letter_notifications.py         |  77 +++-----------
 .../notifications/test_post_notifications.py  |  24 -----
 6 files changed, 53 insertions(+), 233 deletions(-)

diff --git a/app/celery/tasks.py b/app/celery/tasks.py
index ff819e30e..697a160ed 100644
--- a/app/celery/tasks.py
+++ b/app/celery/tasks.py
@@ -119,21 +119,11 @@ def process_job(job_id):
     ).enumerated_recipients_and_personalisation:
         process_row(row_number, recipient, personalisation, template, job, service)
 
-    job_complete(job, service, template.template_type, start=start)
+    job_complete(job, start=start)
 
 
-def job_complete(job, service, template_type, resumed=False, start=None):
-    if (
-        template_type == LETTER_TYPE and
-        not service.has_permission('letters_as_pdf')
-    ):
-        if service.research_mode:
-            update_job_to_sent_to_dvla.apply_async([str(job.id)], queue=QueueNames.RESEARCH_MODE)
-        else:
-            build_dvla_file.apply_async([str(job.id)], queue=QueueNames.JOBS)
-            current_app.logger.debug("send job {} to build-dvla-file in the {} queue".format(job.id, QueueNames.JOBS))
-    else:
-        job.job_status = JOB_STATUS_FINISHED
+def job_complete(job, resumed=False, start=None):
+    job.job_status = JOB_STATUS_FINISHED
 
     finished = datetime.utcnow()
     job.processing_finished = finished
@@ -326,19 +316,18 @@ def save_letter(
             status=status
         )
 
-        if service.has_permission('letters_as_pdf'):
-            if not service.research_mode:
-                letters_pdf_tasks.create_letters_pdf.apply_async(
-                    [str(saved_notification.id)],
-                    queue=QueueNames.CREATE_LETTERS_PDF
-                )
-            elif current_app.config['NOTIFY_ENVIRONMENT'] in ['preview', 'development']:
-                research_mode_tasks.create_fake_letter_response_file.apply_async(
-                    (saved_notification.reference,),
-                    queue=QueueNames.RESEARCH_MODE
-                )
-            else:
-                update_notification_status_by_reference(saved_notification.reference, 'delivered')
+        if not service.research_mode:
+            letters_pdf_tasks.create_letters_pdf.apply_async(
+                [str(saved_notification.id)],
+                queue=QueueNames.CREATE_LETTERS_PDF
+            )
+        elif current_app.config['NOTIFY_ENVIRONMENT'] in ['preview', 'development']:
+            research_mode_tasks.create_fake_letter_response_file.apply_async(
+                (saved_notification.reference,),
+                queue=QueueNames.RESEARCH_MODE
+            )
+        else:
+            update_notification_status_by_reference(saved_notification.reference, 'delivered')
 
         current_app.logger.debug("Letter {} created at {}".format(saved_notification.id, saved_notification.created_at))
     except SQLAlchemyError as e:
@@ -608,4 +597,4 @@ def process_incomplete_job(job_id):
         if row_number > resume_from_row:
             process_row(row_number, recipient, personalisation, template, job, job.service)
 
-    job_complete(job, job.service, template.template_type, resumed=True)
+    job_complete(job, resumed=True)
diff --git a/app/config.py b/app/config.py
index a29e654b9..3cbe9130c 100644
--- a/app/config.py
+++ b/app/config.py
@@ -170,11 +170,6 @@ class Config(object):
             'schedule': crontab(minute=1),
             'options': {'queue': QueueNames.PERIODIC}
         },
-        # 'send-scheduled-notifications': {
-        #     'task': 'send-scheduled-notifications',
-        #     'schedule': crontab(minute='*/15'),
-        #     'options': {'queue': 'periodic'}
-        # },
         'delete-verify-codes': {
             'task': 'delete-verify-codes',
             'schedule': timedelta(minutes=63),
@@ -252,11 +247,6 @@ class Config(object):
             'schedule': crontab(hour=16, minute=30),
             'options': {'queue': QueueNames.PERIODIC}
         },
-        'run-letter-jobs': {
-            'task': 'run-letter-jobs',
-            'schedule': crontab(hour=17, minute=30),
-            'options': {'queue': QueueNames.PERIODIC}
-        },
         'trigger-letter-pdfs-for-day': {
             'task': 'trigger-letter-pdfs-for-day',
             'schedule': crontab(hour=17, minute=50),
@@ -267,11 +257,6 @@ class Config(object):
             'schedule': crontab(hour=23, minute=00),
             'options': {'queue': QueueNames.PERIODIC}
         },
-        'run-letter-api-notifications': {
-            'task': 'run-letter-api-notifications',
-            'schedule': crontab(hour=17, minute=40),
-            'options': {'queue': QueueNames.PERIODIC}
-        },
         'check-job-status': {
             'task': 'check-job-status',
             'schedule': crontab(),
diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py
index a619c0dc3..1e802697d 100644
--- a/app/v2/notifications/post_notifications.py
+++ b/app/v2/notifications/post_notifications.py
@@ -183,20 +183,19 @@ def process_letter_notification(*, letter_data, api_key, template, reply_to_text
                                               status=status,
                                               reply_to_text=reply_to_text)
 
-    if api_key.service.has_permission('letters_as_pdf'):
-        if should_send:
-            create_letters_pdf.apply_async(
-                [str(notification.id)],
-                queue=QueueNames.CREATE_LETTERS_PDF
-            )
-        elif (api_key.service.research_mode and
-              current_app.config['NOTIFY_ENVIRONMENT'] in ['preview', 'development']):
-            create_fake_letter_response_file.apply_async(
-                (notification.reference,),
-                queue=QueueNames.RESEARCH_MODE
-            )
-        else:
-            update_notification_status_by_reference(notification.reference, NOTIFICATION_DELIVERED)
+    if should_send:
+        create_letters_pdf.apply_async(
+            [str(notification.id)],
+            queue=QueueNames.CREATE_LETTERS_PDF
+        )
+    elif (api_key.service.research_mode and
+          current_app.config['NOTIFY_ENVIRONMENT'] in ['preview', 'development']):
+        create_fake_letter_response_file.apply_async(
+            (notification.reference,),
+            queue=QueueNames.RESEARCH_MODE
+        )
+    else:
+        update_notification_status_by_reference(notification.reference, NOTIFICATION_DELIVERED)
 
     return notification
 
diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py
index f3967cff9..1f0f3407e 100644
--- a/tests/app/celery/test_tasks.py
+++ b/tests/app/celery/test_tasks.py
@@ -20,7 +20,6 @@ from app.celery.scheduled_tasks import check_job_status
 from app.celery.tasks import (
     build_dvla_file,
     create_dvla_file_contents_for_job,
-    job_complete,
     process_job,
     process_row,
     save_sms,
@@ -33,7 +32,7 @@ from app.celery.tasks import (
     send_inbound_sms_to_service,
 )
 from app.config import QueueNames
-from app.dao import jobs_dao, services_dao, service_permissions_dao
+from app.dao import jobs_dao, services_dao
 from app.models import (
     Job,
     Notification,
@@ -110,9 +109,7 @@ def test_should_process_sms_job(sample_job, mocker):
     mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('sms'))
     mocker.patch('app.celery.tasks.save_sms.apply_async')
     mocker.patch('app.encryption.encrypt', return_value="something_encrypted")
-    mocker.patch('app.celery.tasks.build_dvla_file')
     mocker.patch('app.celery.tasks.create_uuid', return_value="uuid")
-    mocker.patch('app.celery.tasks.build_dvla_file')
 
     process_job(sample_job.id)
     s3.get_job_from_s3.assert_called_once_with(
@@ -132,7 +129,6 @@ def test_should_process_sms_job(sample_job, mocker):
     )
     job = jobs_dao.dao_get_job_by_id(sample_job.id)
     assert job.job_status == 'finished'
-    tasks.build_dvla_file.assert_not_called()
 
 
 @freeze_time("2016-01-01 11:09:00.061258")
@@ -144,7 +140,6 @@ def test_should_not_process_sms_job_if_would_exceed_send_limits(notify_db,
 
     mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('multiple_sms'))
     mocker.patch('app.celery.tasks.process_row')
-    mocker.patch('app.celery.tasks.build_dvla_file')
 
     process_job(job.id)
 
@@ -152,7 +147,6 @@ def test_should_not_process_sms_job_if_would_exceed_send_limits(notify_db,
     assert job.job_status == 'sending limits exceeded'
     assert s3.get_job_from_s3.called is False
     assert tasks.process_row.called is False
-    tasks.build_dvla_file.assert_not_called()
 
 
 def test_should_not_process_sms_job_if_would_exceed_send_limits_inc_today(notify_db,
@@ -165,7 +159,6 @@ def test_should_not_process_sms_job_if_would_exceed_send_limits_inc_today(notify
 
     mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('sms'))
     mocker.patch('app.celery.tasks.process_row')
-    mocker.patch('app.celery.tasks.build_dvla_file')
 
     process_job(job.id)
 
@@ -173,7 +166,6 @@ def test_should_not_process_sms_job_if_would_exceed_send_limits_inc_today(notify
     assert job.job_status == 'sending limits exceeded'
     assert s3.get_job_from_s3.called is False
     assert tasks.process_row.called is False
-    tasks.build_dvla_file.assert_not_called()
 
 
 def test_should_not_process_email_job_if_would_exceed_send_limits_inc_today(notify_db, notify_db_session, mocker):
@@ -185,7 +177,6 @@ def test_should_not_process_email_job_if_would_exceed_send_limits_inc_today(noti
 
     mocker.patch('app.celery.tasks.s3.get_job_from_s3')
     mocker.patch('app.celery.tasks.process_row')
-    mocker.patch('app.celery.tasks.build_dvla_file')
 
     process_job(job.id)
 
@@ -193,7 +184,6 @@ def test_should_not_process_email_job_if_would_exceed_send_limits_inc_today(noti
     assert job.job_status == 'sending limits exceeded'
     assert s3.get_job_from_s3.called is False
     assert tasks.process_row.called is False
-    tasks.build_dvla_file.assert_not_called()
 
 
 @freeze_time("2016-01-01 11:09:00.061258")
@@ -204,7 +194,6 @@ def test_should_not_process_email_job_if_would_exceed_send_limits(notify_db, not
 
     mocker.patch('app.celery.tasks.s3.get_job_from_s3')
     mocker.patch('app.celery.tasks.process_row')
-    mocker.patch('app.celery.tasks.build_dvla_file')
 
     process_job(job.id)
 
@@ -212,7 +201,6 @@ def test_should_not_process_email_job_if_would_exceed_send_limits(notify_db, not
     assert job.job_status == 'sending limits exceeded'
     assert s3.get_job_from_s3.called is False
     assert tasks.process_row.called is False
-    tasks.build_dvla_file.assert_not_called()
 
 
 def test_should_not_process_job_if_already_pending(notify_db, notify_db_session, mocker):
@@ -220,13 +208,11 @@ def test_should_not_process_job_if_already_pending(notify_db, notify_db_session,
 
     mocker.patch('app.celery.tasks.s3.get_job_from_s3')
     mocker.patch('app.celery.tasks.process_row')
-    mocker.patch('app.celery.tasks.build_dvla_file')
 
     process_job(job.id)
 
     assert s3.get_job_from_s3.called is False
     assert tasks.process_row.called is False
-    tasks.build_dvla_file.assert_not_called()
 
 
 def test_should_process_email_job_if_exactly_on_send_limits(notify_db,
@@ -313,7 +299,6 @@ def test_should_process_letter_job(sample_letter_job, mocker):
     s3_mock = mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=csv)
     process_row_mock = mocker.patch('app.celery.tasks.process_row')
     mocker.patch('app.celery.tasks.create_uuid', return_value="uuid")
-    mocker.patch('app.celery.tasks.build_dvla_file')
 
     process_job(sample_letter_job.id)
 
@@ -338,8 +323,7 @@ def test_should_process_letter_job(sample_letter_job, mocker):
 
     assert process_row_mock.call_count == 1
 
-    assert sample_letter_job.job_status == 'in progress'
-    tasks.build_dvla_file.apply_async.assert_called_once_with([str(sample_letter_job.id)], queue="job-tasks")
+    assert sample_letter_job.job_status == 'finished'
 
 
 def test_should_process_all_sms_job(sample_job_with_placeholdered_template,
@@ -650,60 +634,6 @@ def test_should_put_save_email_task_in_research_mode_queue_if_research_mode_serv
     )
 
 
-def test_should_not_build_dvla_file_in_research_mode_for_letter_job(
-        mocker, sample_letter_job, fake_uuid
-):
-    test_encrypted_data = 'some encrypted data'
-    sample_letter_job.service.research_mode = True
-
-    csv = """address_line_1,address_line_2,address_line_3,address_line_4,postcode,name
-    A1,A2,A3,A4,A_POST,Alice
-    """
-    mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=csv)
-    mocker.patch('app.celery.tasks.update_job_to_sent_to_dvla.apply_async')
-    mocker.patch('app.celery.tasks.save_letter.apply_async')
-    mocker.patch('app.celery.tasks.create_uuid', return_value=fake_uuid)
-    mocker.patch('app.celery.tasks.encryption.encrypt', return_value=test_encrypted_data)
-    mock_dvla_file_task = mocker.patch('app.celery.tasks.build_dvla_file.apply_async')
-
-    process_job(sample_letter_job.id)
-
-    assert not mock_dvla_file_task.called
-
-
-def test_should_update_job_to_sent_to_dvla_in_research_mode_for_letter_job(
-        mocker, sample_letter_job, fake_uuid
-):
-    test_encrypted_data = 'some encrypted data'
-    sample_letter_job.service.research_mode = True
-
-    csv = """address_line_1,address_line_2,address_line_3,address_line_4,postcode,name
-    A1,A2,A3,A4,A_POST,Alice
-    """
-    mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=csv)
-    mock_update_job_task = mocker.patch('app.celery.tasks.update_job_to_sent_to_dvla.apply_async')
-    mocker.patch('app.celery.tasks.save_letter.apply_async')
-    mocker.patch('app.celery.tasks.create_uuid', return_value=fake_uuid)
-    mocker.patch('app.celery.tasks.encryption.encrypt', return_value=test_encrypted_data)
-    mocker.patch('app.celery.tasks.build_dvla_file.apply_async')
-
-    process_job(sample_letter_job.id)
-
-    job = jobs_dao.dao_get_job_by_id(sample_letter_job.id)
-
-    save_letter.apply_async.assert_called_once_with(
-        (
-            str(sample_letter_job.service_id),
-            fake_uuid,
-            test_encrypted_data,
-        ),
-        queue=QueueNames.RESEARCH_MODE
-    )
-
-    mock_update_job_task.assert_called_once_with(
-        [str(job.id)], queue=QueueNames.RESEARCH_MODE)
-
-
 def test_should_save_sms_template_to_and_persist_with_job_id(sample_job, sample_api_key, mocker):
     notification = _notification_json(
         sample_job.template,
@@ -999,6 +929,7 @@ def test_save_letter_saves_letter_to_database(mocker, notify_db_session):
     job = create_job(template=template)
 
     mocker.patch('app.celery.tasks.create_random_identifier', return_value="this-is-random-in-real-life")
+    mocker.patch('app.celery.tasks.letters_pdf_tasks.create_letters_pdf.apply_async')
 
     personalisation = {
         'addressline1': 'Foo',
@@ -1048,6 +979,7 @@ def test_save_letter_saves_letter_to_database_right_reply_to(mocker, notify_db_s
     job = create_job(template=template)
 
     mocker.patch('app.celery.tasks.create_random_identifier', return_value="this-is-random-in-real-life")
+    mocker.patch('app.celery.tasks.letters_pdf_tasks.create_letters_pdf.apply_async')
 
     personalisation = {
         'addressline1': 'Foo',
@@ -1107,6 +1039,7 @@ def test_save_letter_uses_template_reply_to_text(mocker, notify_db_session):
     job = create_job(template=template)
 
     mocker.patch('app.celery.tasks.create_random_identifier', return_value="this-is-random-in-real-life")
+    mocker.patch('app.celery.tasks.letters_pdf_tasks.create_letters_pdf.apply_async')
 
     personalisation = {
         'addressline1': 'Foo',
@@ -1155,7 +1088,6 @@ def test_save_letter_sets_delivered_letters_as_pdf_permission_in_research_mode_i
         notify_api, mocker, notify_db_session, sample_letter_job, env):
     sample_letter_job.service.research_mode = True
     sample_reference = "this-is-random-in-real-life"
-    service_permissions_dao.dao_add_service_permission(sample_letter_job.service.id, 'letters_as_pdf')
     mock_create_fake_letter_response_file = mocker.patch(
         'app.celery.research_mode_tasks.create_fake_letter_response_file.apply_async')
     mocker.patch('app.celery.tasks.create_random_identifier', return_value=sample_reference)
@@ -1189,11 +1121,10 @@ def test_save_letter_sets_delivered_letters_as_pdf_permission_in_research_mode_i
 
 
 @pytest.mark.parametrize('env', ['development', 'preview'])
-def test_save_letter_calls_create_fake_response_for_letters_as_pdf_permission_in_research_mode_on_development_preview(
+def test_save_letter_calls_create_fake_response_for_letters_in_research_mode_on_development_preview(
         notify_api, mocker, notify_db_session, sample_letter_job, env):
     sample_letter_job.service.research_mode = True
     sample_reference = "this-is-random-in-real-life"
-    service_permissions_dao.dao_add_service_permission(sample_letter_job.service.id, 'letters_as_pdf')
     mock_create_fake_letter_response_file = mocker.patch(
         'app.celery.research_mode_tasks.create_fake_letter_response_file.apply_async')
     mocker.patch('app.celery.tasks.create_random_identifier', return_value=sample_reference)
@@ -1227,9 +1158,8 @@ def test_save_letter_calls_create_fake_response_for_letters_as_pdf_permission_in
     )
 
 
-def test_save_letter_calls_create_letters_pdf_task_with_letters_as_pdf_permission_and_not_in_research(
+def test_save_letter_calls_create_letters_pdf_task_not_in_research(
         mocker, notify_db_session, sample_letter_job):
-    service_permissions_dao.dao_add_service_permission(sample_letter_job.service.id, 'letters_as_pdf')
     mock_create_letters_pdf = mocker.patch('app.celery.letters_pdf_tasks.create_letters_pdf.apply_async')
 
     personalisation = {
@@ -1259,18 +1189,6 @@ def test_save_letter_calls_create_letters_pdf_task_with_letters_as_pdf_permissio
     )
 
 
-def test_job_complete_does_not_call_build_dvla_file_with_letters_as_pdf_permission(
-        mocker, notify_db_session, sample_letter_job):
-    service_permissions_dao.dao_add_service_permission(sample_letter_job.service.id, 'letters_as_pdf')
-    mock_build_dvla_files = mocker.patch('app.celery.tasks.build_dvla_file.apply_async')
-
-    job_complete(sample_letter_job, sample_letter_job.service, sample_letter_job.template.template_type)
-
-    assert not sample_letter_job.service.research_mode
-    assert not mock_build_dvla_files.called
-    assert sample_letter_job.job_status == JOB_STATUS_FINISHED
-
-
 def test_should_cancel_job_if_service_is_inactive(sample_service,
                                                   sample_job,
                                                   mocker):
@@ -1278,7 +1196,6 @@ def test_should_cancel_job_if_service_is_inactive(sample_service,
 
     mocker.patch('app.celery.tasks.s3.get_job_from_s3')
     mocker.patch('app.celery.tasks.process_row')
-    mock_dvla_file_task = mocker.patch('app.celery.tasks.build_dvla_file')
 
     process_job(sample_job.id)
 
@@ -1286,7 +1203,6 @@ def test_should_cancel_job_if_service_is_inactive(sample_service,
     assert job.job_status == 'cancelled'
     s3.get_job_from_s3.assert_not_called()
     tasks.process_row.assert_not_called()
-    mock_dvla_file_task.assert_not_called()
 
 
 @pytest.mark.parametrize('template_type, expected_class', [
@@ -1669,7 +1585,6 @@ def test_process_incomplete_job_email(mocker, sample_email_template):
 def test_process_incomplete_job_letter(mocker, sample_letter_template):
     mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('multiple_letter'))
     mock_letter_saver = mocker.patch('app.celery.tasks.save_letter.apply_async')
-    mock_build_dvla = mocker.patch('app.celery.tasks.build_dvla_file.apply_async')
 
     job = create_job(template=sample_letter_template, notification_count=10,
                      created_at=datetime.utcnow() - timedelta(hours=2),
@@ -1684,5 +1599,4 @@ def test_process_incomplete_job_letter(mocker, sample_letter_template):
 
     process_incomplete_job(str(job.id))
 
-    assert mock_build_dvla.called
     assert mock_letter_saver.call_count == 8
diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py
index 31830da45..f426e2efa 100644
--- a/tests/app/v2/notifications/test_post_letter_notifications.py
+++ b/tests/app/v2/notifications/test_post_letter_notifications.py
@@ -5,7 +5,6 @@ from flask import url_for
 import pytest
 
 from app.config import QueueNames
-from app.dao import service_permissions_dao
 from app.models import EMAIL_TYPE
 from app.models import Job
 from app.models import KEY_TYPE_NORMAL
@@ -46,6 +45,7 @@ def letter_request(client, data, service_id, key_type=KEY_TYPE_NORMAL, _expected
 
 @pytest.mark.parametrize('reference', [None, 'reference_from_client'])
 def test_post_letter_notification_returns_201(client, sample_letter_template, mocker, reference):
+    mock = mocker.patch('app.celery.tasks.letters_pdf_tasks.create_letters_pdf.apply_async')
     data = {
         'template_id': str(sample_letter_template.id),
         'personalisation': {
@@ -81,38 +81,15 @@ def test_post_letter_notification_returns_201(client, sample_letter_template, mo
     )
     assert not resp_json['scheduled_for']
     assert not notification.reply_to_text
-
-
-def test_post_letter_notification_for_letters_as_pdf_calls_celery_task(client, sample_letter_template, mocker):
-    service_permissions_dao.dao_add_service_permission(sample_letter_template.service.id, 'letters_as_pdf')
-
-    data = {
-        'template_id': str(sample_letter_template.id),
-        'personalisation': {
-            'address_line_1': 'Her Royal Highness Queen Elizabeth II',
-            'address_line_2': 'Buckingham Palace',
-            'address_line_3': 'London',
-            'postcode': 'SW1 1AA',
-            'name': 'Lizzie'
-        },
-        'reference': 'foo'
-    }
-    fake_task = mocker.patch('app.celery.tasks.letters_pdf_tasks.create_letters_pdf.apply_async')
-
-    letter_request(client, data, service_id=sample_letter_template.service_id)
-
-    notification = Notification.query.one()
-
-    fake_task.assert_called_once_with([str(notification.id)], queue=QueueNames.CREATE_LETTERS_PDF)
+    mock.assert_called_once_with([str(notification.id)], queue=QueueNames.CREATE_LETTERS_PDF)
 
 
 @pytest.mark.parametrize('env', [
     'development',
     'preview',
 ])
-def test_post_letter_notification_for_letters_as_pdf_calls_create_fake_response_in_research_and_test_key_correct_env(
+def test_post_letter_notification_calls_create_fake_response_in_research_and_test_key_correct_env(
         notify_api, client, sample_letter_template, mocker, env):
-    service_permissions_dao.dao_add_service_permission(sample_letter_template.service.id, 'letters_as_pdf')
     sample_letter_template.service.research_mode = True
 
     data = {
@@ -147,9 +124,8 @@ def test_post_letter_notification_for_letters_as_pdf_calls_create_fake_response_
     'staging',
     'live',
 ])
-def test_post_letter_noti_for_letters_as_pdf_sets_status_delivered_in_research_and_test_key_incorrect_env(
+def test_post_letter_notification_sets_status_delivered_in_research_and_test_key_incorrect_env(
         notify_api, client, sample_letter_template, mocker, env):
-    service_permissions_dao.dao_add_service_permission(sample_letter_template.service.id, 'letters_as_pdf')
     sample_letter_template.service.research_mode = True
 
     data = {
@@ -186,9 +162,8 @@ def test_post_letter_noti_for_letters_as_pdf_sets_status_delivered_in_research_a
     'staging',
     'live',
 ])
-def test_post_letter_noti_for_letters_as_pdf_sets_status_to_delivered_using_test_key_and_not_research_all_env(
+def test_post_letter_notification_sets_status_to_delivered_using_test_key_and_not_research_all_env(
         notify_api, client, sample_letter_template, mocker, env):
-    service_permissions_dao.dao_add_service_permission(sample_letter_template.service.id, 'letters_as_pdf')
     sample_letter_template.service.research_mode = False
 
     data = {
@@ -350,32 +325,8 @@ def test_post_letter_notification_returns_403_if_not_allowed_to_send_notificatio
     ]
 
 
-@pytest.mark.parametrize('research_mode, key_type', [
-    (True, KEY_TYPE_NORMAL),
-    (False, KEY_TYPE_TEST)
-])
-def test_post_letter_notification_updates_noti_sending(
-    client,
-    notify_db_session,
-    mocker,
-    research_mode,
-    key_type
-):
-    service = create_service(research_mode=research_mode, service_permissions=[LETTER_TYPE])
-    template = create_template(service, template_type=LETTER_TYPE)
-
-    data = {
-        'template_id': str(template.id),
-        'personalisation': {'address_line_1': 'Foo', 'address_line_2': 'Bar', 'postcode': 'Baz'}
-    }
-
-    letter_request(client, data, service_id=service.id, key_type=key_type)
-
-    notification = Notification.query.one()
-    assert notification.status == NOTIFICATION_SENDING
-
-
-def test_post_letter_notification_doesnt_accept_team_key(client, sample_letter_template):
+def test_post_letter_notification_doesnt_accept_team_key(client, sample_letter_template, mocker):
+    mocker.patch('app.celery.letters_pdf_tasks.create_letters_pdf.apply_async')
     data = {
         'template_id': str(sample_letter_template.id),
         'personalisation': {'address_line_1': 'Foo', 'address_line_2': 'Bar', 'postcode': 'Baz'}
@@ -393,7 +344,8 @@ def test_post_letter_notification_doesnt_accept_team_key(client, sample_letter_t
     assert error_json['errors'] == [{'error': 'BadRequestError', 'message': 'Cannot send letters with a team api key'}]
 
 
-def test_post_letter_notification_doesnt_send_in_trial(client, sample_trial_letter_template):
+def test_post_letter_notification_doesnt_send_in_trial(client, sample_trial_letter_template, mocker):
+    mocker.patch('app.celery.letters_pdf_tasks.create_letters_pdf.apply_async')
     data = {
         'template_id': str(sample_trial_letter_template.id),
         'personalisation': {'address_line_1': 'Foo', 'address_line_2': 'Bar', 'postcode': 'Baz'}
@@ -411,11 +363,13 @@ def test_post_letter_notification_doesnt_send_in_trial(client, sample_trial_lett
         {'error': 'BadRequestError', 'message': 'Cannot send letters when service is in trial mode'}]
 
 
-def test_post_letter_notification_fakes_dvla_when_service_is_in_trial_mode_but_using_test_key(
+def test_post_letter_notification_is_delivered_if_in_trial_mode_and_using_test_key(
     client,
     sample_trial_letter_template,
     mocker
 ):
+    fake_create_letter_task = mocker.patch('app.celery.letters_pdf_tasks.create_letters_pdf.apply_async')
+
     data = {
         "template_id": sample_trial_letter_template.id,
         "personalisation": {'address_line_1': 'Foo', 'address_line_2': 'Bar', 'postcode': 'Baz'}
@@ -424,12 +378,15 @@ def test_post_letter_notification_fakes_dvla_when_service_is_in_trial_mode_but_u
     letter_request(client, data=data, service_id=sample_trial_letter_template.service_id, key_type=KEY_TYPE_TEST)
 
     notification = Notification.query.one()
-    assert notification.status == NOTIFICATION_SENDING
+    assert notification.status == NOTIFICATION_DELIVERED
+    assert not fake_create_letter_task.called
 
 
 def test_post_letter_notification_persists_notification_reply_to_text(
-    client, notify_db_session
+    client, notify_db_session, mocker
 ):
+    mocker.patch('app.celery.letters_pdf_tasks.create_letters_pdf.apply_async')
+
     service = create_service(service_permissions=[LETTER_TYPE])
     service_address = "12 Main Street, London"
     letter_contact = create_letter_contact(service=service, contact_block=service_address, is_default=True)
diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py
index df669d258..7ce825ac7 100644
--- a/tests/app/v2/notifications/test_post_notifications.py
+++ b/tests/app/v2/notifications/test_post_notifications.py
@@ -28,7 +28,6 @@ from tests.app.db import (
     create_service,
     create_template,
     create_reply_to_email,
-    create_letter_contact,
     create_service_sms_sender,
     create_service_with_inbound_number
 )
@@ -695,26 +694,3 @@ def test_post_email_notification_with_invalid_reply_to_id_returns_400(client, sa
     assert 'email_reply_to_id {} does not exist in database for service id {}'. \
         format(fake_uuid, sample_email_template.service_id) in resp_json['errors'][0]['message']
     assert 'BadRequestError' in resp_json['errors'][0]['error']
-
-
-def test_letter_notification_should_use_template_reply_to(client, sample_letter_template):
-    letter_contact = create_letter_contact(sample_letter_template.service, "Edinburgh, ED1 1AA", is_default=False)
-    sample_letter_template.reply_to = str(letter_contact.id)
-
-    data = {
-        'template_id': str(sample_letter_template.id),
-        'personalisation': {
-            'address_line_1': '123',
-            'address_line_2': '234',
-            'postcode': 'W1A1AA',
-        }
-    }
-    response = client.post("v2/notifications/letter",
-                           data=json.dumps(data),
-                           headers=[('Content-Type', 'application/json'),
-                                    create_authorization_header(service_id=sample_letter_template.service.id)]
-                           )
-    assert response.status_code == 201, response.get_data(as_text=True)
-    notifications = Notification.query.all()
-    assert len(notifications) == 1
-    assert notifications[0].reply_to_text == "Edinburgh, ED1 1AA"

From ea139191902356346e077f466e761d82a79561b5 Mon Sep 17 00:00:00 2001
From: pyup-bot <github-bot@pyup.io>
Date: Tue, 13 Feb 2018 23:25:18 +0000
Subject: [PATCH 04/11] Update boto3 from 1.5.27 to 1.5.28

---
 requirements.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/requirements.txt b/requirements.txt
index 3d8880a0b..4af753171 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -1,4 +1,4 @@
-boto3==1.5.27
+boto3==1.5.28
 cffi==1.11.0 # pyup: != 1.11.1, != 1.11.2 # These versions are missing .whl
 celery==3.1.25 # pyup: <4
 docopt==0.6.2

From a6cf634ab69b8c40ff085925dbba67397cf57115 Mon Sep 17 00:00:00 2001
From: pyup-bot <github-bot@pyup.io>
Date: Tue, 13 Feb 2018 23:25:22 +0000
Subject: [PATCH 05/11] Update awscli from 1.14.37 to 1.14.38

---
 requirements.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/requirements.txt b/requirements.txt
index 3d8880a0b..e6c5118cc 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -23,7 +23,7 @@ SQLAlchemy==1.2.2
 notifications-python-client==4.7.2
 
 # PaaS
-awscli==1.14.37
+awscli==1.14.38
 awscli-cwlogs>=1.4,<1.5
 
 git+https://github.com/alphagov/notifications-utils.git@23.6.1#egg=notifications-utils==23.6.1

From e1cc8175d7278c2f723e8e8efe847d93cd81fe6b Mon Sep 17 00:00:00 2001
From: Katie Smith <katie.smith@digital.cabinet-office.gov.uk>
Date: Tue, 13 Feb 2018 15:25:24 +0000
Subject: [PATCH 06/11] Allow services to add puntuation to or change the case
 of their name

Changed the '/service/unique' endpoint to optionally accept the
service_id parameter. It now doesn't matter if a user tries to change
the capitalization or add punctuation to their own service name. But
there should still be an error if a user tries to change the punctuation
or capitalization of another service.

service_id needs to be allowed to be None until notifications-admin is
updated to always pass in the service_id.
---
 app/service/rest.py            | 15 ++++++++--
 tests/app/service/test_rest.py | 52 ++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 3 deletions(-)

diff --git a/app/service/rest.py b/app/service/rest.py
index 13646653d..9d8098d95 100644
--- a/app/service/rest.py
+++ b/app/service/rest.py
@@ -673,15 +673,24 @@ def get_organisation_for_service(service_id):
 
 @service_blueprint.route('/unique', methods=["GET"])
 def is_service_name_unique():
-    name, email_from = check_request_args(request)
+    service_id, name, email_from = check_request_args(request)
 
     name_exists = Service.query.filter_by(name=name).first()
-    email_from_exists = Service.query.filter_by(email_from=email_from).first()
+
+    if service_id:
+        email_from_exists = Service.query.filter(
+            Service.email_from == email_from,
+            Service.id != service_id
+        ).first()
+    else:
+        email_from_exists = Service.query.filter_by(email_from=email_from).first()
+
     result = not (name_exists or email_from_exists)
     return jsonify(result=result), 200
 
 
 def check_request_args(request):
+    service_id = request.args.get('service_id')
     name = request.args.get('name', None)
     email_from = request.args.get('email_from', None)
     errors = []
@@ -691,4 +700,4 @@ def check_request_args(request):
         errors.append({'email_from': ["Can't be empty"]})
     if errors:
         raise InvalidRequest(errors, status_code=400)
-    return name, email_from
+    return service_id, name, email_from
diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py
index d1725ac72..d4185a239 100644
--- a/tests/app/service/test_rest.py
+++ b/tests/app/service/test_rest.py
@@ -2202,6 +2202,58 @@ def test_is_service_name_unique_returns_200_if_unique(client):
     assert json.loads(response.get_data(as_text=True)) == {"result": True}
 
 
+def test_is_service_name_unique_returns_200_if_unique_and_service_id_given(
+    client,
+    notify_db,
+    notify_db_session
+):
+    service = create_service(service_name='unique', email_from='unique')
+    service_id = str(service.id)
+
+    response = client.get(
+        '/service/unique?service_id={}&name=something&email_from=something'.format(service_id),
+        headers=[create_authorization_header()]
+    )
+
+    assert response.status_code == 200
+    assert json.loads(response.get_data(as_text=True)) == {"result": True}
+
+
+def test_is_service_name_unique_returns_200_when_capitalized(
+    client,
+    notify_db,
+    notify_db_session
+):
+    service = create_service(service_name='unique', email_from='unique')
+    service_id = str(service.id)
+
+    response = client.get(
+        '/service/unique?service_id={}&name={}&email_from={}'.format(service_id, 'UNIQUE', 'unique'),
+        headers=[create_authorization_header()]
+    )
+
+    assert response.status_code == 200
+    assert json.loads(response.get_data(as_text=True)) == {"result": True}
+
+
+def test_is_service_name_unique_returns_false_if_checking_capitalization_of_different_service(
+    client,
+    notify_db,
+    notify_db_session
+):
+    create_service(service_name='unique', email_from='unique')
+    different_service_id = '111aa111-2222-bbbb-aaaa-111111111111'
+
+    response = client.get(
+        '/service/unique?service_id={}&name={}&email_from={}'.format(
+            different_service_id, 'UNIQUE', 'unique'),
+        headers=[create_authorization_header()]
+    )
+
+    assert response.status_code == 200
+    assert json.loads(response.get_data(as_text=True)) == {"result": False}
+
+
 @pytest.mark.parametrize('name, email_from',
                          [("something unique", "something"),
                           ("unique", "something.unique"),

From f070b4eab12ba5adc70be597e67acde28115ff23 Mon Sep 17 00:00:00 2001
From: Leo Hemsted <leohemsted@gmail.com>
Date: Wed, 14 Feb 2018 11:31:17 +0000
Subject: [PATCH 07/11] Update requirements.txt

email validation
---
 requirements.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/requirements.txt b/requirements.txt
index 3d8880a0b..d54ce6717 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -26,6 +26,6 @@ notifications-python-client==4.7.2
 awscli==1.14.37
 awscli-cwlogs>=1.4,<1.5
 
-git+https://github.com/alphagov/notifications-utils.git@23.6.1#egg=notifications-utils==23.6.1
+git+https://github.com/alphagov/notifications-utils.git@23.6.2#egg=notifications-utils==23.6.2
 
 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3

From e87ed0f68a17568b32e4d29e2bc3207c0417fed1 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Wed, 14 Feb 2018 11:54:40 +0000
Subject: [PATCH 08/11] Stop pytest on first failing test
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If a PR is going to fail because tests aren’t passing then you:
- should know about it as quick as possible
- shouldn’t waste precious Jenkins CPU running subsequent tests

This commit adds the `-x` flag to pytest, which stops the test run as
soon as one failing test is discovered.
---
 scripts/run_tests.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/run_tests.sh b/scripts/run_tests.sh
index 8f5c41561..08451eb9c 100755
--- a/scripts/run_tests.sh
+++ b/scripts/run_tests.sh
@@ -31,5 +31,5 @@ flake8 .
 display_result $? 1 "Code style check"
 
 # run with four concurrent threads
-py.test --cov=app --cov-report=term-missing tests/ --junitxml=test_results.xml -n 4 -v
+py.test --cov=app --cov-report=term-missing tests/ --junitxml=test_results.xml -n4 -v -x
 display_result $? 2 "Unit tests"

From d36b742e14338c371f35fa21acc5bf3b7cb2efa3 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Wed, 14 Feb 2018 11:19:11 +0000
Subject: [PATCH 09/11] Automatically set environment vars before tests
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Sometimes you just wanna run some tests directly using the `pytest`
command. But you’re in a new shell, and have forgotten to do
`source environment_test.sh`. The screen fills with red, and your day
just got a little bit worse.

This commit will stop this from ever happening again, by making the
setting environment variables part of running Pytest. It does this with
a plugin called pytest-env[1].

pytest.ini is the standard way of configuring pytest. Creating this file
where it didn’t exist before changes the behaviour of pytest, in that
it will now look for tests in the same directory as the file, rather
than defaulting to the `tests/` directory. So we also have to explicitly
configure pytest[2] to tell it that it should only look in this
directory. Otherwise it gets lost in the weeds of `node_modules`.

This also changes the way that `SQLALCHEMY_DATABASE_URI` is overriden to
the convention used by this plugin.

1. https://github.com/MobileDynasty/pytest-env
2. https://docs.pytest.org/en/latest/customize.html#confval-testpaths

fixup! Remove environment_test.sh
---
 Makefile                  |  2 +-
 pytest.ini                | 20 ++++++++++++++++++++
 requirements_for_test.txt |  1 +
 3 files changed, 22 insertions(+), 1 deletion(-)
 create mode 100644 pytest.ini

diff --git a/Makefile b/Makefile
index b96a292a2..ebf56145a 100644
--- a/Makefile
+++ b/Makefile
@@ -128,7 +128,7 @@ test-with-docker: prepare-docker-build-image create-docker-test-db ## Run tests
 		--link "${DOCKER_CONTAINER_PREFIX}-db:postgres" \
 		-e UID=$(shell id -u) \
 		-e GID=$(shell id -g) \
-		-e TEST_DATABASE=postgresql://postgres:postgres@postgres/test_notification_api \
+		-e SQLALCHEMY_DATABASE_URI=postgresql://postgres:postgres@postgres/test_notification_api \
 		-e GIT_COMMIT=${GIT_COMMIT} \
 		-e BUILD_NUMBER=${BUILD_NUMBER} \
 		-e BUILD_URL=${BUILD_URL} \
diff --git a/pytest.ini b/pytest.ini
new file mode 100644
index 000000000..a76815501
--- /dev/null
+++ b/pytest.ini
@@ -0,0 +1,20 @@
+[pytest]
+testpaths = tests
+env =
+    D:SQLALCHEMY_DATABASE_URI=postgresql://@localhost/test_notification_api
+    SECRET_KEY=secret-key
+    DANGEROUS_SALT=dangerous-salt
+    NOTIFY_ENVIRONMENT=test
+    ADMIN_CLIENT_SECRET=dev-notify-secret-key
+    ADMIN_BASE_URL=http://localhost:6012
+    FROM_NUMBER=from_number
+    MMG_URL=https://api.mmg.co.uk/json/api.php
+    MMG_API_KEY=mmg-secret-key
+    LOADTESTING_API_KEY=loadtesting
+    FIRETEXT_API_KEY=Firetext
+    STATSD_PREFIX=stats-prefix
+    NOTIFICATION_QUEUE_PREFIX=testing
+    REDIS_URL=redis://localhost:6379/0
+    FLASK_APP=application.py
+    FLASK_DEBUG=1
+    WERKZEUG_DEBUG_PIN=off
diff --git a/requirements_for_test.txt b/requirements_for_test.txt
index 71a8c290d..1e62201eb 100644
--- a/requirements_for_test.txt
+++ b/requirements_for_test.txt
@@ -1,6 +1,7 @@
 -r requirements.txt
 flake8==3.5.0
 pytest==3.4.0
+pytest-env==0.6.2
 pytest-mock==1.6.3
 pytest-cov==2.5.1
 pytest-xdist==1.22.0

From 845aad1183d9f4ab9b94a7981d9e018228ddc5a2 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Wed, 14 Feb 2018 12:18:46 +0000
Subject: [PATCH 10/11] Remove environment_test.sh

These config variables are now set in `pytest.ini` instead.
---
 environment_test.sh  | 17 -----------------
 scripts/run_tests.sh |  4 ----
 2 files changed, 21 deletions(-)
 delete mode 100644 environment_test.sh

diff --git a/environment_test.sh b/environment_test.sh
deleted file mode 100644
index f165e3334..000000000
--- a/environment_test.sh
+++ /dev/null
@@ -1,17 +0,0 @@
-export SQLALCHEMY_DATABASE_URI=${TEST_DATABASE:='postgresql://localhost/test_notification_api'}
-export SECRET_KEY='secret-key'
-export DANGEROUS_SALT='dangerous-salt'
-export NOTIFY_ENVIRONMENT='test'
-export ADMIN_CLIENT_SECRET='dev-notify-secret-key'
-export ADMIN_BASE_URL='http://localhost:6012'
-export FROM_NUMBER='from_number'
-export MMG_URL="https://api.mmg.co.uk/json/api.php"
-export MMG_API_KEY='mmg-secret-key'
-export LOADTESTING_API_KEY="loadtesting"
-export FIRETEXT_API_KEY="Firetext"
-export STATSD_PREFIX="stats-prefix"
-export NOTIFICATION_QUEUE_PREFIX='testing'
-export REDIS_URL="redis://localhost:6379/0"
-export FLASK_APP=application.py
-export FLASK_DEBUG=1
-export WERKZEUG_DEBUG_PIN=off
\ No newline at end of file
diff --git a/scripts/run_tests.sh b/scripts/run_tests.sh
index 8f5c41561..f1668e697 100755
--- a/scripts/run_tests.sh
+++ b/scripts/run_tests.sh
@@ -5,12 +5,8 @@
 # NOTE: This script expects to be run from the project root with
 # ./scripts/run_tests.sh
 
-# Use default environment vars for localhost if not already set
-
 set -o pipefail
 
-source environment_test.sh
-
 function display_result {
   RESULT=$1
   EXIT_STATUS=$2

From faa76755c3994328ecb5c19ba98486d89e4d1174 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Wed, 14 Feb 2018 13:27:06 +0000
Subject: [PATCH 11/11] Allow up to 10 tests to fail before stopping

This is a sensible compromise between 1 test and ALL THE TESTS.

Uses the `maxfail` option as documented here:
https://docs.pytest.org/en/latest/usage.html#stopping-after-the-first-or-n-failures
---
 scripts/run_tests.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/run_tests.sh b/scripts/run_tests.sh
index 08451eb9c..3087f544f 100755
--- a/scripts/run_tests.sh
+++ b/scripts/run_tests.sh
@@ -31,5 +31,5 @@ flake8 .
 display_result $? 1 "Code style check"
 
 # run with four concurrent threads
-py.test --cov=app --cov-report=term-missing tests/ --junitxml=test_results.xml -n4 -v -x
+py.test --cov=app --cov-report=term-missing tests/ --junitxml=test_results.xml -n4 -v --maxfail=10
 display_result $? 2 "Unit tests"