Added a test

By added the notification_type to the filter we get a better performance on the query. Especially when selecting for NotificationHistory.

app/authentication/auth.py

@@ -6,17 +6,18 @@ 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 import db
-from app.dao.services_dao import dao_fetch_service_by_id
-from app.serialised_models import (
-    SerialisedAPIKeyCollection,
-    SerialisedService,
-)
+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):
@@ -92,9 +93,8 @@ def requires_auth():
     issuer = __get_token_issuer(auth_token)  # ie the `iss` claim which should be a service ID
 
     try:
-        service = SerialisedService.from_id(issuer)
-        service.api_keys = SerialisedAPIKeyCollection.from_service_id(issuer)
-        db.session.commit()
+        with AUTH_DB_CONNECTION_DURATION_SECONDS.time():
+            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:
@@ -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 = service.id
+        g.service_id = api_key.service_id
         _request_ctx_stack.top.authenticated_service = service
         _request_ctx_stack.top.api_user = api_key
 

app/celery/letters_pdf_tasks.py

@@ -42,6 +42,7 @@ from app.models import (
     NOTIFICATION_TECHNICAL_FAILURE,
     NOTIFICATION_VALIDATION_FAILED,
     NOTIFICATION_VIRUS_SCAN_FAILED,
+    LETTER_TYPE
 )
 from app.cronitor import cronitor
 
@@ -216,7 +217,7 @@ def group_letters(letter_pdfs):
 def sanitise_letter(self, filename):
     try:
         reference = get_reference_from_filename(filename)
-        notification = dao_get_notification_by_reference(reference)
+        notification = dao_get_notification_by_reference(reference=reference, notification_type=LETTER_TYPE)
 
         current_app.logger.info('Notification ID {} Virus scan passed: {}'.format(notification.id, filename))
 
