Cache template in redis

This commit changes the code in post notification endpoint to handle a
serialised template (ie a `dict`) rather than a database object.

This is the first step towards being able to cache the template and not
hit the database on every request.

There should be no functional changes here, it’s just refactoring.

There are some changes to the tests where the signature of functions
has changed.

Importing of the template schema has to be done at a function level,
otherwise Marshmallow gets weird.

app/__init__.py

@@ -1,23 +1,20 @@
-import time
 import os
 import random
 import string
 import uuid
 
-from celery import current_task
-from flask import _request_ctx_stack, request, g, jsonify, make_response, current_app, has_request_context
+from flask import _request_ctx_stack, request, g, jsonify, make_response
 from flask_sqlalchemy import SQLAlchemy as _SQLAlchemy
 from flask_marshmallow import Marshmallow
 from flask_migrate import Migrate
 from gds_metrics import GDSMetrics
-from gds_metrics.metrics import Gauge, Histogram
 from time import monotonic
 from notifications_utils.clients.zendesk.zendesk_client import ZendeskClient
 from notifications_utils.clients.statsd.statsd_client import StatsdClient
+from notifications_utils.clients.redis import RequestCache
 from notifications_utils.clients.redis.redis_client import RedisClient
 from notifications_utils.clients.encryption.encryption_client import Encryption
 from notifications_utils import logging, request_helper
-from sqlalchemy import event
 from werkzeug.exceptions import HTTPException as WerkzeugHTTPException
 from werkzeug.local import LocalProxy
 
@@ -59,6 +56,7 @@ encryption = Encryption()
 zendesk_client = ZendeskClient()
 statsd_client = StatsdClient()
 redis_store = RedisClient()
+request_cache = RequestCache(redis_store)
 performance_platform_client = PerformancePlatformClient()
 document_download_client = DocumentDownloadClient()
 metrics = GDSMetrics()
@@ -68,11 +66,6 @@ clients = Clients()
 api_user = LocalProxy(lambda: _request_ctx_stack.top.api_user)
 authenticated_service = LocalProxy(lambda: _request_ctx_stack.top.authenticated_service)
 
-CONCURRENT_REQUESTS = Gauge(
-    'concurrent_web_request_count',
-    'How many concurrent requests are currently being served',
-)
-
 
 def create_app(application):
     from app.config import configs
@@ -117,9 +110,6 @@ def create_app(application):
     from app.commands import setup_commands
     setup_commands(application)
 
-    # set up sqlalchemy events
-    setup_sqlalchemy_events(application)
-
     return application
 
 
@@ -267,22 +257,17 @@ def register_v2_blueprints(application):
 
 
 def init_app(app):
-
     @app.before_request
     def record_user_agent():
         statsd_client.incr("user-agent.{}".format(process_user_agent(request.headers.get('User-Agent', None))))
 
     @app.before_request
     def record_request_details():
-        CONCURRENT_REQUESTS.inc()
-
         g.start = monotonic()
         g.endpoint = request.endpoint
 
     @app.after_request
     def after_request(response):
-        CONCURRENT_REQUESTS.dec()
-
         response.headers.add('Access-Control-Allow-Origin', '*')
         response.headers.add('Access-Control-Allow-Headers', 'Content-Type,Authorization')
         response.headers.add('Access-Control-Allow-Methods', 'GET,PUT,POST,DELETE')
@@ -328,83 +313,3 @@ def process_user_agent(user_agent_string):
         return "non-notify-user-agent"
     else:
         return "unknown"
-
-
-def setup_sqlalchemy_events(app):
-
-    TOTAL_DB_CONNECTIONS = Gauge(
-        'db_connection_total_connected',
-        'How many db connections are currently held (potentially idle) by the server',
-    )
-
-    TOTAL_CHECKED_OUT_DB_CONNECTIONS = Gauge(
-        'db_connection_total_checked_out',
-        'How many db connections are currently checked out by web requests',
-    )
-
-    DB_CONNECTION_OPEN_DURATION_SECONDS = Histogram(
-        'db_connection_open_duration_seconds',
-        'How long db connections are held open for in seconds',
-        ['method', 'host', 'path']
-    )
-
-    # need this or db.engine isn't accessible
-    with app.app_context():
-        @event.listens_for(db.engine, 'connect')
-        def connect(dbapi_connection, connection_record):
-            # connection first opened with db
-            TOTAL_DB_CONNECTIONS.inc()
-
-        @event.listens_for(db.engine, 'close')
-        def close(dbapi_connection, connection_record):
-            # connection closed (probably only happens with overflow connections)
-            TOTAL_DB_CONNECTIONS.dec()
-
-        @event.listens_for(db.engine, 'checkout')
-        def checkout(dbapi_connection, connection_record, connection_proxy):
-            # connection given to a web worker
-            TOTAL_CHECKED_OUT_DB_CONNECTIONS.inc()
-
-            # this will overwrite any previous checkout_at timestamp
-            connection_record.info['checkout_at'] = time.monotonic()
-
-            # checkin runs after the request is already torn down, therefore we add the request_data onto the
-            # connection_record as otherwise it won't have that information when checkin actually runs.
-            # Note: this is not a problem for checkouts as the checkout always happens within a web request or task
-
-            # web requests
-            if has_request_context():
-                connection_record.info['request_data'] = {
-                    'method': request.method,
-                    'host': request.host,
-                    'url_rule': request.url_rule.rule if request.url_rule else 'No endpoint'
-                }
-            # celery apps
-            elif current_task:
-                connection_record.info['request_data'] = {
-                    'method': 'celery',
-                    'host': current_app.config['NOTIFY_APP_NAME'],  # worker name
-                    'url_rule': current_task.name,  # task name
-                }
-            # anything else. migrations possibly.
-            else:
-                current_app.logger.warning('Checked out sqlalchemy connection from outside of request/task')
-                connection_record.info['request_data'] = {
-                    'method': 'unknown',
-                    'host': 'unknown',
-                    'url_rule': 'unknown',
-                }
-
-        @event.listens_for(db.engine, 'checkin')
-        def checkin(dbapi_connection, connection_record):
-            # connection returned by a web worker
-            TOTAL_CHECKED_OUT_DB_CONNECTIONS.dec()
-
-            # duration that connection was held by a single web request
-            duration = time.monotonic() - connection_record.info['checkout_at']
-
-            DB_CONNECTION_OPEN_DURATION_SECONDS.labels(
-                connection_record.info['request_data']['method'],
-                connection_record.info['request_data']['host'],
-                connection_record.info['request_data']['url_rule']
-            ).observe(duration)

app/authentication/auth.py

