Refactor

We think that holding open database transactions while we go and do
something else is causing us to have poor performance.

Because we’re not serialising everything as soon as we pull it out of
the database we can guarantee that we don’t need to go back to the
database again.

So let’s see if explicitly closing the transaction helps with
performance.

app/authentication/auth.py

@@ -6,18 +6,17 @@ 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
+from app import db
+from app.dao.services_dao import dao_fetch_service_by_id
+from app.serialised_models import (
+    SerialisedAPIKeyCollection,
+    SerialisedService,
+)
 
 
 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 +92,9 @@ 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 = SerialisedService.from_id(issuer)
+        service.api_keys = SerialisedAPIKeyCollection.from_service_id(issuer)
+        db.session.commit()
     except DataError:
         raise AuthError("Invalid token: service id is not the right data type", 403)
     except NoResultFound:
@@ -129,7 +129,7 @@ def requires_auth():
         if api_key.expiry_date:
             raise AuthError("Invalid token: API key revoked", 403, service_id=service.id, api_key_id=api_key.id)
 
-        g.service_id = api_key.service_id
+        g.service_id = service.id
         _request_ctx_stack.top.authenticated_service = service
         _request_ctx_stack.top.api_user = api_key
 

app/notifications/process_letter_notifications.py

@@ -5,14 +5,22 @@ from app.models import LETTER_TYPE
 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):
