From 743acf71e4351a0d422d106a5d8435b6a9af1b74 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 12 Feb 2020 14:38:09 +0000 Subject: [PATCH 1/2] add nullable document_count field to Notifications intention is for this to be null, 1, or many, based on how many documents were linked to within the message. nullable column, so that it doesn't require a lengthy access exclusive lock on the table when creating. --- app/models.py | 4 ++++ .../versions/0315_document_download_count.py | 23 +++++++++++++++++++ 2 files changed, 27 insertions(+) create mode 100644 migrations/versions/0315_document_download_count.py diff --git a/app/models.py b/app/models.py index d11f83cc6..23c0f9cd6 100644 --- a/app/models.py +++ b/app/models.py @@ -1407,6 +1407,8 @@ class Notification(db.Model): reply_to_text = db.Column(db.String, nullable=True) + document_download_count = db.Column(db.Integer, nullable=True) + postage = db.Column(db.String, nullable=True) CheckConstraint(""" CASE WHEN notification_type = 'letter' THEN @@ -1683,6 +1685,8 @@ class NotificationHistory(db.Model, HistoryModel): END """) + document_download_count = db.Column(db.Integer, nullable=True) + __table_args__ = ( db.ForeignKeyConstraint( ['template_id', 'template_version'], diff --git a/migrations/versions/0315_document_download_count.py b/migrations/versions/0315_document_download_count.py new file mode 100644 index 000000000..b59307a8c --- /dev/null +++ b/migrations/versions/0315_document_download_count.py @@ -0,0 +1,23 @@ +""" + +Revision ID: 0315_document_download_count +Revises: 0314_populate_email_access +Create Date: 2020-02-12 14:19:18.066425 + +""" +from alembic import op +import sqlalchemy as sa + + +revision = '0315_document_download_count' +down_revision = '0314_populate_email_access' + + +def upgrade(): + op.add_column('notifications', sa.Column('document_download_count', sa.Integer(), nullable=True)) + op.add_column('notification_history', sa.Column('document_download_count', sa.Integer(), nullable=True)) + + +def downgrade(): + op.drop_column('notifications', 'document_download_count') + op.drop_column('notification_history', 'document_download_count') From 1694395b172ffe56674f01ddf2c93d3ffdd86960 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 12 Feb 2020 16:07:07 +0000 Subject: [PATCH 2/2] 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):