From d0ed3bcacc93419bfb4a2826b96ece1551d47937 Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Thu, 1 Mar 2018 11:11:18 +0000 Subject: [PATCH 01/42] Update pytest-mock from 1.7.0 to 1.7.1 --- requirements_for_test.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements_for_test.txt b/requirements_for_test.txt index b41e1024d..214aaacd1 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -2,7 +2,7 @@ flake8==3.5.0 pytest==3.4.1 pytest-env==0.6.2 -pytest-mock==1.7.0 +pytest-mock==1.7.1 pytest-cov==2.5.1 pytest-xdist==1.22.2 coveralls==1.2.0 From 7ef6af2d1466ac6f768831419936dd098a67edf1 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 2 Mar 2018 14:12:38 +0000 Subject: [PATCH 02/42] Remove anything to do with DVLA format letters --- app/models.py | 5 ++--- requirements.txt | 2 +- tests/app/celery/test_tasks.py | 10 +--------- 3 files changed, 4 insertions(+), 13 deletions(-) diff --git a/app/models.py b/app/models.py index 6678d91e6..bd23dc65c 100644 --- a/app/models.py +++ b/app/models.py @@ -23,7 +23,7 @@ from notifications_utils.letter_timings import get_letter_timings from notifications_utils.template import ( PlainTextEmailTemplate, SMSMessageTemplate, - LetterDVLATemplate, + LetterPrintTemplate, ) from app.encryption import ( @@ -728,9 +728,8 @@ class TemplateBase(db.Model): {'content': self.content} ) if self.template_type == LETTER_TYPE: - return LetterDVLATemplate( + return LetterPrintTemplate( {'content': self.content, 'subject': self.subject}, - notification_reference=1, contact_block=self.service.get_default_letter_contact(), ) diff --git a/requirements.txt b/requirements.txt index 7868b54fa..96ed83f1e 100644 --- a/requirements.txt +++ b/requirements.txt @@ -23,6 +23,6 @@ notifications-python-client==4.7.2 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@23.8.0#egg=notifications-utils==23.8.0 +git+https://github.com/alphagov/notifications-utils.git@24.0.0#egg=notifications-utils==24.0.0 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 0ff66a4f7..f8e00651f 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -9,7 +9,7 @@ from freezegun import freeze_time from requests import RequestException from sqlalchemy.exc import SQLAlchemyError from celery.exceptions import Retry -from notifications_utils.template import SMSMessageTemplate, WithSubjectTemplate, LetterDVLATemplate +from notifications_utils.template import SMSMessageTemplate, WithSubjectTemplate from app import (encryption, DATETIME_FORMAT) from app.celery import provider_tasks @@ -1209,14 +1209,6 @@ def test_get_template_class(template_type, expected_class): assert get_template_class(template_type) == expected_class -@freeze_time("2017-03-23 11:09:00.061258") -def test_dvla_letter_template(sample_letter_notification): - t = {"content": sample_letter_notification.template.content, - "subject": sample_letter_notification.template.subject} - letter = LetterDVLATemplate(t, sample_letter_notification.personalisation, "random-string") - assert str(letter) == "140|500|001||random-string|||||||||||||A1||A2|A3|A4|A5|A6|A_POST|||||||||23 March 2017

Template subjectDear Sir/Madam, Hello. Yours Truly, The Government." # noqa - - def test_send_inbound_sms_to_service_post_https_request_to_service(notify_api, sample_service): inbound_api = create_service_inbound_api(service=sample_service, url="https://some.service.gov.uk/", bearer_token="something_unique") From a9a67ce54298937b2b8c006e5fc26fde04cfb38e Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Fri, 2 Mar 2018 14:54:28 +0000 Subject: [PATCH 03/42] Updated API to handle pre-compiled pdfs * added a method to letter/utils.py to get the PDF document from the S3 bucket * added the logic to return the pdf or to produce a png of the pdf --- app/letters/utils.py | 28 +++++++++++++++ app/template/rest.py | 83 ++++++++++++++++++++++++++++++-------------- 2 files changed, 85 insertions(+), 26 deletions(-) diff --git a/app/letters/utils.py b/app/letters/utils.py index 176944667..a7f05c00f 100644 --- a/app/letters/utils.py +++ b/app/letters/utils.py @@ -1,5 +1,6 @@ from datetime import datetime, timedelta +import boto3 from flask import current_app from notifications_utils.s3 import s3upload @@ -10,6 +11,8 @@ from app.variables import Retention LETTERS_PDF_FILE_LOCATION_STRUCTURE = \ '{folder}/NOTIFY.{reference}.{duplex}.{letter_class}.{colour}.{crown}.{date}.pdf' +PRECOMPILED_BUCKET_PREFIX = '{folder}/NOTIFY.{reference}' + def get_letter_pdf_filename(reference, crown): now = datetime.utcnow() @@ -31,6 +34,15 @@ def get_letter_pdf_filename(reference, crown): return upload_file_name +def get_bucket_prefix_for_notification(notification): + upload_file_name = PRECOMPILED_BUCKET_PREFIX.format( + folder=notification.created_at.date(), + reference=notification.reference + ).upper() + + return upload_file_name + + def upload_letter_pdf(notification, pdf_data): current_app.logger.info("PDF Letter {} reference {} created at {}, {} bytes".format( notification.id, notification.reference, notification.created_at, len(pdf_data))) @@ -48,3 +60,19 @@ def upload_letter_pdf(notification, pdf_data): current_app.logger.info("Uploaded letters PDF {} to {} for notification id {}".format( upload_file_name, current_app.config['LETTERS_PDF_BUCKET_NAME'], notification.id)) + + +def get_letter_pdf(notification): + bucket_name = current_app.config['LETTERS_PDF_BUCKET_NAME'] + + s3 = boto3.resource('s3') + bucket = s3.Bucket(bucket_name) + + for item in bucket.objects.filter(Prefix=get_bucket_prefix_for_notification(notification)): + obj = s3.Object( + bucket_name=bucket_name, + key=item.key + ) + file_content = obj.get()["Body"].read() + + return file_content diff --git a/app/template/rest.py b/app/template/rest.py index ca30fa71d..c0e91c69b 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -18,6 +18,7 @@ from app.dao.templates_dao import ( dao_get_template_by_id) from notifications_utils.template import SMSMessageTemplate from app.dao.services_dao import dao_fetch_service_by_id +from app.letters.utils import get_letter_pdf from app.models import SMS_TYPE from app.notifications.validators import service_has_permission, check_reply_to from app.schemas import (template_schema, template_history_schema) @@ -185,6 +186,7 @@ def redact_template(template, data): @template_blueprint.route('/preview//', methods=['GET']) def preview_letter_template_by_notification_id(service_id, notification_id, file_type): + if file_type not in ('pdf', 'png'): raise InvalidRequest({'content': ["file_type must be pdf or png"]}, status_code=400) @@ -194,36 +196,65 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file template = dao_get_template_by_id(notification.template_id) - template_for_letter_print = { - "id": str(notification.template_id), - "subject": template.subject, - "content": template.content, - "version": str(template.version) - } + if template.hidden and template.name == 'Pre-compiled PDF': - service = dao_fetch_service_by_id(service_id) + pdf_file = get_letter_pdf(notification) - data = { - 'letter_contact_block': notification.reply_to_text, - 'template': template_for_letter_print, - 'values': notification.personalisation, - 'dvla_org_id': service.dvla_organisation_id, - } + content = base64.b64encode(pdf_file).decode('utf-8') - resp = requests_post( - '{}/preview.{}{}'.format( - current_app.config['TEMPLATE_PREVIEW_API_HOST'], - file_type, - '?page={}'.format(page) if page else '' - ), - json=data, - headers={'Authorization': 'Token {}'.format(current_app.config['TEMPLATE_PREVIEW_API_KEY'])} - ) + if file_type == 'png': - if resp.status_code != 200: - raise InvalidRequest( - 'Error generating preview for {}'.format(notification_id), status_code=500 + url = '{}//precompiled-preview.png{}'.format( + current_app.config['TEMPLATE_PREVIEW_API_HOST'], + '?page={}'.format(page) if page else '' + ) + + resp = requests_post( + url, + data=content, + headers={'Authorization': 'Token {}'.format(current_app.config['TEMPLATE_PREVIEW_API_KEY'])} + ) + + if resp.status_code != 200: + raise InvalidRequest( + 'Error generating preview for {}'.format(notification_id), status_code=500 + ) + + content = base64.b64encode(resp.content).decode('utf-8') + + else: + + template_for_letter_print = { + "id": str(notification.template_id), + "subject": template.subject, + "content": template.content, + "version": str(template.version) + } + + service = dao_fetch_service_by_id(service_id) + + data = { + 'letter_contact_block': notification.reply_to_text, + 'template': template_for_letter_print, + 'values': notification.personalisation, + 'dvla_org_id': service.dvla_organisation_id, + } + + resp = requests_post( + '{}/preview.{}{}'.format( + current_app.config['TEMPLATE_PREVIEW_API_HOST'], + file_type, + '?page={}'.format(page) if page else '' + ), + json=data, + headers={'Authorization': 'Token {}'.format(current_app.config['TEMPLATE_PREVIEW_API_KEY'])} ) - content = base64.b64encode(resp.content).decode('utf-8') + if resp.status_code != 200: + raise InvalidRequest( + 'Error generating preview for {}'.format(notification_id), status_code=500 + ) + + content = base64.b64encode(resp.content).decode('utf-8') + return jsonify({"content": content}) From 7e1aa03371042ea09578025faadcf859b7de5ac6 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 2 Mar 2018 16:05:26 +0000 Subject: [PATCH 04/42] Send count of sent letters to performance platform MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now we’ve been sending real letters for quite a while it would be nice to show how many. --- app/celery/scheduled_tasks.py | 7 +++++++ app/performance_platform/total_sent_notifications.py | 6 +++++- .../performance_platform/test_total_sent_notifications.py | 4 ++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 9d5249e8a..7500111a7 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -221,6 +221,7 @@ def send_total_sent_notifications_to_performance_platform(): count_dict = total_sent_notifications.get_total_sent_notifications_yesterday() email_sent_count = count_dict.get('email').get('count') sms_sent_count = count_dict.get('sms').get('count') + letter_sent_count = count_dict.get('letter').get('count') start_date = count_dict.get('start_date') current_app.logger.info( @@ -240,6 +241,12 @@ def send_total_sent_notifications_to_performance_platform(): email_sent_count ) + total_sent_notifications.send_total_notifications_sent_for_day_stats( + start_date, + 'letter', + letter_sent_count + ) + @notify_celery.task(name='switch-current-sms-provider-on-slow-delivery') @statsd(namespace="tasks") diff --git a/app/performance_platform/total_sent_notifications.py b/app/performance_platform/total_sent_notifications.py index 4ad57171e..4aa62c786 100644 --- a/app/performance_platform/total_sent_notifications.py +++ b/app/performance_platform/total_sent_notifications.py @@ -27,6 +27,7 @@ def get_total_sent_notifications_yesterday(): email_count = get_total_sent_notifications_in_date_range(start_date, end_date, 'email') sms_count = get_total_sent_notifications_in_date_range(start_date, end_date, 'sms') + letter_count = get_total_sent_notifications_in_date_range(start_date, end_date, 'letter') return { "start_date": start_date, @@ -35,5 +36,8 @@ def get_total_sent_notifications_yesterday(): }, "sms": { "count": sms_count - } + }, + "letter": { + "count": letter_count + }, } diff --git a/tests/app/performance_platform/test_total_sent_notifications.py b/tests/app/performance_platform/test_total_sent_notifications.py index e4fe597d0..2ad2a93f6 100644 --- a/tests/app/performance_platform/test_total_sent_notifications.py +++ b/tests/app/performance_platform/test_total_sent_notifications.py @@ -56,6 +56,7 @@ def test_get_total_sent_notifications_yesterday_returns_expected_totals_dict( # Create some notifications for the day before yesterday = datetime(2016, 1, 10, 15, 30, 0, 0) with freeze_time(yesterday): + notification_history(notification_type='letter') notification_history(notification_type='sms') notification_history(notification_type='sms') notification_history(notification_type='email') @@ -71,5 +72,8 @@ def test_get_total_sent_notifications_yesterday_returns_expected_totals_dict( }, "sms": { "count": 2 + }, + "letter": { + "count": 1 } } From 5e44449e661c588182eef09805516a6d224b8fce Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Sat, 3 Mar 2018 00:00:41 +0000 Subject: [PATCH 05/42] Update coveralls from 1.2.0 to 1.3.0 --- requirements_for_test.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements_for_test.txt b/requirements_for_test.txt index b41e1024d..1cefc06a0 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -5,7 +5,7 @@ pytest-env==0.6.2 pytest-mock==1.7.0 pytest-cov==2.5.1 pytest-xdist==1.22.2 -coveralls==1.2.0 +coveralls==1.3.0 freezegun==0.3.9 requests-mock==1.4.0 # optional requirements for jsonschema From a4feaba3098d026aa96fd762ad820aae74ed70ea Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Mon, 5 Mar 2018 14:11:37 +0000 Subject: [PATCH 06/42] Added tests to tests for precompiled flow and refactored a little * Added is_precompiled_letter method to letter/utils.py * Added tests for letter/utils.py * Added tests for the rest endpoint * Moved the Precompiled name to a central location * Added hidden field to the test method to create a template --- app/config.py | 1 + app/letters/utils.py | 4 + app/template/rest.py | 23 ++- tests/app/db.py | 14 +- tests/app/letters/test_letter_utils.py | 29 ++++ tests/app/template/test_rest.py | 215 ++++++++++++++++++++++++- 6 files changed, 272 insertions(+), 14 deletions(-) create mode 100644 tests/app/letters/test_letter_utils.py diff --git a/app/config.py b/app/config.py index 65dee5bc3..d38039647 100644 --- a/app/config.py +++ b/app/config.py @@ -150,6 +150,7 @@ class Config(object): CHANGE_EMAIL_CONFIRMATION_TEMPLATE_ID = 'eb4d9930-87ab-4aef-9bce-786762687884' SERVICE_NOW_LIVE_TEMPLATE_ID = '618185c6-3636-49cd-b7d2-6f6f5eb3bdde' ORGANISATION_INVITATION_EMAIL_TEMPLATE_ID = '203566f0-d835-47c5-aa06-932439c86573' + PRECOMPILED_TEMPLATE_NAME = 'Pre-compiled PDF' BROKER_URL = 'sqs://' BROKER_TRANSPORT_OPTIONS = { diff --git a/app/letters/utils.py b/app/letters/utils.py index a7f05c00f..5e7730882 100644 --- a/app/letters/utils.py +++ b/app/letters/utils.py @@ -76,3 +76,7 @@ def get_letter_pdf(notification): file_content = obj.get()["Body"].read() return file_content + + +def is_precompiled_letter(template): + return template.hidden and template.name == current_app.config['PRECOMPILED_TEMPLATE_NAME'] diff --git a/app/template/rest.py b/app/template/rest.py index c0e91c69b..5f36557e0 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -1,10 +1,14 @@ import base64 + +import botocore from flask import ( Blueprint, current_app, jsonify, request) from requests import post as requests_post +from werkzeug.exceptions import abort + from app.dao.notifications_dao import get_notification_by_id from app.dao.templates_dao import ( @@ -18,7 +22,7 @@ from app.dao.templates_dao import ( dao_get_template_by_id) from notifications_utils.template import SMSMessageTemplate from app.dao.services_dao import dao_fetch_service_by_id -from app.letters.utils import get_letter_pdf +from app.letters.utils import get_letter_pdf, is_precompiled_letter from app.models import SMS_TYPE from app.notifications.validators import service_has_permission, check_reply_to from app.schemas import (template_schema, template_history_schema) @@ -196,18 +200,23 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file template = dao_get_template_by_id(notification.template_id) - if template.hidden and template.name == 'Pre-compiled PDF': + if is_precompiled_letter(template): - pdf_file = get_letter_pdf(notification) + try: + + pdf_file = get_letter_pdf(notification) + + except botocore.exceptions.ClientError: + abort(404) content = base64.b64encode(pdf_file).decode('utf-8') if file_type == 'png': - url = '{}//precompiled-preview.png{}'.format( - current_app.config['TEMPLATE_PREVIEW_API_HOST'], - '?page={}'.format(page) if page else '' - ) + url = '{}/precompiled-preview.png{}'.format( + current_app.config['TEMPLATE_PREVIEW_API_HOST'], + '?page={}'.format(page) if page else '' + ) resp = requests_post( url, diff --git a/tests/app/db.py b/tests/app/db.py index 16ff29910..6a09413a8 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -125,12 +125,13 @@ def create_service_with_defined_sms_sender( def create_template( - service, - template_type=SMS_TYPE, - template_name=None, - subject='Template subject', - content='Dear Sir/Madam, Hello. Yours Truly, The Government.', - reply_to=None + service, + template_type=SMS_TYPE, + template_name=None, + subject='Template subject', + content='Dear Sir/Madam, Hello. Yours Truly, The Government.', + reply_to=None, + hidden=False ): data = { 'name': template_name or '{} Template Name'.format(template_type), @@ -139,6 +140,7 @@ def create_template( 'service': service, 'created_by': service.created_by, 'reply_to': reply_to, + 'hidden': hidden } if template_type != SMS_TYPE: data['subject'] = subject diff --git a/tests/app/letters/test_letter_utils.py b/tests/app/letters/test_letter_utils.py new file mode 100644 index 000000000..758c35021 --- /dev/null +++ b/tests/app/letters/test_letter_utils.py @@ -0,0 +1,29 @@ +import pytest +from flask import current_app + +from app.letters.utils import get_bucket_prefix_for_notification, is_precompiled_letter + + +def test_get_bucket_prefix_for_notification_valid_notification(sample_notification): + + bucket_prefix = get_bucket_prefix_for_notification(sample_notification) + + assert bucket_prefix == '{folder}/NOTIFY.{reference}'.format( + folder=sample_notification.created_at.date(), + reference=sample_notification.reference + ).upper() + + +def test_get_bucket_prefix_for_notification_invalid_notification(): + with pytest.raises(AttributeError): + get_bucket_prefix_for_notification(None) + + +def test_is_precompiled_letter_false(sample_letter_template): + assert not is_precompiled_letter(sample_letter_template) + + +def test_is_precompiled_letter_true(sample_letter_template): + sample_letter_template.hidden = True + sample_letter_template.name = current_app.config['PRECOMPILED_TEMPLATE_NAME'] + assert is_precompiled_letter(sample_letter_template) diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index db87bb1fb..34192148d 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -4,6 +4,7 @@ import random import string from datetime import datetime, timedelta +import botocore import pytest from freezegun import freeze_time @@ -16,7 +17,7 @@ from tests.app.conftest import ( sample_template_without_email_permission, sample_template_without_letter_permission, sample_template_without_sms_permission) -from tests.app.db import create_service, create_letter_contact, create_template +from tests.app.db import create_service, create_letter_contact, create_template, create_notification from tests.conftest import set_config_values @@ -874,3 +875,215 @@ def test_preview_letter_template_by_id_template_preview_500( ) assert resp['message'] == 'Error generating preview for {}'.format(sample_letter_notification.id) + + +def test_preview_letter_template_precompiled_pdf_file_type( + notify_api, + client, + admin_request, + sample_service, + mocker +): + + template = create_template(sample_service, + template_type='letter', + template_name='Pre-compiled PDF', + subject='Pre-compiled PDF', + hidden=True) + + notification = create_notification(template) + + with set_config_values(notify_api, { + 'TEMPLATE_PREVIEW_API_HOST': 'http://localhost/notifications-template-preview', + 'TEMPLATE_PREVIEW_API_KEY': 'test-key' + }): + import requests_mock + with requests_mock.Mocker(): + + content = b'\x00\x01' + + mock_get_letter_pdf = mocker.patch('app.template.rest.get_letter_pdf', return_value=content) + + resp = admin_request.get( + 'template.preview_letter_template_by_notification_id', + service_id=notification.service_id, + notification_id=notification.id, + file_type='pdf' + ) + + assert mock_get_letter_pdf.called_once_with(notification) + assert base64.b64decode(resp['content']) == content + + +def test_preview_letter_template_precompiled_s3_error( + notify_api, + client, + admin_request, + sample_service, + mocker +): + + template = create_template(sample_service, + template_type='letter', + template_name='Pre-compiled PDF', + subject='Pre-compiled PDF', + hidden=True) + + notification = create_notification(template) + + with set_config_values(notify_api, { + 'TEMPLATE_PREVIEW_API_HOST': 'http://localhost/notifications-template-preview', + 'TEMPLATE_PREVIEW_API_KEY': 'test-key' + }): + import requests_mock + with requests_mock.Mocker(): + + mocker.patch('app.template.rest.get_letter_pdf', + side_effect=botocore.exceptions.ClientError( + {'Error': {'Code': '403', 'Message': 'Unauthorized'}}, + 'GetObject' + )) + + admin_request.get( + 'template.preview_letter_template_by_notification_id', + service_id=notification.service_id, + notification_id=notification.id, + file_type='pdf', + _expected_status=404 + ) + + +def test_preview_letter_template_precompiled_png_file_type( + notify_api, + client, + admin_request, + sample_service, + mocker +): + + template = create_template(sample_service, + template_type='letter', + template_name='Pre-compiled PDF', + subject='Pre-compiled PDF', + hidden=True) + + notification = create_notification(template) + + with set_config_values(notify_api, { + 'TEMPLATE_PREVIEW_API_HOST': 'http://localhost/notifications-template-preview', + 'TEMPLATE_PREVIEW_API_KEY': 'test-key' + }): + import requests_mock + with requests_mock.Mocker() as request_mock: + + pdf_content = b'\x00\x01' + png_content = b'\x00\x02' + + mock_get_letter_pdf = mocker.patch('app.template.rest.get_letter_pdf', return_value=pdf_content) + + request_mock.post( + 'http://localhost/notifications-template-preview/precompiled-preview.png', + content=png_content, + headers={'X-pdf-page-count': '1'}, + status_code=200 + ) + + resp = admin_request.get( + 'template.preview_letter_template_by_notification_id', + service_id=notification.service_id, + notification_id=notification.id, + file_type='png' + ) + + assert mock_get_letter_pdf.called_once_with(notification) + assert base64.b64decode(resp['content']) == png_content + + +def test_preview_letter_template_precompiled_png_template_preview_500_error( + notify_api, + client, + admin_request, + sample_service, + mocker +): + + template = create_template(sample_service, + template_type='letter', + template_name='Pre-compiled PDF', + subject='Pre-compiled PDF', + hidden=True) + + notification = create_notification(template) + + with set_config_values(notify_api, { + 'TEMPLATE_PREVIEW_API_HOST': 'http://localhost/notifications-template-preview', + 'TEMPLATE_PREVIEW_API_KEY': 'test-key' + }): + import requests_mock + with requests_mock.Mocker() as request_mock: + + pdf_content = b'\x00\x01' + png_content = b'\x00\x02' + + mocker.patch('app.template.rest.get_letter_pdf', return_value=pdf_content) + + request_mock.post( + 'http://localhost/notifications-template-preview/precompiled-preview.png', + content=png_content, + headers={'X-pdf-page-count': '1'}, + status_code=500 + ) + + admin_request.get( + 'template.preview_letter_template_by_notification_id', + service_id=notification.service_id, + notification_id=notification.id, + file_type='png', + _expected_status=500 + + ) + + +def test_preview_letter_template_precompiled_png_template_preview_400_error( + notify_api, + client, + admin_request, + sample_service, + mocker +): + + template = create_template(sample_service, + template_type='letter', + template_name='Pre-compiled PDF', + subject='Pre-compiled PDF', + hidden=True) + + notification = create_notification(template) + + with set_config_values(notify_api, { + 'TEMPLATE_PREVIEW_API_HOST': 'http://localhost/notifications-template-preview', + 'TEMPLATE_PREVIEW_API_KEY': 'test-key' + }): + import requests_mock + with requests_mock.Mocker() as request_mock: + + pdf_content = b'\x00\x01' + png_content = b'\x00\x02' + + mocker.patch('app.template.rest.get_letter_pdf', return_value=pdf_content) + + request_mock.post( + 'http://localhost/notifications-template-preview/precompiled-preview.png', + content=png_content, + headers={'X-pdf-page-count': '1'}, + status_code=404 + ) + + admin_request.get( + 'template.preview_letter_template_by_notification_id', + service_id=notification.service_id, + notification_id=notification.id, + file_type='png', + _expected_status=500 + + ) From e91a0efc43dd57e13277eb1cba8e815dcf15fbe4 Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Mon, 5 Mar 2018 14:54:18 +0000 Subject: [PATCH 07/42] Refactored code to make it more maintainable and changed an error type * Rather than an abort 404 returned a 500 and InvalidRequest so that the error is more easily handled on the admin console. If the file is missing but expected to be there is actually an internal error for admin * Refactored the code to remove duplicate code in calls to template preview by creating a new private method which is called with specific parameters --- app/template/rest.py | 52 +++++++++++++++------------------ tests/app/template/test_rest.py | 2 +- 2 files changed, 25 insertions(+), 29 deletions(-) diff --git a/app/template/rest.py b/app/template/rest.py index 5f36557e0..e49eea762 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -7,7 +7,6 @@ from flask import ( jsonify, request) from requests import post as requests_post -from werkzeug.exceptions import abort from app.dao.notifications_dao import get_notification_by_id @@ -207,7 +206,9 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file pdf_file = get_letter_pdf(notification) except botocore.exceptions.ClientError: - abort(404) + current_app.logger.info + raise InvalidRequest('Error getting letter file from S3 notification id {}'.format(notification_id), + status_code=500) content = base64.b64encode(pdf_file).decode('utf-8') @@ -218,18 +219,7 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file '?page={}'.format(page) if page else '' ) - resp = requests_post( - url, - data=content, - headers={'Authorization': 'Token {}'.format(current_app.config['TEMPLATE_PREVIEW_API_KEY'])} - ) - - if resp.status_code != 200: - raise InvalidRequest( - 'Error generating preview for {}'.format(notification_id), status_code=500 - ) - - content = base64.b64encode(resp.content).decode('utf-8') + content = _get_png_preview(url, content, notification.id) else: @@ -249,21 +239,27 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file 'dvla_org_id': service.dvla_organisation_id, } - resp = requests_post( - '{}/preview.{}{}'.format( - current_app.config['TEMPLATE_PREVIEW_API_HOST'], - file_type, - '?page={}'.format(page) if page else '' - ), - json=data, - headers={'Authorization': 'Token {}'.format(current_app.config['TEMPLATE_PREVIEW_API_KEY'])} + url = '{}/preview.{}{}'.format( + current_app.config['TEMPLATE_PREVIEW_API_HOST'], + file_type, + '?page={}'.format(page) if page else '' ) - if resp.status_code != 200: - raise InvalidRequest( - 'Error generating preview for {}'.format(notification_id), status_code=500 - ) - - content = base64.b64encode(resp.content).decode('utf-8') + content = _get_png_preview(url, data, notification.id) return jsonify({"content": content}) + + +def _get_png_preview(url, data, notification_id): + resp = requests_post( + url, + data=data, + headers={'Authorization': 'Token {}'.format(current_app.config['TEMPLATE_PREVIEW_API_KEY'])} + ) + + if resp.status_code != 200: + raise InvalidRequest( + 'Error generating preview for {}'.format(notification_id), status_code=500 + ) + + return base64.b64encode(resp.content).decode('utf-8') diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 34192148d..cfd92f0d3 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -949,7 +949,7 @@ def test_preview_letter_template_precompiled_s3_error( service_id=notification.service_id, notification_id=notification.id, file_type='pdf', - _expected_status=404 + _expected_status=500 ) From 033a4099bcf7a8a00a5568c2dfff3ad6d6a2dd2f Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Mon, 5 Mar 2018 16:57:48 +0000 Subject: [PATCH 08/42] Added tests for all conditions for s_precompiled_letter. * Added tests for hidden = true but name not precompiled * Added test where name is precompiled but hidden is false --- tests/app/letters/test_letter_utils.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/app/letters/test_letter_utils.py b/tests/app/letters/test_letter_utils.py index 758c35021..efa55bc0b 100644 --- a/tests/app/letters/test_letter_utils.py +++ b/tests/app/letters/test_letter_utils.py @@ -27,3 +27,13 @@ def test_is_precompiled_letter_true(sample_letter_template): sample_letter_template.hidden = True sample_letter_template.name = current_app.config['PRECOMPILED_TEMPLATE_NAME'] assert is_precompiled_letter(sample_letter_template) + + +def test_is_precompiled_letter_hidden_true_not_name(sample_letter_template): + sample_letter_template.hidden = True + assert not is_precompiled_letter(sample_letter_template) + + +def test_is_precompiled_letter_name_correct_not_hidden(sample_letter_template): + sample_letter_template.name = current_app.config['PRECOMPILED_TEMPLATE_NAME'] + assert not is_precompiled_letter(sample_letter_template) From ca167206d5dd913e888429b25e1821268ed73e2d Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 5 Mar 2018 16:44:20 +0000 Subject: [PATCH 09/42] Add command to backfill Performance Platform totals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We don’t have any way of playing back the totals we send to performance platform. This commit copies the command used to backfill the processing time and adapts it to backfill the totals instead. Under the hood it uses the same code that we use in the scheduled tasks to update performance platform on a daily basis. I had to modify this code to take a `day` argument because it was hardcoded to only work for ‘yesterday’. --- app/celery/scheduled_tasks.py | 11 ++++---- app/commands.py | 28 +++++++++++++++++-- .../total_sent_notifications.py | 14 ++++------ tests/app/celery/test_scheduled_tasks.py | 2 +- .../test_total_sent_notifications.py | 14 ++++++++-- tests/app/test_commands.py | 15 +++++++++- 6 files changed, 64 insertions(+), 20 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 7500111a7..b5f904f2b 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -213,20 +213,21 @@ def timeout_notifications(): @statsd(namespace="tasks") def send_daily_performance_platform_stats(): if performance_platform_client.active: - send_total_sent_notifications_to_performance_platform() + yesterday = datetime.utcnow() - timedelta(days=1) + send_total_sent_notifications_to_performance_platform(yesterday) processing_time.send_processing_time_to_performance_platform() -def send_total_sent_notifications_to_performance_platform(): - count_dict = total_sent_notifications.get_total_sent_notifications_yesterday() +def send_total_sent_notifications_to_performance_platform(day): + count_dict = total_sent_notifications.get_total_sent_notifications_for_day(day) email_sent_count = count_dict.get('email').get('count') sms_sent_count = count_dict.get('sms').get('count') letter_sent_count = count_dict.get('letter').get('count') start_date = count_dict.get('start_date') current_app.logger.info( - "Attempting to update performance platform for date {} with email count {} and sms count {}" - .format(start_date, email_sent_count, sms_sent_count) + "Attempting to update Performance Platform for {} with {} emails, {} text messages and {} letters" + .format(start_date, email_sent_count, sms_sent_count, letter_sent_count) ) total_sent_notifications.send_total_notifications_sent_for_day_stats( diff --git a/app/commands.py b/app/commands.py index 06a9b7a75..d8775f217 100644 --- a/app/commands.py +++ b/app/commands.py @@ -22,7 +22,8 @@ from app.dao.services_dao import ( from app.dao.provider_rates_dao import create_provider_rates as dao_create_provider_rates from app.dao.users_dao import (delete_model_user, delete_user_verify_codes) from app.utils import get_midnight_for_day_before, get_london_midnight_in_utc -from app.performance_platform.processing_time import send_processing_time_for_start_and_end +from app.performance_platform.processing_time import (send_processing_time_for_start_and_end) +from app.celery.scheduled_tasks import send_total_sent_notifications_to_performance_platform @click.group(name='command', help='Additional commands') @@ -209,12 +210,35 @@ def populate_monthly_billing(year): populate(service_id, year, i) +@notify_command() +@click.option('-s', '--start_date', required=True, help="start date inclusive", type=click_dt(format='%Y-%m-%d')) +@click.option('-e', '--end_date', required=True, help="end date inclusive", type=click_dt(format='%Y-%m-%d')) +def backfill_performance_platform_totals(start_date, end_date): + """ + Send historical total messages sent to Performance Platform. + """ + + delta = end_date - start_date + + print('Sending total messages sent for all days between {} and {}'.format(start_date, end_date)) + + for i in range(delta.days + 1): + + process_date = start_date + timedelta(days=i) + + print('Sending total messages sent for {}'.format( + process_date.isoformat() + )) + + send_total_sent_notifications_to_performance_platform(process_date) + + @notify_command() @click.option('-s', '--start_date', required=True, help="start date inclusive", type=click_dt(format='%Y-%m-%d')) @click.option('-e', '--end_date', required=True, help="end date inclusive", type=click_dt(format='%Y-%m-%d')) def backfill_processing_time(start_date, end_date): """ - Send historical performance platform stats. + Send historical processing time to Performance Platform. """ delta = end_date - start_date diff --git a/app/performance_platform/total_sent_notifications.py b/app/performance_platform/total_sent_notifications.py index 4aa62c786..14695e9e9 100644 --- a/app/performance_platform/total_sent_notifications.py +++ b/app/performance_platform/total_sent_notifications.py @@ -1,11 +1,8 @@ -from datetime import datetime +from datetime import timedelta from app import performance_platform_client from app.dao.notifications_dao import get_total_sent_notifications_in_date_range -from app.utils import ( - get_london_midnight_in_utc, - get_midnight_for_day_before -) +from app.utils import get_london_midnight_in_utc def send_total_notifications_sent_for_day_stats(date, notification_type, count): @@ -20,10 +17,9 @@ def send_total_notifications_sent_for_day_stats(date, notification_type, count): performance_platform_client.send_stats_to_performance_platform(payload) -def get_total_sent_notifications_yesterday(): - today = datetime.utcnow() - start_date = get_midnight_for_day_before(today) - end_date = get_london_midnight_in_utc(today) +def get_total_sent_notifications_for_day(day): + start_date = get_london_midnight_in_utc(day) + end_date = start_date + timedelta(days=1) email_count = get_total_sent_notifications_in_date_range(start_date, end_date, 'email') sms_count = get_total_sent_notifications_in_date_range(start_date, end_date, 'sms') diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 6c2094cf8..8e3a3a33e 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -335,7 +335,7 @@ def test_send_total_sent_notifications_to_performance_platform_calls_with_correc new_callable=PropertyMock ) as mock_active: mock_active.return_value = True - send_total_sent_notifications_to_performance_platform() + send_total_sent_notifications_to_performance_platform(yesterday) perf_mock.assert_has_calls([ call(get_london_midnight_in_utc(yesterday), 'sms', 2), diff --git a/tests/app/performance_platform/test_total_sent_notifications.py b/tests/app/performance_platform/test_total_sent_notifications.py index 2ad2a93f6..3aecf7447 100644 --- a/tests/app/performance_platform/test_total_sent_notifications.py +++ b/tests/app/performance_platform/test_total_sent_notifications.py @@ -6,7 +6,7 @@ from freezegun import freeze_time from app.utils import get_midnight_for_day_before from app.performance_platform.total_sent_notifications import ( send_total_notifications_sent_for_day_stats, - get_total_sent_notifications_yesterday + get_total_sent_notifications_for_day ) from tests.app.conftest import ( @@ -55,6 +55,7 @@ def test_get_total_sent_notifications_yesterday_returns_expected_totals_dict( # Create some notifications for the day before yesterday = datetime(2016, 1, 10, 15, 30, 0, 0) + ereyesterday = datetime(2016, 1, 9, 15, 30, 0, 0) with freeze_time(yesterday): notification_history(notification_type='letter') notification_history(notification_type='sms') @@ -63,7 +64,7 @@ def test_get_total_sent_notifications_yesterday_returns_expected_totals_dict( notification_history(notification_type='email') notification_history(notification_type='email') - total_count_dict = get_total_sent_notifications_yesterday() + total_count_dict = get_total_sent_notifications_for_day(yesterday) assert total_count_dict == { "start_date": get_midnight_for_day_before(datetime.utcnow()), @@ -77,3 +78,12 @@ def test_get_total_sent_notifications_yesterday_returns_expected_totals_dict( "count": 1 } } + + another_day = get_total_sent_notifications_for_day(ereyesterday) + + assert another_day == { + 'email': {'count': 0}, + 'letter': {'count': 0}, + 'sms': {'count': 0}, + 'start_date': datetime(2016, 1, 9, 0, 0), + } diff --git a/tests/app/test_commands.py b/tests/app/test_commands.py index d5313d128..e7b8ac360 100644 --- a/tests/app/test_commands.py +++ b/tests/app/test_commands.py @@ -1,6 +1,6 @@ from datetime import datetime -from app.commands import backfill_processing_time +from app.commands import backfill_performance_platform_totals, backfill_processing_time def test_backfill_processing_time_works_for_correct_dates(mocker, notify_api): @@ -14,3 +14,16 @@ def test_backfill_processing_time_works_for_correct_dates(mocker, notify_api): send_mock.assert_any_call(datetime(2017, 7, 31, 23, 0), datetime(2017, 8, 1, 23, 0)) send_mock.assert_any_call(datetime(2017, 8, 1, 23, 0), datetime(2017, 8, 2, 23, 0)) send_mock.assert_any_call(datetime(2017, 8, 2, 23, 0), datetime(2017, 8, 3, 23, 0)) + + +def test_backfill_totals_works_for_correct_dates(mocker, notify_api): + send_mock = mocker.patch('app.commands.send_total_sent_notifications_to_performance_platform') + + # backfill_processing_time is a click.Command object - if you try invoking the callback on its own, it + # throws a `RuntimeError: There is no active click context.` - so get at the original function using __wrapped__ + backfill_performance_platform_totals.callback.__wrapped__(datetime(2017, 8, 1), datetime(2017, 8, 3)) + + assert send_mock.call_count == 3 + send_mock.assert_any_call(datetime(2017, 8, 1)) + send_mock.assert_any_call(datetime(2017, 8, 2)) + send_mock.assert_any_call(datetime(2017, 8, 3)) From bfdd385ba39e94993632db7ba529e859749e4c4e Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Mon, 5 Mar 2018 17:08:09 +0000 Subject: [PATCH 10/42] Tidied up imports. Removed local imports and made a global import so its easier to maintain in future. --- tests/app/template/test_rest.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index cfd92f0d3..14e0aba55 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -6,6 +6,7 @@ from datetime import datetime, timedelta import botocore import pytest +import requests_mock from freezegun import freeze_time from app.models import Template, SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, TemplateHistory @@ -824,7 +825,6 @@ def test_preview_letter_template_by_id_valid_file_type( 'TEMPLATE_PREVIEW_API_HOST': 'http://localhost/notifications-template-preview', 'TEMPLATE_PREVIEW_API_KEY': 'test-key' }): - import requests_mock with requests_mock.Mocker() as request_mock: content = b'\x00\x01' @@ -897,7 +897,6 @@ def test_preview_letter_template_precompiled_pdf_file_type( 'TEMPLATE_PREVIEW_API_HOST': 'http://localhost/notifications-template-preview', 'TEMPLATE_PREVIEW_API_KEY': 'test-key' }): - import requests_mock with requests_mock.Mocker(): content = b'\x00\x01' @@ -935,7 +934,6 @@ def test_preview_letter_template_precompiled_s3_error( 'TEMPLATE_PREVIEW_API_HOST': 'http://localhost/notifications-template-preview', 'TEMPLATE_PREVIEW_API_KEY': 'test-key' }): - import requests_mock with requests_mock.Mocker(): mocker.patch('app.template.rest.get_letter_pdf', @@ -973,7 +971,6 @@ def test_preview_letter_template_precompiled_png_file_type( 'TEMPLATE_PREVIEW_API_HOST': 'http://localhost/notifications-template-preview', 'TEMPLATE_PREVIEW_API_KEY': 'test-key' }): - import requests_mock with requests_mock.Mocker() as request_mock: pdf_content = b'\x00\x01' @@ -1019,7 +1016,6 @@ def test_preview_letter_template_precompiled_png_template_preview_500_error( 'TEMPLATE_PREVIEW_API_HOST': 'http://localhost/notifications-template-preview', 'TEMPLATE_PREVIEW_API_KEY': 'test-key' }): - import requests_mock with requests_mock.Mocker() as request_mock: pdf_content = b'\x00\x01' @@ -1064,7 +1060,6 @@ def test_preview_letter_template_precompiled_png_template_preview_400_error( 'TEMPLATE_PREVIEW_API_HOST': 'http://localhost/notifications-template-preview', 'TEMPLATE_PREVIEW_API_KEY': 'test-key' }): - import requests_mock with requests_mock.Mocker() as request_mock: pdf_content = b'\x00\x01' From acd6b0c05ba7e0e6246cd24fe6107882110a8cff Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Tue, 6 Mar 2018 00:07:20 +0000 Subject: [PATCH 11/42] Update pytest from 3.4.1 to 3.4.2 --- requirements_for_test.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements_for_test.txt b/requirements_for_test.txt index 9240716a8..5b44ae7d2 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -1,6 +1,6 @@ -r requirements.txt flake8==3.5.0 -pytest==3.4.1 +pytest==3.4.2 pytest-env==0.6.2 pytest-mock==1.7.1 pytest-cov==2.5.1 From 3a9e3909029f44130b3b71354fd0fe8db76598cb Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Tue, 6 Mar 2018 03:11:22 +0000 Subject: [PATCH 12/42] Update freezegun from 0.3.9 to 0.3.10 --- requirements_for_test.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements_for_test.txt b/requirements_for_test.txt index 9240716a8..0cfd4c24f 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -6,7 +6,7 @@ pytest-mock==1.7.1 pytest-cov==2.5.1 pytest-xdist==1.22.2 coveralls==1.3.0 -freezegun==0.3.9 +freezegun==0.3.10 requests-mock==1.4.0 # optional requirements for jsonschema strict-rfc3339==0.7 From b0de3ba4d9d65d4a80060d34fd48001e99147394 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Thu, 15 Feb 2018 14:27:38 +0000 Subject: [PATCH 13/42] Create DailySortedLetter table The response files we receive from the DVLA when we send letters contain a row for each letter and a field with a value of 'Unsorted' or 'Sorted'. This table will be used to store the total number of 'Unsorted' and 'Sorted' letter notifications per day. --- app/models.py | 10 +++++++ .../0173_create_daily_sorted_letter.py | 30 +++++++++++++++++++ 2 files changed, 40 insertions(+) create mode 100644 migrations/versions/0173_create_daily_sorted_letter.py diff --git a/app/models.py b/app/models.py index 6678d91e6..47256a63d 100644 --- a/app/models.py +++ b/app/models.py @@ -1752,3 +1752,13 @@ class StatsTemplateUsageByMonth(db.Model): 'year': self.year, 'count': self.count } + + +class DailySortedLetter(db.Model): + __tablename__ = "daily_sorted_letter" + + id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) + billing_day = db.Column(db.Date, nullable=False, index=True, unique=True) + unsorted_count = db.Column(db.Integer, nullable=False, default=0) + sorted_count = db.Column(db.Integer, nullable=False, default=0) + updated_at = db.Column(db.DateTime, nullable=True, onupdate=datetime.datetime.utcnow) diff --git a/migrations/versions/0173_create_daily_sorted_letter.py b/migrations/versions/0173_create_daily_sorted_letter.py new file mode 100644 index 000000000..3215134b9 --- /dev/null +++ b/migrations/versions/0173_create_daily_sorted_letter.py @@ -0,0 +1,30 @@ +""" + +Revision ID: 0173_create_daily_sorted_letter +Revises: 0172_deprioritise_examples +Create Date: 2018-03-01 11:53:32.964256 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +revision = '0173_create_daily_sorted_letter' +down_revision = '0172_deprioritise_examples' + + +def upgrade(): + op.create_table('daily_sorted_letter', + sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('billing_day', sa.Date(), nullable=False), + sa.Column('unsorted_count', sa.Integer(), nullable=False), + sa.Column('sorted_count', sa.Integer(), nullable=False), + sa.Column('updated_at', sa.DateTime(), nullable=True), + sa.PrimaryKeyConstraint('id') + ) + op.create_index(op.f('ix_daily_sorted_letter_billing_day'), 'daily_sorted_letter', ['billing_day'], unique=True) + + +def downgrade(): + op.drop_index(op.f('ix_daily_sorted_letter_billing_day'), table_name='daily_sorted_letter') + op.drop_table('daily_sorted_letter') From 1b5aaed10a2a0eb05aeb1c52679d6948e3a985e7 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Mon, 19 Feb 2018 14:00:33 +0000 Subject: [PATCH 14/42] Create DAO for daily sorted letter model --- app/dao/daily_sorted_letter_dao.py | 37 +++++++++++++++ tests/app/dao/test_daily_sorted_letter_dao.py | 47 +++++++++++++++++++ tests/app/db.py | 16 ++++++- 3 files changed, 99 insertions(+), 1 deletion(-) create mode 100644 app/dao/daily_sorted_letter_dao.py create mode 100644 tests/app/dao/test_daily_sorted_letter_dao.py diff --git a/app/dao/daily_sorted_letter_dao.py b/app/dao/daily_sorted_letter_dao.py new file mode 100644 index 000000000..3afad4b2a --- /dev/null +++ b/app/dao/daily_sorted_letter_dao.py @@ -0,0 +1,37 @@ +from datetime import datetime + +from sqlalchemy.dialects.postgresql import insert + +from app import db +from app.dao.dao_utils import transactional +from app.models import DailySortedLetter + + +def dao_get_daily_sorted_letter_by_billing_day(billing_day): + return DailySortedLetter.query.filter_by( + billing_day=billing_day + ).first() + + +@transactional +def dao_create_or_update_daily_sorted_letter(new_daily_sorted_letter): + ''' + This uses the Postgres upsert to avoid race conditions when two threads try and insert + at the same row. The excluded object refers to values that we tried to insert but were + rejected. + http://docs.sqlalchemy.org/en/latest/dialects/postgresql.html#insert-on-conflict-upsert + ''' + table = DailySortedLetter.__table__ + stmt = insert(table).values( + billing_day=new_daily_sorted_letter.billing_day, + unsorted_count=new_daily_sorted_letter.unsorted_count, + sorted_count=new_daily_sorted_letter.sorted_count) + stmt = stmt.on_conflict_do_update( + index_elements=[table.c.billing_day], + set_={ + 'unsorted_count': table.c.unsorted_count + stmt.excluded.unsorted_count, + 'sorted_count': table.c.sorted_count + stmt.excluded.sorted_count, + 'updated_at': datetime.utcnow() + } + ) + db.session.connection().execute(stmt) diff --git a/tests/app/dao/test_daily_sorted_letter_dao.py b/tests/app/dao/test_daily_sorted_letter_dao.py new file mode 100644 index 000000000..2a4ceb318 --- /dev/null +++ b/tests/app/dao/test_daily_sorted_letter_dao.py @@ -0,0 +1,47 @@ +from datetime import date + +from app.dao.daily_sorted_letter_dao import ( + dao_create_or_update_daily_sorted_letter, + dao_get_daily_sorted_letter_by_billing_day, +) +from app.models import DailySortedLetter +from tests.app.db import create_daily_sorted_letter + + +def test_dao_get_daily_sorted_letter_by_billing_day(notify_db, notify_db_session): + billing_day = date(2018, 2, 1) + other_day = date(2017, 9, 8) + + daily_sorted_letters = create_daily_sorted_letter(billing_day=billing_day) + + assert dao_get_daily_sorted_letter_by_billing_day(billing_day) == daily_sorted_letters + assert not dao_get_daily_sorted_letter_by_billing_day(other_day) + + +def test_dao_create_or_update_daily_sorted_letter_creates_a_new_entry(notify_db, notify_db_session): + billing_day = date(2018, 2, 1) + dsl = DailySortedLetter(billing_day=billing_day, unsorted_count=2, sorted_count=0) + dao_create_or_update_daily_sorted_letter(dsl) + + daily_sorted_letter = dao_get_daily_sorted_letter_by_billing_day(billing_day) + + assert daily_sorted_letter.billing_day == billing_day + assert daily_sorted_letter.unsorted_count == 2 + assert daily_sorted_letter.sorted_count == 0 + assert not daily_sorted_letter.updated_at + + +def test_dao_create_or_update_daily_sorted_letter_updates_an_existing_entry( + notify_db, + notify_db_session +): + create_daily_sorted_letter(unsorted_count=2, sorted_count=3) + + dsl = DailySortedLetter(billing_day=date(2018, 1, 18), unsorted_count=5, sorted_count=17) + dao_create_or_update_daily_sorted_letter(dsl) + + daily_sorted_letter = dao_get_daily_sorted_letter_by_billing_day(dsl.billing_day) + + assert daily_sorted_letter.unsorted_count == 7 + assert daily_sorted_letter.sorted_count == 20 + assert daily_sorted_letter.updated_at diff --git a/tests/app/db.py b/tests/app/db.py index 6a09413a8..b159976da 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -1,4 +1,4 @@ -from datetime import datetime +from datetime import datetime, date import uuid from app import db @@ -9,6 +9,7 @@ from app.dao.service_sms_sender_dao import update_existing_sms_sender_with_inbou from app.dao.invited_org_user_dao import save_invited_org_user from app.models import ( ApiKey, + DailySortedLetter, InboundSms, InboundNumber, Job, @@ -506,3 +507,16 @@ def create_invited_org_user(organisation, invited_by, email_address='invite@exam ) save_invited_org_user(invited_org_user) return invited_org_user + + +def create_daily_sorted_letter(billing_day=date(2018, 1, 18), unsorted_count=0, sorted_count=0): + daily_sorted_letter = DailySortedLetter( + billing_day=billing_day, + unsorted_count=unsorted_count, + sorted_count=sorted_count + ) + + db.session.add(daily_sorted_letter) + db.session.commit() + + return daily_sorted_letter From 136d89e2f1f246633d73523cfdcd883f234fd9c8 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Tue, 20 Feb 2018 10:57:25 +0000 Subject: [PATCH 15/42] Persist daily sorted letter counts from response file In the update_letter_notifications_statuses task we now check whether each row in the response file that we receive from the DVLA has the value of 'Sorted' or 'Unsorted' in the postcode validation field. We then calculate the number of Sorted and Unsorted rows for each day and save each day as a row in the daily_sorted_letter table. The data in daily_sorted_letter table should be in local time, so we convert the datetime before saving. --- app/celery/tasks.py | 33 ++++++- tests/app/celery/test_ftp_update_tasks.py | 107 +++++++++++++++++++--- 2 files changed, 128 insertions(+), 12 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 20b3088c2..f317c634b 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -1,6 +1,6 @@ import json from datetime import datetime -from collections import namedtuple +from collections import namedtuple, defaultdict from celery.signals import worker_process_shutdown from flask import current_app @@ -31,6 +31,7 @@ from app import ( from app.aws import s3 from app.celery import provider_tasks, letters_pdf_tasks, research_mode_tasks from app.config import QueueNames +from app.dao.daily_sorted_letter_dao import dao_create_or_update_daily_sorted_letter from app.dao.inbound_sms_dao import dao_get_inbound_sms_by_id from app.dao.jobs_dao import ( dao_update_job, @@ -66,9 +67,11 @@ from app.models import ( NOTIFICATION_TEMPORARY_FAILURE, NOTIFICATION_TECHNICAL_FAILURE, SMS_TYPE, + DailySortedLetter, ) from app.notifications.process_notifications import persist_notification from app.service.utils import service_allowed_to_send_to +from app.utils import convert_utc_to_bst @worker_process_shutdown.connect @@ -404,6 +407,7 @@ def get_template_class(template_type): def update_letter_notifications_statuses(self, filename): bucket_location = '{}-ftp'.format(current_app.config['NOTIFY_EMAIL_DOMAIN']) response_file_content = s3.get_s3_file(bucket_location, filename) + sorted_letter_counts = defaultdict(int) try: notification_updates = process_updates_from_file(response_file_content) @@ -414,6 +418,7 @@ def update_letter_notifications_statuses(self, filename): for update in notification_updates: check_billable_units(update) update_letter_notification(filename, temporary_failures, update) + sorted_letter_counts[update.cost_threshold] += 1 if temporary_failures: # This will alert Notify that DVLA was unable to deliver the letters, we need to investigate @@ -421,6 +426,32 @@ def update_letter_notifications_statuses(self, filename): filename=filename, failures=temporary_failures) raise DVLAException(message) + if sorted_letter_counts.keys() - {'Unsorted', 'Sorted'}: + unknown_status = sorted_letter_counts.keys() - {'Unsorted', 'Sorted'} + + message = 'DVLA response file: {} contains unknown Sorted status {}'.format( + filename, unknown_status + ) + raise DVLAException(message) + + billing_date = get_billing_date_in_bst_from_filename(filename) + persist_daily_sorted_letter_counts(billing_date, sorted_letter_counts) + + +def get_billing_date_in_bst_from_filename(filename): + datetime_string = filename.split('.')[1] + datetime_obj = datetime.strptime(datetime_string, '%Y%m%d%H%M%S') + return convert_utc_to_bst(datetime_obj).date() + + +def persist_daily_sorted_letter_counts(day, sorted_letter_counts): + daily_letter_count = DailySortedLetter( + billing_day=day, + unsorted_count=sorted_letter_counts['Unsorted'], + sorted_count=sorted_letter_counts['Sorted'] + ) + dao_create_or_update_daily_sorted_letter(daily_letter_count) + def process_updates_from_file(response_file): NotificationUpdate = namedtuple('NotificationUpdate', ['reference', 'status', 'page_count', 'cost_threshold']) diff --git a/tests/app/celery/test_ftp_update_tasks.py b/tests/app/celery/test_ftp_update_tasks.py index 05b73329f..435ccab93 100644 --- a/tests/app/celery/test_ftp_update_tasks.py +++ b/tests/app/celery/test_ftp_update_tasks.py @@ -1,5 +1,5 @@ -from collections import namedtuple -from datetime import datetime +from collections import namedtuple, defaultdict +from datetime import datetime, date import pytest from freezegun import freeze_time @@ -18,12 +18,15 @@ from app.models import ( ) from app.celery.tasks import ( check_billable_units, + get_billing_date_in_bst_from_filename, + persist_daily_sorted_letter_counts, process_updates_from_file, update_dvla_job_to_error, update_letter_notifications_statuses, update_letter_notifications_to_error, update_letter_notifications_to_sent_to_dvla ) +from app.dao.daily_sorted_letter_dao import dao_get_daily_sorted_letter_by_billing_day from tests.app.db import create_notification, create_service_callback_api from tests.conftest import set_config @@ -56,8 +59,8 @@ def test_update_letter_notifications_statuses_raises_for_invalid_format(notify_a mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=invalid_file) with pytest.raises(DVLAException) as e: - update_letter_notifications_statuses(filename='foo.txt') - assert 'DVLA response file: {} has an invalid format'.format('foo.txt') in str(e) + update_letter_notifications_statuses(filename='NOTIFY.20170823160812.RSP.TXT') + assert 'DVLA response file: {} has an invalid format'.format('NOTIFY.20170823160812.RSP.TXT') in str(e) def test_update_letter_notification_statuses_when_notification_does_not_exist_updates_notification_history( @@ -70,7 +73,7 @@ def test_update_letter_notification_statuses_when_notification_does_not_exist_up billable_units=1) Notification.query.filter_by(id=notification.id).delete() - update_letter_notifications_statuses(filename="older_than_7_days.txt") + update_letter_notifications_statuses(filename="NOTIFY.20170823160812.RSP.TXT") updated_history = NotificationHistory.query.filter_by(id=notification.id).one() assert updated_history.status == NOTIFICATION_DELIVERED @@ -90,12 +93,35 @@ def test_update_letter_notifications_statuses_raises_dvla_exception(notify_api, ) in str(e) +def test_update_letter_notifications_statuses_raises_error_for_unknown_sorted_status( + notify_api, + mocker, + sample_letter_template +): + sent_letter_1 = create_notification(sample_letter_template, reference='ref-foo', status=NOTIFICATION_SENDING) + sent_letter_2 = create_notification(sample_letter_template, reference='ref-bar', status=NOTIFICATION_SENDING) + valid_file = '{}|Sent|1|Unsorted\n{}|Sent|2|Error'.format( + sent_letter_1.reference, sent_letter_2.reference) + + mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=valid_file) + + with pytest.raises(DVLAException) as e: + update_letter_notifications_statuses(filename='NOTIFY.20170823160812.RSP.TXT') + + assert "DVLA response file: {filename} contains unknown Sorted status {unknown_status}".format( + filename="NOTIFY.20170823160812.RSP.TXT", unknown_status="{'Error'}" + ) in str(e) + + def test_update_letter_notifications_statuses_calls_with_correct_bucket_location(notify_api, mocker): s3_mock = mocker.patch('app.celery.tasks.s3.get_s3_object') with set_config(notify_api, 'NOTIFY_EMAIL_DOMAIN', 'foo.bar'): - update_letter_notifications_statuses(filename='foo.txt') - s3_mock.assert_called_with('{}-ftp'.format(current_app.config['NOTIFY_EMAIL_DOMAIN']), 'foo.txt') + update_letter_notifications_statuses(filename='NOTIFY.20170823160812.RSP.TXT') + s3_mock.assert_called_with('{}-ftp'.format( + current_app.config['NOTIFY_EMAIL_DOMAIN']), + 'NOTIFY.20170823160812.RSP.TXT' + ) def test_update_letter_notifications_statuses_builds_updates_from_content(notify_api, mocker): @@ -103,7 +129,7 @@ def test_update_letter_notifications_statuses_builds_updates_from_content(notify mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=valid_file) update_mock = mocker.patch('app.celery.tasks.process_updates_from_file') - update_letter_notifications_statuses(filename='foo.txt') + update_letter_notifications_statuses(filename='NOTIFY.20170823160812.RSP.TXT') update_mock.assert_called_with('ref-foo|Sent|1|Unsorted\nref-bar|Sent|2|Sorted') @@ -136,7 +162,7 @@ def test_update_letter_notifications_statuses_persisted(notify_api, mocker, samp mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=valid_file) with pytest.raises(expected_exception=DVLAException) as e: - update_letter_notifications_statuses(filename='foo.txt') + update_letter_notifications_statuses(filename='NOTIFY.20170823160812.RSP.TXT') assert sent_letter.status == NOTIFICATION_DELIVERED assert sent_letter.billable_units == 1 @@ -145,7 +171,45 @@ def test_update_letter_notifications_statuses_persisted(notify_api, mocker, samp assert failed_letter.billable_units == 2 assert failed_letter.updated_at assert "DVLA response file: {filename} has failed letters with notification.reference {failures}".format( - filename="foo.txt", failures=[format(failed_letter.reference)]) in str(e) + filename="NOTIFY.20170823160812.RSP.TXT", failures=[format(failed_letter.reference)]) in str(e) + + +def test_update_letter_notifications_statuses_persists_daily_sorted_letter_count( + notify_api, + mocker, + sample_letter_template +): + sent_letter_1 = create_notification(sample_letter_template, reference='ref-foo', status=NOTIFICATION_SENDING) + sent_letter_2 = create_notification(sample_letter_template, reference='ref-bar', status=NOTIFICATION_SENDING) + valid_file = '{}|Sent|1|Unsorted\n{}|Sent|2|Sorted'.format( + sent_letter_1.reference, sent_letter_2.reference) + + mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=valid_file) + persist_letter_count_mock = mocker.patch('app.celery.tasks.persist_daily_sorted_letter_counts') + + update_letter_notifications_statuses(filename='NOTIFY.20170823160812.RSP.TXT') + + persist_letter_count_mock.assert_called_once_with(date(2017, 8, 23), {'Unsorted': 1, 'Sorted': 1}) + + +def test_update_letter_notifications_statuses_persists_daily_sorted_letter_count_with_no_sorted_values( + notify_api, + mocker, + sample_letter_template, + notify_db_session +): + sent_letter_1 = create_notification(sample_letter_template, reference='ref-foo', status=NOTIFICATION_SENDING) + sent_letter_2 = create_notification(sample_letter_template, reference='ref-bar', status=NOTIFICATION_SENDING) + valid_file = '{}|Sent|1|Unsorted\n{}|Sent|2|Unsorted'.format( + sent_letter_1.reference, sent_letter_2.reference) + mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=valid_file) + + update_letter_notifications_statuses(filename='NOTIFY.20170823160812.RSP.TXT') + + daily_sorted_letter = dao_get_daily_sorted_letter_by_billing_day(date(2017, 8, 23)) + + assert daily_sorted_letter.unsorted_count == 2 + assert daily_sorted_letter.sorted_count == 0 def test_update_letter_notifications_does_not_call_send_callback_if_no_db_entry(notify_api, mocker, @@ -159,7 +223,7 @@ def test_update_letter_notifications_does_not_call_send_callback_if_no_db_entry( 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' ) - update_letter_notifications_statuses(filename='foo.txt') + update_letter_notifications_statuses(filename='NOTIFY.20170823160812.RSP.TXT') send_mock.assert_not_called() @@ -230,3 +294,24 @@ def test_check_billable_units_when_billable_units_does_not_match_page_count( mock_logger.assert_called_once_with( 'Notification with id {} had 3 billable_units but a page count of 1'.format(notification.id) ) + + +@pytest.mark.parametrize('filename_date, billing_date', [ + ('20170820230000', date(2017, 8, 21)), + ('20170120230000', date(2017, 1, 20)) +]) +def test_get_billing_date_in_bst_from_filename(filename_date, billing_date): + filename = 'NOTIFY.{}.RSP.TXT'.format(filename_date) + result = get_billing_date_in_bst_from_filename(filename) + + assert result == billing_date + + +@freeze_time("2018-01-11 09:00:00") +def test_persist_daily_sorted_letter_counts_saves_sorted_and_unsorted_values(client, notify_db_session): + letter_counts = defaultdict(int, **{'Unsorted': 5, 'Sorted': 1}) + persist_daily_sorted_letter_counts(date.today(), letter_counts) + day = dao_get_daily_sorted_letter_by_billing_day(date.today()) + + assert day.unsorted_count == 5 + assert day.sorted_count == 1 From ee9b6f1fe05ece770301a5402229afe8dead7e7e Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 2 Mar 2018 13:43:27 +0000 Subject: [PATCH 16/42] Return hidden field as part of json for notification by id - required by Admin app so that it can distinguish between created and precompiled letters --- app/schemas.py | 2 +- tests/app/db.py | 14 ++++++------- tests/app/service/test_rest.py | 38 ++++++++++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 8 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index 5592026b1..b36de4871 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -454,7 +454,7 @@ class NotificationWithTemplateSchema(BaseSchema): template = fields.Nested( TemplateSchema, - only=['id', 'version', 'name', 'template_type', 'content', 'subject', 'redact_personalisation'], + only=['id', 'version', 'name', 'template_type', 'content', 'subject', 'redact_personalisation', 'hidden'], dump_only=True ) job = fields.Nested(JobSchema, only=["id", "original_file_name"], dump_only=True) diff --git a/tests/app/db.py b/tests/app/db.py index 6a09413a8..20e1c7997 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -125,13 +125,13 @@ def create_service_with_defined_sms_sender( def create_template( - service, - template_type=SMS_TYPE, - template_name=None, - subject='Template subject', - content='Dear Sir/Madam, Hello. Yours Truly, The Government.', - reply_to=None, - hidden=False + service, + template_type=SMS_TYPE, + template_name=None, + subject='Template subject', + content='Dear Sir/Madam, Hello. Yours Truly, The Government.', + reply_to=None, + hidden=False ): data = { 'name': template_name or '{} Template Name'.format(template_type), diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 7998b6b91..92a74c233 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -2139,6 +2139,17 @@ def test_get_notification_for_service_includes_template_redacted(admin_request, assert resp['template']['redact_personalisation'] is False +def test_get_notification_for_service_includes_template_hidden(admin_request, sample_notification): + resp = admin_request.get( + 'service.get_notification_for_service', + service_id=sample_notification.service_id, + notification_id=sample_notification.id + ) + + assert resp['id'] == str(sample_notification.id) + assert resp['template']['hidden'] is False + + def test_get_all_notifications_for_service_includes_template_redacted(admin_request, sample_service): normal_template = create_template(sample_service) @@ -2162,6 +2173,33 @@ def test_get_all_notifications_for_service_includes_template_redacted(admin_requ assert resp['notifications'][1]['template']['redact_personalisation'] is True +def test_get_all_notifications_for_service_includes_template_hidden(admin_request, sample_service): + letter_template = create_template(sample_service, template_type=LETTER_TYPE) + precompiled_template = create_template( + sample_service, + template_type=LETTER_TYPE, + template_name='Pre-compiled PDF', + subject='Pre-compiled PDF', + hidden=True + ) + + with freeze_time('2000-01-01'): + letter_noti = create_notification(letter_template) + with freeze_time('2000-01-02'): + precompiled_noti = create_notification(precompiled_template) + + resp = admin_request.get( + 'service.get_all_notifications_for_service', + service_id=sample_service.id + ) + + assert resp['notifications'][0]['id'] == str(precompiled_noti.id) + assert resp['notifications'][0]['template']['hidden'] is True + + assert resp['notifications'][1]['id'] == str(letter_noti.id) + assert resp['notifications'][1]['template']['hidden'] is False + + def test_search_for_notification_by_to_field_returns_personlisation( client, notify_db, From 564504bf97a785589f1ffe7ba2551f890f76f68c Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 5 Mar 2018 13:38:41 +0000 Subject: [PATCH 17/42] Add template hidden field in response --- app/dao/notifications_dao.py | 1 + app/dao/services_dao.py | 4 ++++ app/dao/stats_template_usage_by_month_dao.py | 1 + app/dao/templates_dao.py | 1 + app/service/rest.py | 3 ++- app/template_statistics/rest.py | 3 ++- tests/app/service/test_rest.py | 5 ++++- 7 files changed, 15 insertions(+), 3 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 155616801..efd13426e 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -83,6 +83,7 @@ def dao_get_template_usage(service_id, limit_days=None): Template.id.label('template_id'), Template.name, Template.template_type, + Template.hidden, notifications_aggregate_query.c.count ).join( notifications_aggregate_query, diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 57d5aa866..6ad83d214 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -522,6 +522,7 @@ def dao_fetch_monthly_historical_usage_by_template_for_service(service_id, year) stat.month = result.month stat.year = result.year stat.count = result.count + stat.hidden = result.hidden stats.append(stat) month = get_london_month_from_utc_column(Notification.created_at) @@ -533,6 +534,7 @@ def dao_fetch_monthly_historical_usage_by_template_for_service(service_id, year) if fy_start < datetime.now() < fy_end: today_results = db.session.query( Notification.template_id, + Template.hidden, Template.name, Template.template_type, extract('month', month).label('month'), @@ -547,6 +549,7 @@ def dao_fetch_monthly_historical_usage_by_template_for_service(service_id, year) Notification.key_type != KEY_TYPE_TEST ).group_by( Notification.template_id, + Template.hidden, Template.name, Template.template_type, month, @@ -571,6 +574,7 @@ def dao_fetch_monthly_historical_usage_by_template_for_service(service_id, year) new_stat.month = int(today_result.month) new_stat.year = int(today_result.year) new_stat.count = today_result.count + new_stat.hidden = today_result.hidden stats.append(new_stat) return stats diff --git a/app/dao/stats_template_usage_by_month_dao.py b/app/dao/stats_template_usage_by_month_dao.py index 29aba1cdc..49e95cfe1 100644 --- a/app/dao/stats_template_usage_by_month_dao.py +++ b/app/dao/stats_template_usage_by_month_dao.py @@ -35,6 +35,7 @@ def insert_or_update_stats_for_template(template_id, month, year, count): def dao_get_template_usage_stats_by_service(service_id, year): return db.session.query( StatsTemplateUsageByMonth.template_id, + Template.hidden, Template.name, Template.template_type, StatsTemplateUsageByMonth.month, diff --git a/app/dao/templates_dao.py b/app/dao/templates_dao.py index fb7dc7602..c664c4346 100644 --- a/app/dao/templates_dao.py +++ b/app/dao/templates_dao.py @@ -135,6 +135,7 @@ def dao_get_templates_for_cache(cache): query = db.session.query(Template.id.label('template_id'), Template.template_type, Template.name, + Template.hidden, cache_subq.c.count.label('count') ).join(cache_subq, Template.id == cache_subq.c.template_id diff --git a/app/service/rest.py b/app/service/rest.py index 99f03cee9..32305e301 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -536,7 +536,8 @@ def get_monthly_template_usage(service_id): 'type': i.template_type, 'month': i.month, 'year': i.year, - 'count': i.count + 'count': i.count, + 'hidden': i.hidden } ) diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index 86548dc00..f71a40492 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -47,7 +47,8 @@ def get_template_statistics_for_service_by_day(service_id): 'count': data.count, 'template_id': str(data.template_id), 'template_name': data.name, - 'template_type': data.template_type + 'template_type': data.template_type, + 'template_hidden': data.hidden } return jsonify(data=[serialize(row) for row in stats]) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 92a74c233..86f7641de 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1831,7 +1831,7 @@ def test_get_template_usage_by_month_returns_two_templates( sample_service ): - template_one = create_template(sample_service) + template_one = create_template(sample_service, hidden=True) # add a historical notification for template not1 = create_notification_history( @@ -1889,6 +1889,7 @@ def test_get_template_usage_by_month_returns_two_templates( assert resp_json[0]["month"] == 4 assert resp_json[0]["year"] == 2017 assert resp_json[0]["count"] == 1 + assert resp_json[0]["hidden"] is True assert resp_json[1]["template_id"] == str(sample_template.id) assert resp_json[1]["name"] == sample_template.name @@ -1896,6 +1897,7 @@ def test_get_template_usage_by_month_returns_two_templates( assert resp_json[1]["month"] == 4 assert resp_json[1]["year"] == 2017 assert resp_json[1]["count"] == 3 + assert resp_json[1]["hidden"] is False assert resp_json[2]["template_id"] == str(sample_template.id) assert resp_json[2]["name"] == sample_template.name @@ -1903,6 +1905,7 @@ def test_get_template_usage_by_month_returns_two_templates( assert resp_json[2]["month"] == 11 assert resp_json[2]["year"] == 2017 assert resp_json[2]["count"] == 1 + assert resp_json[2]["hidden"] is False def test_search_for_notification_by_to_field(client, notify_db, notify_db_session): From 5dd3c62b5c2bf1ff034acf7f9b991a67888aa2aa Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 5 Mar 2018 16:56:14 +0000 Subject: [PATCH 18/42] Fix template caching tests --- tests/app/dao/test_templates_dao.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index 8891d2e26..e9daa86ae 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -489,7 +489,8 @@ def test_get_templates_by_ids_successful(notify_db, notify_db_session): notify_db_session, template_name='Sample Template 2', template_type="sms", - content="Template content" + content="Template content", + hidden=True ) create_sample_template( notify_db, @@ -503,8 +504,8 @@ def test_get_templates_by_ids_successful(notify_db, notify_db_session): cache = [[k, v] for k, v in sample_cache_dict.items()] templates = dao_get_templates_for_cache(cache) assert len(templates) == 2 - assert [(template_1.id, template_1.template_type, template_1.name, 2), - (template_2.id, template_2.template_type, template_2.name, 3)] == templates + assert [(template_1.id, template_1.template_type, template_1.name, False, 2), + (template_2.id, template_2.template_type, template_2.name, True, 3)] == templates def test_get_templates_by_ids_successful_for_one_cache_item(notify_db, notify_db_session): @@ -519,7 +520,7 @@ def test_get_templates_by_ids_successful_for_one_cache_item(notify_db, notify_db cache = [[k, v] for k, v in sample_cache_dict.items()] templates = dao_get_templates_for_cache(cache) assert len(templates) == 1 - assert [(template_1.id, template_1.template_type, template_1.name, 2)] == templates + assert [(template_1.id, template_1.template_type, template_1.name, False, 2)] == templates def test_get_templates_by_ids_returns_empty_list(): From b5a2dbb24e8c18d1aad641e4c3469ae90d706f4c Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 5 Mar 2018 18:47:45 +0000 Subject: [PATCH 19/42] Update schema to dump precompiled_letter - to be a precompiled letter, the template must be a letter, be hidden, and have a matching template name to the one expected in config['PRECOMPILED_TEMPLATE_NAME'] --- app/schemas.py | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/app/schemas.py b/app/schemas.py index b36de4871..9b4ae055c 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -2,6 +2,7 @@ from datetime import ( datetime, date, timedelta) +from flask import current_app from flask_marshmallow.fields import fields from marshmallow import ( post_load, @@ -305,6 +306,7 @@ class NotificationModelSchema(BaseSchema): class BaseTemplateSchema(BaseSchema): reply_to = fields.Method("get_reply_to", allow_none=True) reply_to_text = fields.Method("get_reply_to_text", allow_none=True) + precompiled_letter = fields.Method("get_precompiled_letter") def get_reply_to(self, template): return template.reply_to @@ -312,6 +314,13 @@ class BaseTemplateSchema(BaseSchema): def get_reply_to_text(self, template): return template.get_reply_to_text() + def get_precompiled_letter(self, template): + return ( + template.template_type == 'letter' and + template.hidden and + template.name == current_app.config['PRECOMPILED_TEMPLATE_NAME'] + ) + class Meta: model = models.Template exclude = ("service_id", "jobs", "service_letter_contact_id") @@ -454,7 +463,16 @@ class NotificationWithTemplateSchema(BaseSchema): template = fields.Nested( TemplateSchema, - only=['id', 'version', 'name', 'template_type', 'content', 'subject', 'redact_personalisation', 'hidden'], + only=[ + 'id', + 'version', + 'name', + 'template_type', + 'content', + 'subject', + 'redact_personalisation', + 'precompiled_letter' + ], dump_only=True ) job = fields.Nested(JobSchema, only=["id", "original_file_name"], dump_only=True) From bca858f4a87473b3d44eb134e4a1f86e2ecf912c Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 5 Mar 2018 18:51:04 +0000 Subject: [PATCH 20/42] Return precompiled_letter flag rather than hidden --- app/service/rest.py | 6 +++++- app/template_statistics/rest.py | 6 +++++- tests/app/service/test_rest.py | 21 +++++++++++++-------- 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/app/service/rest.py b/app/service/rest.py index 32305e301..33c647131 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -537,7 +537,11 @@ def get_monthly_template_usage(service_id): 'month': i.month, 'year': i.year, 'count': i.count, - 'hidden': i.hidden + 'precompiled_letter': ( + i.template_type == 'letter' and + i.hidden and + i.name == current_app.config['PRECOMPILED_TEMPLATE_NAME'] + ) } ) diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index f71a40492..6dae8bec5 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -48,7 +48,11 @@ def get_template_statistics_for_service_by_day(service_id): 'template_id': str(data.template_id), 'template_name': data.name, 'template_type': data.template_type, - 'template_hidden': data.hidden + 'precompiled_letter': ( + data.template_type == 'letter' and + data.hidden and + data.name == current_app.config['PRECOMPILED_TEMPLATE_NAME'] + ) } return jsonify(data=[serialize(row) for row in stats]) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 86f7641de..b600b239b 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1831,7 +1831,12 @@ def test_get_template_usage_by_month_returns_two_templates( sample_service ): - template_one = create_template(sample_service, hidden=True) + template_one = create_template( + sample_service, + template_type=LETTER_TYPE, + template_name=current_app.config['PRECOMPILED_TEMPLATE_NAME'], + hidden=True + ) # add a historical notification for template not1 = create_notification_history( @@ -1889,7 +1894,7 @@ def test_get_template_usage_by_month_returns_two_templates( assert resp_json[0]["month"] == 4 assert resp_json[0]["year"] == 2017 assert resp_json[0]["count"] == 1 - assert resp_json[0]["hidden"] is True + assert resp_json[0]["precompiled_letter"] is True assert resp_json[1]["template_id"] == str(sample_template.id) assert resp_json[1]["name"] == sample_template.name @@ -1897,7 +1902,7 @@ def test_get_template_usage_by_month_returns_two_templates( assert resp_json[1]["month"] == 4 assert resp_json[1]["year"] == 2017 assert resp_json[1]["count"] == 3 - assert resp_json[1]["hidden"] is False + assert resp_json[1]["precompiled_letter"] is False assert resp_json[2]["template_id"] == str(sample_template.id) assert resp_json[2]["name"] == sample_template.name @@ -1905,7 +1910,7 @@ def test_get_template_usage_by_month_returns_two_templates( assert resp_json[2]["month"] == 11 assert resp_json[2]["year"] == 2017 assert resp_json[2]["count"] == 1 - assert resp_json[2]["hidden"] is False + assert resp_json[2]["precompiled_letter"] is False def test_search_for_notification_by_to_field(client, notify_db, notify_db_session): @@ -2142,7 +2147,7 @@ def test_get_notification_for_service_includes_template_redacted(admin_request, assert resp['template']['redact_personalisation'] is False -def test_get_notification_for_service_includes_template_hidden(admin_request, sample_notification): +def test_get_notification_for_service_includes_precompiled_letter(admin_request, sample_notification): resp = admin_request.get( 'service.get_notification_for_service', service_id=sample_notification.service_id, @@ -2150,7 +2155,7 @@ def test_get_notification_for_service_includes_template_hidden(admin_request, sa ) assert resp['id'] == str(sample_notification.id) - assert resp['template']['hidden'] is False + assert resp['template']['precompiled_letter'] is False def test_get_all_notifications_for_service_includes_template_redacted(admin_request, sample_service): @@ -2197,10 +2202,10 @@ def test_get_all_notifications_for_service_includes_template_hidden(admin_reques ) assert resp['notifications'][0]['id'] == str(precompiled_noti.id) - assert resp['notifications'][0]['template']['hidden'] is True + assert resp['notifications'][0]['template']['precompiled_letter'] is True assert resp['notifications'][1]['id'] == str(letter_noti.id) - assert resp['notifications'][1]['template']['hidden'] is False + assert resp['notifications'][1]['template']['precompiled_letter'] is False def test_search_for_notification_by_to_field_returns_personlisation( From 28136734e4cb9a58d5587a9b8df844cadf96be62 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 6 Mar 2018 13:04:57 +0000 Subject: [PATCH 21/42] Refactor to use is_precompiled_letter in letters/utils.py --- app/letters/utils.py | 7 ++++++- app/schemas.py | 8 ++------ app/template_statistics/rest.py | 7 ++----- 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/app/letters/utils.py b/app/letters/utils.py index 5e7730882..a5f67c11b 100644 --- a/app/letters/utils.py +++ b/app/letters/utils.py @@ -5,6 +5,7 @@ from flask import current_app from notifications_utils.s3 import s3upload +from app.models import LETTER_TYPE from app.variables import Retention @@ -79,4 +80,8 @@ def get_letter_pdf(notification): def is_precompiled_letter(template): - return template.hidden and template.name == current_app.config['PRECOMPILED_TEMPLATE_NAME'] + return ( + template.template_type == LETTER_TYPE and + template.hidden and + template.name == current_app.config['PRECOMPILED_TEMPLATE_NAME'] + ) diff --git a/app/schemas.py b/app/schemas.py index 9b4ae055c..f2282ac05 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -2,7 +2,6 @@ from datetime import ( datetime, date, timedelta) -from flask import current_app from flask_marshmallow.fields import fields from marshmallow import ( post_load, @@ -25,6 +24,7 @@ from notifications_utils.recipients import ( from app import ma from app import models +from app.letters.utils import is_precompiled_letter from app.models import ServicePermission from app.dao.permissions_dao import permission_dao from app.utils import get_template_instance @@ -315,11 +315,7 @@ class BaseTemplateSchema(BaseSchema): return template.get_reply_to_text() def get_precompiled_letter(self, template): - return ( - template.template_type == 'letter' and - template.hidden and - template.name == current_app.config['PRECOMPILED_TEMPLATE_NAME'] - ) + return is_precompiled_letter(template) class Meta: model = models.Template diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index 6dae8bec5..f00741a7b 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -14,6 +14,7 @@ from app.dao.templates_dao import ( dao_get_template_by_id_and_service_id ) +from app.letters.utils import is_precompiled_letter from app.schemas import notification_with_template_schema from app.utils import cache_key_for_service_template_counter from app.errors import register_errors, InvalidRequest @@ -48,11 +49,7 @@ def get_template_statistics_for_service_by_day(service_id): 'template_id': str(data.template_id), 'template_name': data.name, 'template_type': data.template_type, - 'precompiled_letter': ( - data.template_type == 'letter' and - data.hidden and - data.name == current_app.config['PRECOMPILED_TEMPLATE_NAME'] - ) + 'precompiled_letter': is_precompiled_letter(data) } return jsonify(data=[serialize(row) for row in stats]) From d0df85a6020d55d86bc5e6bec1c161ef9c97ee06 Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Tue, 6 Mar 2018 14:24:30 +0000 Subject: [PATCH 22/42] Fixed bug where the content header was not being passed onto the post request. Changed data => json. Added extra logging to display the error with more detail --- app/template/rest.py | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/app/template/rest.py b/app/template/rest.py index e49eea762..22f3295c9 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -205,8 +205,9 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file pdf_file = get_letter_pdf(notification) - except botocore.exceptions.ClientError: - current_app.logger.info + except botocore.exceptions.ClientError as e: + current_app.logger.exception( + 'Error getting letter file from S3 notification id {}'.format(notification_id), e) raise InvalidRequest('Error getting letter file from S3 notification id {}'.format(notification_id), status_code=500) @@ -253,13 +254,23 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file def _get_png_preview(url, data, notification_id): resp = requests_post( url, - data=data, + json=data, headers={'Authorization': 'Token {}'.format(current_app.config['TEMPLATE_PREVIEW_API_KEY'])} ) if resp.status_code != 200: + current_app.logger.exception( + 'Error generating preview letter for {} \nStatus code: {}\n{}'.format( + notification_id, + resp.status_code, + resp.content + )) raise InvalidRequest( - 'Error generating preview for {}'.format(notification_id), status_code=500 + 'Error generating preview letter for {} \nStatus code: {}\n{}'.format( + notification_id, + resp.status_code, + resp.content + ), status_code=500 ) return base64.b64encode(resp.content).decode('utf-8') From ed9936bba03dd1a3014644489e9e10285870c7f6 Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Tue, 6 Mar 2018 14:42:53 +0000 Subject: [PATCH 23/42] Fixed bug where the content header was not being passed onto the post request. Changed data => json. Added extra logging to display the error with more detail --- app/template/rest.py | 2 +- tests/app/template/test_rest.py | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/template/rest.py b/app/template/rest.py index 22f3295c9..c938b2ea6 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -266,7 +266,7 @@ def _get_png_preview(url, data, notification_id): resp.content )) raise InvalidRequest( - 'Error generating preview letter for {} \nStatus code: {}\n{}'.format( + 'Error generating preview letter for {}\nStatus code: {}\n{}'.format( notification_id, resp.status_code, resp.content diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 14e0aba55..99889e238 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -874,7 +874,9 @@ def test_preview_letter_template_by_id_template_preview_500( _expected_status=500 ) - assert resp['message'] == 'Error generating preview for {}'.format(sample_letter_notification.id) + assert 'Status code: 404' in resp['message'] + assert 'Error generating preview letter for {}'.format(sample_letter_notification.id) in resp['message'] + def test_preview_letter_template_precompiled_pdf_file_type( From a487293bf04e1706e7bfaebd077e7dd0950ea44c Mon Sep 17 00:00:00 2001 From: chrisw Date: Tue, 6 Mar 2018 12:49:46 +0000 Subject: [PATCH 24/42] Add check org name is unique endpoint --- app/organisation/rest.py | 23 +++++++ tests/app/organisation/test_rest.py | 99 +++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+) diff --git a/app/organisation/rest.py b/app/organisation/rest.py index 3bf3972b4..04100ce2a 100644 --- a/app/organisation/rest.py +++ b/app/organisation/rest.py @@ -107,3 +107,26 @@ def get_organisation_users(organisation_id): result = user_schema.dump(org_users, many=True) return jsonify(data=result.data) + + +@organisation_blueprint.route('/unique', methods=["GET"]) +def is_organisation_name_unique(): + organisation_id, name = check_request_args(request) + + name_exists = Organisation.query.filter(Organisation.name.ilike(name)).first() + + result = (not name_exists) or str(name_exists.id) == organisation_id + return jsonify(result=result), 200 + + +def check_request_args(request): + org_id = request.args.get('org_id') + name = request.args.get('name', None) + errors = [] + if not org_id: + errors.append({'org_id': ["Can't be empty"]}) + if not name: + errors.append({'name': ["Can't be empty"]}) + if errors: + raise InvalidRequest(errors, status_code=400) + return org_id, name diff --git a/tests/app/organisation/test_rest.py b/tests/app/organisation/test_rest.py index 55db57330..dee403245 100644 --- a/tests/app/organisation/test_rest.py +++ b/tests/app/organisation/test_rest.py @@ -1,5 +1,7 @@ import uuid +import pytest + from app.models import Organisation from app.dao.organisation_dao import dao_add_service_to_organisation, dao_add_user_to_organisation from tests.app.db import create_organisation, create_service, create_user @@ -310,3 +312,100 @@ def test_get_organisation_users_returns_users_for_organisation(admin_request, sa assert len(response['data']) == 2 assert response['data'][0]['id'] == str(first.id) + + +def test_is_organisation_name_unique_returns_200_if_unique(admin_request, notify_db, notify_db_session): + organisation = create_organisation(name='unique') + + response = admin_request.get( + 'organisation.is_organisation_name_unique', + _expected_status=200, + org_id=organisation.id, + name='something' + ) + + assert response == {"result": True} + + +@pytest.mark.parametrize('name', ["UNIQUE", "Unique.", "**uniQUE**"]) +def test_is_organisation_name_unique_returns_200_and_name_capitalized_or_punctuation_added( + admin_request, + notify_db, + notify_db_session, + name +): + organisation = create_organisation(name='unique') + + response = admin_request.get( + 'organisation.is_organisation_name_unique', + _expected_status=200, + org_id=organisation.id, + name=name + ) + + assert response == {"result": True} + + +@pytest.mark.parametrize('name', ["UNIQUE", "Unique"]) +def test_is_organisation_name_unique_returns_200_and_false_with_same_name_and_different_case_of_other_organisation( + admin_request, + notify_db, + notify_db_session, + name +): + create_organisation(name='unique') + different_organisation_id = '111aa111-2222-bbbb-aaaa-111111111111' + + response = admin_request.get( + 'organisation.is_organisation_name_unique', + _expected_status=200, + org_id=different_organisation_id, + name=name + ) + + assert response == {"result": False} + + +def test_is_organisation_name_unique_returns_200_and_false_if_name_exists_for_a_different_organisation( + admin_request, + notify_db, + notify_db_session +): + create_organisation(name='existing name') + different_organisation_id = '111aa111-2222-bbbb-aaaa-111111111111' + + response = admin_request.get( + 'organisation.is_organisation_name_unique', + _expected_status=200, + org_id=different_organisation_id, + name='existing name' + ) + + assert response == {"result": False} + + +def test_is_organisation_name_unique_returns_200_and_true_if_name_exists_for_the_same_organisation( + admin_request, + notify_db, + notify_db_session +): + organisation = create_organisation(name='unique') + + response = admin_request.get( + 'organisation.is_organisation_name_unique', + _expected_status=200, + org_id=organisation.id, + name='unique' + ) + + assert response == {"result": True} + + +def test_is_organisation_name_unique_returns_400_when_name_does_not_exist(admin_request): + response = admin_request.get( + 'organisation.is_organisation_name_unique', + _expected_status=400 + ) + + assert response["message"][0]["org_id"] == ["Can't be empty"] + assert response["message"][1]["name"] == ["Can't be empty"] From 77a3397ce505ffd13258a69e71934bc4c2edef22 Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Tue, 6 Mar 2018 15:35:00 +0000 Subject: [PATCH 25/42] Added option flag to the _get_png_preview method to determine if the post method content is data or json format. --- app/template/rest.py | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/app/template/rest.py b/app/template/rest.py index c938b2ea6..024a2c708 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -8,7 +8,6 @@ from flask import ( request) from requests import post as requests_post - from app.dao.notifications_dao import get_notification_by_id from app.dao.templates_dao import ( dao_update_template, @@ -33,7 +32,6 @@ from app.utils import get_template_instance, get_public_notify_type_text template_blueprint = Blueprint('template', __name__, url_prefix='/service//template') - register_errors(template_blueprint) @@ -189,7 +187,6 @@ def redact_template(template, data): @template_blueprint.route('/preview//', methods=['GET']) def preview_letter_template_by_notification_id(service_id, notification_id, file_type): - if file_type not in ('pdf', 'png'): raise InvalidRequest({'content': ["file_type must be pdf or png"]}, status_code=400) @@ -214,13 +211,12 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file content = base64.b64encode(pdf_file).decode('utf-8') if file_type == 'png': - url = '{}/precompiled-preview.png{}'.format( current_app.config['TEMPLATE_PREVIEW_API_HOST'], '?page={}'.format(page) if page else '' ) - content = _get_png_preview(url, content, notification.id) + content = _get_png_preview(url, content, notification.id, json=False) else: @@ -246,17 +242,24 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file '?page={}'.format(page) if page else '' ) - content = _get_png_preview(url, data, notification.id) + content = _get_png_preview(url, data, notification.id, json=True) return jsonify({"content": content}) -def _get_png_preview(url, data, notification_id): - resp = requests_post( - url, - json=data, - headers={'Authorization': 'Token {}'.format(current_app.config['TEMPLATE_PREVIEW_API_KEY'])} - ) +def _get_png_preview(url, data, notification_id, json=True): + if json: + resp = requests_post( + url, + json=data, + headers={'Authorization': 'Token {}'.format(current_app.config['TEMPLATE_PREVIEW_API_KEY'])} + ) + else: + resp = requests_post( + url, + data=data, + headers={'Authorization': 'Token {}'.format(current_app.config['TEMPLATE_PREVIEW_API_KEY'])} + ) if resp.status_code != 200: current_app.logger.exception( From 5db3eef8f2fd953c56f9201f73f561b3cfc0d025 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 6 Mar 2018 14:42:50 +0000 Subject: [PATCH 26/42] Add warning to Performance Platform command --- app/commands.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/commands.py b/app/commands.py index d8775f217..8c807f1e0 100644 --- a/app/commands.py +++ b/app/commands.py @@ -216,6 +216,9 @@ def populate_monthly_billing(year): def backfill_performance_platform_totals(start_date, end_date): """ Send historical total messages sent to Performance Platform. + + WARNING: This does not overwrite existing data. You need to delete + the existing data or Performance Platform will double-count. """ delta = end_date - start_date From e1e7f13f2365298ba2dbe675fdcdd335a985c665 Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Tue, 6 Mar 2018 16:16:29 +0000 Subject: [PATCH 27/42] Added tests to ensure the parameter to the mock post request are the correct type for the call as there are some that require json and some binary. The additional checks ensure that that json decode either fails or succeeds in the correct case. --- tests/app/template/test_rest.py | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 99889e238..6dac7e084 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -3,6 +3,7 @@ import json import random import string from datetime import datetime, timedelta +from json import JSONDecodeError import botocore import pytest @@ -828,7 +829,7 @@ def test_preview_letter_template_by_id_valid_file_type( with requests_mock.Mocker() as request_mock: content = b'\x00\x01' - request_mock.post( + mock_post = request_mock.post( 'http://localhost/notifications-template-preview/preview.pdf', content=content, headers={'X-pdf-page-count': '1'}, @@ -842,6 +843,7 @@ def test_preview_letter_template_by_id_valid_file_type( file_type='pdf' ) + assert mock_post.last_request.json() assert base64.b64decode(resp['content']) == content @@ -859,7 +861,7 @@ def test_preview_letter_template_by_id_template_preview_500( with requests_mock.Mocker() as request_mock: content = b'\x00\x01' - request_mock.post( + mock_post = request_mock.post( 'http://localhost/notifications-template-preview/preview.pdf', content=content, headers={'X-pdf-page-count': '1'}, @@ -874,11 +876,11 @@ def test_preview_letter_template_by_id_template_preview_500( _expected_status=500 ) + assert mock_post.last_request.json() assert 'Status code: 404' in resp['message'] assert 'Error generating preview letter for {}'.format(sample_letter_notification.id) in resp['message'] - def test_preview_letter_template_precompiled_pdf_file_type( notify_api, client, @@ -980,7 +982,7 @@ def test_preview_letter_template_precompiled_png_file_type( mock_get_letter_pdf = mocker.patch('app.template.rest.get_letter_pdf', return_value=pdf_content) - request_mock.post( + mock_post = request_mock.post( 'http://localhost/notifications-template-preview/precompiled-preview.png', content=png_content, headers={'X-pdf-page-count': '1'}, @@ -994,6 +996,8 @@ def test_preview_letter_template_precompiled_png_file_type( file_type='png' ) + with pytest.raises(JSONDecodeError): + mock_post.last_request.json() assert mock_get_letter_pdf.called_once_with(notification) assert base64.b64decode(resp['content']) == png_content @@ -1025,7 +1029,7 @@ def test_preview_letter_template_precompiled_png_template_preview_500_error( mocker.patch('app.template.rest.get_letter_pdf', return_value=pdf_content) - request_mock.post( + mock_post = request_mock.post( 'http://localhost/notifications-template-preview/precompiled-preview.png', content=png_content, headers={'X-pdf-page-count': '1'}, @@ -1041,6 +1045,9 @@ def test_preview_letter_template_precompiled_png_template_preview_500_error( ) + with pytest.raises(JSONDecodeError): + mock_post.last_request.json() + def test_preview_letter_template_precompiled_png_template_preview_400_error( notify_api, @@ -1069,7 +1076,7 @@ def test_preview_letter_template_precompiled_png_template_preview_400_error( mocker.patch('app.template.rest.get_letter_pdf', return_value=pdf_content) - request_mock.post( + mock_post = request_mock.post( 'http://localhost/notifications-template-preview/precompiled-preview.png', content=png_content, headers={'X-pdf-page-count': '1'}, @@ -1082,5 +1089,7 @@ def test_preview_letter_template_precompiled_png_template_preview_400_error( notification_id=notification.id, file_type='png', _expected_status=500 - ) + + with pytest.raises(JSONDecodeError): + mock_post.last_request.json() From 3f9a37d8a3d22ac54df577b33546d82244e19df7 Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Tue, 6 Mar 2018 17:31:08 +0000 Subject: [PATCH 28/42] Updated the import for JSONDecodeError to import from from json.decoder as it was failing on Jenkins. --- tests/app/template/test_rest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 6dac7e084..099a887c7 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -3,7 +3,7 @@ import json import random import string from datetime import datetime, timedelta -from json import JSONDecodeError +from json.decoder import JSONDecodeError import botocore import pytest From cf2a506b3bb6c768bbebc9e61d7aa61123d1085d Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Tue, 6 Mar 2018 18:20:45 +0000 Subject: [PATCH 29/42] Removed the import and directly used json.decoder.JSONDecodeError as it was failing on Jenkins. --- tests/app/template/test_rest.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 099a887c7..c84a86525 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -3,7 +3,6 @@ import json import random import string from datetime import datetime, timedelta -from json.decoder import JSONDecodeError import botocore import pytest @@ -996,7 +995,7 @@ def test_preview_letter_template_precompiled_png_file_type( file_type='png' ) - with pytest.raises(JSONDecodeError): + with pytest.raises(json.decoder.JSONDecodeError): mock_post.last_request.json() assert mock_get_letter_pdf.called_once_with(notification) assert base64.b64decode(resp['content']) == png_content @@ -1045,7 +1044,7 @@ def test_preview_letter_template_precompiled_png_template_preview_500_error( ) - with pytest.raises(JSONDecodeError): + with pytest.raises(json.decoder.JSONDecodeError): mock_post.last_request.json() @@ -1091,5 +1090,5 @@ def test_preview_letter_template_precompiled_png_template_preview_400_error( _expected_status=500 ) - with pytest.raises(JSONDecodeError): + with pytest.raises(json.decoder.JSONDecodeError): mock_post.last_request.json() From e2a308a7031466c2d6f04f91706d75abaf29c911 Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Tue, 6 Mar 2018 19:17:29 +0000 Subject: [PATCH 30/42] Update sqlalchemy from 1.2.4 to 1.2.5 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 7868b54fa..b7adae9b4 100644 --- a/requirements.txt +++ b/requirements.txt @@ -16,7 +16,7 @@ marshmallow==2.15.0 monotonic==1.4 psycopg2-binary==2.7.4 PyJWT==1.5.3 -SQLAlchemy==1.2.4 +SQLAlchemy==1.2.5 notifications-python-client==4.7.2 From 7f2e9f507e5fc7ba8c1c63f6896fc22ac59fa5d1 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Thu, 1 Mar 2018 16:52:52 +0000 Subject: [PATCH 31/42] Delete functions which call the job statistics tasks The JobStatistics table is going to be deleted. There are currently 3 tasks which use the JobStatistics model via the Statistics DAO, so we need to make sure that these tasks aren't being used before they are deleted in a separate PR. This commit deletes: * The `create_initial_notification_statistic_tasks` function which gets used to call the `record_initial_job_statistics` task. * The `create_outcome_notification_statistic_tasks` function which gets used to call the `record_outcome_job_statistics` task. * And the scheduling of the `timeout-job-statistics` scheduled task. --- app/celery/statistics_tasks.py | 11 -- app/config.py | 5 - app/delivery/send_to_providers.py | 5 - .../notifications_ses_callback.py | 2 - app/notifications/process_client_response.py | 2 - tests/app/celery/test_statistics_tasks.py | 169 ------------------ tests/app/delivery/test_send_to_providers.py | 42 +---- .../test_notifications_ses_callback.py | 34 +--- .../test_process_client_response.py | 10 -- 9 files changed, 4 insertions(+), 276 deletions(-) delete mode 100644 tests/app/celery/test_statistics_tasks.py diff --git a/app/celery/statistics_tasks.py b/app/celery/statistics_tasks.py index 141f829b0..df6774f71 100644 --- a/app/celery/statistics_tasks.py +++ b/app/celery/statistics_tasks.py @@ -10,20 +10,9 @@ from app.dao.statistics_dao import ( update_job_stats_outcome_count ) from app.dao.notifications_dao import get_notification_by_id -from app.models import NOTIFICATION_STATUS_TYPES_COMPLETED from app.config import QueueNames -def create_initial_notification_statistic_tasks(notification): - if notification.job_id and notification.status: - record_initial_job_statistics.apply_async((str(notification.id),), queue=QueueNames.STATISTICS) - - -def create_outcome_notification_statistic_tasks(notification): - if notification.job_id and notification.status in NOTIFICATION_STATUS_TYPES_COMPLETED: - record_outcome_job_statistics.apply_async((str(notification.id),), queue=QueueNames.STATISTICS) - - @worker_process_shutdown.connect def worker_process_shutdown(sender, signal, pid, exitcode): current_app.logger.info('Statistics worker shutdown: PID: {} Exitcode: {}'.format(pid, exitcode)) diff --git a/app/config.py b/app/config.py index 951f5ece3..d27e4fdf3 100644 --- a/app/config.py +++ b/app/config.py @@ -231,11 +231,6 @@ class Config(object): 'schedule': crontab(hour=4, minute=40), 'options': {'queue': QueueNames.PERIODIC} }, - 'timeout-job-statistics': { - 'task': 'timeout-job-statistics', - 'schedule': crontab(hour=5, minute=0), - 'options': {'queue': QueueNames.PERIODIC} - }, 'populate_monthly_billing': { 'task': 'populate_monthly_billing', 'schedule': crontab(hour=5, minute=10), diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index b272f116b..4c1a3600b 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -31,7 +31,6 @@ from app.models import ( NOTIFICATION_SENT, NOTIFICATION_SENDING ) -from app.celery.statistics_tasks import create_initial_notification_statistic_tasks def send_sms_to_provider(notification): @@ -83,8 +82,6 @@ def send_sms_to_provider(notification): notification.billable_units = template.fragment_count update_notification(notification, provider, notification.international) - create_initial_notification_statistic_tasks(notification) - current_app.logger.debug( "SMS {} sent to provider {} at {}".format(notification.id, provider.get_name(), notification.sent_at) ) @@ -138,8 +135,6 @@ def send_email_to_provider(notification): notification.reference = reference update_notification(notification, provider) - create_initial_notification_statistic_tasks(notification) - current_app.logger.debug( "Email {} sent to provider at {}".format(notification.id, notification.sent_at) ) diff --git a/app/notifications/notifications_ses_callback.py b/app/notifications/notifications_ses_callback.py index cf3da7508..99909272a 100644 --- a/app/notifications/notifications_ses_callback.py +++ b/app/notifications/notifications_ses_callback.py @@ -11,7 +11,6 @@ from app.dao import ( notifications_dao ) from app.dao.service_callback_api_dao import get_service_callback_api_for_service -from app.celery.statistics_tasks import create_outcome_notification_statistic_tasks from app.notifications.process_client_response import validate_callback_data from app.celery.service_callback_tasks import send_delivery_status_to_service from app.config import QueueNames @@ -77,7 +76,6 @@ def process_ses_response(ses_request): notification.sent_at ) - create_outcome_notification_statistic_tasks(notification) _check_and_queue_callback_task(notification.id, notification.service_id) return diff --git a/app/notifications/process_client_response.py b/app/notifications/process_client_response.py index 19aa2d290..55380b0cc 100644 --- a/app/notifications/process_client_response.py +++ b/app/notifications/process_client_response.py @@ -8,7 +8,6 @@ from app.clients import ClientException from app.dao import notifications_dao from app.clients.sms.firetext import get_firetext_responses from app.clients.sms.mmg import get_mmg_responses -from app.celery.statistics_tasks import create_outcome_notification_statistic_tasks from app.celery.service_callback_tasks import send_delivery_status_to_service from app.config import QueueNames from app.dao.service_callback_api_dao import get_service_callback_api_for_service @@ -80,7 +79,6 @@ def _process_for_status(notification_status, client_name, reference): notification.sent_at ) - create_outcome_notification_statistic_tasks(notification) # queue callback task only if the service_callback_api exists service_callback_api = get_service_callback_api_for_service(service_id=notification.service_id) diff --git a/tests/app/celery/test_statistics_tasks.py b/tests/app/celery/test_statistics_tasks.py deleted file mode 100644 index bb6c4c5b6..000000000 --- a/tests/app/celery/test_statistics_tasks.py +++ /dev/null @@ -1,169 +0,0 @@ -import pytest -from app.celery.statistics_tasks import ( - record_initial_job_statistics, - record_outcome_job_statistics, - create_initial_notification_statistic_tasks, - create_outcome_notification_statistic_tasks) -from sqlalchemy.exc import SQLAlchemyError -from app import create_uuid -from tests.app.conftest import sample_notification -from app.models import ( - NOTIFICATION_STATUS_TYPES_COMPLETED, - NOTIFICATION_SENDING, - NOTIFICATION_PENDING, - NOTIFICATION_CREATED, - NOTIFICATION_DELIVERED, -) - - -def test_should_create_initial_job_task_if_notification_is_related_to_a_job( - notify_db, notify_db_session, sample_job, mocker -): - mock = mocker.patch("app.celery.statistics_tasks.record_initial_job_statistics.apply_async") - notification = sample_notification(notify_db, notify_db_session, job=sample_job) - create_initial_notification_statistic_tasks(notification) - mock.assert_called_once_with((str(notification.id), ), queue="statistics-tasks") - - -@pytest.mark.parametrize('status', [ - NOTIFICATION_SENDING, NOTIFICATION_CREATED, NOTIFICATION_PENDING -]) -def test_should_create_intial_job_task_if_notification_is_not_in_completed_state( - notify_db, notify_db_session, sample_job, mocker, status -): - mock = mocker.patch("app.celery.statistics_tasks.record_initial_job_statistics.apply_async") - notification = sample_notification(notify_db, notify_db_session, job=sample_job, status=status) - create_initial_notification_statistic_tasks(notification) - mock.assert_called_once_with((str(notification.id), ), queue="statistics-tasks") - - -def test_should_not_create_initial_job_task_if_notification_is_not_related_to_a_job( - notify_db, notify_db_session, mocker -): - notification = sample_notification(notify_db, notify_db_session, status=NOTIFICATION_CREATED) - mock = mocker.patch("app.celery.statistics_tasks.record_initial_job_statistics.apply_async") - create_initial_notification_statistic_tasks(notification) - mock.assert_not_called() - - -def test_should_create_outcome_job_task_if_notification_is_related_to_a_job( - notify_db, notify_db_session, sample_job, mocker -): - mock = mocker.patch("app.celery.statistics_tasks.record_outcome_job_statistics.apply_async") - notification = sample_notification(notify_db, notify_db_session, job=sample_job, status=NOTIFICATION_DELIVERED) - create_outcome_notification_statistic_tasks(notification) - mock.assert_called_once_with((str(notification.id), ), queue="statistics-tasks") - - -@pytest.mark.parametrize('status', NOTIFICATION_STATUS_TYPES_COMPLETED) -def test_should_create_outcome_job_task_if_notification_is_in_completed_state( - notify_db, notify_db_session, sample_job, mocker, status -): - mock = mocker.patch("app.celery.statistics_tasks.record_outcome_job_statistics.apply_async") - notification = sample_notification(notify_db, notify_db_session, job=sample_job, status=status) - create_outcome_notification_statistic_tasks(notification) - mock.assert_called_once_with((str(notification.id), ), queue="statistics-tasks") - - -@pytest.mark.parametrize('status', [ - NOTIFICATION_SENDING, NOTIFICATION_CREATED, NOTIFICATION_PENDING -]) -def test_should_not_create_outcome_job_task_if_notification_is_not_in_completed_state_already( - notify_db, notify_db_session, sample_job, mocker, status -): - mock = mocker.patch("app.celery.statistics_tasks.record_initial_job_statistics.apply_async") - notification = sample_notification(notify_db, notify_db_session, job=sample_job, status=status) - create_outcome_notification_statistic_tasks(notification) - mock.assert_not_called() - - -def test_should_not_create_outcome_job_task_if_notification_is_not_related_to_a_job( - notify_db, notify_db_session, sample_notification, mocker -): - mock = mocker.patch("app.celery.statistics_tasks.record_outcome_job_statistics.apply_async") - create_outcome_notification_statistic_tasks(sample_notification) - mock.assert_not_called() - - -def test_should_call_create_job_stats_dao_methods(notify_db, notify_db_session, sample_notification, mocker): - dao_mock = mocker.patch("app.celery.statistics_tasks.create_or_update_job_sending_statistics") - record_initial_job_statistics(str(sample_notification.id)) - - dao_mock.assert_called_once_with(sample_notification) - - -def test_should_retry_if_persisting_the_job_stats_has_a_sql_alchemy_exception( - notify_db, - notify_db_session, - sample_notification, - mocker): - dao_mock = mocker.patch( - "app.celery.statistics_tasks.create_or_update_job_sending_statistics", - side_effect=SQLAlchemyError() - ) - retry_mock = mocker.patch('app.celery.statistics_tasks.record_initial_job_statistics.retry') - - record_initial_job_statistics(str(sample_notification.id)) - dao_mock.assert_called_once_with(sample_notification) - retry_mock.assert_called_with(queue="retry-tasks") - - -def test_should_call_update_job_stats_dao_outcome_methods(notify_db, notify_db_session, sample_notification, mocker): - dao_mock = mocker.patch("app.celery.statistics_tasks.update_job_stats_outcome_count") - record_outcome_job_statistics(str(sample_notification.id)) - - dao_mock.assert_called_once_with(sample_notification) - - -def test_should_retry_if_persisting_the_job_outcome_stats_has_a_sql_alchemy_exception( - notify_db, - notify_db_session, - sample_notification, - mocker): - dao_mock = mocker.patch( - "app.celery.statistics_tasks.update_job_stats_outcome_count", - side_effect=SQLAlchemyError() - ) - retry_mock = mocker.patch('app.celery.statistics_tasks.record_outcome_job_statistics.retry') - - record_outcome_job_statistics(str(sample_notification.id)) - dao_mock.assert_called_once_with(sample_notification) - retry_mock.assert_called_with(queue="retry-tasks") - - -def test_should_retry_if_persisting_the_job_outcome_stats_updates_zero_rows( - notify_db, - notify_db_session, - sample_notification, - mocker): - dao_mock = mocker.patch("app.celery.statistics_tasks.update_job_stats_outcome_count", return_value=0) - retry_mock = mocker.patch('app.celery.statistics_tasks.record_outcome_job_statistics.retry') - - record_outcome_job_statistics(str(sample_notification.id)) - dao_mock.assert_called_once_with(sample_notification) - retry_mock.assert_called_with(queue="retry-tasks") - - -def test_should_retry_if_persisting_the_job_stats_creation_cant_find_notification_by_id( - notify_db, - notify_db_session, - mocker): - dao_mock = mocker.patch("app.celery.statistics_tasks.create_or_update_job_sending_statistics") - retry_mock = mocker.patch('app.celery.statistics_tasks.record_initial_job_statistics.retry') - - record_initial_job_statistics(str(create_uuid())) - dao_mock.assert_not_called() - retry_mock.assert_called_with(queue="retry-tasks") - - -def test_should_retry_if_persisting_the_job_stats_outcome_cant_find_notification_by_id( - notify_db, - notify_db_session, - mocker): - - dao_mock = mocker.patch("app.celery.statistics_tasks.update_job_stats_outcome_count") - retry_mock = mocker.patch('app.celery.statistics_tasks.record_outcome_job_statistics.retry') - - record_outcome_job_statistics(str(create_uuid())) - dao_mock.assert_not_called() - retry_mock.assert_called_with(queue="retry-tasks") diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 4b973975e..627768220 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -1,7 +1,7 @@ import uuid from collections import namedtuple from datetime import datetime -from unittest.mock import ANY, call +from unittest.mock import ANY import pytest from flask import current_app @@ -75,7 +75,6 @@ def test_should_send_personalised_template_to_correct_sms_provider_and_persist( reply_to_text=sample_sms_template_with_html.service.get_default_sms_sender()) mocker.patch('app.mmg_client.send_sms') - stats_mock = mocker.patch('app.delivery.send_to_providers.create_initial_notification_statistic_tasks') send_to_providers.send_sms_to_provider( db_notification @@ -88,8 +87,6 @@ def test_should_send_personalised_template_to_correct_sms_provider_and_persist( sender=current_app.config['FROM_NUMBER'] ) - stats_mock.assert_called_once_with(db_notification) - notification = Notification.query.filter_by(id=db_notification.id).one() assert notification.status == 'sending' @@ -110,7 +107,6 @@ def test_should_send_personalised_template_to_correct_email_provider_and_persist ) mocker.patch('app.aws_ses_client.send_email', return_value='reference') - stats_mock = mocker.patch('app.delivery.send_to_providers.create_initial_notification_statistic_tasks') send_to_providers.send_email_to_provider( db_notification @@ -124,7 +120,6 @@ def test_should_send_personalised_template_to_correct_email_provider_and_persist html_body=ANY, reply_to_address=None ) - stats_mock.assert_called_once_with(db_notification) assert ' Date: Wed, 7 Mar 2018 09:32:56 +0000 Subject: [PATCH 32/42] Replaced the JSONDecodeError with a ValueError as it was failing on Jenkins. --- tests/app/template/test_rest.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index c84a86525..6611c772d 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -995,7 +995,7 @@ def test_preview_letter_template_precompiled_png_file_type( file_type='png' ) - with pytest.raises(json.decoder.JSONDecodeError): + with pytest.raises(ValueError): mock_post.last_request.json() assert mock_get_letter_pdf.called_once_with(notification) assert base64.b64decode(resp['content']) == png_content @@ -1044,7 +1044,7 @@ def test_preview_letter_template_precompiled_png_template_preview_500_error( ) - with pytest.raises(json.decoder.JSONDecodeError): + with pytest.raises(ValueError): mock_post.last_request.json() @@ -1090,5 +1090,5 @@ def test_preview_letter_template_precompiled_png_template_preview_400_error( _expected_status=500 ) - with pytest.raises(json.decoder.JSONDecodeError): + with pytest.raises(ValueError): mock_post.last_request.json() From d60e802f3568b1e384d7781379028f5a308145ba Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Wed, 7 Mar 2018 09:51:58 +0000 Subject: [PATCH 33/42] Removed the superfluous variable and pass through as it pulls it automatically out of sys.exc_info. --- app/template/rest.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/template/rest.py b/app/template/rest.py index 024a2c708..a4bc1b4a9 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -202,9 +202,9 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file pdf_file = get_letter_pdf(notification) - except botocore.exceptions.ClientError as e: + except botocore.exceptions.ClientError: current_app.logger.exception( - 'Error getting letter file from S3 notification id {}'.format(notification_id), e) + 'Error getting letter file from S3 notification id {}'.format(notification_id)) raise InvalidRequest('Error getting letter file from S3 notification id {}'.format(notification_id), status_code=500) From a84a70d7915b9e1e4a32fe441a7d37fffbd039b6 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Wed, 7 Mar 2018 15:56:41 +0000 Subject: [PATCH 34/42] Install libffi-dev and python-dev in docker This is to allow us to build cffi --- docker/Dockerfile | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docker/Dockerfile b/docker/Dockerfile index 2ccfe0e1e..bbafc0762 100644 --- a/docker/Dockerfile +++ b/docker/Dockerfile @@ -19,6 +19,8 @@ RUN \ build-essential \ zip \ libpq-dev \ + libffi-dev \ + python-dev \ jq \ && echo "Clean up" \ && rm -rf /var/lib/apt/lists/* /tmp/* From 7011b90bd4efdf33f85e8bfe1b9024e7747db9af Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 7 Mar 2018 15:42:59 +0000 Subject: [PATCH 35/42] Refactor is_precompiled_letter to model --- app/config.py | 1 - app/dao/notifications_dao.py | 2 +- app/dao/services_dao.py | 6 +-- app/dao/stats_template_usage_by_month_dao.py | 2 +- app/dao/templates_dao.py | 8 +++- app/letters/utils.py | 9 ----- app/models.py | 12 ++++++ app/schemas.py | 3 +- app/service/rest.py | 6 +-- app/template/rest.py | 4 +- app/template_statistics/rest.py | 3 +- .../test_stats_template_usage_by_month_dao.py | 32 +++++++++++++++- tests/app/dao/test_templates_dao.py | 37 +++++++++++++++++-- tests/app/letters/test_letter_utils.py | 23 +----------- tests/app/service/test_rest.py | 17 +++++---- tests/app/test_model.py | 23 +++++++++++- 16 files changed, 126 insertions(+), 62 deletions(-) diff --git a/app/config.py b/app/config.py index 951f5ece3..f9b125be1 100644 --- a/app/config.py +++ b/app/config.py @@ -149,7 +149,6 @@ class Config(object): CHANGE_EMAIL_CONFIRMATION_TEMPLATE_ID = 'eb4d9930-87ab-4aef-9bce-786762687884' SERVICE_NOW_LIVE_TEMPLATE_ID = '618185c6-3636-49cd-b7d2-6f6f5eb3bdde' ORGANISATION_INVITATION_EMAIL_TEMPLATE_ID = '203566f0-d835-47c5-aa06-932439c86573' - PRECOMPILED_TEMPLATE_NAME = 'Pre-compiled PDF' BROKER_URL = 'sqs://' BROKER_TRANSPORT_OPTIONS = { diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index efd13426e..7c25f8d21 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -83,7 +83,7 @@ def dao_get_template_usage(service_id, limit_days=None): Template.id.label('template_id'), Template.name, Template.template_type, - Template.hidden, + Template.is_precompiled_letter, notifications_aggregate_query.c.count ).join( notifications_aggregate_query, diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 6ad83d214..838a6336e 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -522,7 +522,7 @@ def dao_fetch_monthly_historical_usage_by_template_for_service(service_id, year) stat.month = result.month stat.year = result.year stat.count = result.count - stat.hidden = result.hidden + stat.is_precompiled_letter = result.is_precompiled_letter stats.append(stat) month = get_london_month_from_utc_column(Notification.created_at) @@ -534,7 +534,7 @@ def dao_fetch_monthly_historical_usage_by_template_for_service(service_id, year) if fy_start < datetime.now() < fy_end: today_results = db.session.query( Notification.template_id, - Template.hidden, + Template.is_precompiled_letter, Template.name, Template.template_type, extract('month', month).label('month'), @@ -574,7 +574,7 @@ def dao_fetch_monthly_historical_usage_by_template_for_service(service_id, year) new_stat.month = int(today_result.month) new_stat.year = int(today_result.year) new_stat.count = today_result.count - new_stat.hidden = today_result.hidden + new_stat.is_precompiled_letter = today_result.is_precompiled_letter stats.append(new_stat) return stats diff --git a/app/dao/stats_template_usage_by_month_dao.py b/app/dao/stats_template_usage_by_month_dao.py index 49e95cfe1..541ab7193 100644 --- a/app/dao/stats_template_usage_by_month_dao.py +++ b/app/dao/stats_template_usage_by_month_dao.py @@ -35,9 +35,9 @@ def insert_or_update_stats_for_template(template_id, month, year, count): def dao_get_template_usage_stats_by_service(service_id, year): return db.session.query( StatsTemplateUsageByMonth.template_id, - Template.hidden, Template.name, Template.template_type, + Template.is_precompiled_letter, StatsTemplateUsageByMonth.month, StatsTemplateUsageByMonth.year, StatsTemplateUsageByMonth.count diff --git a/app/dao/templates_dao.py b/app/dao/templates_dao.py index c664c4346..9dd397ef0 100644 --- a/app/dao/templates_dao.py +++ b/app/dao/templates_dao.py @@ -5,7 +5,11 @@ from sqlalchemy import asc, desc from sqlalchemy.sql.expression import bindparam from app import db -from app.models import (Template, TemplateHistory, TemplateRedacted) +from app.models import ( + Template, + TemplateHistory, + TemplateRedacted +) from app.dao.dao_utils import ( transactional, version_class @@ -135,7 +139,7 @@ def dao_get_templates_for_cache(cache): query = db.session.query(Template.id.label('template_id'), Template.template_type, Template.name, - Template.hidden, + Template.is_precompiled_letter, cache_subq.c.count.label('count') ).join(cache_subq, Template.id == cache_subq.c.template_id diff --git a/app/letters/utils.py b/app/letters/utils.py index a5f67c11b..a7f05c00f 100644 --- a/app/letters/utils.py +++ b/app/letters/utils.py @@ -5,7 +5,6 @@ from flask import current_app from notifications_utils.s3 import s3upload -from app.models import LETTER_TYPE from app.variables import Retention @@ -77,11 +76,3 @@ def get_letter_pdf(notification): file_content = obj.get()["Body"].read() return file_content - - -def is_precompiled_letter(template): - return ( - template.template_type == LETTER_TYPE and - template.hidden and - template.name == current_app.config['PRECOMPILED_TEMPLATE_NAME'] - ) diff --git a/app/models.py b/app/models.py index 6678d91e6..4f54f04fc 100644 --- a/app/models.py +++ b/app/models.py @@ -6,6 +6,7 @@ from flask import url_for, current_app from sqlalchemy.ext.declarative import declared_attr from sqlalchemy.ext.associationproxy import association_proxy +from sqlalchemy.ext.hybrid import hybrid_property from sqlalchemy.dialects.postgresql import ( UUID, JSON @@ -641,6 +642,9 @@ class TemplateProcessTypes(db.Model): name = db.Column(db.String(255), primary_key=True) +PRECOMPILED_TEMPLATE_NAME = 'Pre-compiled PDF' + + class TemplateBase(db.Model): __abstract__ = True @@ -718,6 +722,14 @@ class TemplateBase(db.Model): else: return None + @hybrid_property + def is_precompiled_letter(self): + return self.hidden and self.name == PRECOMPILED_TEMPLATE_NAME and self.template_type == LETTER_TYPE + + @is_precompiled_letter.setter + def is_precompiled_letter(self, value): + pass + def _as_utils_template(self): if self.template_type == EMAIL_TYPE: return PlainTextEmailTemplate( diff --git a/app/schemas.py b/app/schemas.py index f2282ac05..c76c2c922 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -24,7 +24,6 @@ from notifications_utils.recipients import ( from app import ma from app import models -from app.letters.utils import is_precompiled_letter from app.models import ServicePermission from app.dao.permissions_dao import permission_dao from app.utils import get_template_instance @@ -315,7 +314,7 @@ class BaseTemplateSchema(BaseSchema): return template.get_reply_to_text() def get_precompiled_letter(self, template): - return is_precompiled_letter(template) + return template.is_precompiled_letter class Meta: model = models.Template diff --git a/app/service/rest.py b/app/service/rest.py index 33c647131..2ccf51635 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -68,7 +68,7 @@ from app.errors import ( InvalidRequest, register_errors ) -from app.models import Service, EmailBranding +from app.models import Service, EmailBranding, LETTER_TYPE, PRECOMPILED_TEMPLATE_NAME from app.schema_validation import validate from app.service import statistics from app.service.service_senders_schema import ( @@ -538,9 +538,9 @@ def get_monthly_template_usage(service_id): 'year': i.year, 'count': i.count, 'precompiled_letter': ( - i.template_type == 'letter' and + i.template_type == LETTER_TYPE and i.hidden and - i.name == current_app.config['PRECOMPILED_TEMPLATE_NAME'] + i.name == PRECOMPILED_TEMPLATE_NAME ) } ) diff --git a/app/template/rest.py b/app/template/rest.py index e49eea762..b068d268c 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -21,7 +21,7 @@ from app.dao.templates_dao import ( dao_get_template_by_id) from notifications_utils.template import SMSMessageTemplate from app.dao.services_dao import dao_fetch_service_by_id -from app.letters.utils import get_letter_pdf, is_precompiled_letter +from app.letters.utils import get_letter_pdf from app.models import SMS_TYPE from app.notifications.validators import service_has_permission, check_reply_to from app.schemas import (template_schema, template_history_schema) @@ -199,7 +199,7 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file template = dao_get_template_by_id(notification.template_id) - if is_precompiled_letter(template): + if template.is_precompiled_letter: try: diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index f00741a7b..2af5efdae 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -14,7 +14,6 @@ from app.dao.templates_dao import ( dao_get_template_by_id_and_service_id ) -from app.letters.utils import is_precompiled_letter from app.schemas import notification_with_template_schema from app.utils import cache_key_for_service_template_counter from app.errors import register_errors, InvalidRequest @@ -49,7 +48,7 @@ def get_template_statistics_for_service_by_day(service_id): 'template_id': str(data.template_id), 'template_name': data.name, 'template_type': data.template_type, - 'precompiled_letter': is_precompiled_letter(data) + 'precompiled_letter': data.is_precompiled_letter } return jsonify(data=[serialize(row) for row in stats]) diff --git a/tests/app/dao/test_stats_template_usage_by_month_dao.py b/tests/app/dao/test_stats_template_usage_by_month_dao.py index 7d21696b5..676e00952 100644 --- a/tests/app/dao/test_stats_template_usage_by_month_dao.py +++ b/tests/app/dao/test_stats_template_usage_by_month_dao.py @@ -3,7 +3,7 @@ from app.dao.stats_template_usage_by_month_dao import ( insert_or_update_stats_for_template, dao_get_template_usage_stats_by_service ) -from app.models import StatsTemplateUsageByMonth +from app.models import StatsTemplateUsageByMonth, LETTER_TYPE, PRECOMPILED_TEMPLATE_NAME from tests.app.db import create_service, create_template @@ -74,6 +74,36 @@ def test_dao_get_template_usage_stats_by_service(sample_service): assert len(result) == 1 +def test_dao_get_template_usage_stats_by_service_for_precompiled_letters(sample_service): + + letter_template = create_template(service=sample_service, template_type=LETTER_TYPE) + + precompiled_letter_template = create_template( + service=sample_service, template_name=PRECOMPILED_TEMPLATE_NAME, hidden=True, template_type=LETTER_TYPE) + + db.session.add(StatsTemplateUsageByMonth( + template_id=letter_template.id, + month=5, + year=2017, + count=10 + )) + + db.session.add(StatsTemplateUsageByMonth( + template_id=precompiled_letter_template.id, + month=4, + year=2017, + count=20 + )) + + result = dao_get_template_usage_stats_by_service(sample_service.id, 2017) + + assert len(result) == 2 + assert [ + (letter_template.id, 'letter Template Name', 'letter', False, 5, 2017, 10), + (precompiled_letter_template.id, PRECOMPILED_TEMPLATE_NAME, 'letter', True, 4, 2017, 20) + ] == result + + def test_dao_get_template_usage_stats_by_service_specific_year(sample_service): email_template = create_template(service=sample_service, template_type="email") diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index e9daa86ae..d4768f598 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -13,7 +13,12 @@ from app.dao.templates_dao import ( dao_get_templates_for_cache, dao_redact_template, dao_update_template_reply_to ) -from app.models import Template, TemplateHistory, TemplateRedacted +from app.models import ( + Template, + TemplateHistory, + TemplateRedacted, + PRECOMPILED_TEMPLATE_NAME +) from tests.app.conftest import sample_template as create_sample_template from tests.app.db import create_template, create_letter_contact @@ -489,8 +494,7 @@ def test_get_templates_by_ids_successful(notify_db, notify_db_session): notify_db_session, template_name='Sample Template 2', template_type="sms", - content="Template content", - hidden=True + content="Template content" ) create_sample_template( notify_db, @@ -505,7 +509,32 @@ def test_get_templates_by_ids_successful(notify_db, notify_db_session): templates = dao_get_templates_for_cache(cache) assert len(templates) == 2 assert [(template_1.id, template_1.template_type, template_1.name, False, 2), - (template_2.id, template_2.template_type, template_2.name, True, 3)] == templates + (template_2.id, template_2.template_type, template_2.name, False, 3)] == templates + + +def test_get_letter_templates_by_ids_successful(notify_db, notify_db_session): + template_1 = create_sample_template( + notify_db, + notify_db_session, + template_name=PRECOMPILED_TEMPLATE_NAME, + template_type="letter", + content="Template content", + hidden=True + ) + template_2 = create_sample_template( + notify_db, + notify_db_session, + template_name='Sample Template 2', + template_type="letter", + content="Template content" + ) + sample_cache_dict = {str.encode(str(template_1.id)): str.encode('2'), + str.encode(str(template_2.id)): str.encode('3')} + cache = [[k, v] for k, v in sample_cache_dict.items()] + templates = dao_get_templates_for_cache(cache) + assert len(templates) == 2 + assert [(template_1.id, template_1.template_type, template_1.name, True, 2), + (template_2.id, template_2.template_type, template_2.name, False, 3)] == templates def test_get_templates_by_ids_successful_for_one_cache_item(notify_db, notify_db_session): diff --git a/tests/app/letters/test_letter_utils.py b/tests/app/letters/test_letter_utils.py index efa55bc0b..244c21f6a 100644 --- a/tests/app/letters/test_letter_utils.py +++ b/tests/app/letters/test_letter_utils.py @@ -1,7 +1,6 @@ import pytest -from flask import current_app -from app.letters.utils import get_bucket_prefix_for_notification, is_precompiled_letter +from app.letters.utils import get_bucket_prefix_for_notification def test_get_bucket_prefix_for_notification_valid_notification(sample_notification): @@ -17,23 +16,3 @@ def test_get_bucket_prefix_for_notification_valid_notification(sample_notificati def test_get_bucket_prefix_for_notification_invalid_notification(): with pytest.raises(AttributeError): get_bucket_prefix_for_notification(None) - - -def test_is_precompiled_letter_false(sample_letter_template): - assert not is_precompiled_letter(sample_letter_template) - - -def test_is_precompiled_letter_true(sample_letter_template): - sample_letter_template.hidden = True - sample_letter_template.name = current_app.config['PRECOMPILED_TEMPLATE_NAME'] - assert is_precompiled_letter(sample_letter_template) - - -def test_is_precompiled_letter_hidden_true_not_name(sample_letter_template): - sample_letter_template.hidden = True - assert not is_precompiled_letter(sample_letter_template) - - -def test_is_precompiled_letter_name_correct_not_hidden(sample_letter_template): - sample_letter_template.name = current_app.config['PRECOMPILED_TEMPLATE_NAME'] - assert not is_precompiled_letter(sample_letter_template) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index b600b239b..7fb44db12 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -25,7 +25,8 @@ from app.models import ( User, DVLA_ORG_LAND_REGISTRY, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST, - EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INBOUND_SMS_TYPE + EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INBOUND_SMS_TYPE, + PRECOMPILED_TEMPLATE_NAME ) from tests import create_authorization_header from tests.app.conftest import ( @@ -1834,7 +1835,7 @@ def test_get_template_usage_by_month_returns_two_templates( template_one = create_template( sample_service, template_type=LETTER_TYPE, - template_name=current_app.config['PRECOMPILED_TEMPLATE_NAME'], + template_name=PRECOMPILED_TEMPLATE_NAME, hidden=True ) @@ -1894,7 +1895,7 @@ def test_get_template_usage_by_month_returns_two_templates( assert resp_json[0]["month"] == 4 assert resp_json[0]["year"] == 2017 assert resp_json[0]["count"] == 1 - assert resp_json[0]["precompiled_letter"] is True + assert resp_json[0]["is_precompiled_letter"] is True assert resp_json[1]["template_id"] == str(sample_template.id) assert resp_json[1]["name"] == sample_template.name @@ -1902,7 +1903,7 @@ def test_get_template_usage_by_month_returns_two_templates( assert resp_json[1]["month"] == 4 assert resp_json[1]["year"] == 2017 assert resp_json[1]["count"] == 3 - assert resp_json[1]["precompiled_letter"] is False + assert resp_json[1]["is_precompiled_letter"] is False assert resp_json[2]["template_id"] == str(sample_template.id) assert resp_json[2]["name"] == sample_template.name @@ -1910,7 +1911,7 @@ def test_get_template_usage_by_month_returns_two_templates( assert resp_json[2]["month"] == 11 assert resp_json[2]["year"] == 2017 assert resp_json[2]["count"] == 1 - assert resp_json[2]["precompiled_letter"] is False + assert resp_json[2]["is_precompiled_letter"] is False def test_search_for_notification_by_to_field(client, notify_db, notify_db_session): @@ -2155,7 +2156,7 @@ def test_get_notification_for_service_includes_precompiled_letter(admin_request, ) assert resp['id'] == str(sample_notification.id) - assert resp['template']['precompiled_letter'] is False + assert resp['template']['is_precompiled_letter'] is False def test_get_all_notifications_for_service_includes_template_redacted(admin_request, sample_service): @@ -2202,10 +2203,10 @@ def test_get_all_notifications_for_service_includes_template_hidden(admin_reques ) assert resp['notifications'][0]['id'] == str(precompiled_noti.id) - assert resp['notifications'][0]['template']['precompiled_letter'] is True + assert resp['notifications'][0]['template']['is_precompiled_letter'] is True assert resp['notifications'][1]['id'] == str(letter_noti.id) - assert resp['notifications'][1]['template']['precompiled_letter'] is False + assert resp['notifications'][1]['template']['is_precompiled_letter'] is False def test_search_for_notification_by_to_field_returns_personlisation( diff --git a/tests/app/test_model.py b/tests/app/test_model.py index a0b8d4804..c7e253470 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -18,7 +18,8 @@ from app.models import ( NOTIFICATION_STATUS_LETTER_ACCEPTED, NOTIFICATION_STATUS_LETTER_RECEIVED, NOTIFICATION_STATUS_TYPES_FAILED, - NOTIFICATION_TECHNICAL_FAILURE + NOTIFICATION_TECHNICAL_FAILURE, + PRECOMPILED_TEMPLATE_NAME ) from tests.app.conftest import ( sample_template as create_sample_template, @@ -319,3 +320,23 @@ def test_letter_notification_postcode_can_be_null_for_precompiled_letters(client assert json['line_1'] == 'test' assert json['line_2'] == 'London' assert json['postcode'] is None + + +def test_is_precompiled_letter_false(sample_letter_template): + assert not sample_letter_template.is_precompiled_letter + + +def test_is_precompiled_letter_true(sample_letter_template): + sample_letter_template.hidden = True + sample_letter_template.name = PRECOMPILED_TEMPLATE_NAME + assert sample_letter_template.is_precompiled_letter + + +def test_is_precompiled_letter_hidden_true_not_name(sample_letter_template): + sample_letter_template.hidden = True + assert not sample_letter_template.is_precompiled_letter + + +def test_is_precompiled_letter_name_correct_not_hidden(sample_letter_template): + sample_letter_template.name = PRECOMPILED_TEMPLATE_NAME + assert not sample_letter_template.is_precompiled_letter From 23ce36dc484f37079afcc780120e5ae2b2dc28c3 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 7 Mar 2018 23:02:38 +0000 Subject: [PATCH 36/42] Update response to return is_precompiled_letter --- app/schemas.py | 6 +----- app/service/rest.py | 8 ++------ app/template_statistics/rest.py | 2 +- 3 files changed, 4 insertions(+), 12 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index c76c2c922..2131bb9b7 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -305,7 +305,6 @@ class NotificationModelSchema(BaseSchema): class BaseTemplateSchema(BaseSchema): reply_to = fields.Method("get_reply_to", allow_none=True) reply_to_text = fields.Method("get_reply_to_text", allow_none=True) - precompiled_letter = fields.Method("get_precompiled_letter") def get_reply_to(self, template): return template.reply_to @@ -313,9 +312,6 @@ class BaseTemplateSchema(BaseSchema): def get_reply_to_text(self, template): return template.get_reply_to_text() - def get_precompiled_letter(self, template): - return template.is_precompiled_letter - class Meta: model = models.Template exclude = ("service_id", "jobs", "service_letter_contact_id") @@ -466,7 +462,7 @@ class NotificationWithTemplateSchema(BaseSchema): 'content', 'subject', 'redact_personalisation', - 'precompiled_letter' + 'is_precompiled_letter' ], dump_only=True ) diff --git a/app/service/rest.py b/app/service/rest.py index 2ccf51635..f6c7b8650 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -68,7 +68,7 @@ from app.errors import ( InvalidRequest, register_errors ) -from app.models import Service, EmailBranding, LETTER_TYPE, PRECOMPILED_TEMPLATE_NAME +from app.models import Service, EmailBranding from app.schema_validation import validate from app.service import statistics from app.service.service_senders_schema import ( @@ -537,11 +537,7 @@ def get_monthly_template_usage(service_id): 'month': i.month, 'year': i.year, 'count': i.count, - 'precompiled_letter': ( - i.template_type == LETTER_TYPE and - i.hidden and - i.name == PRECOMPILED_TEMPLATE_NAME - ) + 'is_precompiled_letter': i.is_precompiled_letter } ) diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index 2af5efdae..b7198409e 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -48,7 +48,7 @@ def get_template_statistics_for_service_by_day(service_id): 'template_id': str(data.template_id), 'template_name': data.name, 'template_type': data.template_type, - 'precompiled_letter': data.is_precompiled_letter + 'is_precompiled_letter': data.is_precompiled_letter } return jsonify(data=[serialize(row) for row in stats]) From 22f86aa1b5c85b6f4b73b8b9a5a1f4e21195ecd1 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 8 Mar 2018 13:35:53 +0000 Subject: [PATCH 37/42] Revert service callback worker eventlets We have seen problems with the service callback workers due to the db connection pool being exhausted. When the worker picks up the task, it makes a db query to get the notification, a query to get the callback url, and then closes the session before it makes the 3rd party request. However, even closing the session before the (potentially lengthy) web request wasn't enough - we've seen significant amounts of `sqlalchemy.exc.TimeoutError`s. This reverts commit 2dfbd93c7ed65028706e04762ea5102c60e5acbf --- manifest-delivery-base.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/manifest-delivery-base.yml b/manifest-delivery-base.yml index e141c6482..82bad9b2d 100644 --- a/manifest-delivery-base.yml +++ b/manifest-delivery-base.yml @@ -95,6 +95,6 @@ applications: NOTIFY_APP_NAME: delivery-worker-receipts - name: notify-delivery-worker-service-callbacks - command: scripts/run_app_paas.sh celery -A run_celery.notify_celery worker --loglevel=INFO -P eventlet -c 1000 -Q service-callbacks + command: scripts/run_app_paas.sh celery -A run_celery.notify_celery worker --loglevel=INFO --concurrency=11 -Q service-callbacks env: - NOTIFY_APP_NAME: delivery-worker-service-callbacks + NOTIFY_APP_NAME: delivery-worker-service-callbacks \ No newline at end of file From 19129a1313a3042a7211505d7c524970926f9766 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Thu, 8 Mar 2018 14:02:08 +0000 Subject: [PATCH 38/42] Describe process for creating a new worker app --- README.md | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/README.md b/README.md index b85fe6479..11db3cb26 100644 --- a/README.md +++ b/README.md @@ -115,3 +115,30 @@ cf run-task notify-api "flask command purge_functional_test_data -u make cf-push` + +Once this is done, you can push your deployment changes to jenkins to have your app deployed on every deployment. From 651c3062b96c7329bd73f67a1a2cf6a2acf571c6 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 8 Mar 2018 14:03:01 +0000 Subject: [PATCH 39/42] retry service callbacks if the db queries fail we don't expect them to fail, but they might if we accidentally exhaust our connection pool. Just in case, lets retry. --- app/celery/service_callback_tasks.py | 59 +++++++++++-------- .../app/celery/test_service_callback_tasks.py | 21 +++++-- 2 files changed, 51 insertions(+), 29 deletions(-) diff --git a/app/celery/service_callback_tasks.py b/app/celery/service_callback_tasks.py index d1179ff17..eaa63dc00 100644 --- a/app/celery/service_callback_tasks.py +++ b/app/celery/service_callback_tasks.py @@ -24,28 +24,29 @@ from app.config import QueueNames @notify_celery.task(bind=True, name="send-delivery-status", max_retries=5, default_retry_delay=300) @statsd(namespace="tasks") def send_delivery_status_to_service(self, notification_id): - # TODO: do we need to do rate limit this? - notification = get_notification_by_id(notification_id) - service_callback_api = get_service_callback_api_for_service(service_id=notification.service_id) - if not service_callback_api: - # No delivery receipt API info set - return - - # Release DB connection before performing an external HTTP request - db.session.close() - - data = { - "id": str(notification_id), - "reference": str(notification.client_reference), - "to": notification.to, - "status": notification.status, - "created_at": notification.created_at.strftime(DATETIME_FORMAT), # the time service sent the request - "completed_at": notification.updated_at.strftime(DATETIME_FORMAT), # the last time the status was updated - "sent_at": notification.sent_at.strftime(DATETIME_FORMAT), # the time the email was sent - "notification_type": notification.notification_type - } - + retry = False try: + # TODO: do we need to do rate limit this? + notification = get_notification_by_id(notification_id) + service_callback_api = get_service_callback_api_for_service(service_id=notification.service_id) + if not service_callback_api: + # No delivery receipt API info set + return + + # Release DB connection before performing an external HTTP request + db.session.close() + + data = { + "id": str(notification_id), + "reference": str(notification.client_reference), + "to": notification.to, + "status": notification.status, + "created_at": notification.created_at.strftime(DATETIME_FORMAT), # the time service sent the request + "completed_at": notification.updated_at.strftime(DATETIME_FORMAT), # the last time the status was updated + "sent_at": notification.sent_at.strftime(DATETIME_FORMAT), # the time the email was sent + "notification_type": notification.notification_type + } + response = request( method="POST", url=service_callback_api.url, @@ -71,7 +72,15 @@ def send_delivery_status_to_service(self, notification_id): ) ) if not isinstance(e, HTTPError) or e.response.status_code >= 500: - try: - self.retry(queue=QueueNames.RETRY) - except self.MaxRetriesExceededError: - current_app.logger.exception('Retry: send_delivery_status_to_service has retried the max num of times') + retry = True + except Exception as e: + current_app.logger.exception( + 'Unhandled exception when sending callback for notification {}'.format(notification_id) + ) + retry = True + + if retry: + try: + self.retry(queue=QueueNames.RETRY) + except self.MaxRetriesExceededError: + current_app.logger.exception('Retry: send_delivery_status_to_service has retried the max num of times') diff --git a/tests/app/celery/test_service_callback_tasks.py b/tests/app/celery/test_service_callback_tasks.py index a7c9aaeb4..c387fdb44 100644 --- a/tests/app/celery/test_service_callback_tasks.py +++ b/tests/app/celery/test_service_callback_tasks.py @@ -1,10 +1,11 @@ +import uuid import json from datetime import datetime +from requests import RequestException import pytest import requests_mock - -from requests import RequestException +from sqlalchemy.exc import SQLAlchemyError from app import (DATETIME_FORMAT) @@ -18,6 +19,7 @@ from tests.app.db import ( create_service_callback_api ) from app.celery.service_callback_tasks import send_delivery_status_to_service +from app.config import QueueNames @pytest.mark.parametrize("notification_type", @@ -88,7 +90,7 @@ def test_send_delivery_status_to_service_does_not_sent_request_when_service_call mocked = mocker.patch("requests.request") send_delivery_status_to_service(notification.id) - mocked.call_count == 0 + assert mocked.call_count == 0 @pytest.mark.parametrize("notification_type", @@ -182,4 +184,15 @@ def test_send_delivery_status_to_service_does_not_retries_if_request_returns_404 status_code=404) send_delivery_status_to_service(notification.id) - mocked.call_count == 0 + assert mocked.call_count == 0 + + +def test_send_delivery_status_to_service_retries_if_database_error(client, mocker): + notification_id = uuid.uuid4() + db_call = mocker.patch('app.celery.service_callback_tasks.get_notification_by_id', side_effect=SQLAlchemyError) + retry = mocker.patch('app.celery.service_callback_tasks.send_delivery_status_to_service.retry') + + send_delivery_status_to_service(notification_id) + + db_call.assert_called_once_with(notification_id) + retry.assert_called_once_with(queue=QueueNames.RETRY) From 00b17b5ad7e49c55ee8762212e25015676d056b5 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 8 Mar 2018 16:03:16 +0000 Subject: [PATCH 40/42] When we sent the service the status callback for a notification, we have all the information we need. Which means we can remove the need to request the data from the database. In order for the PR to be backwards compatible I have added an optional parameter "encrypted_status_update". If this is not None then the new code is called. The next PR will send the encrypted data to this task. A final PR will remove the code that uses the database to get the notification and service callback api. --- app/celery/service_callback_tasks.py | 62 +++++- .../app/celery/test_service_callback_tasks.py | 186 +++++++++++++----- 2 files changed, 191 insertions(+), 57 deletions(-) diff --git a/app/celery/service_callback_tasks.py b/app/celery/service_callback_tasks.py index eaa63dc00..79f85e651 100644 --- a/app/celery/service_callback_tasks.py +++ b/app/celery/service_callback_tasks.py @@ -6,6 +6,7 @@ from app import ( db, DATETIME_FORMAT, notify_celery, + encryption ) from app.dao.notifications_dao import ( get_notification_by_id, @@ -23,10 +24,61 @@ from app.config import QueueNames @notify_celery.task(bind=True, name="send-delivery-status", max_retries=5, default_retry_delay=300) @statsd(namespace="tasks") -def send_delivery_status_to_service(self, notification_id): +def send_delivery_status_to_service(self, notification_id, + encrypted_status_update=None + ): + if not encrypted_status_update: + process_update_with_notification_id(self, notification_id=notification_id) + else: + try: + status_update = encryption.decrypt(encrypted_status_update) + + data = { + "id": str(notification_id), + "reference": status_update['notification_client_reference'], + "to": status_update['notification_to'], + "status": status_update['notification_status'], + "created_at": status_update['notification_created_at'], + "completed_at": status_update['notification_updated_at'], + "sent_at": status_update['notification_sent_at'], + "notification_type": status_update['notification_type'] + } + + response = request( + method="POST", + url=status_update['service_callback_api_url'], + data=json.dumps(data), + headers={ + 'Content-Type': 'application/json', + 'Authorization': 'Bearer {}'.format(status_update['service_callback_api_bearer_token']) + }, + timeout=60 + ) + current_app.logger.info('send_delivery_status_to_service sending {} to {}, response {}'.format( + notification_id, + status_update['service_callback_api_url'], + response.status_code + )) + response.raise_for_status() + except RequestException as e: + current_app.logger.warning( + "send_delivery_status_to_service request failed for service_id: {} and url: {}. exc: {}".format( + notification_id, + status_update['service_callback_api_url'], + e + ) + ) + if not isinstance(e, HTTPError) or e.response.status_code >= 500: + try: + self.retry(queue=QueueNames.RETRY) + except self.MaxRetriesExceededError: + current_app.logger.exception( + 'Retry: send_delivery_status_to_service has retried the max num of times') + + +def process_update_with_notification_id(self, notification_id): retry = False try: - # TODO: do we need to do rate limit this? notification = get_notification_by_id(notification_id) service_callback_api = get_service_callback_api_for_service(service_id=notification.service_id) if not service_callback_api: @@ -41,9 +93,9 @@ def send_delivery_status_to_service(self, notification_id): "reference": str(notification.client_reference), "to": notification.to, "status": notification.status, - "created_at": notification.created_at.strftime(DATETIME_FORMAT), # the time service sent the request - "completed_at": notification.updated_at.strftime(DATETIME_FORMAT), # the last time the status was updated - "sent_at": notification.sent_at.strftime(DATETIME_FORMAT), # the time the email was sent + "created_at": notification.created_at.strftime(DATETIME_FORMAT), + "completed_at": notification.updated_at.strftime(DATETIME_FORMAT), + "sent_at": notification.sent_at.strftime(DATETIME_FORMAT), "notification_type": notification.notification_type } diff --git a/tests/app/celery/test_service_callback_tasks.py b/tests/app/celery/test_service_callback_tasks.py index c387fdb44..11abf5f0f 100644 --- a/tests/app/celery/test_service_callback_tasks.py +++ b/tests/app/celery/test_service_callback_tasks.py @@ -7,16 +7,13 @@ import pytest import requests_mock from sqlalchemy.exc import SQLAlchemyError -from app import (DATETIME_FORMAT) +from app import (DATETIME_FORMAT, encryption) -from tests.app.conftest import ( - sample_service as create_sample_service, - sample_template as create_sample_template, -) from tests.app.db import ( create_notification, - create_user, - create_service_callback_api + create_service_callback_api, + create_service, + create_template ) from app.celery.service_callback_tasks import send_delivery_status_to_service from app.config import QueueNames @@ -24,18 +21,127 @@ from app.config import QueueNames @pytest.mark.parametrize("notification_type", ["email", "letter", "sms"]) -def test_send_delivery_status_to_service_post_https_request_to_service(notify_db, - notify_db_session, - notification_type): - user = create_user() - service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) +def test_send_delivery_status_to_service_post_https_request_to_service_with_encrypted_data( + notify_db_session, notification_type): + callback_api, template = _set_up_test_data(notification_type) + datestr = datetime(2017, 6, 20) + + notification = create_notification(template=template, + created_at=datestr, + updated_at=datestr, + sent_at=datestr, + status='sent' + ) + encrypted_status_update = _set_up_encrypted_data(callback_api, notification) + with requests_mock.Mocker() as request_mock: + request_mock.post(callback_api.url, + json={}, + status_code=200) + send_delivery_status_to_service(notification.id, encrypted_status_update=encrypted_status_update) + + mock_data = { + "id": str(notification.id), + "reference": notification.client_reference, + "to": notification.to, + "status": notification.status, + "created_at": datestr.strftime(DATETIME_FORMAT), + "completed_at": datestr.strftime(DATETIME_FORMAT), + "sent_at": datestr.strftime(DATETIME_FORMAT), + "notification_type": notification_type + } + + assert request_mock.call_count == 1 + assert request_mock.request_history[0].url == callback_api.url + assert request_mock.request_history[0].method == 'POST' + assert request_mock.request_history[0].text == json.dumps(mock_data) + assert request_mock.request_history[0].headers["Content-type"] == "application/json" + assert request_mock.request_history[0].headers["Authorization"] == "Bearer {}".format(callback_api.bearer_token) + + +@pytest.mark.parametrize("notification_type", + ["email", "letter", "sms"]) +def test_send_delivery_status_to_service_retries_if_request_returns_500_with_encrypted_data( + notify_db_session, mocker, notification_type +): + callback_api, template = _set_up_test_data(notification_type) + datestr = datetime(2017, 6, 20) + notification = create_notification(template=template, + created_at=datestr, + updated_at=datestr, + sent_at=datestr, + status='sent' + ) + encrypted_data = _set_up_encrypted_data(callback_api, notification) + mocked = mocker.patch('app.celery.service_callback_tasks.send_delivery_status_to_service.retry') + with requests_mock.Mocker() as request_mock: + request_mock.post(callback_api.url, + json={}, + status_code=500) + send_delivery_status_to_service(notification.id, encrypted_status_update=encrypted_data) + + assert mocked.call_count == 1 + assert mocked.call_args[1]['queue'] == 'retry-tasks' + + +@pytest.mark.parametrize("notification_type", + ["email", "letter", "sms"]) +def test_send_delivery_status_to_service_does_not_retries_if_request_returns_404_with_encrypted_data( + notify_db_session, + mocker, + notification_type +): + callback_api, template = _set_up_test_data(notification_type) + datestr = datetime(2017, 6, 20) + notification = create_notification(template=template, + created_at=datestr, + updated_at=datestr, + sent_at=datestr, + status='sent' + ) + encrypted_data = _set_up_encrypted_data(callback_api, notification) + mocked = mocker.patch('app.celery.service_callback_tasks.send_delivery_status_to_service.retry') + with requests_mock.Mocker() as request_mock: + request_mock.post(callback_api.url, + json={}, + status_code=404) + send_delivery_status_to_service(notification.id, encrypted_status_update=encrypted_data) + + assert mocked.call_count == 0 + + +def _set_up_test_data(notification_type): + service = create_service(restricted=True) + template = create_template(service=service, template_type=notification_type, subject='Hello') callback_api = create_service_callback_api(service=service, url="https://some.service.gov.uk/", bearer_token="something_unique") - template = create_sample_template( - notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' - ) + return callback_api, template + +def _set_up_encrypted_data(callback_api, notification): + data = { + "notification_id": str(notification.id), + "notification_client_reference": notification.client_reference, + "notification_to": notification.to, + "notification_status": notification.status, + "notification_created_at": notification.created_at.strftime(DATETIME_FORMAT), + "notification_updated_at": notification.updated_at.strftime(DATETIME_FORMAT), + "notification_sent_at": notification.sent_at.strftime(DATETIME_FORMAT), + "notification_type": notification.notification_type, + "service_callback_api_url": callback_api.url, + "service_callback_api_bearer_token": callback_api.bearer_token, + } + encrypted_status_update = encryption.encrypt(data) + return encrypted_status_update + + +# We are updating the task to take everything it needs so that there are no db calls. +# The following tests will be deleted once that is complete. +@pytest.mark.parametrize("notification_type", + ["email", "letter", "sms"]) +def test_send_delivery_status_to_service_post_https_request_to_service( + notify_db_session, notification_type): + callback_api, template = _set_up_test_data(notification_type) datestr = datetime(2017, 6, 20) notification = create_notification(template=template, @@ -73,12 +179,9 @@ def test_send_delivery_status_to_service_post_https_request_to_service(notify_db @pytest.mark.parametrize("notification_type", ["email", "letter", "sms"]) def test_send_delivery_status_to_service_does_not_sent_request_when_service_callback_api_does_not_exist( - notify_db, notify_db_session, mocker, notification_type): - service = create_sample_service(notify_db, notify_db_session, restricted=True) - - template = create_sample_template( - notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' - ) + notify_db_session, mocker, notification_type): + service = create_service(restricted=True) + template = create_template(service=service, template_type=notification_type, subject='Hello') datestr = datetime(2017, 6, 20) notification = create_notification(template=template, @@ -95,18 +198,10 @@ def test_send_delivery_status_to_service_does_not_sent_request_when_service_call @pytest.mark.parametrize("notification_type", ["email", "letter", "sms"]) -def test_send_delivery_status_to_service_retries_if_request_returns_500(notify_db, - notify_db_session, +def test_send_delivery_status_to_service_retries_if_request_returns_500(notify_db_session, mocker, notification_type): - user = create_user() - service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) - - template = create_sample_template( - notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' - ) - callback_api = create_service_callback_api(service=service, url="https://some.service.gov.uk/", - bearer_token="something_unique") + callback_api, template = _set_up_test_data(notification_type) datestr = datetime(2017, 6, 20) notification = create_notification(template=template, created_at=datestr, @@ -127,18 +222,11 @@ def test_send_delivery_status_to_service_retries_if_request_returns_500(notify_d @pytest.mark.parametrize("notification_type", ["email", "letter", "sms"]) -def test_send_delivery_status_to_service_retries_if_request_throws_unknown(notify_db, - notify_db_session, +def test_send_delivery_status_to_service_retries_if_request_throws_unknown(notify_db_session, mocker, notification_type): - user = create_user() - service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) - template = create_sample_template( - notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' - ) - create_service_callback_api(service=service, url="https://some.service.gov.uk/", - bearer_token="something_unique") + callback_api, template = _set_up_test_data(notification_type) datestr = datetime(2017, 6, 20) notification = create_notification(template=template, created_at=datestr, @@ -158,18 +246,12 @@ def test_send_delivery_status_to_service_retries_if_request_throws_unknown(notif @pytest.mark.parametrize("notification_type", ["email", "letter", "sms"]) -def test_send_delivery_status_to_service_does_not_retries_if_request_returns_404(notify_db, - notify_db_session, - mocker, - notification_type): - user = create_user() - service = create_sample_service(notify_db, notify_db_session, user=user, restricted=True) - - template = create_sample_template( - notify_db, notify_db_session, service=service, template_type=notification_type, subject_line='Hello' - ) - callback_api = create_service_callback_api(service=service, url="https://some.service.gov.uk/", - bearer_token="something_unique") +def test_send_delivery_status_to_service_does_not_retries_if_request_returns_404( + notify_db_session, + mocker, + notification_type +): + callback_api, template = _set_up_test_data(notification_type) datestr = datetime(2017, 6, 20) notification = create_notification(template=template, created_at=datestr, From e95740a6b517b4da7141087c19dbeb26e1c38df7 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 9 Mar 2018 11:06:47 +0000 Subject: [PATCH 41/42] There was a problem with the worker that was sending the service updates for the notification. The problem has been resolved but we need to replay the messages that are missing. We have been sent a file containing client_references for all the notificaitons that the service would needs updates for. --- app/commands.py | 70 +++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 62 insertions(+), 8 deletions(-) diff --git a/app/commands.py b/app/commands.py index 8c807f1e0..1098863ae 100644 --- a/app/commands.py +++ b/app/commands.py @@ -1,29 +1,33 @@ +import functools import uuid from datetime import datetime, timedelta from decimal import Decimal -import functools -import flask -from flask import current_app import click +import flask from click_datetime import Datetime as click_dt +from flask import current_app +from sqlalchemy.orm.exc import NoResultFound -from app import db +from app import db, DATETIME_FORMAT, encryption +from app.celery.scheduled_tasks import send_total_sent_notifications_to_performance_platform +from app.celery.service_callback_tasks import send_delivery_status_to_service +from app.config import QueueNames from app.dao.monthly_billing_dao import ( create_or_update_monthly_billing, get_monthly_billing_by_notification_type, get_service_ids_that_need_billing_populated ) -from app.models import PROVIDERS, User, SMS_TYPE, EMAIL_TYPE +from app.dao.provider_rates_dao import create_provider_rates as dao_create_provider_rates +from app.dao.service_callback_api_dao import get_service_callback_api_for_service from app.dao.services_dao import ( delete_service_and_all_associated_db_objects, dao_fetch_all_services_by_user ) -from app.dao.provider_rates_dao import create_provider_rates as dao_create_provider_rates from app.dao.users_dao import (delete_model_user, delete_user_verify_codes) -from app.utils import get_midnight_for_day_before, get_london_midnight_in_utc +from app.models import PROVIDERS, User, SMS_TYPE, EMAIL_TYPE, Notification from app.performance_platform.processing_time import (send_processing_time_for_start_and_end) -from app.celery.scheduled_tasks import send_total_sent_notifications_to_performance_platform +from app.utils import get_midnight_for_day_before, get_london_midnight_in_utc @click.group(name='command', help='Additional commands') @@ -311,5 +315,55 @@ def insert_inbound_numbers_from_file(file_name): file.close() +@notify_command(name='replay-service-callbacks') +@click.option('-f', '--file_name', required=True, + help="""Full path of the file to upload, file is a contains client references of + notifications that need the status to be sent to the service.""") +@click.option('-s', '--service_id', required=True, + help="""The service that the callbacks are for""") +def replay_service_callbacks(file_name, service_id): + print("Start send service callbacks for service: ", service_id) + callback_api = get_service_callback_api_for_service(service_id=service_id) + if not callback_api: + print("Callback api was not found for service: {}".format(service_id)) + return + + errors = [] + notifications = [] + file = open(file_name) + + for ref in file: + try: + notification = Notification.query.filter_by(client_reference=ref.strip()).one() + notifications.append(notification) + except NoResultFound as e: + errors.append("Reference: {} was not found in notifications.".format(ref)) + + for e in errors: + print(e) + if errors: + raise Exception("Some notifications for the given references were not found") + + for n in notifications: + data = { + "notification_id": str(n.id), + "notification_client_reference": n.client_reference, + "notification_to": n.to, + "notification_status": n.status, + "notification_created_at": n.created_at.strftime(DATETIME_FORMAT), + "notification_updated_at": n.updated_at.strftime(DATETIME_FORMAT), + "notification_sent_at": n.sent_at.strftime(DATETIME_FORMAT), + "notification_type": n.notification_type, + "service_callback_api_url": callback_api.url, + "service_callback_api_bearer_token": callback_api.bearer_token, + } + encrypted_status_update = encryption.encrypt(data) + send_delivery_status_to_service.apply_async([str(n.id), encrypted_status_update], + queue=QueueNames.CALLBACKS) + + print("Replay service status for service: {}. Sent {} notification status updates".format(service_id, + len(notifications))) + + def setup_commands(application): application.cli.add_command(command_group) From a3d04ca67229e1d3bcb0b201527f367b3248b9ff Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 9 Mar 2018 12:01:08 +0000 Subject: [PATCH 42/42] Improve log message --- app/celery/service_callback_tasks.py | 9 +++++++-- app/commands.py | 4 ++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/app/celery/service_callback_tasks.py b/app/celery/service_callback_tasks.py index 79f85e651..21e87266e 100644 --- a/app/celery/service_callback_tasks.py +++ b/app/celery/service_callback_tasks.py @@ -73,7 +73,9 @@ def send_delivery_status_to_service(self, notification_id, self.retry(queue=QueueNames.RETRY) except self.MaxRetriesExceededError: current_app.logger.exception( - 'Retry: send_delivery_status_to_service has retried the max num of times') + """Retry: send_delivery_status_to_service has retried the max num of times + for notification: {}""".format(notification_id) + ) def process_update_with_notification_id(self, notification_id): @@ -135,4 +137,7 @@ def process_update_with_notification_id(self, notification_id): try: self.retry(queue=QueueNames.RETRY) except self.MaxRetriesExceededError: - current_app.logger.exception('Retry: send_delivery_status_to_service has retried the max num of times') + current_app.logger.exception( + """Retry: send_delivery_status_to_service has retried the max num of times + for notification: {}""".format(notification_id) + ) diff --git a/app/commands.py b/app/commands.py index 1098863ae..23fc18c0a 100644 --- a/app/commands.py +++ b/app/commands.py @@ -361,8 +361,8 @@ def replay_service_callbacks(file_name, service_id): send_delivery_status_to_service.apply_async([str(n.id), encrypted_status_update], queue=QueueNames.CALLBACKS) - print("Replay service status for service: {}. Sent {} notification status updates".format(service_id, - len(notifications))) + print("Replay service status for service: {}. Sent {} notification status updates to the queue".format( + service_id, len(notifications))) def setup_commands(application):