From 19f7678b0560fcf1d00ab32f0c5b543bc728abb4 Mon Sep 17 00:00:00 2001
From: Pea Tyczynska <pea.tyczynska@digital.cabinet-office.gov.uk>
Date: Mon, 17 Dec 2018 10:03:54 +0000
Subject: [PATCH 01/24] Don't allow to set postage per template if no service
 permission

---
 app/template/rest.py                | 14 +++++++-
 app/template/template_schemas.py    |  3 +-
 tests/app/dao/test_templates_dao.py |  2 +-
 tests/app/db.py                     |  2 ++
 tests/app/template/test_rest.py     | 56 +++++++++++++++++++++++++++++
 5 files changed, 74 insertions(+), 3 deletions(-)

diff --git a/app/template/rest.py b/app/template/rest.py
index 8cbb4125f..9dc32c94f 100644
--- a/app/template/rest.py
+++ b/app/template/rest.py
@@ -31,7 +31,7 @@ from app.errors import (
     InvalidRequest
 )
 from app.letters.utils import get_letter_pdf
-from app.models import SMS_TYPE, Template
+from app.models import SMS_TYPE, Template, CHOOSE_POSTAGE
 from app.notifications.validators import service_has_permission, check_reply_to
 from app.schema_validation import validate
 from app.schemas import (template_schema, template_history_schema)
@@ -78,6 +78,12 @@ def create_template(service_id):
         errors = {'template_type': [message]}
         raise InvalidRequest(errors, 403)
 
+    if new_template.postage:
+        if not service_has_permission(CHOOSE_POSTAGE, fetched_service.permissions):
+            message = "Setting postage on templates is not enabled for this service."
+            errors = {'template_postage': [message]}
+            raise InvalidRequest(errors, 403)
+
     new_template.service = fetched_service
 
     over_limit = _content_count_greater_than_limit(new_template.content, new_template.template_type)
@@ -110,6 +116,12 @@ def update_template(service_id, template_id):
     if data.get('redact_personalisation') is True:
         return redact_template(fetched_template, data)
 
+    if data.get('postage'):
+        if not service_has_permission(CHOOSE_POSTAGE, fetched_template.service.permissions):
+            message = "Setting postage on templates is not enabled for this service."
+            errors = {'template_postage': [message]}
+            raise InvalidRequest(errors, 403)
+
     if "reply_to" in data:
         check_reply_to(service_id, data.get("reply_to"), fetched_template.template_type)
         updated = dao_update_template_reply_to(template_id=template_id, reply_to=data.get("reply_to"))