@@ -6,18 +6,12 @@ from notifications_python_client.errors import (
 from notifications_utils import request_helper
 from sqlalchemy.exc import DataError
 from sqlalchemy.orm.exc import NoResultFound
-from gds_metrics import Histogram
 
 from app.dao.services_dao import dao_fetch_service_by_id_with_api_keys
 
 
 GENERAL_TOKEN_ERROR_MESSAGE = 'Invalid token: make sure your API token matches the example at https://docs.notifications.service.gov.uk/rest-api.html#authorisation-header'  # noqa
 
-AUTH_DB_CONNECTION_DURATION_SECONDS = Histogram(
-    'auth_db_connection_duration_seconds',
-    'Time taken to get DB connection and fetch service from database',
-)
-
 
 class AuthError(Exception):
     def __init__(self, message, code, service_id=None, api_key_id=None):
@@ -93,8 +87,7 @@ def requires_auth():
     issuer = __get_token_issuer(auth_token)  # ie the `iss` claim which should be a service ID
 
     try:
-        with AUTH_DB_CONNECTION_DURATION_SECONDS.time():
-            service = dao_fetch_service_by_id_with_api_keys(issuer)
+        service = dao_fetch_service_by_id_with_api_keys(issuer)
     except DataError:
         raise AuthError("Invalid token: service id is not the right data type", 403)
     except NoResultFound:

app/celery/celery.py

@@ -1,6 +1,5 @@
 import time
 
-from gds_metrics.metrics import Histogram
 from celery import Celery, Task
 from celery.signals import worker_process_shutdown
 from flask import g, request
@@ -20,12 +19,6 @@ def log_on_worker_shutdown(sender, signal, pid, exitcode, **kwargs):
 
 
 def make_task(app):
-    SQS_APPLY_ASYNC_DURATION_SECONDS = Histogram(
-        'sqs_apply_async_duration_seconds',
-        'Time taken to put task on queue',
-        ['task_name']
-    )
-
     class NotifyTask(Task):
         abstract = True
         start = None
@@ -59,8 +52,7 @@ def make_task(app):
             if has_request_context() and hasattr(request, 'request_id'):
                 kwargs['request_id'] = request.request_id
 
-            with SQS_APPLY_ASYNC_DURATION_SECONDS.labels(self.name).time():
-                return super().apply_async(args, kwargs, task_id, producer, link, link_error, **options)
+            return super().apply_async(args, kwargs, task_id, producer, link, link_error, **options)
 
     return NotifyTask
 

app/notifications/process_letter_notifications.py

@@ -6,15 +6,21 @@ from app.notifications.process_notifications import persist_notification
 
 
 def create_letter_notification(
-    letter_data, template, api_key, status, reply_to_text=None, billable_units=None, updated_at=None
+    letter_data,
+    template,
+    service,
+    api_key,
+    status,
+    reply_to_text=None,
+    billable_units=None,
 ):
     notification = persist_notification(
         template_id=template.id,
-        template_version=template.version,
-        template_postage=template.postage,
+        template_version=template._template['version'],
+        template_postage=template._template['postage'],
         # we only accept addresses_with_underscores from the API (from CSV we also accept dashes, spaces etc)
         recipient=PostalAddress.from_personalisation(letter_data['personalisation']).normalised,
-        service=template.service,
+        service=service,
         personalisation=letter_data['personalisation'],
         notification_type=LETTER_TYPE,
         api_key_id=api_key.id,
@@ -26,7 +32,6 @@ def create_letter_notification(
         status=status,
         reply_to_text=reply_to_text,
         billable_units=billable_units,
-        postage=letter_data.get('postage'),
-        updated_at=updated_at
+        postage=letter_data.get('postage')
     )
     return notification

app/notifications/process_notifications.py

@@ -9,6 +9,11 @@ from notifications_utils.recipients import (
     validate_and_format_phone_number,
     format_email_address
 )
+from notifications_utils.template import (
+    PlainTextEmailTemplate,
+    SMSMessageTemplate,
+    LetterPrintTemplate,
+)
 from notifications_utils.timezones import convert_bst_to_utc
 
 from app import redis_store
@@ -34,17 +39,18 @@ from app.dao.notifications_dao import (
 from app.v2.errors import BadRequestError
 
 
-from gds_metrics import Histogram
+def create_content_for_notification(template_dict, personalisation):
+    if template_dict['template_type'] == EMAIL_TYPE:
+        template_object = PlainTextEmailTemplate(template_dict, personalisation)
+    if template_dict['template_type'] == SMS_TYPE:
+        template_object = SMSMessageTemplate(template_dict, personalisation)
+    if template_dict['template_type'] == LETTER_TYPE:
+        template_object = LetterPrintTemplate(
+            template_dict,
+            personalisation,
+            contact_block=template_dict['reply_to_text'],
+        )
 
-
-REDIS_GET_AND_INCR_DAILY_LIMIT_DURATION_SECONDS = Histogram(
-    'redis_get_and_incr_daily_limit_duration_seconds',
-    'Time taken to get and possibly incremement the daily limit cache key',
-)
-
-
-def create_content_for_notification(template, personalisation):
-    template_object = template._as_utils_template_with_personalisation(personalisation)
     check_placeholders(template_object)
 
     return template_object
@@ -79,8 +85,7 @@ def persist_notification(
     billable_units=None,
     postage=None,
     template_postage=None,
-    document_download_count=None,
-    updated_at=None
+    document_download_count=None
 ):
     notification_created_at = created_at or datetime.utcnow()
     if not notification_id:
@@ -106,7 +111,6 @@ def persist_notification(
         reply_to_text=reply_to_text,
         billable_units=billable_units,
         document_download_count=document_download_count,
-        updated_at=updated_at
     )
 
     if notification_type == SMS_TYPE:
@@ -122,17 +126,12 @@ def persist_notification(
         notification.postage = postage or template_postage
         notification.normalised_to = ''.join(notification.to.split()).lower()
 
-    # Get service attributes before the commit
-    service_in_trial_mode = service.restricted
-    service_id = service.id
-
     # if simulated create a Notification model to return but do not persist the Notification to the dB
     if not simulated:
         dao_create_notification(notification)
-        # Only keep track of the daily limit for trial mode services.
-        if service_in_trial_mode and key_type != KEY_TYPE_TEST:
-            if redis_store.get(redis.daily_limit_cache_key(service_id)):
-                redis_store.incr(redis.daily_limit_cache_key(service_id))
+        if key_type != KEY_TYPE_TEST:
+            if redis_store.get(redis.daily_limit_cache_key(service.id)):
+                redis_store.incr(redis.daily_limit_cache_key(service.id))
 
         current_app.logger.info(
             "{} {} created at {}".format(notification_type, notification_id, notification_created_at)
@@ -140,43 +139,35 @@ def persist_notification(
     return notification
 
 
-def send_notification_to_queue_detached(
-    key_type, notification_type, notification_id, research_mode, queue=None
-):
-    if research_mode or key_type == KEY_TYPE_TEST:
+def send_notification_to_queue(notification, research_mode, queue=None):
+    if research_mode or notification.key_type == KEY_TYPE_TEST:
         queue = QueueNames.RESEARCH_MODE
 
-    if notification_type == SMS_TYPE:
+    if notification.notification_type == SMS_TYPE:
         if not queue:
             queue = QueueNames.SEND_SMS
         deliver_task = provider_tasks.deliver_sms
-    if notification_type == EMAIL_TYPE:
+    if notification.notification_type == EMAIL_TYPE:
         if not queue:
             queue = QueueNames.SEND_EMAIL
         deliver_task = provider_tasks.deliver_email
-    if notification_type == LETTER_TYPE:
+    if notification.notification_type == LETTER_TYPE:
         if not queue:
             queue = QueueNames.CREATE_LETTERS_PDF
         deliver_task = get_pdf_for_templated_letter
 
     try:
-        deliver_task.apply_async([str(notification_id)], queue=queue)
+        deliver_task.apply_async([str(notification.id)], queue=queue)
     except Exception:
-        dao_delete_notifications_by_id(notification_id)
+        dao_delete_notifications_by_id(notification.id)
         raise
 
     current_app.logger.debug(
-        "{} {} sent to the {} queue for delivery".format(notification_type,
-                                                         notification_id,
+        "{} {} sent to the {} queue for delivery".format(notification.notification_type,
+                                                         notification.id,
                                                          queue))
 
 
-def send_notification_to_queue(notification, research_mode, queue=None):
-    send_notification_to_queue_detached(
-        notification.key_type, notification.notification_type, notification.id, research_mode, queue
-    )
-
-
 def simulated_recipient(to_address, notification_type):
     if notification_type == SMS_TYPE:
         formatted_simulated_numbers = [

app/notifications/rest.py

@@ -100,12 +100,13 @@ def send_notification(notification_type):
 
     check_rate_limiting(authenticated_service, api_user)
 
-    template, template_with_content = validate_template(
+    template_with_content = validate_template(
         template_id=notification_form['template'],
         personalisation=notification_form.get('personalisation', {}),
         service=authenticated_service,
         notification_type=notification_type
     )
+    template_dict = template_with_content._template
 
     _service_allowed_to_send_to(notification_form, authenticated_service)
     if not service_has_permission(notification_type, authenticated_service.permissions):
@@ -118,9 +119,9 @@ def send_notification(notification_type):
         _service_can_send_internationally(authenticated_service, notification_form['to'])
     # Do not persist or send notification to the queue if it is a simulated recipient
     simulated = simulated_recipient(notification_form['to'], notification_type)
-    notification_model = persist_notification(template_id=template.id,
-                                              template_version=template.version,
-                                              template_postage=template.postage,
+    notification_model = persist_notification(template_id=template_dict['id'],
+                                              template_version=template_dict['version'],
+                                              template_postage=template_dict['postage'],
                                               recipient=request.get_json()['to'],
                                               service=authenticated_service,
                                               personalisation=notification_form.get('personalisation', None),
@@ -128,16 +129,16 @@ def send_notification(notification_type):
                                               api_key_id=api_user.id,
                                               key_type=api_user.key_type,
                                               simulated=simulated,
-                                              reply_to_text=template.get_reply_to_text()
+                                              reply_to_text=template_dict['reply_to_text']
                                               )
     if not simulated:
-        queue_name = QueueNames.PRIORITY if template.process_type == PRIORITY else None
+        queue_name = QueueNames.PRIORITY if template_dict['process_type'] == PRIORITY else None
         send_notification_to_queue(notification=notification_model,
                                    research_mode=authenticated_service.research_mode,
                                    queue=queue_name)
     else:
         current_app.logger.debug("POST simulated notification for id: {}".format(notification_model.id))
-    notification_form.update({"template_version": template.version})
+    notification_form.update({"template_version": template_dict['version']})
 
     return jsonify(
         data=get_notification_return_data(

app/notifications/validators.py

@@ -16,30 +16,21 @@ from app.models import (
 )
 from app.service.utils import service_allowed_to_send_to
 from app.v2.errors import TooManyRequestsError, BadRequestError, RateLimitError
-from app import redis_store
+from app import redis_store, request_cache
 from app.notifications.process_notifications import create_content_for_notification
 from app.utils import get_public_notify_type_text
 from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_id
 from app.dao.service_letter_contact_dao import dao_get_letter_contact_by_id
 
-from gds_metrics.metrics import Histogram
-
-
-REDIS_EXCEEDED_RATE_LIMIT_DURATION_SECONDS = Histogram(
-    'redis_exceeded_rate_limit_duration_seconds',
-    'Time taken to check rate limit',
-)
-
 
 def check_service_over_api_rate_limit(service, api_key):
     if current_app.config['API_RATE_LIMIT_ENABLED'] and current_app.config['REDIS_ENABLED']:
         cache_key = rate_limit_cache_key(service.id, api_key.key_type)
         rate_limit = service.rate_limit
         interval = 60
-        with REDIS_EXCEEDED_RATE_LIMIT_DURATION_SECONDS.time():
-            if redis_store.exceeded_rate_limit(cache_key, rate_limit, interval):
-                current_app.logger.info("service {} has been rate limited for throughput".format(service.id))
-                raise RateLimitError(rate_limit, interval, api_key.key_type)
+        if redis_store.exceeded_rate_limit(cache_key, rate_limit, interval):
+            current_app.logger.info("service {} has been rate limited for throughput".format(service.id))
+            raise RateLimitError(rate_limit, interval, api_key.key_type)
 
 
 def check_service_over_daily_message_limit(key_type, service):
@@ -71,7 +62,7 @@ def check_template_is_for_notification_type(notification_type, template_type):
 
 
 def check_template_is_active(template):
-    if template.archived:
+    if template['archived']:
         raise BadRequestError(fields=[{'template': 'Template has been deleted'}],
                               message="Template has been deleted")
 
@@ -147,18 +138,26 @@ def check_notification_content_is_not_empty(template_with_content):
         raise BadRequestError(message=message)
 
 
-def validate_template(template_id, personalisation, service, notification_type):
+@request_cache.set('template-{template_id}-version-None')
+def get_template_dict(template_id, service_id):
+    from app.schemas import template_schema
     try:
-        template = templates_dao.dao_get_template_by_id_and_service_id(
+        fetched_template = templates_dao.dao_get_template_by_id_and_service_id(
             template_id=template_id,
-            service_id=service.id
+            service_id=service_id
         )
     except NoResultFound:
         message = 'Template not found'
         raise BadRequestError(message=message,
                               fields=[{'template': message}])
 
-    check_template_is_for_notification_type(notification_type, template.template_type)
+    return template_schema.dump(fetched_template).data
+
+
+def validate_template(template_id, personalisation, service, notification_type):
+    template = get_template_dict(template_id, service.id)
+
+    check_template_is_for_notification_type(notification_type, template['template_type'])
     check_template_is_active(template)
 
     template_with_content = create_content_for_notification(template, personalisation)
@@ -167,7 +166,7 @@ def validate_template(template_id, personalisation, service, notification_type):
 
     check_content_char_count(template_with_content)
 
-    return template, template_with_content
+    return template_with_content
 
 
 def check_reply_to(service_id, reply_to_id, type_):

app/schemas.py

@@ -2,6 +2,7 @@ from datetime import (
     datetime,
     date,
     timedelta)
+from uuid import UUID
 from flask_marshmallow.fields import fields
 from marshmallow import (
     post_load,
@@ -235,18 +236,6 @@ class ServiceSchema(BaseSchema):
             'letter_contacts',
             'complaints',
             'data_retention',
-            'all_template_folders',
-            'annual_billing',
-            'contact_list',
-            'crown',
-            'inbound_number',
-            'inbound_sms',
-            'letter_logo_filename',
-            'rate_limit',
-            'returned_letters',
-            'users',
-            'version',
-            'whitelist',
         )
         strict = True
 
@@ -301,18 +290,7 @@ class DetailedServiceSchema(BaseSchema):
             'sms_sender',
             'permissions',
             'inbound_number',
-            'inbound_sms',
-            'all_template_folders',
-            'annual_billing',
-            'contact_list',
-            'created_by',
-            'crown',
-            'letter_logo_filename',
-            'rate_limit',
-            'returned_letters',
-            'users',
-            'version',
-            'whitelist',
+            'inbound_sms'
         )
 
 
@@ -357,6 +335,16 @@ class TemplateSchema(BaseTemplateSchema):
             if not subject or subject.strip() == '':
                 raise ValidationError('Invalid template subject', 'subject')
 
+    @post_dump()
+    def __post_dump(self, data):
+        for field in (
+            'service',
+            'created_by',
+            'template_redacted',
+        ):
+            if isinstance(data[field], UUID):
+                data[field] = str(data[field])
+
 
 class TemplateHistorySchema(BaseSchema):
 

app/v2/notifications/create_response.py

@@ -1,70 +1,45 @@
 
 
-def create_post_sms_response_from_notification(
-    notification_id, client_reference, template_id, template_version, service_id,
-    content, from_number, url_root, scheduled_for
-):
-    resp = __create_notification_response(
-        notification_id, client_reference, template_id, template_version, service_id, url_root, scheduled_for
-    )
-    resp['content'] = {
+def create_post_sms_response_from_notification(notification, content, from_number, url_root, scheduled_for):
+    noti = __create_notification_response(notification, url_root, scheduled_for)
+    noti['content'] = {
         'from_number': from_number,
         'body': content
     }
-    return resp
+    return noti
 
 
-def create_post_email_response_from_notification(
-    notification_id,
-    client_reference,
-    template_id,
-    template_version,
-    service_id,
-    content,
-    subject,
-    email_from,
-    url_root,
-    scheduled_for
-):
-    resp = __create_notification_response(
-        notification_id, client_reference, template_id, template_version, service_id, url_root, scheduled_for
-    )
-    resp['content'] = {
+def create_post_email_response_from_notification(notification, content, subject, email_from, url_root, scheduled_for):
+    noti = __create_notification_response(notification, url_root, scheduled_for)
+    noti['content'] = {
         "from_email": email_from,
         "body": content,
         "subject": subject
     }
-    return resp
+    return noti
 
 
-def create_post_letter_response_from_notification(
-    notification_id, client_reference, template_id, template_version, service_id,
-    content, subject, url_root, scheduled_for
-):
-    resp = __create_notification_response(
-        notification_id, client_reference, template_id, template_version, service_id, url_root, scheduled_for
-    )
-    resp['content'] = {
+def create_post_letter_response_from_notification(notification, content, subject, url_root, scheduled_for):
+    noti = __create_notification_response(notification, url_root, scheduled_for)
+    noti['content'] = {
         "body": content,
         "subject": subject
     }
-    return resp
+    return noti
 
 
-def __create_notification_response(
-    notification_id, client_reference, template_id, template_version, service_id, url_root, scheduled_for
-):
+def __create_notification_response(notification, url_root, scheduled_for):
     return {
-        "id": notification_id,
-        "reference": client_reference,
-        "uri": "{}v2/notifications/{}".format(url_root, str(notification_id)),
+        "id": notification.id,
+        "reference": notification.client_reference,
+        "uri": "{}v2/notifications/{}".format(url_root, str(notification.id)),
         'template': {
-            "id": template_id,
-            "version": template_version,
+            "id": notification.template_id,
+            "version": notification.template_version,
             "uri": "{}services/{}/templates/{}".format(
                 url_root,
-                str(service_id),
-                str(template_id)
+                str(notification.service_id),
+                str(notification.template_id)
             )
         },
         "scheduled_for": scheduled_for if scheduled_for else None

app/v2/notifications/post_notifications.py

@@ -7,7 +7,6 @@ from boto.exception import SQSError
 from flask import request, jsonify, current_app, abort
 from notifications_utils.postal_address import PostalAddress
 from notifications_utils.recipients import try_validate_and_format_phone_number
-from gds_metrics import Histogram
 
 from app import (
     api_user,
@@ -15,13 +14,14 @@ from app import (
     notify_celery,
     document_download_client,
     encryption,
-    DATETIME_FORMAT,
-    db)
+    DATETIME_FORMAT
+)
 from app.celery.letters_pdf_tasks import get_pdf_for_templated_letter, sanitise_letter
 from app.celery.research_mode_tasks import create_fake_letter_response_file
 from app.celery.tasks import save_api_email
 from app.clients.document_download import DocumentDownloadError
 from app.config import QueueNames, TaskNames
+from app.dao.notifications_dao import update_notification_status_by_reference
 from app.dao.templates_dao import get_precompiled_letter_template
 from app.letters.utils import upload_letter_pdf
 from app.models import (
@@ -42,10 +42,12 @@ from app.notifications.process_letter_notifications import (
     create_letter_notification
 )
 from app.notifications.process_notifications import (
+    create_content_for_notification,
     persist_notification,
     persist_scheduled_notification,
-    simulated_recipient,
-    send_notification_to_queue_detached)
+    send_notification_to_queue,
+    simulated_recipient
+)
 from app.notifications.validators import (
     check_if_service_can_send_files_by_email,
     check_rate_limiting,
@@ -60,8 +62,10 @@ from app.schema_validation import validate
 from app.v2.errors import BadRequestError, ValidationError
 from app.v2.notifications import v2_notification_blueprint
 from app.v2.notifications.create_response import (
-    create_post_sms_response_from_notification, create_post_email_response_from_notification,
-    create_post_letter_response_from_notification)
+    create_post_sms_response_from_notification,
+    create_post_email_response_from_notification,
+    create_post_letter_response_from_notification
+)
 from app.v2.notifications.notification_schemas import (
     post_sms_request,
     post_email_request,
@@ -71,14 +75,9 @@ from app.v2.notifications.notification_schemas import (
 from app.v2.utils import get_valid_json
 
 
-POST_NOTIFICATION_JSON_PARSE_DURATION_SECONDS = Histogram(
-    'post_notification_json_parse_duration_seconds',
-    'Time taken to parse and validate post request json',
-)
-
-
 @v2_notification_blueprint.route('/{}'.format(LETTER_TYPE), methods=['POST'])
 def post_precompiled_letter_notification():
+    from app.schemas import template_schema
     request_json = get_valid_json()
     if 'content' not in (request_json or {}):
         return post_notification(LETTER_TYPE)
@@ -91,6 +90,9 @@ def post_precompiled_letter_notification():
     check_rate_limiting(authenticated_service, api_user)
 
     template = get_precompiled_letter_template(authenticated_service.id)
+    template = create_content_for_notification(
+        template_schema.dump(template).data, {}
+    )
 
     # For precompiled letters the to field will be set to Provided as PDF until the validation passes,
     # then the address of the letter will be set as the to field
@@ -98,33 +100,36 @@ def post_precompiled_letter_notification():
         'address_line_1': 'Provided as PDF'
     }
 
-    reply_to = get_reply_to_text(LETTER_TYPE, form, template)
-
     notification = process_letter_notification(
         letter_data=form,
         api_key=api_user,
         template=template,
-        template_with_content=None,  # not required for precompiled
-        reply_to_text=reply_to,
+        service=authenticated_service,
+        reply_to_text=template._template['reply_to_text'],
         precompiled=True
     )
 
-    return jsonify(notification), 201
+    resp = {
+        'id': notification.id,
+        'reference': notification.client_reference,
+        'postage': notification.postage
+    }
+
+    return jsonify(resp), 201
 
 
 @v2_notification_blueprint.route('/<notification_type>', methods=['POST'])
 def post_notification(notification_type):
-    with POST_NOTIFICATION_JSON_PARSE_DURATION_SECONDS.time():
-        request_json = get_valid_json()
+    request_json = get_valid_json()
 
-        if notification_type == EMAIL_TYPE:
-            form = validate(request_json, post_email_request)
-        elif notification_type == SMS_TYPE:
-            form = validate(request_json, post_sms_request)
-        elif notification_type == LETTER_TYPE:
-            form = validate(request_json, post_letter_request)
-        else:
-            abort(404)
+    if notification_type == EMAIL_TYPE:
+        form = validate(request_json, post_email_request)
+    elif notification_type == SMS_TYPE:
+        form = validate(request_json, post_sms_request)
+    elif notification_type == LETTER_TYPE:
+        form = validate(request_json, post_letter_request)
+    else:
+        abort(404)
 
     check_service_has_permission(notification_type, authenticated_service.permissions)
 
@@ -134,21 +139,21 @@ def post_notification(notification_type):
 
     check_rate_limiting(authenticated_service, api_user)
 
-    template, template_with_content = validate_template(
+    template_with_content = validate_template(
         form['template_id'],
         form.get('personalisation', {}),
         authenticated_service,
         notification_type,
     )
 
-    reply_to = get_reply_to_text(notification_type, form, template)
+    reply_to = get_reply_to_text(notification_type, form, template_with_content)
 
     if notification_type == LETTER_TYPE:
         notification = process_letter_notification(
             letter_data=form,
             api_key=api_user,
-            template=template,
-            template_with_content=template_with_content,
+            template=template_with_content,
+            service=authenticated_service,
             reply_to_text=reply_to
         )
     else:
@@ -157,18 +162,41 @@ def post_notification(notification_type):
             notification_type=notification_type,
             api_key=api_user,
             template=template_with_content,
-            template_process_type=template.process_type,
             service=authenticated_service,
             reply_to_text=reply_to
         )
 
-    return jsonify(notification), 201
+        # Think this is redundant
+        template_with_content.values = notification.personalisation
+
+    if notification_type == SMS_TYPE:
+        create_resp_partial = functools.partial(
+            create_post_sms_response_from_notification,
+            from_number=reply_to,
+        )
+    elif notification_type == EMAIL_TYPE:
+        create_resp_partial = functools.partial(
+            create_post_email_response_from_notification,
+            subject=template_with_content.subject,
+            email_from='{}@{}'.format(authenticated_service.email_from, current_app.config['NOTIFY_EMAIL_DOMAIN']),
+        )
+    elif notification_type == LETTER_TYPE:
+        create_resp_partial = functools.partial(
+            create_post_letter_response_from_notification,
+            subject=template_with_content.subject,
+        )
+
+    resp = create_resp_partial(
+        notification=notification,
+        url_root=request.url_root,
+        scheduled_for=scheduled_for,
+        content=template_with_content.content_with_placeholders_filled_in,
+    )
+    return jsonify(resp), 201
 
 
-def process_sms_or_email_notification(
-    *, form, notification_type, api_key, template, template_process_type, service, reply_to_text=None
-):
-    notification_id = uuid.uuid4()
+def process_sms_or_email_notification(*, form, notification_type, api_key, template, service, reply_to_text=None):
+    notification_id = None
     form_send_to = form['email_address'] if notification_type == EMAIL_TYPE else form['phone_number']
 
     send_to = validate_and_format_recipient(send_to=form_send_to,
@@ -184,46 +212,27 @@ def process_sms_or_email_notification(
         service,
         simulated=simulated
     )
-    if document_download_count:
-        # We changed personalisation which means we need to update the content
-        template.values = personalisation
-    api_key_id = api_key.id
-    key_type = api_key.key_type
-    service_in_research_mode = service.research_mode
-    template_version = template._template['version']
-    resp = create_response_for_post_notification(
-        notification_id=notification_id,
-        client_reference=form.get('reference', None),
-        template_id=template.id,
-        template_version=template._template['version'],
-        service_id=service.id,
-        notification_type=notification_type,
-        reply_to=reply_to_text,
-        scheduled_for=form.get("scheduled_for", None),
-        template_with_content=template)
 
-    if str(service.id) in current_app.config.get('HIGH_VOLUME_SERVICE') and key_type == KEY_TYPE_NORMAL \
+    if str(service.id) in current_app.config.get('HIGH_VOLUME_SERVICE') and api_key.key_type == KEY_TYPE_NORMAL \
        and notification_type == EMAIL_TYPE:
         # Put GOV.UK Email notifications onto a queue
         # To take the pressure off the db for API requests put the notification for our high volume service onto a queue
         # the task will then save the notification, then call send_notification_to_queue.
         # We know that this team does not use the GET request, but relies on callbacks to get the status updates.
         try:
-            db.session.commit()
-            save_email_to_queue(
+            notification_id = uuid.uuid4()
+            notification = save_email_to_queue(
                 form=form,
                 notification_id=str(notification_id),
                 notification_type=notification_type,
-                api_key_id=api_key_id,
-                key_type=key_type,
-                template_id=template.id,
-                template_version=template_version,
+                api_key=api_key,
+                template=template,
                 service_id=service.id,
                 personalisation=personalisation,
                 document_download_count=document_download_count,
                 reply_to_text=reply_to_text
             )
-            return resp
+            return notification
         except SQSError:
             # if SQS cannot put the task on the queue, it's probably because the notification body was too long and it
             # went over SQS's 256kb message limit. If so, we
@@ -231,16 +240,16 @@ def process_sms_or_email_notification(
                 f'Notification {notification_id} failed to save to high volume queue. Using normal flow instead'
             )
 
-    persist_notification(
+    notification = persist_notification(
         notification_id=notification_id,
         template_id=template.id,
-        template_version=template_version,
+        template_version=template._template['version'],
         recipient=form_send_to,
         service=service,
         personalisation=personalisation,
         notification_type=notification_type,
-        api_key_id=api_key_id,
-        key_type=key_type,
+        api_key_id=api_key.id,
+        key_type=api_key.key_type,
         client_reference=form.get('reference', None),
         simulated=simulated,
         reply_to_text=reply_to_text,
@@ -249,21 +258,19 @@ def process_sms_or_email_notification(
 
     scheduled_for = form.get("scheduled_for", None)
     if scheduled_for:
-        persist_scheduled_notification(notification_id, form["scheduled_for"])
+        persist_scheduled_notification(notification.id, form["scheduled_for"])
     else:
         if not simulated:
-            queue_name = QueueNames.PRIORITY if template_process_type == PRIORITY else None
-            send_notification_to_queue_detached(
-                key_type=key_type,
-                notification_type=notification_type,
-                notification_id=notification_id,
-                research_mode=service_in_research_mode,  # research_mode is deprecated
+            queue_name = QueueNames.PRIORITY if template._template['process_type'] == PRIORITY else None
+            send_notification_to_queue(
+                notification=notification,
+                research_mode=service.research_mode,
                 queue=queue_name
             )
         else:
-            current_app.logger.debug("POST simulated notification for id: {}".format(notification_id))
+            current_app.logger.debug("POST simulated notification for id: {}".format(notification.id))
 
-    return resp
+    return notification
 
 
 def save_email_to_queue(
@@ -271,26 +278,23 @@ def save_email_to_queue(
     notification_id,
     form,
     notification_type,
-    api_key_id,
-    key_type,
-    template_id,
-    template_version,
+    api_key,
+    template,
     service_id,
     personalisation,
     document_download_count,
     reply_to_text=None
 ):
-    db.session.commit()
     data = {
         "id": notification_id,
-        "template_id": str(template_id),
-        "template_version": template_version,
+        "template_id": str(template.id),
+        "template_version": template._template['version'],
         "to": form['email_address'],
         "service_id": str(service_id),
         "personalisation": personalisation,
         "notification_type": notification_type,
-        "api_key_id": str(api_key_id),
-        "key_type": key_type,
+        "api_key_id": str(api_key.id),
+        "key_type": api_key.key_type,
         "client_reference": form.get('reference', None),
         "reply_to_text": reply_to_text,
         "document_download_count": document_download_count,
@@ -335,9 +339,7 @@ def process_document_uploads(personalisation_data, service, simulated=False):
     return personalisation_data, len(file_keys)
 
 
-def process_letter_notification(
-    *, letter_data, api_key, template, template_with_content, reply_to_text, precompiled=False
-):
+def process_letter_notification(*, letter_data, api_key, template, service, reply_to_text, precompiled=False):
     if api_key.key_type == KEY_TYPE_TEAM:
         raise BadRequestError(message='Cannot send letters with a team api key', status_code=403)
 
@@ -348,70 +350,24 @@ def process_letter_notification(
         return process_precompiled_letter_notifications(letter_data=letter_data,
                                                         api_key=api_key,
                                                         template=template,
+                                                        service=service,
                                                         reply_to_text=reply_to_text)
 
-    validate_address(api_key, letter_data)
-
-    test_key = api_key.key_type == KEY_TYPE_TEST
-
-    status = NOTIFICATION_CREATED
-    updated_at = None
-    if test_key:
-        # if we don't want to actually send the letter, then start it off in SENDING so we don't pick it up
-        if current_app.config['NOTIFY_ENVIRONMENT'] in ['preview', 'development']:
-            status = NOTIFICATION_SENDING
-        # mark test letter as delivered and do not create a fake response later
-        else:
-            status = NOTIFICATION_DELIVERED
-            updated_at = datetime.utcnow()
-
-    queue = QueueNames.CREATE_LETTERS_PDF if not test_key else QueueNames.RESEARCH_MODE
-
-    notification = create_letter_notification(letter_data=letter_data,
-                                              template=template,
-                                              api_key=api_key,
-                                              status=status,
-                                              reply_to_text=reply_to_text,
-                                              updated_at=updated_at
-                                              )
-
-    get_pdf_for_templated_letter.apply_async(
-        [str(notification.id)],
-        queue=queue
-    )
-
-    if test_key and current_app.config['NOTIFY_ENVIRONMENT'] in ['preview', 'development']:
-        create_fake_letter_response_file.apply_async(
-            (notification.reference,),
-            queue=queue
-        )
-    resp = create_response_for_post_notification(
-        notification_id=notification.id,
-        client_reference=notification.client_reference,
-        template_id=notification.template_id,
-        template_version=notification.template_version,
-        notification_type=notification.notification_type,
-        reply_to=reply_to_text,
-        scheduled_for=letter_data.get('scheduled_for', None),
-        service_id=notification.service_id,
-        template_with_content=template_with_content
-    )
-    return resp
-
-
-def validate_address(api_key, letter_data):
     address = PostalAddress.from_personalisation(
         letter_data['personalisation'],
         allow_international_letters=api_key.service.has_permission(INTERNATIONAL_LETTERS),
     )
+
     if not address.has_enough_lines:
         raise ValidationError(
             message=f'Address must be at least {PostalAddress.MIN_LINES} lines'
         )
+
     if address.has_too_many_lines:
         raise ValidationError(
             message=f'Address must be no more than {PostalAddress.MAX_LINES} lines'
         )
+
     if not address.has_valid_last_line:
         if address.allow_international_letters:
             raise ValidationError(
@@ -421,8 +377,37 @@ def validate_address(api_key, letter_data):
             message='Must be a real UK postcode'
         )
 
+    test_key = api_key.key_type == KEY_TYPE_TEST
 
-def process_precompiled_letter_notifications(*, letter_data, api_key, template, reply_to_text):
+    # if we don't want to actually send the letter, then start it off in SENDING so we don't pick it up
+    status = NOTIFICATION_CREATED if not test_key else NOTIFICATION_SENDING
+    queue = QueueNames.CREATE_LETTERS_PDF if not test_key else QueueNames.RESEARCH_MODE
+
+    notification = create_letter_notification(letter_data=letter_data,
+                                              template=template,
+                                              service=service,
+                                              api_key=api_key,
+                                              status=status,
+                                              reply_to_text=reply_to_text)
+
+    get_pdf_for_templated_letter.apply_async(
+        [str(notification.id)],
+        queue=queue
+    )
+
+    if test_key:
+        if current_app.config['NOTIFY_ENVIRONMENT'] in ['preview', 'development']:
+            create_fake_letter_response_file.apply_async(
+                (notification.reference,),
+                queue=queue
+            )
+        else:
+            update_notification_status_by_reference(notification.reference, NOTIFICATION_DELIVERED)
+
+    return notification
+
+
+def process_precompiled_letter_notifications(*, letter_data, api_key, template, service, reply_to_text):
     try:
         status = NOTIFICATION_PENDING_VIRUS_CHECK
         letter_content = base64.b64decode(letter_data['content'])
@@ -431,16 +416,11 @@ def process_precompiled_letter_notifications(*, letter_data, api_key, template,
 
     notification = create_letter_notification(letter_data=letter_data,
                                               template=template,
+                                              service=service,
                                               api_key=api_key,
                                               status=status,
                                               reply_to_text=reply_to_text)
 
-    resp = {
-        'id': notification.id,
-        'reference': notification.client_reference,
-        'postage': notification.postage
-    }
-
     filename = upload_letter_pdf(notification, letter_content, precompiled=True)
 
     current_app.logger.info('Calling task scan-file for {}'.format(filename))
@@ -459,7 +439,7 @@ def process_precompiled_letter_notifications(*, letter_data, api_key, template,
             queue=QueueNames.LETTERS
         )
 
-    return resp
+    return notification
 
 
 def get_reply_to_text(notification_type, form, template):
@@ -468,7 +448,7 @@ def get_reply_to_text(notification_type, form, template):
         service_email_reply_to_id = form.get("email_reply_to_id", None)
         reply_to = check_service_email_reply_to_id(
             str(authenticated_service.id), service_email_reply_to_id, notification_type
-        ) or template.get_reply_to_text()
+        ) or template._template['reply_to_text']
 
     elif notification_type == SMS_TYPE:
         service_sms_sender_id = form.get("sms_sender_id", None)
@@ -478,37 +458,9 @@ def get_reply_to_text(notification_type, form, template):
         if sms_sender_id:
             reply_to = try_validate_and_format_phone_number(sms_sender_id)
         else:
-            reply_to = template.get_reply_to_text()
+            reply_to = template._template['reply_to_text']
 
     elif notification_type == LETTER_TYPE:
-        reply_to = template.get_reply_to_text()
+        reply_to = template._template['reply_to_text']
 
     return reply_to
-
-
-def create_response_for_post_notification(notification_id, client_reference, template_id, template_version, service_id,
-                                          notification_type, reply_to, scheduled_for,
-                                          template_with_content):
-    if notification_type == SMS_TYPE:
-        create_resp_partial = functools.partial(
-            create_post_sms_response_from_notification,
-            from_number=reply_to,
-        )
-    elif notification_type == EMAIL_TYPE:
-        create_resp_partial = functools.partial(
-            create_post_email_response_from_notification,
-            subject=template_with_content.subject,
-            email_from='{}@{}'.format(authenticated_service.email_from, current_app.config['NOTIFY_EMAIL_DOMAIN']),
-        )
-    elif notification_type == LETTER_TYPE:
-        create_resp_partial = functools.partial(
-            create_post_letter_response_from_notification,
-            subject=template_with_content.subject,
-        )
-    resp = create_resp_partial(
-        notification_id, client_reference, template_id, template_version, service_id,
-        url_root=request.url_root,
-        scheduled_for=scheduled_for,
-        content=template_with_content.content_with_placeholders_filled_in,
-    )
-    return resp

requirements-app.txt

@@ -26,8 +26,6 @@ notifications-python-client==5.5.1
 # PaaS
 awscli-cwlogs>=1.4,<1.5
 
-git+https://github.com/alphagov/notifications-utils.git@39.6.0#egg=notifications-utils==39.6.0
+git+https://github.com/alphagov/notifications-utils.git@39.4.4#egg=notifications-utils==39.4.4
 
-# gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains
-prometheus-client==0.7.1
 gds-metrics==0.2.0

requirements.txt

@@ -28,10 +28,8 @@ notifications-python-client==5.5.1
 # PaaS
 awscli-cwlogs>=1.4,<1.5
 
-git+https://github.com/alphagov/notifications-utils.git@39.6.0#egg=notifications-utils==39.6.0
+git+https://github.com/alphagov/notifications-utils.git@39.4.4#egg=notifications-utils==39.4.4
 
-# gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains
-prometheus-client==0.7.1
 gds-metrics==0.2.0
 
 ## The following requirements were added by pip freeze:
@@ -39,15 +37,14 @@ alembic==1.4.2
 amqp==1.4.9
 anyjson==0.3.3
 attrs==19.3.0
-awscli==1.18.82
+awscli==1.18.75
 bcrypt==3.1.7
 billiard==3.3.0.23
 bleach==3.1.4
 blinker==1.4
 boto==2.49.0
 boto3==1.10.38
-botocore==1.17.5
-cachetools==4.1.0
+botocore==1.16.25
 certifi==2020.4.5.2
 chardet==3.0.4
 click==7.1.2
@@ -69,6 +66,7 @@ mistune==0.8.4
 monotonic==1.5
 orderedset==2.0.1
 phonenumbers==8.11.2
+prometheus-client==0.2.0
 pyasn1==0.4.8
 pycparser==2.20
 PyPDF2==1.26.0
@@ -79,7 +77,7 @@ python-json-logger==0.1.11
 pytz==2020.1
 PyYAML==5.3.1
 redis==3.5.3
-requests==2.24.0
+requests==2.23.0
 rsa==3.4.2
 s3transfer==0.3.3
 six==1.15.0

tests/app/notifications/test_process_letter_notifications.py

@@ -2,6 +2,8 @@ from app.models import LETTER_TYPE
 from app.models import Notification
 from app.models import NOTIFICATION_CREATED
 from app.notifications.process_letter_notifications import create_letter_notification
+from app.notifications.process_notifications import create_content_for_notification
+from app.notifications.validators import get_template_dict
 
 
 def test_create_letter_notification_creates_notification(sample_letter_template, sample_api_key):
@@ -13,7 +15,17 @@ def test_create_letter_notification_creates_notification(sample_letter_template,
         }
     }
 
-    notification = create_letter_notification(data, sample_letter_template, sample_api_key, NOTIFICATION_CREATED)
+    template = create_content_for_notification(get_template_dict(
+        sample_letter_template.id, sample_letter_template.service_id
+    ), {})
+
+    notification = create_letter_notification(
+        data,
+        template,
+        sample_letter_template.service,
+        sample_api_key,
+        NOTIFICATION_CREATED,
+    )
 
     assert notification == Notification.query.one()
     assert notification.job is None
@@ -38,7 +50,17 @@ def test_create_letter_notification_sets_reference(sample_letter_template, sampl
         'reference': 'foo'
     }
 
-    notification = create_letter_notification(data, sample_letter_template, sample_api_key, NOTIFICATION_CREATED)
+    template = create_content_for_notification(get_template_dict(
+        sample_letter_template.id, sample_letter_template.service_id
+    ), {})
+
+    notification = create_letter_notification(
+        data,
+        template,
+        sample_letter_template.service,
+        sample_api_key,
+        NOTIFICATION_CREATED,
+    )
 
     assert notification.client_reference == 'foo'
 
@@ -52,7 +74,17 @@ def test_create_letter_notification_sets_billable_units(sample_letter_template,
         },
     }
 
-    notification = create_letter_notification(data, sample_letter_template, sample_api_key, NOTIFICATION_CREATED,
-                                              billable_units=3)
+    template = create_content_for_notification(get_template_dict(
+        sample_letter_template.id, sample_letter_template.service_id
+    ), {})
+
+    notification = create_letter_notification(
+        data,
+        template,
+        sample_letter_template.service,
+        sample_api_key,
+        NOTIFICATION_CREATED,
+        billable_units=3,
+    )
 
     assert notification.billable_units == 3

tests/app/notifications/test_process_notification.py

@@ -21,37 +21,43 @@ from app.notifications.process_notifications import (
     send_notification_to_queue,
     simulated_recipient
 )
+from app.notifications.validators import get_template_dict
 from notifications_utils.recipients import validate_and_format_phone_number, validate_and_format_email_address
 from app.v2.errors import BadRequestError
-from tests.app.db import create_service, create_template, 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)
-    content = create_content_for_notification(template, None)
+    template_dict = get_template_dict(template.id, template.service_id)
+    content = create_content_for_notification(template_dict, None)
     assert str(content) == template.content + '\n'
 
 
 def test_create_content_for_notification_with_placeholders_passes(sample_template_with_placeholders):
     template = Template.query.get(sample_template_with_placeholders.id)
-    content = create_content_for_notification(template, {'name': 'Bobby'})
+    template_dict = get_template_dict(template.id, template.service_id)
+    content = create_content_for_notification(template_dict, {'name': 'Bobby'})
     assert content.content == template.content
     assert 'Bobby' in str(content)
 
 
 def test_create_content_for_notification_fails_with_missing_personalisation(sample_template_with_placeholders):
     template = Template.query.get(sample_template_with_placeholders.id)
+    template_dict = get_template_dict(template.id, template.service_id)
     with pytest.raises(BadRequestError):
-        create_content_for_notification(template, None)
+        create_content_for_notification(template_dict, None)
 
 
 def test_create_content_for_notification_allows_additional_personalisation(sample_template_with_placeholders):
     template = Template.query.get(sample_template_with_placeholders.id)
-    create_content_for_notification(template, {'name': 'Bobby', 'Additional placeholder': 'Data'})
+    template_dict = get_template_dict(template.id, template.service_id)
+    create_content_for_notification(template_dict, {'name': 'Bobby', 'Additional placeholder': 'Data'})
 
 
 @freeze_time("2016-01-01 11:09:00.061258")
-def test_persist_notification_creates_and_save_to_db(sample_template, sample_api_key, sample_job):
+def test_persist_notification_creates_and_save_to_db(sample_template, sample_api_key, sample_job, mocker):
+    mocked_redis = mocker.patch('app.notifications.process_notifications.redis_store.get')
 
     assert Notification.query.count() == 0
     assert NotificationHistory.query.count() == 0
@@ -90,6 +96,8 @@ def test_persist_notification_creates_and_save_to_db(sample_template, sample_api
     assert notification_from_db.created_by_id == notification.created_by_id
     assert notification_from_db.reply_to_text == sample_template.service.get_default_sms_sender()
 
+    mocked_redis.assert_called_once_with(str(sample_template.service_id) + "-2016-01-01-count")
+
 
 def test_persist_notification_throws_exception_when_missing_template(sample_api_key):
     assert Notification.query.count() == 0
@@ -154,9 +162,10 @@ def test_persist_notification_does_not_increment_cache_if_test_key(
 
 
 @freeze_time("2016-01-01 11:09:00.061258")
-def test_persist_notification_with_optionals(sample_job, sample_api_key):
+def test_persist_notification_with_optionals(sample_job, sample_api_key, mocker):
     assert Notification.query.count() == 0
     assert NotificationHistory.query.count() == 0
+    mocked_redis = mocker.patch('app.notifications.process_notifications.redis_store.get')
     n_id = uuid.uuid4()
     created_at = datetime.datetime(2016, 11, 11, 16, 8, 18)
     persist_notification(
@@ -182,7 +191,7 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key):
     persisted_notification.job_id == sample_job.id
     assert persisted_notification.job_row_number == 10
     assert persisted_notification.created_at == created_at
-
+    mocked_redis.assert_called_once_with(str(sample_job.service_id) + "-2016-01-01-count")
     assert persisted_notification.client_reference == "ref from client"
     assert persisted_notification.reference is None
     assert persisted_notification.international is False
@@ -193,77 +202,44 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key):
 
 
 @freeze_time("2016-01-01 11:09:00.061258")
-def test_persist_notification_doesnt_touch_cache_for_old_keys_that_dont_exist(notify_db_session, mocker):
-    service = create_service(restricted=True)
-    template = create_template(service=service)
-    api_key = create_api_key(service=service)
+def test_persist_notification_doesnt_touch_cache_for_old_keys_that_dont_exist(sample_template, sample_api_key, mocker):
     mock_incr = mocker.patch('app.notifications.process_notifications.redis_store.incr')
     mocker.patch('app.notifications.process_notifications.redis_store.get', return_value=None)
     mocker.patch('app.notifications.process_notifications.redis_store.get_all_from_hash', return_value=None)
 
     persist_notification(
-        template_id=template.id,
-        template_version=template.version,
+        template_id=sample_template.id,
+        template_version=sample_template.version,
         recipient='+447111111111',
-        service=template.service,
+        service=sample_template.service,
         personalisation={},
         notification_type='sms',
-        api_key_id=api_key.id,
-        key_type=api_key.key_type,
+        api_key_id=sample_api_key.id,
+        key_type=sample_api_key.key_type,
         reference="ref"
     )
     mock_incr.assert_not_called()
 
 
 @freeze_time("2016-01-01 11:09:00.061258")
-def test_persist_notification_increments_cache_if_key_exists_and_for_trial_service(
-    notify_db_session, mocker
-):
-    service = create_service(restricted=True)
-    template = create_template(service=service)
-    api_key = create_api_key(service=service)
+def test_persist_notification_increments_cache_if_key_exists(sample_template, sample_api_key, mocker):
     mock_incr = mocker.patch('app.notifications.process_notifications.redis_store.incr')
     mocker.patch('app.notifications.process_notifications.redis_store.get', return_value=1)
     mocker.patch('app.notifications.process_notifications.redis_store.get_all_from_hash',
-                 return_value={template.id, 1})
+                 return_value={sample_template.id, 1})
 
     persist_notification(
-        template_id=template.id,
-        template_version=template.version,
+        template_id=sample_template.id,
+        template_version=sample_template.version,
         recipient='+447111111122',
-        service=template.service,
+        service=sample_template.service,
         personalisation={},
         notification_type='sms',
-        api_key_id=api_key.id,
-        key_type=api_key.key_type,
+        api_key_id=sample_api_key.id,
+        key_type=sample_api_key.key_type,
         reference="ref2")
 
-    mock_incr.assert_called_once_with(str(service.id) + "-2016-01-01-count", )
-
-
-def test_persist_notification_does_not_increments_cache_live_service(
-    notify_db_session, mocker
-):
-    service = create_service(restricted=False)
-    template = create_template(service=service)
-    api_key = create_api_key(service=service)
-    mock_incr = mocker.patch('app.notifications.process_notifications.redis_store.incr')
-    mocker.patch('app.notifications.process_notifications.redis_store.get', return_value=1)
-    mocker.patch('app.notifications.process_notifications.redis_store.get_all_from_hash',
-                 return_value={template.id, 1})
-
-    persist_notification(
-        template_id=template.id,
-        template_version=template.version,
-        recipient='+447111111122',
-        service=template.service,
-        personalisation={},
-        notification_type='sms',
-        api_key_id=api_key.id,
-        key_type=api_key.key_type,
-        reference="ref2")
-
-    assert not mock_incr.called
+    mock_incr.assert_called_once_with(str(sample_template.service_id) + "-2016-01-01-count", )
 
 
 @pytest.mark.parametrize((

tests/app/notifications/test_validators.py

@@ -19,6 +19,7 @@ from app.notifications.validators import (
     check_service_sms_sender_id,
     check_service_letter_contact_id,
     check_reply_to,
+    get_template_dict,
     service_can_send_to_recipient,
     validate_and_format_recipient,
     validate_template,
@@ -175,15 +176,17 @@ def test_check_template_is_for_notification_type_fails_when_template_type_does_n
 
 
 def test_check_template_is_active_passes(sample_template):
-    assert check_template_is_active(sample_template) is None
+    template_dict = get_template_dict(sample_template.id, sample_template.service_id)
+    assert check_template_is_active(template_dict) is None
 
 
 def test_check_template_is_active_fails(sample_template):
     sample_template.archived = True
     from app.dao.templates_dao import dao_update_template
     dao_update_template(sample_template)
+    template_dict = get_template_dict(sample_template.id, sample_template.service_id)
     with pytest.raises(BadRequestError) as e:
-        check_template_is_active(sample_template)
+        check_template_is_active(template_dict)
     assert e.value.status_code == 400
     assert e.value.message == 'Template has been deleted'
     assert e.value.fields == [{'template': 'Template has been deleted'}]
@@ -314,11 +317,11 @@ def test_check_content_char_count_passes_for_long_email_or_letter(sample_service
 
 def test_check_notification_content_is_not_empty_passes(notify_api, mocker, sample_service):
     template_id = create_template(sample_service, content="Content is not empty").id
-    template = templates_dao.dao_get_template_by_id_and_service_id(
+    template_dict = get_template_dict(
         template_id=template_id,
         service_id=sample_service.id
     )
-    template_with_content = create_content_for_notification(template, {})
+    template_with_content = create_content_for_notification(template_dict, {})
     assert check_notification_content_is_not_empty(template_with_content) is None
 
 
@@ -330,11 +333,11 @@ def test_check_notification_content_is_not_empty_fails(
     notify_api, mocker, sample_service, template_content, notification_values
 ):
     template_id = create_template(sample_service, content=template_content).id
-    template = templates_dao.dao_get_template_by_id_and_service_id(
+    template_dict = get_template_dict(
         template_id=template_id,
         service_id=sample_service.id
     )
-    template_with_content = create_content_for_notification(template, notification_values)
+    template_with_content = create_content_for_notification(template_dict, notification_values)
     with pytest.raises(BadRequestError) as e:
         check_notification_content_is_not_empty(template_with_content)
     assert e.value.status_code == 400
@@ -349,6 +352,10 @@ def test_validate_template(sample_service):
 
 def test_validate_template_calls_all_validators(mocker, fake_uuid, sample_service):
     template = create_template(sample_service, template_type="email")
+    template_dict = get_template_dict(
+        template_id=template.id,
+        service_id=sample_service.id
+    )
     mock_check_type = mocker.patch('app.notifications.validators.check_template_is_for_notification_type')
     mock_check_if_active = mocker.patch('app.notifications.validators.check_template_is_active')
     mock_create_conent = mocker.patch(
@@ -359,8 +366,8 @@ def test_validate_template_calls_all_validators(mocker, fake_uuid, sample_servic
     validate_template(template.id, {}, sample_service, "email")
 
     mock_check_type.assert_called_once_with("email", "email")
-    mock_check_if_active.assert_called_once_with(template)
-    mock_create_conent.assert_called_once_with(template, {})
+    mock_check_if_active.assert_called_once_with(template_dict)
+    mock_create_conent.assert_called_once_with(template_dict, {})
     mock_check_not_empty.assert_called_once_with("content")
     mock_check_message_is_too_long.assert_called_once_with("content")
 

tests/app/service/test_rest.py

@@ -235,6 +235,7 @@ def test_get_service_by_id(admin_request, sample_service):
     assert json_resp['data']['email_branding'] is None
     assert 'branding' not in json_resp['data']
     assert json_resp['data']['prefix_sms'] is True
+    assert json_resp['data']['letter_logo_filename'] is None
 
 
 @pytest.mark.parametrize('detailed', [True, False])
@@ -346,6 +347,7 @@ def test_create_service(
     assert json_resp['data']['name'] == 'created service'
     assert json_resp['data']['email_from'] == 'created.service'
     assert not json_resp['data']['research_mode']
+    assert json_resp['data']['rate_limit'] == 3000
     assert json_resp['data']['letter_branding'] is None
     assert json_resp['data']['count_as_live'] is expected_count_as_live
 
@@ -1223,6 +1225,7 @@ def test_add_existing_user_to_another_service_with_all_permissions(
             )
             assert resp.status_code == 200
             json_resp = resp.json
+            assert str(user_to_add.id) in json_resp['data']['users']
 
             # check user has all permissions
             auth_header = create_authorization_header()

tests/app/v2/notifications/test_post_letter_notifications.py

@@ -245,7 +245,6 @@ def test_post_letter_notification_with_test_key_creates_pdf_and_sets_status_to_d
     fake_create_letter_task.assert_called_once_with([str(notification.id)], queue='research-mode-tasks')
     assert not fake_create_dvla_response_task.called
     assert notification.status == NOTIFICATION_DELIVERED
-    assert notification.updated_at is not None
 
 
 @pytest.mark.parametrize('env', [

tests/app/v2/notifications/test_post_notifications.py

@@ -49,7 +49,6 @@ def test_post_sms_notification_returns_201(client, sample_template_with_placehol
         path='/v2/notifications/sms',
         data=json.dumps(data),
         headers=[('Content-Type', 'application/json'), auth_header])
-
     assert response.status_code == 201
     resp_json = json.loads(response.get_data(as_text=True))
     assert validate(resp_json, post_sms_response) == resp_json
@@ -425,7 +424,7 @@ def test_returns_a_429_limit_exceeded_if_rate_limit_exceeded(
 ):
     sample = create_template(service=sample_service, template_type=notification_type)
     persist_mock = mocker.patch('app.v2.notifications.post_notifications.persist_notification')
-    deliver_mock = mocker.patch('app.v2.notifications.post_notifications.send_notification_to_queue_detached')
+    deliver_mock = mocker.patch('app.v2.notifications.post_notifications.send_notification_to_queue')
     mocker.patch(
         'app.v2.notifications.post_notifications.check_rate_limiting',
         side_effect=RateLimitError("LIMIT", "INTERVAL", "TYPE"))
@@ -1006,14 +1005,11 @@ def test_post_notifications_saves_email_normally_if_save_email_to_queue_fails(cl
         "template_id": template.id,
         "personalisation": {"message": "Dear citizen, have a nice day"}
     }
-
-    print("******** Start")
     response = client.post(
         path='/v2/notifications/email',
         data=json.dumps(data),
         headers=[('Content-Type', 'application/json'), create_authorization_header(service_id=service.id)]
     )
-    print("********** End")
 
     json_resp = response.get_json()
 

tests/app/v2/test_errors.py

@@ -4,7 +4,7 @@ from sqlalchemy.exc import DataError
 
 
 @pytest.fixture(scope='function')
-def app_for_test():
+def app_for_test(mocker):
     import flask
     from flask import Blueprint
     from app.authentication.auth import AuthError