+def create_letter_notification(
+    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,
         # 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,

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
@@ -44,7 +49,34 @@ REDIS_GET_AND_INCR_DAILY_LIMIT_DURATION_SECONDS = Histogram(
 
 
 def create_content_for_notification(template, personalisation):
-    template_object = template._as_utils_template_with_personalisation(personalisation)
+    if template.template_type == EMAIL_TYPE:
+        template_object = PlainTextEmailTemplate(
+            {
+                'content': template.content,
+                'subject': template.subject,
+                'template_type': template.template_type,
+            },
+            personalisation,
+        )
+    if template.template_type == SMS_TYPE:
+        template_object = SMSMessageTemplate(
+            {
+                'content': template.content,
+                'template_type': template.template_type,
+            },
+            personalisation,
+        )
+    if template.template_type == LETTER_TYPE:
+        template_object = LetterPrintTemplate(
+            {
+                'content': template.content,
+                'subject': template.subject,
+                'template_type': template.template_type,
+            },
+            personalisation,
+            contact_block=template.reply_to_text,
+        )
+
     check_placeholders(template_object)
 
     return template_object
@@ -90,7 +122,6 @@ def persist_notification(
         template_version=template_version,
         to=recipient,
         service_id=service.id,
-        service=service,
         personalisation=personalisation,
         notification_type=notification_type,
         api_key_id=api_key_id,

app/notifications/rest.py

@@ -128,7 +128,7 @@ 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.reply_to_text
                                               )
     if not simulated:
         queue_name = QueueNames.PRIORITY if template.process_type == PRIORITY else None
@@ -164,7 +164,7 @@ def _service_can_send_internationally(service, number):
     international_phone_info = get_international_phone_info(number)
 
     if international_phone_info.international and \
-            INTERNATIONAL_SMS_TYPE not in [p.permission for p in service.permissions]:
+            INTERNATIONAL_SMS_TYPE not in service.permissions:
         raise InvalidRequest(
             {'to': ["Cannot send to international mobile numbers"]},
             status_code=400

app/notifications/validators.py

@@ -8,7 +8,7 @@ from notifications_utils.recipients import (
 )
 from notifications_utils.clients.redis import rate_limit_cache_key, daily_limit_cache_key
 
-from app.dao import services_dao, templates_dao
+from app.dao import services_dao
 from app.dao.service_sms_sender_dao import dao_get_service_sms_senders_by_id
 from app.models import (
     INTERNATIONAL_SMS_TYPE, SMS_TYPE, EMAIL_TYPE, LETTER_TYPE,
@@ -21,6 +21,7 @@ from app.notifications.process_notifications import create_content_for_notificat
 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 app.serialised_models import SerialisedTemplate
 
 from gds_metrics.metrics import Histogram
 
@@ -89,7 +90,7 @@ def service_can_send_to_recipient(send_to, key_type, service, allow_whitelisted_
 
 
 def service_has_permission(notify_type, permissions):
-    return notify_type in [p.permission for p in permissions]
+    return notify_type in permissions
 
 
 def check_service_has_permission(notify_type, permissions):
@@ -123,7 +124,7 @@ def validate_and_format_recipient(send_to, key_type, service, notification_type,
         international_phone_info = get_international_phone_info(send_to)
 
         if international_phone_info.international and \
-                INTERNATIONAL_SMS_TYPE not in [p.permission for p in service.permissions]:
+                INTERNATIONAL_SMS_TYPE not in service.permissions:
             raise BadRequestError(message="Cannot send to international mobile numbers")
 
         return validate_and_format_phone_number(
@@ -148,11 +149,9 @@ def check_notification_content_is_not_empty(template_with_content):
 
 
 def validate_template(template_id, personalisation, service, notification_type):
+
     try:
-        template = templates_dao.dao_get_template_by_id_and_service_id(
-            template_id=template_id,
-            service_id=service.id
-        )
+        template = SerialisedTemplate.from_template_id_and_service_id(template_id, service.id)
     except NoResultFound:
         message = 'Template not found'
         raise BadRequestError(message=message,

app/serialised_models.py

@@ -0,0 +1,151 @@
+from abc import ABC, abstractmethod
+from collections import defaultdict
+from functools import partial
+from threading import RLock
+
+import cachetools
+
+from gds_metrics import Histogram
+
+from app import db
+from app.dao.services_dao import dao_fetch_service_by_id
+from app.dao.api_key_dao import get_model_api_keys
+
+caches = defaultdict(partial(cachetools.TTLCache, maxsize=1024, ttl=2))
+locks = defaultdict(RLock)
+
+
+AUTH_DB_CONNECTION_DURATION_SECONDS = Histogram(
+    'auth_db_connection_duration_seconds',
+    'Time taken to get DB connection and fetch service from database',
+)
+
+
+def cache(func):
+    @cachetools.cached(
+        cache=caches[func.__qualname__],
+        lock=locks[func.__qualname__],
+    )
+    def wrapper(*args, **kwargs):
+        return func(*args, **kwargs)
+
+    return wrapper
+
+
+class SerialisedModel(ABC):
+
+    """
+    A SerialisedModel takes a dictionary, typically created by
+    serialising a database object. It then takes the value of specified
+    keys from the dictionary and adds them to itself as properties, so
+    that it can be interacted with like a normal database model object,
+    but with no risk that it will actually go back to the database.
+    """
+
+    @property
+    @abstractmethod
+    def ALLOWED_PROPERTIES(self):
+        pass
+
+    def __init__(self, _dict):
+        for property in self.ALLOWED_PROPERTIES:
+            setattr(self, property, _dict[property])
+
+    def __dir__(self):
+        return super().__dir__() + list(sorted(self.ALLOWED_PROPERTIES))
+
+
+class SerialisedModelCollection(ABC):
+
+    """
+    A SerialisedModelCollection takes a list of dictionaries, typically
+    created by serialising database objects. When iterated over it
+    returns a SerialisedModel instance for each of the items in the list.
+    """
+
+    @property
+    @abstractmethod
+    def model(self):
+        pass
+
+    def __init__(self, items):
+        self.items = items
+
+    def __bool__(self):
+        return bool(self.items)
+
+    def __getitem__(self, index):
+        return self.model(self.items[index])
+
+
+class SerialisedTemplate(SerialisedModel):
+
+    ALLOWED_PROPERTIES = {
+        'archived',
+        'content',
+        'id',
+        'postage',
+        'process_type',
+        'reply_to_text',
+        'subject',
+        'template_type',
+        'version',
+    }
+
+    @classmethod
+    @cache
+    def from_template_id_and_service_id(cls, template_id, service_id):
+
+        from app.dao.templates_dao import dao_get_template_by_id_and_service_id
+        from app.schemas import template_schema
+
+        fetched_template = dao_get_template_by_id_and_service_id(
+            template_id=template_id,
+            service_id=service_id
+        )
+
+        template_dict = template_schema.dump(fetched_template).data
+
+        db.session.commit()
+        return cls(template_dict)
+
+
+class SerialisedService(SerialisedModel):
+    ALLOWED_PROPERTIES = {
+        'id',
+        'active',
+        'contact_link',
+        'email_from',
+        'permissions',
+        'research_mode',
+        'restricted',
+    }
+
+    @classmethod
+    @cache
+    def from_id(cls, service_id):
+        from app.schemas import service_schema
+        with AUTH_DB_CONNECTION_DURATION_SECONDS.time():
+            fetched = dao_fetch_service_by_id(service_id)
+        return cls(service_schema.dump(fetched).data)
+
+
+class SerialisedAPIKey(SerialisedModel):
+    ALLOWED_PROPERTIES = {
+        'id',
+        'secret',
+        'expiry_date',
+        'key_type',
+    }
+
+
+class SerialisedAPIKeyCollection(SerialisedModelCollection):
+    model = SerialisedAPIKey
+
+    @classmethod
+    @cache
+    def from_service_id(cls, service_id):
+        return cls([
+            {k: getattr(key, k) for k in SerialisedAPIKey.ALLOWED_PROPERTIES}
+            for key in get_model_api_keys(service_id)
+        ])

app/service/send_notification.py

@@ -137,7 +137,9 @@ def get_reply_to_text(notification_type, sender_id, service, template):
 def send_pdf_letter_notification(service_id, post_data):
     service = dao_fetch_service_by_id(service_id)
 
-    check_service_has_permission(LETTER_TYPE, service.permissions)
+    check_service_has_permission(LETTER_TYPE, [
+        p.permission for p in service.permissions
+    ])
     check_service_over_daily_message_limit(KEY_TYPE_NORMAL, service)
     validate_created_by(service, post_data['created_by'])
     validate_and_format_recipient(

app/service/utils.py

@@ -7,6 +7,8 @@ from app.models import (
     MOBILE_TYPE, EMAIL_TYPE,
     KEY_TYPE_TEST, KEY_TYPE_TEAM, KEY_TYPE_NORMAL)
 
+from app.dao.services_dao import dao_fetch_service_by_id
+
 
 def get_recipients_from_request(request_json, key, type):
     return [(type, recipient) for recipient in request_json.get(key)]
@@ -33,6 +35,10 @@ def service_allowed_to_send_to(recipient, service, key_type, allow_whitelisted_r
     if key_type == KEY_TYPE_NORMAL and not service.restricted:
         return True
 
+    # Revert back to the ORM model here so we can get some things which
+    # aren’t in the serialised model
+    service = dao_fetch_service_by_id(service.id)
+
     team_members = itertools.chain.from_iterable(
         [user.mobile_number, user.email_address] for user in service.users
     )

app/template/rest.py

@@ -69,7 +69,9 @@ def validate_parent_folder(template_json):
 def create_template(service_id):
     fetched_service = dao_fetch_service_by_id(service_id=service_id)
     # permissions needs to be placed here otherwise marshmallow will interfere with versioning
-    permissions = fetched_service.permissions
+    permissions = [
+        p.permission for p in fetched_service.permissions
+    ]
     template_json = validate(request.get_json(), post_create_template_schema)
     folder = validate_parent_folder(template_json=template_json)
     new_template = Template.from_json(template_json, folder)
@@ -102,7 +104,12 @@ def create_template(service_id):
 def update_template(service_id, template_id):
     fetched_template = dao_get_template_by_id_and_service_id(template_id=template_id, service_id=service_id)
 
-    if not service_has_permission(fetched_template.template_type, fetched_template.service.permissions):
+    if not service_has_permission(
+        fetched_template.template_type,
+        [
+            p.permission for p in fetched_template.service.permissions
+        ]
+    ):
         message = "Updating {} templates is not allowed".format(
             get_public_notify_type_text(fetched_template.template_type))
         errors = {'template_type': [message]}

app/v2/notifications/post_notifications.py

@@ -98,14 +98,13 @@ 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,
+        service=authenticated_service,
         template=template,
         template_with_content=None,  # not required for precompiled
-        reply_to_text=reply_to,
+        reply_to_text='',  # not required for precompiled
         precompiled=True
     )
 
@@ -147,6 +146,7 @@ def post_notification(notification_type):
         notification = process_letter_notification(
             letter_data=form,
             api_key=api_user,
+            service=authenticated_service,
             template=template,
             template_with_content=template_with_content,
             reply_to_text=reply_to
@@ -156,7 +156,8 @@ def post_notification(notification_type):
             form=form,
             notification_type=notification_type,
             api_key=api_user,
-            template=template_with_content,
+            template=template,
+            template_with_content=template_with_content,
             template_process_type=template.process_type,
             service=authenticated_service,
             reply_to_text=reply_to
@@ -166,7 +167,15 @@ def post_notification(notification_type):
 
 
 def process_sms_or_email_notification(
-    *, form, notification_type, api_key, template, template_process_type, service, reply_to_text=None
+    *,
+    form,
+    notification_type,
+    api_key,
+    template,
+    template_with_content,
+    template_process_type,
+    service,
+    reply_to_text=None,
 ):
     notification_id = uuid.uuid4()
     form_send_to = form['email_address'] if notification_type == EMAIL_TYPE else form['phone_number']
@@ -186,19 +195,20 @@ def process_sms_or_email_notification(
     )
     if document_download_count:
         # We changed personalisation which means we need to update the content
-        template.values = personalisation
+        template_with_content.values = personalisation
     key_type = api_key.key_type
     service_in_research_mode = service.research_mode
     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'],
+        template_version=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)
+        template_with_content=template_with_content,
+    )
 
     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:
@@ -229,7 +239,7 @@ def process_sms_or_email_notification(
     persist_notification(
         notification_id=notification_id,
         template_id=template.id,
-        template_version=template._template['version'],
+        template_version=template.version,
         recipient=form_send_to,
         service=service,
         personalisation=personalisation,
@@ -276,7 +286,7 @@ def save_email_to_queue(
     data = {
         "id": notification_id,
         "template_id": str(template.id),
-        "template_version": template._template['version'],
+        "template_version": template.version,
         "to": form['email_address'],
         "service_id": str(service_id),
         "personalisation": personalisation,
@@ -328,21 +338,22 @@ def process_document_uploads(personalisation_data, service, simulated=False):
 
 
 def process_letter_notification(
-    *, letter_data, api_key, template, template_with_content, reply_to_text, precompiled=False
+    *, letter_data, api_key, service, template, template_with_content, 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)
 
-    if not api_key.service.research_mode and api_key.service.restricted and api_key.key_type != KEY_TYPE_TEST:
+    if not service.research_mode and service.restricted and api_key.key_type != KEY_TYPE_TEST:
         raise BadRequestError(message='Cannot send letters when service is in trial mode', status_code=403)
 
     if precompiled:
         return process_precompiled_letter_notifications(letter_data=letter_data,
                                                         api_key=api_key,
+                                                        service=service,
                                                         template=template,
                                                         reply_to_text=reply_to_text)
 
-    validate_address(api_key, letter_data)
+    validate_address(service, letter_data)
 
     test_key = api_key.key_type == KEY_TYPE_TEST
 
@@ -358,6 +369,7 @@ def process_letter_notification(
     queue = QueueNames.CREATE_LETTERS_PDF if not test_key else QueueNames.RESEARCH_MODE
 
     notification = create_letter_notification(letter_data=letter_data,
+                                              service=service,
                                               template=template,
                                               api_key=api_key,
                                               status=status,
@@ -387,10 +399,10 @@ def process_letter_notification(
     return resp
 
 
-def validate_address(api_key, letter_data):
+def validate_address(service, letter_data):
     address = PostalAddress.from_personalisation(
         letter_data['personalisation'],
-        allow_international_letters=api_key.service.has_permission(INTERNATIONAL_LETTERS),
+        allow_international_letters=(INTERNATIONAL_LETTERS in service.permissions),
     )
     if not address.has_enough_lines:
         raise ValidationError(
@@ -410,7 +422,7 @@ def validate_address(api_key, letter_data):
         )
 
 
-def process_precompiled_letter_notifications(*, letter_data, api_key, template, reply_to_text):
+def process_precompiled_letter_notifications(*, letter_data, api_key, service, template, reply_to_text):
     try:
         status = NOTIFICATION_PENDING_VIRUS_CHECK
         letter_content = base64.b64decode(letter_data['content'])
@@ -418,6 +430,7 @@ def process_precompiled_letter_notifications(*, letter_data, api_key, template,
         raise BadRequestError(message='Cannot decode letter content (invalid base64 encoding)', status_code=400)
 
     notification = create_letter_notification(letter_data=letter_data,
+                                              service=service,
                                               template=template,
                                               api_key=api_key,
                                               status=status,
@@ -456,7 +469,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.reply_to_text
 
     elif notification_type == SMS_TYPE:
         service_sms_sender_id = form.get("sms_sender_id", None)
@@ -466,10 +479,10 @@ 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.reply_to_text
 
     elif notification_type == LETTER_TYPE:
-        reply_to = template.get_reply_to_text()
+        reply_to = template.reply_to_text
 
     return reply_to
 

tests/app/authentication/test_authentication.py

@@ -8,11 +8,13 @@ import pytest
 from flask import json, current_app, request
 from freezegun import freeze_time
 from notifications_python_client.authentication import create_jwt_token
+from unittest.mock import call
 
 from app import api_user
 from app.dao.api_key_dao import get_unsigned_secrets, save_model_api_key, get_unsigned_secret, expire_api_key
 from app.models import ApiKey, KEY_TYPE_NORMAL
 from app.authentication.auth import AuthError, requires_admin_auth, requires_auth, GENERAL_TOKEN_ERROR_MESSAGE
+from app.serialised_models import caches, get_model_api_keys, dao_fetch_service_by_id
 
 from tests.conftest import set_config
 
@@ -300,7 +302,7 @@ def test_authentication_returns_token_expired_when_service_uses_expired_key_and_
     with pytest.raises(AuthError) as exc:
         requires_auth()
     assert exc.value.short_message == 'Invalid token: API key revoked'
-    assert exc.value.service_id == expired_api_key.service_id
+    assert exc.value.service_id == str(expired_api_key.service_id)
     assert exc.value.api_key_id == expired_api_key.id
 
 
@@ -376,7 +378,7 @@ def test_authentication_returns_error_when_service_has_no_secrets(client,
     with pytest.raises(AuthError) as exc:
         requires_auth()
     assert exc.value.short_message == 'Invalid token: service has no API keys'
-    assert exc.value.service_id == sample_service.id
+    assert exc.value.service_id == str(sample_service.id)
 
 
 def test_should_attach_the_current_api_key_to_current_app(notify_api, sample_service, sample_api_key):
@@ -387,7 +389,7 @@ def test_should_attach_the_current_api_key_to_current_app(notify_api, sample_ser
             headers={'Authorization': 'Bearer {}'.format(token)}
         )
         assert response.status_code == 200
-        assert api_user == sample_api_key
+        assert str(api_user.id) == str(sample_api_key.id)
 
 
 def test_should_return_403_when_token_is_expired(client,
@@ -399,8 +401,8 @@ def test_should_return_403_when_token_is_expired(client,
             request.headers = {'Authorization': 'Bearer {}'.format(token)}
             requires_auth()
     assert exc.value.short_message == 'Error: Your system clock must be accurate to within 30 seconds'
-    assert exc.value.service_id == sample_api_key.service_id
-    assert exc.value.api_key_id == sample_api_key.id
+    assert exc.value.service_id == str(sample_api_key.service_id)
+    assert str(exc.value.api_key_id) == str(sample_api_key.id)
 
 
 def __create_token(service_id):
@@ -457,3 +459,34 @@ def test_proxy_key_on_admin_auth_endpoint(notify_api, check_proxy_header, header
                 ]
             )
         assert response.status_code == expected_status
+
+
+def test_should_cache_service_and_api_key_lookups(mocker, client, sample_api_key):
+
+    mock_get_api_keys = mocker.patch(
+        'app.serialised_models.get_model_api_keys',
+        wraps=get_model_api_keys,
+    )
+    mock_get_service = mocker.patch(
+        'app.serialised_models.dao_fetch_service_by_id',
+        wraps=dao_fetch_service_by_id,
+    )
+
+    for i in range(5):
+        token = __create_token(sample_api_key.service_id)
+        client.get('/notifications', headers={
+            'Authorization': f'Bearer {token}'
+        })
+
+    assert mock_get_api_keys.call_args_list == [
+        call(str(sample_api_key.service_id))
+    ]
+    assert mock_get_service.call_args_list == [
+        call(str(sample_api_key.service_id))
+    ]
+
+    assert caches['SerialisedService.from_id'].currsize == 1
+    assert caches['SerialisedService.from_id'].ttl == 2
+
+    assert caches['SerialisedAPIKeyCollection.from_service_id'].currsize == 1
+    assert caches['SerialisedAPIKeyCollection.from_service_id'].ttl == 2

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.validators import get_template_dict
+from app.serialised_models import SerialisedTemplate
 
 
 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 = SerialisedTemplate(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 = SerialisedTemplate(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 = SerialisedTemplate(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

@@ -11,7 +11,6 @@ from app.models import (
     Notification,
     NotificationHistory,
     ScheduledNotification,
-    Template,
     LETTER_TYPE
 )
 from app.notifications.process_notifications import (
@@ -21,32 +20,39 @@ from app.notifications.process_notifications import (
     send_notification_to_queue,
     simulated_recipient
 )
+from app.notifications.validators import get_template_model
 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
 
 
 def test_create_content_for_notification_passes(sample_email_template):
-    template = Template.query.get(sample_email_template.id)
+    template = get_template_model(sample_email_template.id, sample_email_template.service_id)
     content = create_content_for_notification(template, 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)
+    template = get_template_model(
+        sample_template_with_placeholders.id, sample_template_with_placeholders.service_id
+    )
     content = create_content_for_notification(template, {'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 = get_template_model(
+        sample_template_with_placeholders.id, sample_template_with_placeholders.service_id
+    )
     with pytest.raises(BadRequestError):
         create_content_for_notification(template, None)
 
 
 def test_create_content_for_notification_allows_additional_personalisation(sample_template_with_placeholders):
-    template = Template.query.get(sample_template_with_placeholders.id)
+    template = get_template_model(
+        sample_template_with_placeholders.id, sample_template_with_placeholders.service_id
+    )
     create_content_for_notification(template, {'name': 'Bobby', 'Additional placeholder': 'Data'})
 
 

tests/app/notifications/test_validators.py

@@ -4,6 +4,7 @@ from flask import current_app
 from notifications_utils import SMS_CHAR_COUNT_LIMIT
 
 import app
+from app.authentication.auth import get_service_model
 from app.dao import templates_dao
 from app.models import SMS_TYPE, EMAIL_TYPE, LETTER_TYPE
 from app.notifications.process_notifications import create_content_for_notification
@@ -19,6 +20,7 @@ from app.notifications.validators import (
     check_service_sms_sender_id,
     check_service_letter_contact_id,
     check_reply_to,
+    get_template_model,
     service_can_send_to_recipient,
     validate_and_format_recipient,
     validate_template,
@@ -314,7 +316,7 @@ 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 = get_template_model(
         template_id=template_id,
         service_id=sample_service.id
     )
@@ -330,7 +332,7 @@ 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 = get_template_model(
         template_id=template_id,
         service_id=sample_service.id
     )
@@ -356,7 +358,7 @@ def test_validate_template_calls_all_validators(mocker, fake_uuid, sample_servic
     )
     mock_check_not_empty = mocker.patch('app.notifications.validators.check_notification_content_is_not_empty')
     mock_check_message_is_too_long = mocker.patch('app.notifications.validators.check_content_char_count')
-    validate_template(template.id, {}, sample_service, "email")
+    template, template_with_content = 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)
@@ -438,8 +440,9 @@ def test_rejects_api_calls_with_international_numbers_if_service_does_not_allow_
         notify_db_session,
 ):
     service = create_service(service_permissions=[SMS_TYPE])
+    service_model = get_service_model(service.id)
     with pytest.raises(BadRequestError) as e:
-        validate_and_format_recipient('20-12-1234-1234', key_type, service, SMS_TYPE)
+        validate_and_format_recipient('20-12-1234-1234', key_type, service_model, SMS_TYPE)
     assert e.value.status_code == 400
     assert e.value.message == 'Cannot send to international mobile numbers'
     assert e.value.fields == []
@@ -448,7 +451,8 @@ def test_rejects_api_calls_with_international_numbers_if_service_does_not_allow_
 @pytest.mark.parametrize('key_type', ['test', 'normal'])
 def test_allows_api_calls_with_international_numbers_if_service_does_allow_int_sms(
         key_type, sample_service_full_permissions):
-    result = validate_and_format_recipient('20-12-1234-1234', key_type, sample_service_full_permissions, SMS_TYPE)
+    service_model = get_service_model(sample_service_full_permissions.id)
+    result = validate_and_format_recipient('20-12-1234-1234', key_type, service_model, SMS_TYPE)
     assert result == '201212341234'
 
 

tests/app/v2/notifications/test_post_notifications.py

@@ -818,8 +818,8 @@ def test_post_notification_with_document_upload(client, notify_db_session, mocke
     assert validate(resp_json, post_email_response) == resp_json
 
     assert document_download_mock.upload_document.call_args_list == [
-        call(service.id, 'abababab', csv_param.get('is_csv')),
-        call(service.id, 'cdcdcdcd', csv_param.get('is_csv'))
+        call(str(service.id), 'abababab', csv_param.get('is_csv')),
+        call(str(service.id), 'cdcdcdcd', csv_param.get('is_csv'))
     ]
 
     notification = Notification.query.one()