@@ -352,7 +353,7 @@ def _move_invalid_letter_and_update_status(
 def process_virus_scan_failed(filename):
     move_failed_pdf(filename, ScanErrorType.FAILURE)
     reference = get_reference_from_filename(filename)
-    notification = dao_get_notification_by_reference(reference)
+    notification = dao_get_notification_by_reference(reference=reference, notification_type=LETTER_TYPE)
     updated_count = update_letter_pdf_status(reference, NOTIFICATION_VIRUS_SCAN_FAILED, billable_units=0)
 
     if updated_count != 1:
@@ -371,7 +372,7 @@ def process_virus_scan_failed(filename):
 def process_virus_scan_error(filename):
     move_failed_pdf(filename, ScanErrorType.ERROR)
     reference = get_reference_from_filename(filename)
-    notification = dao_get_notification_by_reference(reference)
+    notification = dao_get_notification_by_reference(reference=reference, notification_type=LETTER_TYPE)
     updated_count = update_letter_pdf_status(reference, NOTIFICATION_TECHNICAL_FAILURE, billable_units=0)
 
     if updated_count != 1:

app/celery/process_ses_receipts_tasks.py

@@ -10,7 +10,7 @@ from app import notify_celery, statsd_client
 from app.config import QueueNames
 from app.clients.email.aws_ses import get_aws_responses
 from app.dao import notifications_dao
-from app.models import NOTIFICATION_SENDING, NOTIFICATION_PENDING
+from app.models import NOTIFICATION_SENDING, NOTIFICATION_PENDING, EMAIL_TYPE
 
 from app.notifications.notifications_ses_callback import (
     determine_notification_bounce_type,
@@ -39,7 +39,9 @@ def process_ses_results(self, response):
         reference = ses_message['mail']['messageId']
 
         try:
-            notification = notifications_dao.dao_get_notification_or_history_by_reference(reference=reference)
+            notification = notifications_dao.dao_get_notification_or_history_by_reference(
+                reference=reference, notification_type=EMAIL_TYPE
+            )
         except NoResultFound:
             message_time = iso8601.parse_date(ses_message['mail']['timestamp']).replace(tzinfo=None)
             if datetime.utcnow() - message_time < timedelta(minutes=5):

app/celery/tasks.py

@@ -536,7 +536,7 @@ def update_letter_notification(filename, temporary_failures, update):
 
 
 def check_billable_units(notification_update):
-    notification = dao_get_notification_or_history_by_reference(notification_update.reference)
+    notification = dao_get_notification_or_history_by_reference(notification_update.reference, LETTER_TYPE)
 
     if int(notification_update.page_count) != notification.billable_units:
         msg = 'Notification with id {} has {} billable_units but DVLA says page count is {}'.format(

app/dao/notifications_dao.py

@@ -650,33 +650,29 @@ def dao_get_notifications_by_recipient_or_reference(
 
 
 @statsd(namespace="dao")
-def dao_get_notification_by_reference(reference):
+def dao_get_notification_by_reference(reference, notification_type):
     return Notification.query.filter(
-        Notification.reference == reference
+        Notification.reference == reference,
+        Notification.notification_type == notification_type
     ).one()
 
 
 @statsd(namespace="dao")
-def dao_get_notification_or_history_by_reference(reference):
+def dao_get_notification_or_history_by_reference(reference, notification_type):
     try:
         # This try except is necessary because in test keys and research mode does not create notification history.
         # Otherwise we could just search for the NotificationHistory object
         return Notification.query.filter(
-            Notification.reference == reference
+            Notification.reference == reference,
+            Notification.notification_type == notification_type
         ).one()
     except NoResultFound:
         return NotificationHistory.query.filter(
-            NotificationHistory.reference == reference
+            NotificationHistory.reference == reference,
+            NotificationHistory.notification_type == notification_type
         ).one()
 
 
-@statsd(namespace="dao")
-def dao_get_notifications_by_references(references):
-    return Notification.query.filter(
-        Notification.reference.in_(references)
-    ).all()
-
-
 @statsd(namespace="dao")
 def dao_created_scheduled_notification(scheduled_notification):
     db.session.add(scheduled_notification)

app/notifications/notifications_ses_callback.py

@@ -5,7 +5,7 @@ from app.dao.notifications_dao import dao_get_notification_or_history_by_referen
 from app.dao.service_callback_api_dao import (
     get_service_delivery_status_callback_api_for_service, get_service_complaint_callback_api_for_service
 )
-from app.models import Complaint
+from app.models import Complaint, EMAIL_TYPE
 from app.celery.service_callback_tasks import (
     send_delivery_status_to_service,
     send_complaint_to_service,
@@ -33,7 +33,7 @@ def handle_complaint(ses_message):
     except KeyError as e:
         current_app.logger.exception("Complaint from SES failed to get reference from message", e)
         return
-    notification = dao_get_notification_or_history_by_reference(reference)
+    notification = dao_get_notification_or_history_by_reference(reference, EMAIL_TYPE)
     ses_complaint = ses_message.get('complaint', None)
 
     complaint = Complaint(

app/notifications/process_letter_notifications.py

@@ -5,22 +5,14 @@ from app.models import LETTER_TYPE
 from app.notifications.process_notifications import persist_notification
 
 
-def create_letter_notification(
-    letter_data,
-    template,
-    service,
-    api_key,
-    status,
-    reply_to_text=None,
-    billable_units=None,
-):
+def create_letter_notification(letter_data, template, 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=service,
+        service=template.service,
         personalisation=letter_data['personalisation'],
         notification_type=LETTER_TYPE,
         api_key_id=api_key.id,

app/notifications/process_notifications.py

@@ -9,11 +9,6 @@ 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
@@ -49,34 +44,7 @@ REDIS_GET_AND_INCR_DAILY_LIMIT_DURATION_SECONDS = Histogram(
 
 
 def create_content_for_notification(template, 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,
-        )
-
+    template_object = template._as_utils_template_with_personalisation(personalisation)
     check_placeholders(template_object)
 
     return template_object
@@ -122,6 +90,7 @@ 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,
@@ -151,17 +120,13 @@ 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:
+            with REDIS_GET_AND_INCR_DAILY_LIMIT_DURATION_SECONDS.time():
+                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)
@@ -169,43 +134,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

@@ -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.reply_to_text
+                                              reply_to_text=template.get_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 service.permissions:
+            INTERNATIONAL_SMS_TYPE not in [p.permission for p 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
+from app.dao import services_dao, templates_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,7 +21,6 @@ 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
 
@@ -90,7 +89,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 permissions
+    return notify_type in [p.permission for p in permissions]
 
 
 def check_service_has_permission(notify_type, permissions):
@@ -124,7 +123,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 service.permissions:
+                INTERNATIONAL_SMS_TYPE not in [p.permission for p in service.permissions]:
             raise BadRequestError(message="Cannot send to international mobile numbers")
 
         return validate_and_format_phone_number(
@@ -149,9 +148,11 @@ def check_notification_content_is_not_empty(template_with_content):
 
 
 def validate_template(template_id, personalisation, service, notification_type):
-
     try:
-        template = SerialisedTemplate.from_template_id_and_service_id(template_id, service.id)
+        template = templates_dao.dao_get_template_by_id_and_service_id(
+            template_id=template_id,
+            service_id=service.id
+        )
     except NoResultFound:
         message = 'Template not found'
         raise BadRequestError(message=message,

app/schemas.py

@@ -235,18 +235,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 +289,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'
         )
 
 

app/serialised_models.py

@@ -1,151 +0,0 @@
-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,9 +137,7 @@ 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, [
-        p.permission for p in service.permissions
-    ])
+    check_service_has_permission(LETTER_TYPE, 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,8 +7,6 @@ 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)]
@@ -35,10 +33,6 @@ 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,9 +69,7 @@ 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 = [
-        p.permission for p in fetched_service.permissions
-    ]
+    permissions = 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)
@@ -104,12 +102,7 @@ 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,
-        [
-            p.permission for p in fetched_template.service.permissions
-        ]
-    ):
+    if not service_has_permission(fetched_template.template_type, 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/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

@@ -22,6 +22,7 @@ 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 (
@@ -44,8 +45,9 @@ from app.notifications.process_letter_notifications import (
 from app.notifications.process_notifications import (
     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,
@@ -98,17 +102,23 @@ 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='',  # not required for precompiled
+        reply_to_text=reply_to,
         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'])
@@ -146,9 +156,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
         )
     else:
@@ -157,27 +165,40 @@ def post_notification(notification_type):
             notification_type=notification_type,
             api_key=api_user,
             template=template,
-            template_with_content=template_with_content,
-            template_process_type=template.process_type,
             service=authenticated_service,
             reply_to_text=reply_to
         )
 
-    return jsonify(notification), 201
+        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_with_content,
-    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,
@@ -193,22 +214,6 @@ 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_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.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_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:
@@ -217,7 +222,8 @@ def process_sms_or_email_notification(
         # 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:
-            save_email_to_queue(
+            notification_id = uuid.uuid4()
+            notification = save_email_to_queue(
                 form=form,
                 notification_id=str(notification_id),
                 notification_type=notification_type,
@@ -228,7 +234,7 @@ def process_sms_or_email_notification(
                 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
@@ -236,7 +242,7 @@ 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,
@@ -245,7 +251,7 @@ def process_sms_or_email_notification(
         personalisation=personalisation,
         notification_type=notification_type,
         api_key_id=api_key.id,
-        key_type=key_type,
+        key_type=api_key.key_type,
         client_reference=form.get('reference', None),
         simulated=simulated,
         reply_to_text=reply_to_text,
@@ -254,21 +260,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.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(
@@ -337,39 +341,50 @@ def process_document_uploads(personalisation_data, service, simulated=False):
     return personalisation_data, len(file_keys)
 
 
-def process_letter_notification(
-    *, letter_data, api_key, service, template, template_with_content, reply_to_text, precompiled=False
-):
+def process_letter_notification(*, letter_data, api_key, template, 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 service.research_mode and service.restricted and api_key.key_type != KEY_TYPE_TEST:
+    if not api_key.service.research_mode and api_key.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(service, 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(
+                message=f'Last line of address must be a real UK postcode or another country'
+            )
+        raise ValidationError(
+            message='Must be a real UK postcode'
+        )
 
     test_key = api_key.key_type == KEY_TYPE_TEST
 
-    status = NOTIFICATION_CREATED
-    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
-
+    # 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,
-                                              service=service,
                                               template=template,
                                               api_key=api_key,
                                               status=status,
@@ -380,49 +395,19 @@ def process_letter_notification(
         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(service, letter_data):
-    address = PostalAddress.from_personalisation(
-        letter_data['personalisation'],
-        allow_international_letters=(INTERNATIONAL_LETTERS in service.permissions),
-    )
-    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(
-                message=f'Last line of address must be a real UK postcode or another country'
+    if test_key:
+        if current_app.config['NOTIFY_ENVIRONMENT'] in ['preview', 'development']:
+            create_fake_letter_response_file.apply_async(
+                (notification.reference,),
+                queue=queue
             )
-        raise ValidationError(
-            message='Must be a real UK postcode'
-        )
+        else:
+            update_notification_status_by_reference(notification.reference, NOTIFICATION_DELIVERED)
+
+    return notification
 
 
-def process_precompiled_letter_notifications(*, letter_data, api_key, service, template, reply_to_text):
+def process_precompiled_letter_notifications(*, letter_data, api_key, template, reply_to_text):
     try:
         status = NOTIFICATION_PENDING_VIRUS_CHECK
         letter_content = base64.b64decode(letter_data['content'])
@@ -430,18 +415,11 @@ def process_precompiled_letter_notifications(*, letter_data, api_key, service, t
         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,
                                               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))
@@ -460,7 +438,7 @@ def process_precompiled_letter_notifications(*, letter_data, api_key, service, t
             queue=QueueNames.LETTERS
         )
 
-    return resp
+    return notification
 
 
 def get_reply_to_text(notification_type, form, template):
@@ -469,7 +447,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.reply_to_text
+        ) or template.get_reply_to_text()
 
     elif notification_type == SMS_TYPE:
         service_sms_sender_id = form.get("sms_sender_id", None)
@@ -479,37 +457,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.reply_to_text
+            reply_to = template.get_reply_to_text()
 
     elif notification_type == LETTER_TYPE:
-        reply_to = template.reply_to_text
+        reply_to = template.get_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

tests/app/authentication/test_authentication.py

@@ -8,13 +8,11 @@ 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
 
@@ -302,7 +300,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 == str(expired_api_key.service_id)
+    assert exc.value.service_id == expired_api_key.service_id
     assert exc.value.api_key_id == expired_api_key.id
 
 
@@ -378,7 +376,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 == str(sample_service.id)
+    assert exc.value.service_id == sample_service.id
 
 
 def test_should_attach_the_current_api_key_to_current_app(notify_api, sample_service, sample_api_key):
@@ -389,7 +387,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 str(api_user.id) == str(sample_api_key.id)
+        assert api_user == sample_api_key
 
 
 def test_should_return_403_when_token_is_expired(client,
@@ -401,8 +399,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 == str(sample_api_key.service_id)
-    assert str(exc.value.api_key_id) == str(sample_api_key.id)
+    assert exc.value.service_id == sample_api_key.service_id
+    assert exc.value.api_key_id == sample_api_key.id
 
 
 def __create_token(service_id):
@@ -459,34 +457,3 @@ 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/dao/notification_dao/test_notification_dao.py

@@ -28,7 +28,6 @@ from app.dao.notifications_dao import (
     update_notification_status_by_id,
     update_notification_status_by_reference,
     dao_get_notification_by_reference,
-    dao_get_notifications_by_references,
     dao_get_notification_or_history_by_reference,
     notifications_not_yet_sent,
 )
@@ -1613,7 +1612,7 @@ def test_dao_update_notifications_by_reference_updates_history_when_one_of_two_n
 
 def test_dao_get_notification_by_reference_with_one_match_returns_notification(sample_letter_template, notify_db):
     create_notification(template=sample_letter_template, reference='REF1')
-    notification = dao_get_notification_by_reference('REF1')
+    notification = dao_get_notification_by_reference('REF1', 'letter')
 
     assert notification.reference == 'REF1'
 
@@ -1623,30 +1622,25 @@ def test_dao_get_notification_by_reference_with_multiple_matches_raises_error(sa
     create_notification(template=sample_letter_template, reference='REF1')
 
     with pytest.raises(SQLAlchemyError):
-        dao_get_notification_by_reference('REF1')
+        dao_get_notification_by_reference('REF1', 'letter')
 
 
 def test_dao_get_notification_by_reference_with_no_matches_raises_error(notify_db):
     with pytest.raises(SQLAlchemyError):
-        dao_get_notification_by_reference('REF1')
+        dao_get_notification_by_reference('REF1', 'email')
 
 
-def test_dao_get_notifications_by_references(sample_template):
-    create_notification(template=sample_template, reference='noref')
-    notification_1 = create_notification(template=sample_template, reference='ref')
-    notification_2 = create_notification(template=sample_template, reference='ref')
-
-    notifications = dao_get_notifications_by_references(['ref'])
-    assert len(notifications) == 2
-    assert notifications[0].id in [notification_1.id, notification_2.id]
-    assert notifications[1].id in [notification_1.id, notification_2.id]
+def test_dao_get_notification_by_reference_with_no_matches_for_type_raises_error(sample_email_template):
+    create_notification(template=sample_email_template, reference='REF1')
+    with pytest.raises(SQLAlchemyError):
+        dao_get_notification_by_reference('REF1', 'letter')
 
 
 def test_dao_get_notification_or_history_by_reference_with_one_match_returns_notification(
         sample_letter_template
 ):
     create_notification(template=sample_letter_template, reference='REF1')
-    notification = dao_get_notification_or_history_by_reference('REF1')
+    notification = dao_get_notification_or_history_by_reference('REF1', 'letter')
 
     assert notification.reference == 'REF1'
 
@@ -1658,12 +1652,18 @@ def test_dao_get_notification_or_history_by_reference_with_multiple_matches_rais
     create_notification(template=sample_letter_template, reference='REF1')
 
     with pytest.raises(SQLAlchemyError):
-        dao_get_notification_or_history_by_reference('REF1')
+        dao_get_notification_or_history_by_reference('REF1', 'letter')
 
 
-def test_dao_get_notification_or_history_by_reference_with_no_matches_raises_error(notify_db):
+def test_dao_get_notification_or_history_by_reference_with_no_matches_raises_error(sample_letter_template):
+    create_notification(template=sample_letter_template, reference='REF1')
     with pytest.raises(SQLAlchemyError):
-        dao_get_notification_or_history_by_reference('REF1')
+        dao_get_notification_or_history_by_reference('REF1', 'email')
+
+
+def test_dao_get_notification_or_history_by_reference_with_no_matches_for_type_raises_error(notify_db):
+    with pytest.raises(SQLAlchemyError):
+        dao_get_notification_or_history_by_reference('REF1', 'email')
 
 
 @pytest.mark.parametrize("notification_type",

tests/app/notifications/test_process_letter_notifications.py

@@ -2,8 +2,6 @@ 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):
@@ -15,17 +13,7 @@ def test_create_letter_notification_creates_notification(sample_letter_template,
         }
     }
 
-    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,
-    )
+    notification = create_letter_notification(data, sample_letter_template, sample_api_key, NOTIFICATION_CREATED)
 
     assert notification == Notification.query.one()
     assert notification.job is None
@@ -50,17 +38,7 @@ def test_create_letter_notification_sets_reference(sample_letter_template, sampl
         'reference': 'foo'
     }
 
-    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,
-    )
+    notification = create_letter_notification(data, sample_letter_template, sample_api_key, NOTIFICATION_CREATED)
 
     assert notification.client_reference == 'foo'
 
@@ -74,17 +52,7 @@ def test_create_letter_notification_sets_billable_units(sample_letter_template,
         },
     }
 
-    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,
-    )
+    notification = create_letter_notification(data, sample_letter_template, sample_api_key, NOTIFICATION_CREATED,
+                                              billable_units=3)
 
     assert notification.billable_units == 3

tests/app/notifications/test_process_notification.py

@@ -11,6 +11,7 @@ from app.models import (
     Notification,
     NotificationHistory,
     ScheduledNotification,
+    Template,
     LETTER_TYPE
 )
 from app.notifications.process_notifications import (
@@ -20,44 +21,38 @@ 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
+from tests.app.db import create_service, create_template
 
 
 def test_create_content_for_notification_passes(sample_email_template):
-    template = get_template_model(sample_email_template.id, sample_email_template.service_id)
+    template = Template.query.get(sample_email_template.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 = get_template_model(
-        sample_template_with_placeholders.id, sample_template_with_placeholders.service_id
-    )
+    template = Template.query.get(sample_template_with_placeholders.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 = get_template_model(
-        sample_template_with_placeholders.id, sample_template_with_placeholders.service_id
-    )
+    template = Template.query.get(sample_template_with_placeholders.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 = get_template_model(
-        sample_template_with_placeholders.id, sample_template_with_placeholders.service_id
-    )
+    template = Template.query.get(sample_template_with_placeholders.id)
     create_content_for_notification(template, {'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
@@ -96,6 +91,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
@@ -160,9 +157,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(
@@ -188,7 +186,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
@@ -199,77 +197,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

@@ -4,7 +4,6 @@ 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
@@ -20,7 +19,6 @@ 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,
@@ -316,7 +314,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 = get_template_model(
+    template = templates_dao.dao_get_template_by_id_and_service_id(
         template_id=template_id,
         service_id=sample_service.id
     )
@@ -332,7 +330,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 = get_template_model(
+    template = templates_dao.dao_get_template_by_id_and_service_id(
         template_id=template_id,
         service_id=sample_service.id
     )
@@ -358,7 +356,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')
-    template, template_with_content = validate_template(template.id, {}, sample_service, "email")
+    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)
@@ -440,9 +438,8 @@ 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_model, SMS_TYPE)
+        validate_and_format_recipient('20-12-1234-1234', key_type, service, SMS_TYPE)
     assert e.value.status_code == 400
     assert e.value.message == 'Cannot send to international mobile numbers'
     assert e.value.fields == []
@@ -451,8 +448,7 @@ 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):
-    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)
+    result = validate_and_format_recipient('20-12-1234-1234', key_type, sample_service_full_permissions, SMS_TYPE)
     assert result == '201212341234'
 
 

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_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"))
@@ -818,8 +817,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(str(service.id), 'abababab', csv_param.get('is_csv')),
-        call(str(service.id), 'cdcdcdcd', csv_param.get('is_csv'))
+        call(service.id, 'abababab', csv_param.get('is_csv')),
+        call(service.id, 'cdcdcdcd', csv_param.get('is_csv'))
     ]
 
     notification = Notification.query.one()