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

@@ -5,7 +5,9 @@ from app.models import LETTER_TYPE
 from app.notifications.process_notifications import persist_notification
 
 
-def create_letter_notification(letter_data, template, api_key, status, reply_to_text=None, billable_units=None):
+def create_letter_notification(
+    letter_data, template, api_key, status, reply_to_text=None, billable_units=None, updated_at=None
+):
     notification = persist_notification(
         template_id=template.id,
         template_version=template.version,
@@ -24,6 +26,7 @@ def create_letter_notification(letter_data, template, api_key, status, reply_to_
         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

@@ -79,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:
@@ -105,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:
@@ -120,13 +122,17 @@ 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)
-        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))
+        # 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))
 
         current_app.logger.info(
             "{} {} created at {}".format(notification_type, notification_id, notification_created_at)
@@ -134,35 +140,43 @@ def persist_notification(
     return notification
 
 
-def send_notification_to_queue(notification, research_mode, queue=None):
-    if research_mode or notification.key_type == KEY_TYPE_TEST:
+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:
         queue = QueueNames.RESEARCH_MODE
 
-    if notification.notification_type == SMS_TYPE:
+    if notification_type == SMS_TYPE:
         if not queue:
             queue = QueueNames.SEND_SMS
         deliver_task = provider_tasks.deliver_sms
-    if notification.notification_type == EMAIL_TYPE:
+    if notification_type == EMAIL_TYPE:
         if not queue:
             queue = QueueNames.SEND_EMAIL
         deliver_task = provider_tasks.deliver_email
-    if notification.notification_type == LETTER_TYPE:
+    if 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.notification_type,
-                                                         notification.id,
+        "{} {} sent to the {} queue for delivery".format(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/schemas.py

@@ -235,6 +235,18 @@ 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
 
@@ -289,7 +301,18 @@ class DetailedServiceSchema(BaseSchema):
             'sms_sender',
             'permissions',
             'inbound_number',
-            'inbound_sms'
+            'inbound_sms',
+            'all_template_folders',
+            'annual_billing',
+            'contact_list',
+            'created_by',
+            'crown',
+            'letter_logo_filename',
+            'rate_limit',
+            'returned_letters',
+            'users',
+            'version',
+            'whitelist',
         )
 
 

app/v2/notifications/create_response.py

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

app/v2/notifications/post_notifications.py

@@ -15,14 +15,13 @@ 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
 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 (
@@ -45,9 +44,8 @@ from app.notifications.process_letter_notifications import (
 from app.notifications.process_notifications import (
     persist_notification,
     persist_scheduled_notification,
-    send_notification_to_queue,
-    simulated_recipient
-)
+    simulated_recipient,
+    send_notification_to_queue_detached)
 from app.notifications.validators import (
     check_if_service_can_send_files_by_email,
     check_rate_limiting,
@@ -62,10 +60,8 @@ 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,
@@ -108,17 +104,12 @@ def post_precompiled_letter_notification():
         letter_data=form,
         api_key=api_user,
         template=template,
+        template_with_content=None,  # not required for precompiled
         reply_to_text=reply_to,
         precompiled=True
     )
 
-    resp = {
-        'id': notification.id,
-        'reference': notification.client_reference,
-        'postage': notification.postage
-    }
-
-    return jsonify(resp), 201
+    return jsonify(notification), 201
 
 
 @v2_notification_blueprint.route('/<notification_type>', methods=['POST'])
@@ -157,6 +148,7 @@ def post_notification(notification_type):
             letter_data=form,
             api_key=api_user,
             template=template,
+            template_with_content=template_with_content,
             reply_to_text=reply_to
         )
     else:
@@ -164,41 +156,19 @@ def post_notification(notification_type):
             form=form,
             notification_type=notification_type,
             api_key=api_user,
-            template=template,
+            template=template_with_content,
+            template_process_type=template.process_type,
             service=authenticated_service,
             reply_to_text=reply_to
         )
 
-        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
+    return jsonify(notification), 201
 
 
-def process_sms_or_email_notification(*, form, notification_type, api_key, template, service, reply_to_text=None):
-    notification_id = None
+def process_sms_or_email_notification(
+    *, 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']
 
     send_to = validate_and_format_recipient(send_to=form_send_to,
@@ -214,27 +184,46 @@ def process_sms_or_email_notification(*, form, notification_type, api_key, templ
         service,
         simulated=simulated
     )
+    if document_download_count:
+        # We changed personalisation which means we need to update the content
+        template.values = personalisation
+    api_key_id = api_key.id
+    key_type = api_key.key_type
+    service_in_research_mode = service.research_mode
+    template_version = template._template['version']
+    resp = create_response_for_post_notification(
+        notification_id=notification_id,
+        client_reference=form.get('reference', None),
+        template_id=template.id,
+        template_version=template._template['version'],
+        service_id=service.id,
+        notification_type=notification_type,
+        reply_to=reply_to_text,
+        scheduled_for=form.get("scheduled_for", None),
+        template_with_content=template)
 
-    if str(service.id) in current_app.config.get('HIGH_VOLUME_SERVICE') and 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:
-            notification_id = uuid.uuid4()
-            notification = save_email_to_queue(
+            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,
                 reply_to_text=reply_to_text
             )
-            return notification
+            return resp
         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
@@ -242,16 +231,16 @@ def process_sms_or_email_notification(*, form, notification_type, api_key, templ
                 f'Notification {notification_id} failed to save to high volume queue. Using normal flow instead'
             )
 
-    notification = persist_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,
-        key_type=api_key.key_type,
+        api_key_id=api_key_id,
+        key_type=key_type,
         client_reference=form.get('reference', None),
         simulated=simulated,
         reply_to_text=reply_to_text,
@@ -260,19 +249,21 @@ def process_sms_or_email_notification(*, form, notification_type, api_key, templ
 
     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(
-                notification=notification,
-                research_mode=service.research_mode,
+            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=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 notification
+    return resp
 
 
 def save_email_to_queue(
@@ -280,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,
@@ -341,7 +335,9 @@ def process_document_uploads(personalisation_data, service, simulated=False):
     return personalisation_data, len(file_keys)
 
 
-def process_letter_notification(*, letter_data, api_key, template, reply_to_text, precompiled=False):
+def process_letter_notification(
+    *, 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)
 
@@ -354,21 +350,68 @@ def process_letter_notification(*, letter_data, api_key, template, reply_to_text
                                                         template=template,
                                                         reply_to_text=reply_to_text)
 
+    validate_address(api_key, letter_data)
+
+    test_key = api_key.key_type == KEY_TYPE_TEST
+
+    status = NOTIFICATION_CREATED
+    updated_at = None
+    if test_key:
+        # if we don't want to actually send the letter, then start it off in SENDING so we don't pick it up
+        if current_app.config['NOTIFY_ENVIRONMENT'] in ['preview', 'development']:
+            status = NOTIFICATION_SENDING
+        # mark test letter as delivered and do not create a fake response later
+        else:
+            status = NOTIFICATION_DELIVERED
+            updated_at = datetime.utcnow()
+
+    queue = QueueNames.CREATE_LETTERS_PDF if not test_key else QueueNames.RESEARCH_MODE
+
+    notification = create_letter_notification(letter_data=letter_data,
+                                              template=template,
+                                              api_key=api_key,
+                                              status=status,
+                                              reply_to_text=reply_to_text,
+                                              updated_at=updated_at
+                                              )
+
+    get_pdf_for_templated_letter.apply_async(
+        [str(notification.id)],
+        queue=queue
+    )
+
+    if test_key and current_app.config['NOTIFY_ENVIRONMENT'] in ['preview', 'development']:
+        create_fake_letter_response_file.apply_async(
+            (notification.reference,),
+            queue=queue
+        )
+    resp = create_response_for_post_notification(
+        notification_id=notification.id,
+        client_reference=notification.client_reference,
+        template_id=notification.template_id,
+        template_version=notification.template_version,
+        notification_type=notification.notification_type,
+        reply_to=reply_to_text,
+        scheduled_for=letter_data.get('scheduled_for', None),
+        service_id=notification.service_id,
+        template_with_content=template_with_content
+    )
+    return resp
+
+
+def validate_address(api_key, letter_data):
     address = PostalAddress.from_personalisation(
         letter_data['personalisation'],
         allow_international_letters=api_key.service.has_permission(INTERNATIONAL_LETTERS),
     )
-
     if not address.has_enough_lines:
         raise ValidationError(
             message=f'Address must be at least {PostalAddress.MIN_LINES} lines'
         )
-
     if address.has_too_many_lines:
         raise ValidationError(
             message=f'Address must be no more than {PostalAddress.MAX_LINES} lines'
         )
-
     if not address.has_valid_last_line:
         if address.allow_international_letters:
             raise ValidationError(
@@ -378,34 +421,6 @@ def process_letter_notification(*, letter_data, api_key, template, reply_to_text
             message='Must be a real UK postcode'
         )
 
-    test_key = api_key.key_type == KEY_TYPE_TEST
-
-    # if we don't want to actually send the letter, then start it off in SENDING so we don't pick it up
-    status = NOTIFICATION_CREATED if not test_key else NOTIFICATION_SENDING
-    queue = QueueNames.CREATE_LETTERS_PDF if not test_key else QueueNames.RESEARCH_MODE
-
-    notification = create_letter_notification(letter_data=letter_data,
-                                              template=template,
-                                              api_key=api_key,
-                                              status=status,
-                                              reply_to_text=reply_to_text)
-
-    get_pdf_for_templated_letter.apply_async(
-        [str(notification.id)],
-        queue=queue
-    )
-
-    if test_key:
-        if current_app.config['NOTIFY_ENVIRONMENT'] in ['preview', 'development']:
-            create_fake_letter_response_file.apply_async(
-                (notification.reference,),
-                queue=queue
-            )
-        else:
-            update_notification_status_by_reference(notification.reference, NOTIFICATION_DELIVERED)
-
-    return notification
-
 
 def process_precompiled_letter_notifications(*, letter_data, api_key, template, reply_to_text):
     try:
@@ -420,6 +435,12 @@ def process_precompiled_letter_notifications(*, letter_data, api_key, template,
                                               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))
@@ -438,7 +459,7 @@ def process_precompiled_letter_notifications(*, letter_data, api_key, template,
             queue=QueueNames.LETTERS
         )
 
-    return notification
+    return resp
 
 
 def get_reply_to_text(notification_type, form, template):
@@ -463,3 +484,31 @@ def get_reply_to_text(notification_type, form, template):
         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

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

@@ -23,7 +23,7 @@ from app.notifications.process_notifications import (
 )
 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
+from tests.app.db import create_service, create_template, create_api_key
 
 
 def test_create_content_for_notification_passes(sample_email_template):
@@ -51,8 +51,7 @@ def test_create_content_for_notification_allows_additional_personalisation(sampl
 
 
 @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, mocker):
-    mocked_redis = mocker.patch('app.notifications.process_notifications.redis_store.get')
+def test_persist_notification_creates_and_save_to_db(sample_template, sample_api_key, sample_job):
 
     assert Notification.query.count() == 0
     assert NotificationHistory.query.count() == 0
@@ -91,8 +90,6 @@ 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
@@ -157,10 +154,9 @@ 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, mocker):
+def test_persist_notification_with_optionals(sample_job, sample_api_key):
     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(
@@ -186,7 +182,7 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key, mocker)
     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
@@ -197,44 +193,77 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key, mocker)
 
 
 @freeze_time("2016-01-01 11:09:00.061258")
-def test_persist_notification_doesnt_touch_cache_for_old_keys_that_dont_exist(sample_template, sample_api_key, mocker):
+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)
     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=sample_template.id,
-        template_version=sample_template.version,
+        template_id=template.id,
+        template_version=template.version,
         recipient='+447111111111',
-        service=sample_template.service,
+        service=template.service,
         personalisation={},
         notification_type='sms',
-        api_key_id=sample_api_key.id,
-        key_type=sample_api_key.key_type,
+        api_key_id=api_key.id,
+        key_type=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(sample_template, sample_api_key, mocker):
+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)
     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={sample_template.id, 1})
+                 return_value={template.id, 1})
 
     persist_notification(
-        template_id=sample_template.id,
-        template_version=sample_template.version,
+        template_id=template.id,
+        template_version=template.version,
         recipient='+447111111122',
-        service=sample_template.service,
+        service=template.service,
         personalisation={},
         notification_type='sms',
-        api_key_id=sample_api_key.id,
-        key_type=sample_api_key.key_type,
+        api_key_id=api_key.id,
+        key_type=api_key.key_type,
         reference="ref2")
 
-    mock_incr.assert_called_once_with(str(sample_template.service_id) + "-2016-01-01-count", )
+    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
 
 
 @pytest.mark.parametrize((

tests/app/service/test_rest.py

@@ -235,7 +235,6 @@ 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])
@@ -347,7 +346,6 @@ 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
 
@@ -1225,7 +1223,6 @@ def test_add_existing_user_to_another_service_with_all_permissions(
             )
             assert resp.status_code == 200
             json_resp = resp.json
-            assert str(user_to_add.id) in json_resp['data']['users']
 
             # check user has all permissions
             auth_header = create_authorization_header()

tests/app/v2/notifications/test_post_letter_notifications.py

@@ -245,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

@@ -49,6 +49,7 @@ 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
@@ -424,7 +425,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')
+    deliver_mock = mocker.patch('app.v2.notifications.post_notifications.send_notification_to_queue_detached')
     mocker.patch(
         'app.v2.notifications.post_notifications.check_rate_limiting',
         side_effect=RateLimitError("LIMIT", "INTERVAL", "TYPE"))
@@ -1005,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()