diff --git a/app/template/template_schemas.py b/app/template/template_schemas.py
index f63ad4575..9f38262a5 100644
--- a/app/template/template_schemas.py
+++ b/app/template/template_schemas.py
@@ -17,7 +17,8 @@ post_create_template_schema = {
         "content": {"type": "string"},
         "subject": {"type": "string"},
         "created_by": uuid,
-        "parent_folder_id": uuid
+        "parent_folder_id": uuid,
+        "postage": {"type": "string"},
     },
     "if": {
         "properties": {
diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py
index d86cec7a0..cbe6c6b72 100644
--- a/tests/app/dao/test_templates_dao.py
+++ b/tests/app/dao/test_templates_dao.py
@@ -153,7 +153,7 @@ def test_dao_update_template_reply_to_none_to_some(sample_service, sample_user):
     assert template_history.updated_at == updated.updated_at
 
 
-def test_dao_update_tempalte_reply_to_some_to_some(sample_service, sample_user):
+def test_dao_update_template_reply_to_some_to_some(sample_service, sample_user):
     letter_contact = create_letter_contact(sample_service, 'Edinburgh, ED1 1AA')
     letter_contact_2 = create_letter_contact(sample_service, 'London, N1 1DE')
 
diff --git a/tests/app/db.py b/tests/app/db.py
index 94d6ac287..a2db88ae6 100644
--- a/tests/app/db.py
+++ b/tests/app/db.py
@@ -140,6 +140,7 @@ def create_template(
         hidden=False,
         archived=False,
         folder=None,
+        postage=None,
 ):
     data = {
         'name': template_name or '{} Template Name'.format(template_type),
@@ -150,6 +151,7 @@ def create_template(
         'reply_to': reply_to,
         'hidden': hidden,
         'folder': folder,
+        'postage': postage,
     }
     if template_type != SMS_TYPE:
         data['subject'] = subject
diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py
index 7cf46aa80..91c569a00 100644
--- a/tests/app/template/test_rest.py
+++ b/tests/app/template/test_rest.py
@@ -17,6 +17,7 @@ from app.models import (
     EMAIL_TYPE,
     LETTER_TYPE,
     SMS_TYPE,
+    CHOOSE_POSTAGE,
     Template,
     TemplateHistory
 )
@@ -210,6 +211,34 @@ def test_should_raise_error_on_create_if_no_permission(
     assert json_resp['message'] == expected_error
 
 
+def test_should_raise_error_on_create_if_no_choose_postage_permission(client, sample_user):
+    service = create_service(service_permissions=[LETTER_TYPE])
+    data = {
+        'name': 'my template',
+        'template_type': LETTER_TYPE,
+        'content': 'template content',
+        'service': str(service.id),
+        'created_by': str(sample_user.id),
+        'subject': "Some letter",
+        'postage': 'first',
+    }
+
+    data = json.dumps(data)
+    auth_header = create_authorization_header()
+
+    response = client.post(
+        '/service/{}/template'.format(service.id),
+        headers=[('Content-Type', 'application/json'), auth_header],
+        data=data
+    )
+    json_resp = json.loads(response.get_data(as_text=True))
+    assert response.status_code == 403
+    assert json_resp['result'] == 'error'
+    assert json_resp['message'] == {
+        "template_postage": ["Setting postage on templates is not enabled for this service."]
+    }
+
+
 @pytest.mark.parametrize('template_factory, expected_error', [
     (sample_template_without_sms_permission, {'template_type': ['Updating text message templates is not allowed']}),
     (sample_template_without_email_permission, {'template_type': ['Updating email templates is not allowed']}),
@@ -239,6 +268,33 @@ def test_should_be_error_on_update_if_no_permission(
     assert json_resp['message'] == expected_error
 
 
+def test_should_be_error_on_update_if_no_choose_postage_permission(client, sample_user):
+    service = create_service(service_name='some_service', service_permissions=[LETTER_TYPE])
+    template = create_template(service, template_type=LETTER_TYPE)
+    data = {
+        'content': 'new template content',
+        'created_by': str(sample_user.id),
+        'postage': 'first'
+    }
+
+    data = json.dumps(data)
+    auth_header = create_authorization_header()
+
+    update_response = client.post(
+        '/service/{}/template/{}'.format(
+            template.service_id, template.id),
+        headers=[('Content-Type', 'application/json'), auth_header],
+        data=data
+    )
+
+    json_resp = json.loads(update_response.get_data(as_text=True))
+    assert update_response.status_code == 403
+    assert json_resp['result'] == 'error'
+    assert json_resp['message'] == {
+        "template_postage": ["Setting postage on templates is not enabled for this service."]
+    }
+
+
 def test_should_error_if_created_by_missing(client, sample_user, sample_service):
     service_id = str(sample_service.id)
     data = {

From e6524af89cecdab4aea3cef2a0b98cbb1245d924 Mon Sep 17 00:00:00 2001
From: Pea Tyczynska <pea.tyczynska@digital.cabinet-office.gov.uk>
Date: Mon, 17 Dec 2018 17:49:51 +0000
Subject: [PATCH 02/24] Choose postage when persisting a notification

---
 app/notifications/process_notifications.py    | 11 ++++-
 tests/app/db.py                               |  2 +
 .../test_process_notification.py              | 41 ++++++++++++++++++-
 tests/app/template/test_rest.py               |  1 -
 4 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py
index 6483ac766..8fc2f15f6 100644
--- a/app/notifications/process_notifications.py
+++ b/app/notifications/process_notifications.py
@@ -23,7 +23,8 @@ from app.models import (
     LETTER_TYPE,
     NOTIFICATION_CREATED,
     Notification,
-    ScheduledNotification
+    ScheduledNotification,
+    CHOOSE_POSTAGE
 )
 from app.dao.notifications_dao import (
     dao_create_notification,
@@ -31,6 +32,8 @@ from app.dao.notifications_dao import (
     dao_created_scheduled_notification
 )
 
+from app.dao.templates_dao import dao_get_template_by_id
+
 from app.v2.errors import BadRequestError
 from app.utils import (
     cache_key_for_service_template_counter,
@@ -109,7 +112,11 @@ def persist_notification(
     elif notification_type == EMAIL_TYPE:
         notification.normalised_to = format_email_address(notification.to)
     elif notification_type == LETTER_TYPE:
-        notification.postage = service.postage
+        template = dao_get_template_by_id(template_id, template_version)
+        if service.has_permission(CHOOSE_POSTAGE) and template.postage:
+            notification.postage = template.postage
+        else:
+            notification.postage = service.postage
 
     # if simulated create a Notification model to return but do not persist the Notification to the dB
     if not simulated:
diff --git a/tests/app/db.py b/tests/app/db.py
index a2db88ae6..02a2df85c 100644
--- a/tests/app/db.py
+++ b/tests/app/db.py
@@ -81,6 +81,7 @@ def create_service(
         prefix_sms=True,
         message_limit=1000,
         organisation_type='central',
+        postage='second'
 ):
     service = Service(
         name=service_name,
@@ -90,6 +91,7 @@ def create_service(
         created_by=user or create_user(email='{}@digital.cabinet-office.gov.uk'.format(uuid.uuid4())),
         prefix_sms=prefix_sms,
         organisation_type=organisation_type,
+        postage=postage
     )
 
     dao_create_service(service, service.created_by, service_id, service_permissions=service_permissions)
diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py
index 609a863d8..192644bde 100644
--- a/tests/app/notifications/test_process_notification.py
+++ b/tests/app/notifications/test_process_notification.py
@@ -13,7 +13,9 @@ from app.models import (
     Notification,
     NotificationHistory,
     ScheduledNotification,
-    Template
+    Template,
+    LETTER_TYPE,
+    CHOOSE_POSTAGE
 )
 from app.notifications.process_notifications import (
     create_content_for_notification,
@@ -27,6 +29,8 @@ from app.utils import cache_key_for_service_template_counter
 from app.v2.errors import BadRequestError
 from tests.app.conftest import sample_api_key as create_api_key
 
+from tests.app.db import create_service, create_template
+
 
 def test_create_content_for_notification_passes(sample_email_template):
     template = Template.query.get(sample_email_template.id)
@@ -477,6 +481,41 @@ def test_persist_email_notification_stores_normalised_email(
     assert persisted_notification.normalised_to == expected_recipient_normalised
 
 
+@pytest.mark.parametrize(
+    "service_permissions, template_postage, expected_postage",
+    [
+        ([LETTER_TYPE], "first", "second"),
+        ([LETTER_TYPE, CHOOSE_POSTAGE], "first", "first"),
+        ([LETTER_TYPE, CHOOSE_POSTAGE], None, "second"),
+    ]
+)
+def test_persist_letter_notification_finds_correct_postage(
+    mocker,
+    notify_db,
+    notify_db_session,
+    service_permissions,
+    template_postage,
+    expected_postage
+):
+    service = create_service(service_permissions=service_permissions, postage="second")
+    api_key = create_api_key(notify_db, notify_db_session, service=service)
+    template = create_template(service, template_type=LETTER_TYPE, postage=template_postage)
+    mocker.patch('app.dao.templates_dao.dao_get_template_by_id', return_value=template)
+    persist_notification(
+        template_id=template.id,
+        template_version=template.version,
+        recipient="Jane Doe, 10 Downing Street, London",
+        service=service,
+        personalisation=None,
+        notification_type=LETTER_TYPE,
+        api_key_id=api_key.id,
+        key_type=api_key.key_type,
+    )
+    persisted_notification = Notification.query.all()[0]
+
+    assert persisted_notification.postage == expected_postage
+
+
 @pytest.mark.parametrize('utc_time, day_in_key', [
     ('2016-01-01 23:00:00', '2016-01-01'),
     ('2016-06-01 22:59:00', '2016-06-01'),
diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py
index 91c569a00..429172011 100644
--- a/tests/app/template/test_rest.py
+++ b/tests/app/template/test_rest.py
@@ -17,7 +17,6 @@ from app.models import (
     EMAIL_TYPE,
     LETTER_TYPE,
     SMS_TYPE,
-    CHOOSE_POSTAGE,
     Template,
     TemplateHistory
 )

From 1b30e867072d2667aec1d9438a9fe00e37494f53 Mon Sep 17 00:00:00 2001
From: Pea Tyczynska <pea.tyczynska@digital.cabinet-office.gov.uk>
Date: Mon, 17 Dec 2018 17:50:39 +0000
Subject: [PATCH 03/24] Update v2 template schema to include postage

---
 app/v2/template/template_schemas.py | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/app/v2/template/template_schemas.py b/app/v2/template/template_schemas.py
index b1b1f4820..ebd0b8342 100644
--- a/app/v2/template/template_schemas.py
+++ b/app/v2/template/template_schemas.py
@@ -37,6 +37,7 @@ get_template_by_id_response = {
         "body": {"type": "string"},
         "subject": {"type": ["string", "null"]},
         "name": {"type": "string"},
+        "postage": {"type": "string"}
     },
     "required": ["id", "type", "created_at", "updated_at", "version", "created_by", "body", "name"],
 }
@@ -63,7 +64,8 @@ post_template_preview_response = {
         "type": {"enum": TEMPLATE_TYPES},
         "version": {"type": "integer"},
         "body": {"type": "string"},
-        "subject": {"type": ["string", "null"]}
+        "subject": {"type": ["string", "null"]},
+        "postage": {"type": "string"}
     },
     "required": ["id", "type", "version", "body"]
 }
@@ -77,5 +79,6 @@ def create_post_template_preview_response(template, template_object):
         "type": template.template_type,
         "version": template.version,
         "body": str(template_object),
-        "subject": subject
+        "subject": subject,
+        "postage": template.postage
     }

From 4929a6ac083984517cf8461818e2e3d81d9cc2bb Mon Sep 17 00:00:00 2001
From: Pea Tyczynska <pea.tyczynska@digital.cabinet-office.gov.uk>
Date: Tue, 18 Dec 2018 18:21:03 +0000
Subject: [PATCH 04/24] Include postage in checking if template changed

---
 app/template/rest.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/template/rest.py b/app/template/rest.py
index 9dc32c94f..79b1d7d8b 100644
--- a/app/template/rest.py
+++ b/app/template/rest.py
@@ -203,7 +203,7 @@ def get_template_versions(service_id, template_id):
 def _template_has_not_changed(current_data, updated_template):
     return all(
         current_data[key] == updated_template[key]
-        for key in ('name', 'content', 'subject', 'archived', 'process_type')
+        for key in ('name', 'content', 'subject', 'archived', 'process_type', 'postage')
     )
 
 

From 686c58acee0eeb65186be4c229caf5c1715710df Mon Sep 17 00:00:00 2001
From: Pea Tyczynska <pea.tyczynska@digital.cabinet-office.gov.uk>
Date: Mon, 24 Dec 2018 11:27:29 +0000
Subject: [PATCH 05/24] Test post letter request sets notification postage
 correctly

---
 tests/app/conftest.py                         |  2 ++
 .../test_post_letter_notifications.py         | 24 ++++++++++++++-----
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/tests/app/conftest.py b/tests/app/conftest.py
index 15f76846f..6b04e83fe 100644
--- a/tests/app/conftest.py
+++ b/tests/app/conftest.py
@@ -158,6 +158,7 @@ def sample_service(
     email_from=None,
     permissions=None,
     research_mode=None,
+    postage="second",
 ):
     if user is None:
         user = create_user()
@@ -170,6 +171,7 @@ def sample_service(
         'restricted': restricted,
         'email_from': email_from,
         'created_by': user,
+        "postage": postage,
     }
     service = Service.query.filter_by(name=service_name).first()
     if not service:
diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py
index 4b42af6dc..734928088 100644
--- a/tests/app/v2/notifications/test_post_letter_notifications.py
+++ b/tests/app/v2/notifications/test_post_letter_notifications.py
@@ -20,6 +20,7 @@ from app.models import (
     NOTIFICATION_DELIVERED,
     NOTIFICATION_PENDING_VIRUS_CHECK,
     SMS_TYPE,
+    CHOOSE_POSTAGE
 )
 from app.schema_validation import validate
 from app.v2.errors import RateLimitError
@@ -95,12 +96,23 @@ def test_post_letter_notification_returns_201(client, sample_letter_template, mo
     mock.assert_called_once_with([str(notification.id)], queue=QueueNames.CREATE_LETTERS_PDF)
 
 
-@pytest.mark.parametrize('postage', ['first', 'second'])
-def test_post_letter_notification_sets_postage(client, sample_letter_template, mocker, postage):
-    sample_letter_template.service.postage = postage
+@pytest.mark.parametrize('service_permissions, service_postage, template_postage, expected_postage', [
+    ([LETTER_TYPE], "second", "first", "second"),
+    ([LETTER_TYPE], "first", "second", "first"),
+    ([LETTER_TYPE], "first", None, "first"),
+    ([LETTER_TYPE, CHOOSE_POSTAGE], "second", "first", "first"),
+    ([LETTER_TYPE, CHOOSE_POSTAGE], "second", None, "second"),
+    ([LETTER_TYPE, CHOOSE_POSTAGE], "second", "second", "second"),
+    ([LETTER_TYPE, CHOOSE_POSTAGE], "first", "second", "second"),
+])
+def test_post_letter_notification_sets_postage(
+    client, notify_db_session, mocker, service_permissions, service_postage, template_postage, expected_postage
+):
+    service = create_service(service_permissions=service_permissions, postage=service_postage)
+    template = create_template(service, template_type="letter", postage=template_postage)
     mocker.patch('app.celery.tasks.letters_pdf_tasks.create_letters_pdf.apply_async')
     data = {
-        'template_id': str(sample_letter_template.id),
+        'template_id': str(template.id),
         'personalisation': {
             'address_line_1': 'Her Royal Highness Queen Elizabeth II',
             'address_line_2': 'Buckingham Palace',
@@ -110,11 +122,11 @@ def test_post_letter_notification_sets_postage(client, sample_letter_template, m
         }
     }
 
-    resp_json = letter_request(client, data, service_id=sample_letter_template.service_id)
+    resp_json = letter_request(client, data, service_id=service.id)
 
     assert validate(resp_json, post_letter_response) == resp_json
     notification = Notification.query.one()
-    assert notification.postage == postage
+    assert notification.postage == expected_postage
 
 
 @pytest.mark.parametrize('env', [

From fb1ca9b20d636f73a4a37f59fec88488614ea617 Mon Sep 17 00:00:00 2001
From: Pea Tyczynska <pea.tyczynska@digital.cabinet-office.gov.uk>
Date: Mon, 24 Dec 2018 12:24:17 +0000
Subject: [PATCH 06/24] Test postage setting on happy path for create and
 update template

---
 tests/app/template/test_rest.py | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py
index 429172011..64c51becc 100644
--- a/tests/app/template/test_rest.py
+++ b/tests/app/template/test_rest.py
@@ -18,7 +18,8 @@ from app.models import (
     LETTER_TYPE,
     SMS_TYPE,
     Template,
-    TemplateHistory
+    TemplateHistory,
+    CHOOSE_POSTAGE
 )
 from app.dao.templates_dao import dao_get_template_by_id, dao_redact_template
 
@@ -43,7 +44,7 @@ from tests.conftest import set_config_values
 def test_should_create_a_new_template_for_a_service(
     client, sample_user, template_type, subject
 ):
-    service = create_service(service_permissions=[template_type])
+    service = create_service(service_permissions=[template_type, CHOOSE_POSTAGE])
     data = {
         'name': 'my template',
         'template_type': template_type,
@@ -53,6 +54,8 @@ def test_should_create_a_new_template_for_a_service(
     }
     if subject:
         data.update({'subject': subject})
+    if template_type == LETTER_TYPE:
+        data.update({'postage': 'first'})
     data = json.dumps(data)
     auth_header = create_authorization_header()
 
@@ -76,6 +79,11 @@ def test_should_create_a_new_template_for_a_service(
     else:
         assert not json_resp['data']['subject']
 
+    if template_type == LETTER_TYPE:
+        assert json_resp['data']['postage'] == 'first'
+    else:
+        assert not json_resp['data']['postage']
+
     template = Template.query.get(json_resp['data']['id'])
     from app.schemas import template_schema
     assert sorted(json_resp['data']) == sorted(template_schema.dump(template).data)
@@ -356,16 +364,19 @@ def test_must_have_a_subject_on_an_email_or_letter_template(client, sample_user,
     assert json_resp['errors'][0]["message"] == 'subject is a required property'
 
 
-def test_update_should_update_a_template(client, sample_user, sample_template):
+def test_update_should_update_a_template(client, sample_user):
+    service = create_service(service_permissions=[LETTER_TYPE, CHOOSE_POSTAGE])
+    template = create_template(service, template_type="letter", postage="second")
     data = {
-        'content': 'my template has new content <script type="text/javascript">alert("foo")</script>',
-        'created_by': str(sample_user.id)
+        'content': 'my template has new content, swell!',
+        'created_by': str(sample_user.id),
+        'postage': 'first'
     }
     data = json.dumps(data)
     auth_header = create_authorization_header()
 
     update_response = client.post(
-        '/service/{}/template/{}'.format(sample_template.service_id, sample_template.id),
+        '/service/{}/template/{}'.format(service.id, template.id),
         headers=[('Content-Type', 'application/json'), auth_header],
         data=data
     )
@@ -373,10 +384,11 @@ def test_update_should_update_a_template(client, sample_user, sample_template):
     assert update_response.status_code == 200
     update_json_resp = json.loads(update_response.get_data(as_text=True))
     assert update_json_resp['data']['content'] == (
-        'my template has new content <script type="text/javascript">alert("foo")</script>'
+        'my template has new content, swell!'
     )
-    assert update_json_resp['data']['name'] == sample_template.name
-    assert update_json_resp['data']['template_type'] == sample_template.template_type
+    assert update_json_resp['data']['postage'] == 'first'
+    assert update_json_resp['data']['name'] == template.name
+    assert update_json_resp['data']['template_type'] == template.template_type
     assert update_json_resp['data']['version'] == 2
 
 

From 80454579eeffbac5a1c621f0811c467fbd23bf90 Mon Sep 17 00:00:00 2001
From: Leo Hemsted <leo.hemsted@digital.cabinet-office.gov.uk>
Date: Wed, 2 Jan 2019 14:26:48 +0000
Subject: [PATCH 07/24] fix multi worker exit script

previously the script would:

try and SIGTERM each celery process every second for the 9 second
timeout, and then SIGKILL every second after, with no upper bound.

This commit changes this to:
* SIGTERM each process once.
* Wait nine seconds (checking if the pid files are still present each
  second)
* SIGKILL any remaining processes once.
* exit
---
 scripts/run_multi_worker_app_paas.sh | 42 +++++++++++++---------------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/scripts/run_multi_worker_app_paas.sh b/scripts/run_multi_worker_app_paas.sh
index 3965ee577..5ddc933ec 100755
--- a/scripts/run_multi_worker_app_paas.sh
+++ b/scripts/run_multi_worker_app_paas.sh
@@ -39,13 +39,15 @@ log_stream_name = {hostname}
 EOF
 }
 
-# For every PID, check if it's still running
-# if it is, send the sigterm
+# For every PID, check if it's still running. if it is, send the sigterm. then wait 9 seconds before sending sigkill
 function on_exit {
+  echo "multi worker app exiting"
   wait_time=0
-  while true; do
-    # refresh pids to account for the case that
-    # some workers may have terminated but others not
+
+  send_signal_to_celery_processes TERM
+
+  # check if the apps are still running every second
+  while [[ "$wait_time" -le "$TERMINATE_TIMEOUT" ]]; do
     get_celery_pids
 
     # look here for explanation regarding this syntax:
@@ -53,28 +55,14 @@ function on_exit {
     PROCESS_COUNT="${#APP_PIDS[@]}"
     if [[ "${PROCESS_COUNT}" -eq "0" ]]; then
         echo "No more .pid files found, exiting"
-        break
+        return 0
     fi
 
-    echo "Terminating celery processes with pids "${APP_PIDS}
-    for APP_PID in ${APP_PIDS}; do
-      # if TERMINATE_TIMEOUT is reached, send SIGKILL
-      if [[ "$wait_time" -ge "$TERMINATE_TIMEOUT" ]]; then
-        echo "Timeout reached, killing process with pid ${APP_PID}"
-        kill -9 ${APP_PID} || true
-        continue
-      else
-        echo "Timeout not reached yet, checking " ${APP_PID}
-        # else, if process is still running send SIGTERM
-        if [[ $(kill -0 ${APP_PID} 2&>/dev/null) ]]; then
-          echo "Terminating celery process with pid ${APP_PID}"
-          kill ${APP_PID} || true
-        fi
-      fi
-    done
     let wait_time=wait_time+1
     sleep 1
   done
+
+  send_signal_to_celery_processes KILL
 }
 
 function get_celery_pids {
@@ -85,6 +73,16 @@ function get_celery_pids {
   fi
 }
 
+function send_signal_to_celery_processes {
+  # refresh pids to account for the case that some workers may have terminated but others not
+  get_celery_pids
+  # send signal to all remaining apps
+  for APP_PID in ${APP_PIDS}; do
+    echo "Sending signal ${1} to process with pid ${APP_PID}"
+    kill -s ${1} ${APP_PID} || true
+  done
+}
+
 function start_application {
   eval "$@"
   get_celery_pids

From 98e501c4c4e09c67e1a6fe6c7261385dfd3e9b0b Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Wed, 2 Jan 2019 15:45:06 +0000
Subject: [PATCH 08/24] Bump utils to 30.7.2

---
 requirements-app.txt |  2 +-
 requirements.txt     | 24 ++++++++++++------------
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/requirements-app.txt b/requirements-app.txt
index b0fa41bf3..0941e7f4b 100644
--- a/requirements-app.txt
+++ b/requirements-app.txt
@@ -29,6 +29,6 @@ awscli-cwlogs>=1.4,<1.5
 # Putting upgrade on hold due to v1.0.0 using sha512 instead of sha1 by default
 itsdangerous==0.24  # pyup: <1.0.0
 
-git+https://github.com/alphagov/notifications-utils.git@30.7.1#egg=notifications-utils==30.7.1
+git+https://github.com/alphagov/notifications-utils.git@30.7.2#egg=notifications-utils==30.7.2
 
 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3
diff --git a/requirements.txt b/requirements.txt
index 1df4c529f..09bfcfe90 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -31,22 +31,22 @@ awscli-cwlogs>=1.4,<1.5
 # Putting upgrade on hold due to v1.0.0 using sha512 instead of sha1 by default
 itsdangerous==0.24  # pyup: <1.0.0
 
-git+https://github.com/alphagov/notifications-utils.git@30.7.1#egg=notifications-utils==30.7.1
+git+https://github.com/alphagov/notifications-utils.git@30.7.2#egg=notifications-utils==30.7.2
 
 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3
 
 ## The following requirements were added by pip freeze:
-alembic==1.0.3
+alembic==1.0.5
 amqp==1.4.9
 anyjson==0.3.3
 attrs==18.2.0
-awscli==1.16.62
-bcrypt==3.1.4
+awscli==1.16.81
+bcrypt==3.1.5
 billiard==3.3.0.23
 bleach==2.1.3
 boto3==1.6.16
-botocore==1.12.52
-certifi==2018.10.15
+botocore==1.12.71
+certifi==2018.11.29
 chardet==3.0.4
 Click==7.0
 colorama==0.3.9
@@ -55,7 +55,7 @@ Flask-Redis==0.3.0
 future==0.17.1
 greenlet==0.4.15
 html5lib==1.0.1
-idna==2.7
+idna==2.8
 Jinja2==2.10
 jmespath==0.9.3
 kombu==3.0.37
@@ -65,20 +65,20 @@ mistune==0.8.3
 monotonic==1.5
 orderedset==2.0.1
 phonenumbers==8.9.4
-pyasn1==0.4.4
+pyasn1==0.4.5
 pycparser==2.19
 PyPDF2==1.26.0
-pyrsistent==0.14.7
+pyrsistent==0.14.8
 python-dateutil==2.7.5
 python-editor==1.0.3
 python-json-logger==0.1.8
 pytz==2018.7
 PyYAML==3.12
-redis==2.10.6
-requests==2.20.1
+redis==3.0.1
+requests==2.21.0
 rsa==3.4.2
 s3transfer==0.1.13
-six==1.11.0
+six==1.12.0
 smartypants==2.0.1
 statsd==3.2.2
 urllib3==1.24.1

From c12594949f3c7f9fa8dbbe66a557f22c244a3af2 Mon Sep 17 00:00:00 2001
From: Pea Tyczynska <pea.tyczynska@digital.cabinet-office.gov.uk>
Date: Fri, 28 Dec 2018 16:15:12 +0000
Subject: [PATCH 09/24] Refactor service_factory

---
 tests/app/conftest.py | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/tests/app/conftest.py b/tests/app/conftest.py
index 6b04e83fe..38b483f0e 100644
--- a/tests/app/conftest.py
+++ b/tests/app/conftest.py
@@ -76,20 +76,26 @@ def service_factory(notify_db, notify_db_session):
                 user = create_user()
             if not email_from:
                 email_from = service_name
-            service = sample_service(notify_db, notify_db_session, service_name, user, email_from=email_from)
+            service = Service.query.filter_by(name=service_name).first()
+            if not service:
+                service = create_service(
+                    email_from=email_from,
+                    service_name=service_name,
+                    service_permissions=None,
+                    user=user,
+                )
             if template_type == 'email':
-                sample_template(
-                    notify_db,
-                    notify_db_session,
+                create_template(
+                    service,
+                    template_name="Template Name",
                     template_type=template_type,
-                    subject_line=service.email_from,
-                    service=service
+                    subject=service.email_from,
                 )
             else:
-                sample_template(
-                    notify_db,
-                    notify_db_session,
-                    service=service
+                create_template(
+                    service,
+                    template_name="Template Name",
+                    template_type='sms',
                 )
             return service
 

From 923703120b3dc5a21992717443cc35053d6b51ba Mon Sep 17 00:00:00 2001
From: Pea Tyczynska <pea.tyczynska@digital.cabinet-office.gov.uk>
Date: Mon, 31 Dec 2018 13:01:24 +0000
Subject: [PATCH 10/24] Check if test service exists before it gets created

---
 tests/app/conftest.py | 15 +++++++--------
 tests/app/db.py       | 28 +++++++++++++++-------------
 2 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/tests/app/conftest.py b/tests/app/conftest.py
index 38b483f0e..e77deb4a1 100644
--- a/tests/app/conftest.py
+++ b/tests/app/conftest.py
@@ -76,14 +76,13 @@ def service_factory(notify_db, notify_db_session):
                 user = create_user()
             if not email_from:
                 email_from = service_name
-            service = Service.query.filter_by(name=service_name).first()
-            if not service:
-                service = create_service(
-                    email_from=email_from,
-                    service_name=service_name,
-                    service_permissions=None,
-                    user=user,
-                )
+
+            service = create_service(
+                email_from=email_from,
+                service_name=service_name,
+                service_permissions=None,
+                user=user,
+            )
             if template_type == 'email':
                 create_template(
                     service,
diff --git a/tests/app/db.py b/tests/app/db.py
index 02a2df85c..3783026e6 100644
--- a/tests/app/db.py
+++ b/tests/app/db.py
@@ -83,21 +83,23 @@ def create_service(
         organisation_type='central',
         postage='second'
 ):
-    service = Service(
-        name=service_name,
-        message_limit=message_limit,
-        restricted=restricted,
-        email_from=email_from if email_from else service_name.lower().replace(' ', '.'),
-        created_by=user or create_user(email='{}@digital.cabinet-office.gov.uk'.format(uuid.uuid4())),
-        prefix_sms=prefix_sms,
-        organisation_type=organisation_type,
-        postage=postage
-    )
+    service = Service.query.filter_by(name=service_name).first()
+    if not service:
+        service = Service(
+            name=service_name,
+            message_limit=message_limit,
+            restricted=restricted,
+            email_from=email_from if email_from else service_name.lower().replace(' ', '.'),
+            created_by=user or create_user(email='{}@digital.cabinet-office.gov.uk'.format(uuid.uuid4())),
+            prefix_sms=prefix_sms,
+            organisation_type=organisation_type,
+            postage=postage
+        )
 
-    dao_create_service(service, service.created_by, service_id, service_permissions=service_permissions)
+        dao_create_service(service, service.created_by, service_id, service_permissions=service_permissions)
 
-    service.active = active
-    service.research_mode = research_mode
+        service.active = active
+        service.research_mode = research_mode
 
     return service
 

From 3306b9fc97e44b91caad8086e831e85f047bbb4f Mon Sep 17 00:00:00 2001
From: Pea Tyczynska <pea.tyczynska@digital.cabinet-office.gov.uk>
Date: Mon, 31 Dec 2018 14:18:52 +0000
Subject: [PATCH 11/24] use conditional in create_service to establish user

---
 tests/app/db.py | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/app/db.py b/tests/app/db.py
index 3783026e6..e0ca9bddd 100644
--- a/tests/app/db.py
+++ b/tests/app/db.py
@@ -90,12 +90,11 @@ def create_service(
             message_limit=message_limit,
             restricted=restricted,
             email_from=email_from if email_from else service_name.lower().replace(' ', '.'),
-            created_by=user or create_user(email='{}@digital.cabinet-office.gov.uk'.format(uuid.uuid4())),
+            created_by=user if user else create_user(email='{}@digital.cabinet-office.gov.uk'.format(uuid.uuid4())),
             prefix_sms=prefix_sms,
             organisation_type=organisation_type,
             postage=postage
         )
-
         dao_create_service(service, service.created_by, service_id, service_permissions=service_permissions)
 
         service.active = active

From 95115e7ae6ae3eefcb49196e015e728ee3e02409 Mon Sep 17 00:00:00 2001
From: Pea Tyczynska <pea.tyczynska@digital.cabinet-office.gov.uk>
Date: Mon, 31 Dec 2018 14:34:02 +0000
Subject: [PATCH 12/24] Use create_service instead of sample_service when
 creating service permission for tests

---
 tests/app/conftest.py | 2 +-
 tests/app/db.py       | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/tests/app/conftest.py b/tests/app/conftest.py
index e77deb4a1..ff817501e 100644
--- a/tests/app/conftest.py
+++ b/tests/app/conftest.py
@@ -802,7 +802,7 @@ def sample_user_service_permission(
     if user is None:
         user = create_user()
     if service is None:
-        service = sample_service(notify_db, notify_db_session, user=user)
+        service = create_service(user=user)
     data = {
         'user': user,
         'service': service,
diff --git a/tests/app/db.py b/tests/app/db.py
index e0ca9bddd..ee17c48cd 100644
--- a/tests/app/db.py
+++ b/tests/app/db.py
@@ -17,7 +17,7 @@ from app.dao.service_data_retention_dao import insert_service_data_retention
 from app.dao.service_inbound_api_dao import save_service_inbound_api
 from app.dao.service_permissions_dao import dao_add_service_permission
 from app.dao.service_sms_sender_dao import update_existing_sms_sender_with_inbound_number, dao_update_service_sms_sender
-from app.dao.services_dao import dao_create_service
+from app.dao.services_dao import dao_create_service, dao_add_user_to_service
 from app.dao.templates_dao import dao_create_template, dao_update_template
 from app.dao.users_dao import save_model_user
 from app.models import (
@@ -99,6 +99,9 @@ def create_service(
 
         service.active = active
         service.research_mode = research_mode
+    else:
+        if user not in service.users:
+            dao_add_user_to_service(service, user)
 
     return service
 

From e8ce669b723e8c3a11ef322739b2278dc2fe4895 Mon Sep 17 00:00:00 2001
From: Pea Tyczynska <pea.tyczynska@digital.cabinet-office.gov.uk>
Date: Mon, 31 Dec 2018 15:04:01 +0000
Subject: [PATCH 13/24] Use create_service instead of sample_service when
 creating sample_email_template for tests

---
 tests/app/conftest.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/app/conftest.py b/tests/app/conftest.py
index ff817501e..bca4f3776 100644
--- a/tests/app/conftest.py
+++ b/tests/app/conftest.py
@@ -295,7 +295,7 @@ def sample_email_template(
     if user is None:
         user = create_user()
     if service is None:
-        service = sample_service(notify_db, notify_db_session, permissions=permissions)
+        service = create_service(user=user, service_permissions=permissions)
     data = {
         'name': template_name,
         'template_type': template_type,

From 0bcf13d85cf1f5a22cc08810e231de4df803d6d6 Mon Sep 17 00:00:00 2001
From: Pea Tyczynska <pea.tyczynska@digital.cabinet-office.gov.uk>
Date: Mon, 31 Dec 2018 15:16:00 +0000
Subject: [PATCH 14/24] sample_api_key uses create_service instead of
 sample_service

---
 tests/app/conftest.py | 2 +-
 tests/app/db.py       | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/app/conftest.py b/tests/app/conftest.py
index bca4f3776..cadced70d 100644
--- a/tests/app/conftest.py
+++ b/tests/app/conftest.py
@@ -350,7 +350,7 @@ def sample_api_key(notify_db,
                    key_type=KEY_TYPE_NORMAL,
                    name=None):
     if service is None:
-        service = sample_service(notify_db, notify_db_session)
+        service = create_service()
     data = {'service': service, 'name': name or uuid.uuid4(), 'created_by': service.created_by, 'key_type': key_type}
     api_key = ApiKey(**data)
     save_model_api_key(api_key)
diff --git a/tests/app/db.py b/tests/app/db.py
index ee17c48cd..d6d4b9553 100644
--- a/tests/app/db.py
+++ b/tests/app/db.py
@@ -100,7 +100,7 @@ def create_service(
         service.active = active
         service.research_mode = research_mode
     else:
-        if user not in service.users:
+        if user and user not in service.users:
             dao_add_user_to_service(service, user)
 
     return service

From a3310c2da6d7f7bb8cfcc6e2bf439b6bebb1cefd Mon Sep 17 00:00:00 2001
From: Pea Tyczynska <pea.tyczynska@digital.cabinet-office.gov.uk>
Date: Mon, 31 Dec 2018 15:20:30 +0000
Subject: [PATCH 15/24] sample_job uses create_service instead of
 sample_service

---
 tests/app/conftest.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/app/conftest.py b/tests/app/conftest.py
index cadced70d..48fe278d2 100644
--- a/tests/app/conftest.py
+++ b/tests/app/conftest.py
@@ -382,7 +382,7 @@ def sample_job(
     archived=False
 ):
     if service is None:
-        service = sample_service(notify_db, notify_db_session)
+        service = create_service()
     if template is None:
         template = sample_template(notify_db, notify_db_session,
                                    service=service)

From d367daaf6edc684637dfb088015baba4e42cbd12 Mon Sep 17 00:00:00 2001
From: Pea Tyczynska <pea.tyczynska@digital.cabinet-office.gov.uk>
Date: Mon, 31 Dec 2018 15:36:25 +0000
Subject: [PATCH 16/24] Some more conftest fixtures use create_service instead
 of sample_service

---
 tests/app/conftest.py | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/app/conftest.py b/tests/app/conftest.py
index 48fe278d2..8ecb0ae20 100644
--- a/tests/app/conftest.py
+++ b/tests/app/conftest.py
@@ -442,7 +442,7 @@ def sample_email_job(notify_db,
                      service=None,
                      template=None):
     if service is None:
-        service = sample_service(notify_db, notify_db_session)
+        service = create_service()
     if template is None:
         template = sample_email_template(
             notify_db,
@@ -550,7 +550,7 @@ def sample_notification(
     if created_at is None:
         created_at = datetime.utcnow()
     if service is None:
-        service = sample_service(notify_db, notify_db_session)
+        service = create_service()
     if template is None:
         template = sample_template(notify_db, notify_db_session, service=service)
 
@@ -639,7 +639,7 @@ def sample_notification_with_api_key(notify_db, notify_db_session):
 @pytest.fixture(scope='function')
 def sample_email_notification(notify_db, notify_db_session):
     created_at = datetime.utcnow()
-    service = sample_service(notify_db, notify_db_session)
+    service = create_service()
     template = sample_email_template(notify_db, notify_db_session, service=service)
     job = sample_job(notify_db, notify_db_session, service=service, template=template)
 
@@ -741,7 +741,7 @@ def sample_invited_user(notify_db,
                         to_email_address=None):
 
     if service is None:
-        service = sample_service(notify_db, notify_db_session)
+        service = create_service()
     if to_email_address is None:
         to_email_address = 'invited_user@digital.gov.uk'
 
@@ -781,7 +781,7 @@ def sample_permission(notify_db,
         'permission': permission
     }
     if service is None:
-        service = sample_service(notify_db, notify_db_session)
+        service = create_service()
     if service:
         data['service'] = service
     p_model = Permission.query.filter_by(

From 154257027f8c69dd7e6df5c2fe8b272190d1259e Mon Sep 17 00:00:00 2001
From: Pea Tyczynska <pea.tyczynska@digital.cabinet-office.gov.uk>
Date: Mon, 31 Dec 2018 15:53:56 +0000
Subject: [PATCH 17/24] Nothing in conftest uses sample_service now :)

---
 tests/app/conftest.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/app/conftest.py b/tests/app/conftest.py
index 8ecb0ae20..abcb0f64a 100644
--- a/tests/app/conftest.py
+++ b/tests/app/conftest.py
@@ -1036,7 +1036,7 @@ def notify_service(notify_db, notify_db_session):
 @pytest.fixture(scope='function')
 def sample_service_whitelist(notify_db, notify_db_session, service=None, email_address=None, mobile_number=None):
     if service is None:
-        service = sample_service(notify_db, notify_db_session)
+        service = create_service()
 
     if email_address:
         whitelisted_user = ServiceWhitelist.from_string(service.id, EMAIL_TYPE, email_address)

From cde30de100bcf86b8558f3c4ea0ac8330f002783 Mon Sep 17 00:00:00 2001
From: Pea Tyczynska <pea.tyczynska@digital.cabinet-office.gov.uk>
Date: Mon, 31 Dec 2018 16:14:09 +0000
Subject: [PATCH 18/24] Use create_template instead of sample_template in
 sample_notification

---
 tests/app/conftest.py                                           | 2 +-
 .../notification_dao/test_notification_dao_template_usage.py    | 2 +-
 tests/app/notifications/test_rest.py                            | 2 +-
 tests/app/template_statistics/test_rest.py                      | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/app/conftest.py b/tests/app/conftest.py
index abcb0f64a..306eb2637 100644
--- a/tests/app/conftest.py
+++ b/tests/app/conftest.py
@@ -552,7 +552,7 @@ def sample_notification(
     if service is None:
         service = create_service()
     if template is None:
-        template = sample_template(notify_db, notify_db_session, service=service)
+        template = create_template(service=service)
 
     if job is None and api_key is None:
         # we didn't specify in test - lets create it
diff --git a/tests/app/dao/notification_dao/test_notification_dao_template_usage.py b/tests/app/dao/notification_dao/test_notification_dao_template_usage.py
index 76800694f..88aaa783d 100644
--- a/tests/app/dao/notification_dao/test_notification_dao_template_usage.py
+++ b/tests/app/dao/notification_dao/test_notification_dao_template_usage.py
@@ -22,7 +22,7 @@ from tests.app.db import (
 
 def test_last_template_usage_should_get_right_data(sample_notification):
     results = dao_get_last_template_usage(sample_notification.template_id, 'sms', sample_notification.service_id)
-    assert results.template.name == 'Template Name'
+    assert results.template.name == 'sms Template Name'
     assert results.template.template_type == 'sms'
     assert results.created_at == sample_notification.created_at
     assert results.template_id == sample_notification.template_id
diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py
index 0c9051d90..a035500cb 100644
--- a/tests/app/notifications/test_rest.py
+++ b/tests/app/notifications/test_rest.py
@@ -153,7 +153,7 @@ def test_get_all_notifications(client, sample_notification):
 
     assert notifications['notifications'][0]['to'] == '+447700900855'
     assert notifications['notifications'][0]['service'] == str(sample_notification.service_id)
-    assert notifications['notifications'][0]['body'] == "This is a template:\nwith a newline"
+    assert notifications['notifications'][0]['body'] == 'Dear Sir/Madam, Hello. Yours Truly, The Government.'
 
 
 def test_normal_api_key_returns_notifications_created_from_jobs_and_from_api(
diff --git a/tests/app/template_statistics/test_rest.py b/tests/app/template_statistics/test_rest.py
index b9559e134..1a37cc6e7 100644
--- a/tests/app/template_statistics/test_rest.py
+++ b/tests/app/template_statistics/test_rest.py
@@ -59,7 +59,7 @@ def test_get_template_statistics_for_service_by_day_returns_template_info(admin_
 
     assert json_resp['data'][0]['count'] == 1
     assert json_resp['data'][0]['template_id'] == str(sample_notification.template_id)
-    assert json_resp['data'][0]['template_name'] == 'Template Name'
+    assert json_resp['data'][0]['template_name'] == 'sms Template Name'
     assert json_resp['data'][0]['template_type'] == 'sms'
     assert json_resp['data'][0]['is_precompiled_letter'] is False
 

From f79c0b03e7ca00c02f8a4d9f417e673883f8cc98 Mon Sep 17 00:00:00 2001
From: Pea Tyczynska <pea.tyczynska@digital.cabinet-office.gov.uk>
Date: Mon, 31 Dec 2018 16:19:41 +0000
Subject: [PATCH 19/24] Sample job uses create_template instead of sample
 template

---
 tests/app/conftest.py | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/app/conftest.py b/tests/app/conftest.py
index 306eb2637..407372d64 100644
--- a/tests/app/conftest.py
+++ b/tests/app/conftest.py
@@ -384,8 +384,7 @@ def sample_job(
     if service is None:
         service = create_service()
     if template is None:
-        template = sample_template(notify_db, notify_db_session,
-                                   service=service)
+        template = create_template(service=service)
     data = {
         'id': uuid.uuid4(),
         'service_id': service.id,

From eea324a19d0e1b10a44700977ddadaa8a6724d85 Mon Sep 17 00:00:00 2001
From: Pea Tyczynska <pea.tyczynska@digital.cabinet-office.gov.uk>
Date: Wed, 2 Jan 2019 17:15:27 +0000
Subject: [PATCH 20/24] Add flag on create_service to decide whether we should
 also check if service exists

---
 tests/app/conftest.py | 28 +++++++++++++++-------------
 tests/app/db.py       |  8 +++++---
 2 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/tests/app/conftest.py b/tests/app/conftest.py
index 407372d64..5b6aa2234 100644
--- a/tests/app/conftest.py
+++ b/tests/app/conftest.py
@@ -82,6 +82,7 @@ def service_factory(notify_db, notify_db_session):
                 service_name=service_name,
                 service_permissions=None,
                 user=user,
+                check_if_service_exists=True,
             )
             if template_type == 'email':
                 create_template(
@@ -200,7 +201,8 @@ def sample_service(
 def _sample_service_full_permissions(notify_db_session):
     service = create_service(
         service_name="sample service full permissions",
-        service_permissions=set(SERVICE_PERMISSION_TYPES)
+        service_permissions=set(SERVICE_PERMISSION_TYPES),
+        check_if_service_exists=True
     )
     create_inbound_number('12345', service_id=service.id)
     return service
@@ -233,7 +235,7 @@ def sample_template(
     if service is None:
         service = Service.query.filter_by(name='Sample service').first()
         if not service:
-            service = create_service(service_permissions=permissions)
+            service = create_service(service_permissions=permissions, check_if_service_exists=True)
     if created_by is None:
         created_by = create_user()
 
@@ -295,7 +297,7 @@ def sample_email_template(
     if user is None:
         user = create_user()
     if service is None:
-        service = create_service(user=user, service_permissions=permissions)
+        service = create_service(user=user, service_permissions=permissions, check_if_service_exists=True)
     data = {
         'name': template_name,
         'template_type': template_type,
@@ -350,7 +352,7 @@ def sample_api_key(notify_db,
                    key_type=KEY_TYPE_NORMAL,
                    name=None):
     if service is None:
-        service = create_service()
+        service = create_service(check_if_service_exists=True)
     data = {'service': service, 'name': name or uuid.uuid4(), 'created_by': service.created_by, 'key_type': key_type}
     api_key = ApiKey(**data)
     save_model_api_key(api_key)
@@ -382,7 +384,7 @@ def sample_job(
     archived=False
 ):
     if service is None:
-        service = create_service()
+        service = create_service(check_if_service_exists=True)
     if template is None:
         template = create_template(service=service)
     data = {
@@ -441,7 +443,7 @@ def sample_email_job(notify_db,
                      service=None,
                      template=None):
     if service is None:
-        service = create_service()
+        service = create_service(check_if_service_exists=True)
     if template is None:
         template = sample_email_template(
             notify_db,
@@ -549,7 +551,7 @@ def sample_notification(
     if created_at is None:
         created_at = datetime.utcnow()
     if service is None:
-        service = create_service()
+        service = create_service(check_if_service_exists=True)
     if template is None:
         template = create_template(service=service)
 
@@ -638,7 +640,7 @@ def sample_notification_with_api_key(notify_db, notify_db_session):
 @pytest.fixture(scope='function')
 def sample_email_notification(notify_db, notify_db_session):
     created_at = datetime.utcnow()
-    service = create_service()
+    service = create_service(check_if_service_exists=True)
     template = sample_email_template(notify_db, notify_db_session, service=service)
     job = sample_job(notify_db, notify_db_session, service=service, template=template)
 
@@ -740,7 +742,7 @@ def sample_invited_user(notify_db,
                         to_email_address=None):
 
     if service is None:
-        service = create_service()
+        service = create_service(check_if_service_exists=True)
     if to_email_address is None:
         to_email_address = 'invited_user@digital.gov.uk'
 
@@ -780,7 +782,7 @@ def sample_permission(notify_db,
         'permission': permission
     }
     if service is None:
-        service = create_service()
+        service = create_service(check_if_service_exists=True)
     if service:
         data['service'] = service
     p_model = Permission.query.filter_by(
@@ -801,7 +803,7 @@ def sample_user_service_permission(
     if user is None:
         user = create_user()
     if service is None:
-        service = create_service(user=user)
+        service = create_service(user=user, check_if_service_exists=True)
     data = {
         'user': user,
         'service': service,
@@ -1035,7 +1037,7 @@ def notify_service(notify_db, notify_db_session):
 @pytest.fixture(scope='function')
 def sample_service_whitelist(notify_db, notify_db_session, service=None, email_address=None, mobile_number=None):
     if service is None:
-        service = create_service()
+        service = create_service(check_if_service_exists=True)
 
     if email_address:
         whitelisted_user = ServiceWhitelist.from_string(service.id, EMAIL_TYPE, email_address)
@@ -1060,7 +1062,7 @@ def sample_provider_rate(notify_db, notify_db_session, valid_from=None, rate=Non
 
 @pytest.fixture
 def sample_inbound_numbers(notify_db, notify_db_session, sample_service):
-    service = create_service(service_name='sample service 2')
+    service = create_service(service_name='sample service 2', check_if_service_exists=True)
     inbound_numbers = list()
     inbound_numbers.append(create_inbound_number(number='1', provider='mmg'))
     inbound_numbers.append(create_inbound_number(number='2', provider='mmg', active=False, service_id=service.id))
diff --git a/tests/app/db.py b/tests/app/db.py
index d6d4b9553..5c4889057 100644
--- a/tests/app/db.py
+++ b/tests/app/db.py
@@ -81,10 +81,12 @@ def create_service(
         prefix_sms=True,
         message_limit=1000,
         organisation_type='central',
-        postage='second'
+        postage='second',
+        check_if_service_exists=False
 ):
-    service = Service.query.filter_by(name=service_name).first()
-    if not service:
+    if check_if_service_exists:
+        service = Service.query.filter_by(name=service_name).first()
+    if (not check_if_service_exists) or (check_if_service_exists and not service):
         service = Service(
             name=service_name,
             message_limit=message_limit,

From 2355ee011f36f4060bff7325b1b4bf9b997685b7 Mon Sep 17 00:00:00 2001
From: Leo Hemsted <leo.hemsted@digital.cabinet-office.gov.uk>
Date: Thu, 20 Dec 2018 16:01:39 +0000
Subject: [PATCH 21/24] log more info when we receive multiple delivery
 callbacks for one notification

Previously, we logged a warning containing the notification reference
and new status. However it wasn't a great message - this new one
includes the notification id, the old status, the time difference and
more.

This separates out logs for callbacks for notifications we don't know
(error level) and duplicates (info level).
---
 app/dao/notifications_dao.py                  | 62 +++++++++++++------
 .../notifications_ses_callback.py             |  3 -
 app/notifications/process_client_response.py  |  4 --
 3 files changed, 44 insertions(+), 25 deletions(-)

diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py
index f27190ad3..d683cb636 100644
--- a/app/dao/notifications_dao.py
+++ b/app/dao/notifications_dao.py
@@ -15,7 +15,7 @@ from notifications_utils.recipients import (
 )
 from notifications_utils.statsd_decorators import statsd
 from notifications_utils.timezones import convert_utc_to_bst
-from sqlalchemy import (desc, func, or_, asc)
+from sqlalchemy import (desc, func, asc)
 from sqlalchemy.orm import joinedload
 from sqlalchemy.sql import functions
 from sqlalchemy.sql.expression import case
@@ -144,17 +144,23 @@ def _update_notification_status(notification, status):
 @statsd(namespace="dao")
 @transactional
 def update_notification_status_by_id(notification_id, status, sent_by=None):
-    notification = Notification.query.with_for_update().filter(
-        Notification.id == notification_id,
-        or_(
-            Notification.status == NOTIFICATION_CREATED,
-            Notification.status == NOTIFICATION_SENDING,
-            Notification.status == NOTIFICATION_PENDING,
-            Notification.status == NOTIFICATION_SENT,
-            Notification.status == NOTIFICATION_PENDING_VIRUS_CHECK
-        )).first()
+    notification = Notification.query.with_for_update().filter(Notification.id == notification_id).first()
 
     if not notification:
+        current_app.logger.error('notification not found for id {} (update to status {})'.format(
+            notification_id,
+            status
+        ))
+        return None
+
+    if notification.status not in {
+        NOTIFICATION_CREATED,
+        NOTIFICATION_SENDING,
+        NOTIFICATION_PENDING,
+        NOTIFICATION_SENT,
+        NOTIFICATION_PENDING_VIRUS_CHECK
+    }:
+        _duplicate_update_warning(notification, status)
         return None
 
     if notification.international and not country_records_delivery(notification.phone_prefix):
@@ -170,15 +176,19 @@ def update_notification_status_by_id(notification_id, status, sent_by=None):
 @statsd(namespace="dao")
 @transactional
 def update_notification_status_by_reference(reference, status):
-    notification = Notification.query.filter(
-        Notification.reference == reference,
-        or_(
-            Notification.status == NOTIFICATION_SENDING,
-            Notification.status == NOTIFICATION_PENDING,
-            Notification.status == NOTIFICATION_SENT
-        )).first()
+    # this is used to update letters and emails
+    notification = Notification.query.filter(Notification.reference == reference).first()
 
-    if not notification or notification.status == NOTIFICATION_SENT:
+    if not notification:
+        current_app.logger.error('notification not found for reference {} (update to {})'.format(reference, status))
+        return None
+
+    if notification.status not in {
+        NOTIFICATION_SENDING,
+        NOTIFICATION_PENDING,
+        NOTIFICATION_SENT,
+    }:
+        _duplicate_update_warning(notification, status)
         return None
 
     return _update_notification_status(
@@ -693,3 +703,19 @@ def guess_notification_type(search_term):
         return EMAIL_TYPE
     else:
         return SMS_TYPE
+
+
+def _duplicate_update_warning(notification, status):
+    current_app.logger.info(
+        (
+            'Duplicate callback received. Notification id {id} received a status update to {new_status}'
+            '{time_diff} after being set to {old_status}. {type} sent by {sent_by}'
+        ).format(
+            id=notification.id,
+            old_status=notification.status,
+            new_status=status,
+            time_diff=datetime.utcnow() - notification.sent_at,
+            type=notification.notification_type,
+            sent_by=notification.sent_by
+        )
+    )
diff --git a/app/notifications/notifications_ses_callback.py b/app/notifications/notifications_ses_callback.py
index e90cfeced..d75abcd4e 100644
--- a/app/notifications/notifications_ses_callback.py
+++ b/app/notifications/notifications_ses_callback.py
@@ -61,9 +61,6 @@ def process_ses_response(ses_request):
                 notification_status
             )
             if not notification:
-                warning = "SES callback failed: notification either not found or already updated " \
-                          "from sending. Status {} for notification reference {}".format(notification_status, reference)
-                current_app.logger.warning(warning)
                 return
 
             if not aws_response_dict['success']:
diff --git a/app/notifications/process_client_response.py b/app/notifications/process_client_response.py
index d45c3cc92..3de60c587 100644
--- a/app/notifications/process_client_response.py
+++ b/app/notifications/process_client_response.py
@@ -81,10 +81,6 @@ def _process_for_status(notification_status, client_name, provider_reference):
         sent_by=client_name.lower()
     )
     if not notification:
-        current_app.logger.warning("{} callback failed: notification {} either not found or already updated "
-                                   "from sending. Status {}".format(client_name,
-                                                                    provider_reference,
-                                                                    notification_status))
         return
 
     statsd_client.incr('callback.{}.{}'.format(client_name.lower(), notification_status))

From 021625abb3c676e9794fd6107024ee038f9c0a57 Mon Sep 17 00:00:00 2001
From: Leo Hemsted <leo.hemsted@digital.cabinet-office.gov.uk>
Date: Fri, 28 Dec 2018 14:29:59 +0000
Subject: [PATCH 22/24] make sure log line works if notification still in
 created

---
 app/dao/notifications_dao.py | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py
index d683cb636..c78582851 100644
--- a/app/dao/notifications_dao.py
+++ b/app/dao/notifications_dao.py
@@ -185,8 +185,7 @@ def update_notification_status_by_reference(reference, status):
 
     if notification.status not in {
         NOTIFICATION_SENDING,
-        NOTIFICATION_PENDING,
-        NOTIFICATION_SENT,
+        NOTIFICATION_PENDING
     }:
         _duplicate_update_warning(notification, status)
         return None
@@ -714,7 +713,7 @@ def _duplicate_update_warning(notification, status):
             id=notification.id,
             old_status=notification.status,
             new_status=status,
-            time_diff=datetime.utcnow() - notification.sent_at,
+            time_diff=datetime.utcnow() - (notification.updated_at or notification.created_at),
             type=notification.notification_type,
             sent_by=notification.sent_by
         )

From 2e53ba1e3e5d831b0a284c61af34c68686a333a6 Mon Sep 17 00:00:00 2001
From: Leo Hemsted <leo.hemsted@digital.cabinet-office.gov.uk>
Date: Fri, 4 Jan 2019 16:11:21 +0000
Subject: [PATCH 23/24] bump utils

brings in https://github.com/alphagov/notifications-utils/pull/563
---
 requirements-app.txt | 2 +-
 requirements.txt     | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/requirements-app.txt b/requirements-app.txt
index 0941e7f4b..f48c7c058 100644
--- a/requirements-app.txt
+++ b/requirements-app.txt
@@ -29,6 +29,6 @@ awscli-cwlogs>=1.4,<1.5
 # Putting upgrade on hold due to v1.0.0 using sha512 instead of sha1 by default
 itsdangerous==0.24  # pyup: <1.0.0
 
-git+https://github.com/alphagov/notifications-utils.git@30.7.2#egg=notifications-utils==30.7.2
+git+https://github.com/alphagov/notifications-utils.git@30.7.3#egg=notifications-utils==30.7.3
 
 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3
diff --git a/requirements.txt b/requirements.txt
index 09bfcfe90..57ae41765 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -31,7 +31,7 @@ awscli-cwlogs>=1.4,<1.5
 # Putting upgrade on hold due to v1.0.0 using sha512 instead of sha1 by default
 itsdangerous==0.24  # pyup: <1.0.0
 
-git+https://github.com/alphagov/notifications-utils.git@30.7.2#egg=notifications-utils==30.7.2
+git+https://github.com/alphagov/notifications-utils.git@30.7.3#egg=notifications-utils==30.7.3
 
 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3
 
@@ -40,12 +40,12 @@ alembic==1.0.5
 amqp==1.4.9
 anyjson==0.3.3
 attrs==18.2.0
-awscli==1.16.81
+awscli==1.16.83
 bcrypt==3.1.5
 billiard==3.3.0.23
 bleach==2.1.3
 boto3==1.6.16
-botocore==1.12.71
+botocore==1.12.73
 certifi==2018.11.29
 chardet==3.0.4
 Click==7.0

From 40a17a0e93f977cabb5538c972b2519439414c8a Mon Sep 17 00:00:00 2001
From: Leo Hemsted <leo.hemsted@digital.cabinet-office.gov.uk>
Date: Mon, 7 Jan 2019 11:14:25 +0000
Subject: [PATCH 24/24] pin redis to 2.x

while we sort out v3 compatibility
---
 requirements-app.txt |  4 +++-
 requirements.txt     | 13 +++++++------
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/requirements-app.txt b/requirements-app.txt
index f48c7c058..09c0ee350 100644
--- a/requirements-app.txt
+++ b/requirements-app.txt
@@ -29,6 +29,8 @@ awscli-cwlogs>=1.4,<1.5
 # Putting upgrade on hold due to v1.0.0 using sha512 instead of sha1 by default
 itsdangerous==0.24  # pyup: <1.0.0
 
-git+https://github.com/alphagov/notifications-utils.git@30.7.3#egg=notifications-utils==30.7.3
+git+https://github.com/alphagov/notifications-utils.git@30.7.2#egg=notifications-utils==30.7.2
+# pinned until upgrade to redis-py 3 works
+redis==2.10.6
 
 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3
diff --git a/requirements.txt b/requirements.txt
index 57ae41765..f711cac44 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -31,7 +31,9 @@ awscli-cwlogs>=1.4,<1.5
 # Putting upgrade on hold due to v1.0.0 using sha512 instead of sha1 by default
 itsdangerous==0.24  # pyup: <1.0.0
 
-git+https://github.com/alphagov/notifications-utils.git@30.7.3#egg=notifications-utils==30.7.3
+git+https://github.com/alphagov/notifications-utils.git@30.7.2#egg=notifications-utils==30.7.2
+# pinned until upgrade to redis-py 3 works
+redis==2.10.6
 
 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3
 
@@ -40,12 +42,12 @@ alembic==1.0.5
 amqp==1.4.9
 anyjson==0.3.3
 attrs==18.2.0
-awscli==1.16.83
+awscli==1.16.84
 bcrypt==3.1.5
 billiard==3.3.0.23
 bleach==2.1.3
 boto3==1.6.16
-botocore==1.12.73
+botocore==1.12.74
 certifi==2018.11.29
 chardet==3.0.4
 Click==7.0
@@ -68,13 +70,12 @@ phonenumbers==8.9.4
 pyasn1==0.4.5
 pycparser==2.19
 PyPDF2==1.26.0
-pyrsistent==0.14.8
+pyrsistent==0.14.9
 python-dateutil==2.7.5
 python-editor==1.0.3
 python-json-logger==0.1.8
-pytz==2018.7
+pytz==2018.9
 PyYAML==3.12
-redis==3.0.1
 requests==2.21.0
 rsa==3.4.2
 s3transfer==0.1.13