Commit the db session before sending notification to the queue.

When a high volume service posts a notification we want to commit the db session before we send the message to SQS. This will release the db connection earlier.

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/notifications/process_letter_notifications.py

@@ -6,13 +6,7 @@ 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,
+    letter_data, template, api_key, status, reply_to_text=None, billable_units=None, updated_at=None
 ):
     notification = persist_notification(
         template_id=template.id,
@@ -20,7 +14,7 @@ def create_letter_notification(
         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,
@@ -32,6 +26,7 @@ def create_letter_notification(
         status=status,
         reply_to_text=reply_to_text,
         billable_units=billable_units,
-        postage=letter_data.get('postage')
+        postage=letter_data.get('postage'),
+        updated_at=updated_at
     )
     return notification

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
@@ -111,7 +79,8 @@ def persist_notification(
     billable_units=None,
     postage=None,
     template_postage=None,
-    document_download_count=None
+    document_download_count=None,
+    updated_at=None
 ):
     notification_created_at = created_at or datetime.utcnow()
     if not notification_id:
@@ -122,6 +91,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,
@@ -136,6 +106,7 @@ def persist_notification(
         reply_to_text=reply_to_text,
         billable_units=billable_units,
         document_download_count=document_download_count,
+        updated_at=updated_at
     )
 
     if notification_type == SMS_TYPE:

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/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/post_notifications.py

@@ -15,8 +15,8 @@ from app import (
     notify_celery,
     document_download_client,
     encryption,
-    DATETIME_FORMAT
-)
+    DATETIME_FORMAT,
+    db)
 from app.celery.letters_pdf_tasks import get_pdf_for_templated_letter, sanitise_letter
 from app.celery.research_mode_tasks import create_fake_letter_response_file
 from app.celery.tasks import save_api_email
@@ -98,13 +98,14 @@ 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
     )
 
@@ -146,7 +147,6 @@ 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,8 +156,7 @@ def post_notification(notification_type):
             form=form,
             notification_type=notification_type,
             api_key=api_user,
-            template=template,
-            template_with_content=template_with_content,
+            template=template_with_content,
             template_process_type=template.process_type,
             service=authenticated_service,
             reply_to_text=reply_to
@@ -167,15 +166,7 @@ def post_notification(notification_type):
 
 
 def process_sms_or_email_notification(
-    *,
-    form,
-    notification_type,
-    api_key,
-    template,
-    template_with_content,
-    template_process_type,
-    service,
-    reply_to_text=None,
+    *, form, notification_type, api_key, template, 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']
@@ -195,34 +186,38 @@ def process_sms_or_email_notification(
     )
     if document_download_count:
         # We changed personalisation which means we need to update the content
-        template_with_content.values = personalisation
+        template.values = personalisation
+    api_key_id = api_key.id
     key_type = api_key.key_type
     service_in_research_mode = service.research_mode
+    template_version = template._template['version']
     resp = create_response_for_post_notification(
         notification_id=notification_id,
         client_reference=form.get('reference', None),
         template_id=template.id,
-        template_version=template.version,
+        template_version=template._template['version'],
         service_id=service.id,
         notification_type=notification_type,
         reply_to=reply_to_text,
         scheduled_for=form.get("scheduled_for", None),
-        template_with_content=template_with_content,
-    )
+        template_with_content=template)
 
-    if str(service.id) in current_app.config.get('HIGH_VOLUME_SERVICE') and api_key.key_type == KEY_TYPE_NORMAL \
+    if str(service.id) in current_app.config.get('HIGH_VOLUME_SERVICE') and key_type == KEY_TYPE_NORMAL \
        and notification_type == EMAIL_TYPE:
         # Put GOV.UK Email notifications onto a queue
         # To take the pressure off the db for API requests put the notification for our high volume service onto a queue
         # the task will then save the notification, then call send_notification_to_queue.
         # We know that this team does not use the GET request, but relies on callbacks to get the status updates.
         try:
+            db.session.commit()
             save_email_to_queue(
                 form=form,
                 notification_id=str(notification_id),
                 notification_type=notification_type,
-                api_key=api_key,
-                template=template,
+                api_key_id=api_key_id,
+                key_type=key_type,
+                template_id=template.id,
+                template_version=template_version,
                 service_id=service.id,
                 personalisation=personalisation,
                 document_download_count=document_download_count,
@@ -239,12 +234,12 @@ def process_sms_or_email_notification(
     persist_notification(
         notification_id=notification_id,
         template_id=template.id,
-        template_version=template.version,
+        template_version=template_version,
         recipient=form_send_to,
         service=service,
         personalisation=personalisation,
         notification_type=notification_type,
-        api_key_id=api_key.id,
+        api_key_id=api_key_id,
         key_type=key_type,
         client_reference=form.get('reference', None),
         simulated=simulated,
@@ -276,23 +271,26 @@ def save_email_to_queue(
     notification_id,
     form,
     notification_type,
-    api_key,
-    template,
+    api_key_id,
+    key_type,
+    template_id,
+    template_version,
     service_id,
     personalisation,
     document_download_count,
     reply_to_text=None
 ):
+    db.session.commit()
     data = {
         "id": notification_id,
-        "template_id": str(template.id),
-        "template_version": template.version,
+        "template_id": str(template_id),
+        "template_version": template_version,
         "to": form['email_address'],
         "service_id": str(service_id),
         "personalisation": personalisation,
         "notification_type": notification_type,
-        "api_key_id": str(api_key.id),
-        "key_type": api_key.key_type,
+        "api_key_id": str(api_key_id),
+        "key_type": key_type,
         "client_reference": form.get('reference', None),
         "reply_to_text": reply_to_text,
         "document_download_count": document_download_count,
@@ -338,26 +336,26 @@ def process_document_uploads(personalisation_data, service, simulated=False):
 
 
 def process_letter_notification(
-    *, letter_data, api_key, service, template, template_with_content, reply_to_text, precompiled=False
+    *, letter_data, api_key, 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 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)
+    validate_address(api_key, letter_data)
 
     test_key = api_key.key_type == KEY_TYPE_TEST
 
     status = NOTIFICATION_CREATED
+    updated_at = None
     if test_key:
         # if we don't want to actually send the letter, then start it off in SENDING so we don't pick it up
         if current_app.config['NOTIFY_ENVIRONMENT'] in ['preview', 'development']:
@@ -365,15 +363,17 @@ def process_letter_notification(
         # mark test letter as delivered and do not create a fake response later
         else:
             status = NOTIFICATION_DELIVERED
+            updated_at = datetime.utcnow()
 
     queue = QueueNames.CREATE_LETTERS_PDF if not test_key else QueueNames.RESEARCH_MODE
 
     notification = create_letter_notification(letter_data=letter_data,
-                                              service=service,
                                               template=template,
                                               api_key=api_key,
                                               status=status,
-                                              reply_to_text=reply_to_text)
+                                              reply_to_text=reply_to_text,
+                                              updated_at=updated_at
+                                              )
 
     get_pdf_for_templated_letter.apply_async(
         [str(notification.id)],
@@ -399,10 +399,10 @@ def process_letter_notification(
     return resp
 
 
-def validate_address(service, letter_data):
+def validate_address(api_key, letter_data):
     address = PostalAddress.from_personalisation(
         letter_data['personalisation'],
-        allow_international_letters=(INTERNATIONAL_LETTERS in service.permissions),
+        allow_international_letters=api_key.service.has_permission(INTERNATIONAL_LETTERS),
     )
     if not address.has_enough_lines:
         raise ValidationError(
@@ -422,7 +422,7 @@ def validate_address(service, letter_data):
         )
 
 
-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,7 +430,6 @@ 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,
@@ -469,7 +468,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,10 +478,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.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
 

requirements-app.txt

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

requirements.txt

@@ -28,7 +28,7 @@ notifications-python-client==5.5.1
 # PaaS
 awscli-cwlogs>=1.4,<1.5
 
-git+https://github.com/alphagov/notifications-utils.git@39.4.4#egg=notifications-utils==39.4.4
+git+https://github.com/alphagov/notifications-utils.git@39.6.0#egg=notifications-utils==39.6.0
 
 # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains
 prometheus-client==0.7.1
@@ -39,14 +39,15 @@ alembic==1.4.2
 amqp==1.4.9
 anyjson==0.3.3
 attrs==19.3.0
-awscli==1.18.75
+awscli==1.18.82
 bcrypt==3.1.7
 billiard==3.3.0.23
 bleach==3.1.4
 blinker==1.4
 boto==2.49.0
 boto3==1.10.38
-botocore==1.16.25
+botocore==1.17.5
+cachetools==4.1.0
 certifi==2020.4.5.2
 chardet==3.0.4
 click==7.1.2
@@ -78,7 +79,7 @@ python-json-logger==0.1.11
 pytz==2020.1
 PyYAML==5.3.1
 redis==3.5.3
-requests==2.23.0
+requests==2.24.0
 rsa==3.4.2
 s3transfer==0.3.3
 six==1.15.0

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/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,39 +21,32 @@ 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 = 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'})
 
 

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/v2/notifications/test_post_letter_notifications.py

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

tests/app/v2/notifications/test_post_notifications.py

@@ -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(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()
@@ -1006,11 +1006,14 @@ def test_post_notifications_saves_email_normally_if_save_email_to_queue_fails(cl
         "template_id": template.id,
         "personalisation": {"message": "Dear citizen, have a nice day"}
     }
+
+    print("******** Start")
     response = client.post(
         path='/v2/notifications/email',
         data=json.dumps(data),
         headers=[('Content-Type', 'application/json'), create_authorization_header(service_id=service.id)]
     )
+    print("********** End")
 
     json_resp = response.get_json()