From 1694395b172ffe56674f01ddf2c93d3ffdd86960 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 12 Feb 2020 16:07:07 +0000 Subject: [PATCH] record document count when processing api notifications if someone doesn't send any documents, set the value to None. If it's not specified, it defaults to None anyway. --- app/notifications/process_notifications.py | 6 +++-- app/v2/notifications/post_notifications.py | 17 ++++++++++--- tests/app/db.py | 6 +++-- .../notifications/test_post_notifications.py | 25 +++++++++++++++---- 4 files changed, 41 insertions(+), 13 deletions(-) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 2280896d0..60dff19cf 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -70,7 +70,8 @@ def persist_notification( reply_to_text=None, billable_units=None, postage=None, - template_postage=None + template_postage=None, + document_download_count=None, ): notification_created_at = created_at or datetime.utcnow() if not notification_id: @@ -94,7 +95,8 @@ def persist_notification( created_by_id=created_by_id, status=status, reply_to_text=reply_to_text, - billable_units=billable_units + billable_units=billable_units, + document_download_count=document_download_count, ) if notification_type == SMS_TYPE: diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index cffb1dc2f..c37d67d19 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -186,7 +186,11 @@ def process_sms_or_email_notification(*, form, notification_type, api_key, templ # Do not persist or send notification to the queue if it is a simulated recipient simulated = simulated_recipient(send_to, notification_type) - personalisation = process_document_uploads(form.get('personalisation'), service, simulated=simulated) + personalisation, document_download_count = process_document_uploads( + form.get('personalisation'), + service, + simulated=simulated + ) notification = persist_notification( template_id=template.id, @@ -199,7 +203,8 @@ def process_sms_or_email_notification(*, form, notification_type, api_key, templ key_type=api_key.key_type, client_reference=form.get('reference', None), simulated=simulated, - reply_to_text=reply_to_text + reply_to_text=reply_to_text, + document_download_count=document_download_count ) scheduled_for = form.get("scheduled_for", None) @@ -220,9 +225,13 @@ def process_sms_or_email_notification(*, form, notification_type, api_key, templ def process_document_uploads(personalisation_data, service, simulated=False): + """ + Returns modified personalisation dict and a count of document uploads. If there are no document uploads, returns + a count of `None` rather than `0`. + """ file_keys = [k for k, v in (personalisation_data or {}).items() if isinstance(v, dict) and 'file' in v] if not file_keys: - return personalisation_data + return personalisation_data, None personalisation_data = personalisation_data.copy() @@ -239,7 +248,7 @@ def process_document_uploads(personalisation_data, service, simulated=False): except DocumentDownloadError as e: raise BadRequestError(message=e.message, status_code=e.status_code) - return personalisation_data + return personalisation_data, len(file_keys) def process_letter_notification(*, letter_data, api_key, template, reply_to_text, precompiled=False): diff --git a/tests/app/db.py b/tests/app/db.py index 57f3ac5a9..4b4d9688b 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -233,7 +233,8 @@ def create_notification( one_off=False, reply_to_text=None, created_by_id=None, - postage=None + postage=None, + document_download_count=None, ): assert job or template if job: @@ -287,7 +288,8 @@ def create_notification( 'normalised_to': normalised_to, 'reply_to_text': reply_to_text, 'created_by_id': created_by_id, - 'postage': postage + 'postage': postage, + 'document_download_count': document_download_count, } notification = Notification(**data) dao_create_notification(notification) diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 3d337dfac..53b62d730 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -1,4 +1,5 @@ import uuid +from unittest.mock import call import pytest from freezegun import freeze_time @@ -55,6 +56,7 @@ def test_post_sms_notification_returns_201(client, sample_template_with_placehol assert notifications[0].status == NOTIFICATION_CREATED notification_id = notifications[0].id assert notifications[0].postage is None + assert notifications[0].document_download_count is None assert resp_json['id'] == str(notification_id) assert resp_json['reference'] == reference assert resp_json['content']['body'] == sample_template_with_placeholders.content.replace("(( Name))", "Jo") @@ -311,6 +313,7 @@ def test_post_email_notification_returns_201(client, sample_email_template_with_ assert resp_json['reference'] == reference assert notification.reference is None assert notification.reply_to_text is None + assert notification.document_download_count is None assert resp_json['content']['body'] == sample_email_template_with_placeholders.content \ .replace('((name))', 'Bob') assert resp_json['content']['subject'] == sample_email_template_with_placeholders.subject \ @@ -776,17 +779,20 @@ def test_post_notification_with_document_upload(client, notify_db_session, mocke template = create_template( service=service, template_type='email', - content="Document: ((document))" + content="Document 1: ((first_link)). Document 2: ((second_link))" ) mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') document_download_mock = mocker.patch('app.v2.notifications.post_notifications.document_download_client') - document_download_mock.upload_document.return_value = 'https://document-url/' + document_download_mock.upload_document.side_effect = lambda service_id, content: f'{content}-link' data = { "email_address": service.users[0].email_address, "template_id": template.id, - "personalisation": {"document": {"file": "abababab"}} + "personalisation": { + "first_link": {"file": "abababab"}, + "second_link": {"file": "cdcdcdcd"} + } } auth_header = create_authorization_header(service_id=service.id) @@ -799,11 +805,20 @@ def test_post_notification_with_document_upload(client, notify_db_session, mocke resp_json = json.loads(response.get_data(as_text=True)) assert validate(resp_json, post_email_response) == resp_json + assert document_download_mock.upload_document.call_args_list == [ + call(service.id, 'abababab'), + call(service.id, 'cdcdcdcd') + ] + notification = Notification.query.one() assert notification.status == NOTIFICATION_CREATED - assert notification.personalisation == {'document': 'https://document-url/'} + assert notification.personalisation == { + 'first_link': 'abababab-link', + 'second_link': 'cdcdcdcd-link' + } + assert notification.document_download_count == 2 - assert resp_json['content']['body'] == 'Document: https://document-url/' + assert resp_json['content']['body'] == 'Document 1: abababab-link. Document 2: cdcdcdcd-link' def test_post_notification_with_document_upload_simulated(client, notify_db_session, mocker):