From 3355a29d01701aee9d9c442242ea0a7c85d74e48 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 22 Jan 2018 10:18:11 +0000 Subject: [PATCH 1/6] Ensure letter personalisation is serialized correctly The personalisation for letters can take different formats depending on how the letter was generated, for example it can contain either address_line_1 or addressline1. This change ensures that it is always serialized in the same way. --- app/models.py | 14 ++++++++------ tests/app/test_model.py | 13 +++++++++++++ 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/app/models.py b/app/models.py index 3b2bcbb96..7cb6c2e41 100644 --- a/app/models.py +++ b/app/models.py @@ -11,6 +11,7 @@ from sqlalchemy.dialects.postgresql import ( JSON ) from sqlalchemy import UniqueConstraint, CheckConstraint +from notifications_utils.columns import Columns from notifications_utils.recipients import ( validate_email_address, validate_phone_number, @@ -1223,12 +1224,13 @@ class Notification(db.Model): } if self.notification_type == LETTER_TYPE: - serialized['line_1'] = self.personalisation['address_line_1'] - serialized['line_2'] = self.personalisation.get('address_line_2') - serialized['line_3'] = self.personalisation.get('address_line_3') - serialized['line_4'] = self.personalisation.get('address_line_4') - serialized['line_5'] = self.personalisation.get('address_line_5') - serialized['line_6'] = self.personalisation.get('address_line_6') + col = Columns(self.personalisation) + serialized['line_1'] = col.get('address_line_1') + serialized['line_2'] = col.get('address_line_2') + serialized['line_3'] = col.get('address_line_3') + serialized['line_4'] = col.get('address_line_4') + serialized['line_5'] = col.get('address_line_5') + serialized['line_6'] = col.get('address_line_6') serialized['postcode'] = self.personalisation['postcode'] serialized['estimated_delivery'] = \ get_letter_timings(serialized['created_at'])\ diff --git a/tests/app/test_model.py b/tests/app/test_model.py index 406d3a5e0..af73e1460 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -294,3 +294,16 @@ def test_service_get_default_contact_letter(sample_service): def test_service_get_default_sms_sender(notify_db_session): service = create_service() assert service.get_default_sms_sender() == 'testing' + + +def test_letter_notification_serializes_correctly(client, sample_letter_notification): + sample_letter_notification.personalisation = { + 'addressline1': 'test', + 'addressline2': 'London', + 'postcode': 'N1', + } + + json = sample_letter_notification.serialize() + assert json['line_1'] == 'test' + assert json['line_2'] == 'London' + assert json['postcode'] == 'N1' From edd030e5dde12345dd4295e5f88074660892d1d9 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 23 Jan 2018 17:45:35 +0000 Subject: [PATCH 2/6] Refactor code to move business logic - DVLA specific logic is now moved to letters_pdf_tasks --- app/aws/s3.py | 31 --------------- app/celery/letters_pdf_tasks.py | 40 +++++++++++++++++++- tests/app/aws/test_s3.py | 33 ---------------- tests/app/celery/test_letters_pdf_tasks.py | 44 ++++++++++++++++++---- 4 files changed, 75 insertions(+), 73 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index 8020c18a7..aa1bbed97 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -5,10 +5,7 @@ from flask import current_app import pytz from boto3 import client, resource -from notifications_utils.s3 import s3upload as utils_s3upload - FILE_LOCATION_STRUCTURE = 'service-{}-notify/{}.csv' -LETTERS_PDF_FILE_LOCATION_STRUCTURE = '{folder}/NOTIFY.{reference}.{duplex}.{letter_class}.{colour}.{crown}.{date}.pdf' def get_s3_file(bucket_name, file_location): @@ -81,34 +78,6 @@ def remove_transformed_dvla_file(job_id): return obj.delete() -def upload_letters_pdf(reference, crown, filedata): - now = datetime.utcnow() - - print_datetime = now - if now.time() > current_app.config.get('LETTER_PROCESSING_DEADLINE'): - print_datetime = now + timedelta(days=1) - - upload_file_name = LETTERS_PDF_FILE_LOCATION_STRUCTURE.format( - folder=print_datetime.date(), - reference=reference, - duplex="D", - letter_class="2", - colour="C", - crown="C" if crown else "N", - date=now.strftime('%Y%m%d%H%M%S') - ).upper() - - utils_s3upload( - filedata=filedata, - region=current_app.config['AWS_REGION'], - bucket_name=current_app.config['LETTERS_PDF_BUCKET_NAME'], - file_location=upload_file_name - ) - - current_app.logger.info("Uploading letters PDF {} to {}".format( - upload_file_name, current_app.config['LETTERS_PDF_BUCKET_NAME'])) - - def get_list_of_files_by_suffix(bucket_name, subfolder='', suffix='', last_modified=None): s3_client = client('s3', current_app.config['AWS_REGION']) paginator = s3_client.get_paginator('list_objects_v2') diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 8de14f28b..0404a3a4c 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -1,3 +1,4 @@ +from datetime import datetime, timedelta import math from flask import current_app @@ -7,6 +8,8 @@ from requests import ( ) from botocore.exceptions import ClientError as BotoClientError +from notifications_utils.s3 import s3upload + from app import notify_celery from app.aws import s3 from app.config import QueueNames, TaskNames @@ -19,6 +22,29 @@ from app.dao.notifications_dao import ( from app.models import NOTIFICATION_CREATED from app.statsd_decorators import statsd +LETTERS_PDF_FILE_LOCATION_STRUCTURE = \ + '{folder}/NOTIFY.{reference}.{duplex}.{letter_class}.{colour}.{crown}.{date}.pdf' + + +def get_letter_pdf_filename(reference, crown): + now = datetime.utcnow() + + print_datetime = now + if now.time() > current_app.config.get('LETTER_PROCESSING_DEADLINE'): + print_datetime = now + timedelta(days=1) + + upload_file_name = LETTERS_PDF_FILE_LOCATION_STRUCTURE.format( + folder=print_datetime.date(), + reference=reference, + duplex="D", + letter_class="2", + colour="C", + crown="C" if crown else "N", + date=now.strftime('%Y%m%d%H%M%S') + ).upper() + + return upload_file_name + @notify_celery.task(bind=True, name="create-letters-pdf", max_retries=15, default_retry_delay=300) @statsd(namespace="tasks") @@ -34,7 +60,19 @@ def create_letters_pdf(self, notification_id): ) current_app.logger.info("PDF Letter {} reference {} created at {}, {} bytes".format( notification.id, notification.reference, notification.created_at, len(pdf_data))) - s3.upload_letters_pdf(reference=notification.reference, crown=notification.service.crown, filedata=pdf_data) + + upload_file_name = get_letter_pdf_filename( + notification.reference, notification.service.crown) + + s3upload( + filedata=pdf_data, + region=current_app.config['AWS_REGION'], + bucket_name=current_app.config['LETTERS_PDF_BUCKET_NAME'], + file_location=upload_file_name + ) + + current_app.logger.info("Uploaded letters PDF {} to {}".format( + upload_file_name, current_app.config['LETTERS_PDF_BUCKET_NAME'])) notification.billable_units = billable_units dao_update_notification(notification) diff --git a/tests/app/aws/test_s3.py b/tests/app/aws/test_s3.py index 81ee10bfd..f122ebdc5 100644 --- a/tests/app/aws/test_s3.py +++ b/tests/app/aws/test_s3.py @@ -11,7 +11,6 @@ from app.aws.s3 import ( get_s3_file, filter_s3_bucket_objects_within_date_range, remove_transformed_dvla_file, - upload_letters_pdf, get_list_of_files_by_suffix, ) from tests.app.conftest import datetime_in_past @@ -144,38 +143,6 @@ def test_get_s3_bucket_objects_does_not_return_outside_of_date_range(notify_api, assert len(filtered_items) == 0 -@pytest.mark.parametrize('crown_flag,expected_crown_text', [ - (True, 'C'), - (False, 'N'), -]) -@freeze_time("2017-12-04 17:29:00") -def test_upload_letters_pdf_calls_utils_s3upload_with_correct_args( - notify_api, mocker, crown_flag, expected_crown_text): - s3_upload_mock = mocker.patch('app.aws.s3.utils_s3upload') - upload_letters_pdf(reference='foo', crown=crown_flag, filedata='some_data') - - s3_upload_mock.assert_called_with( - filedata='some_data', - region='eu-west-1', - bucket_name='test-letters-pdf', - file_location='2017-12-04/NOTIFY.FOO.D.2.C.{}.20171204172900.PDF'.format(expected_crown_text) - ) - - -@freeze_time("2017-12-04 17:31:00") -def test_upload_letters_pdf_puts_in_tomorrows_bucket_after_half_five(notify_api, mocker): - s3_upload_mock = mocker.patch('app.aws.s3.utils_s3upload') - upload_letters_pdf(reference='foo', crown=True, filedata='some_data') - - s3_upload_mock.assert_called_with( - filedata='some_data', - region='eu-west-1', - bucket_name='test-letters-pdf', - # in tomorrow's folder, but still has this evening's timestamp - file_location='2017-12-05/NOTIFY.FOO.D.2.C.C.20171204173100.PDF' - ) - - @freeze_time("2018-01-11 00:00:00") @pytest.mark.parametrize('suffix_str, days_before, returned_no', [ ('.ACK.txt', None, 1), diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 95942712c..7d58d278c 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -1,5 +1,8 @@ +from flask import current_app + from unittest.mock import call +from freezegun import freeze_time import pytest import requests_mock from botocore.exceptions import ClientError @@ -13,6 +16,7 @@ from app.celery.letters_pdf_tasks import ( collate_letter_pdfs_for_day, group_letters, letter_in_created_state, + get_letter_pdf_filename, ) from app.models import Notification, NOTIFICATION_SENDING @@ -23,6 +27,25 @@ def test_should_have_decorated_tasks_functions(): assert create_letters_pdf.__wrapped__.__name__ == 'create_letters_pdf' +@pytest.mark.parametrize('crown_flag,expected_crown_text', [ + (True, 'C'), + (False, 'N'), +]) +@freeze_time("2017-12-04 17:29:00") +def test_get_letter_pdf_filename_returns_correct_filename( + notify_api, mocker, crown_flag, expected_crown_text): + filename = get_letter_pdf_filename(reference='foo', crown=crown_flag) + + assert filename == '2017-12-04/NOTIFY.FOO.D.2.C.{}.20171204172900.PDF'.format(expected_crown_text) + + +@freeze_time("2017-12-04 17:31:00") +def test_get_letter_pdf_filename_returns_tomorrows_filename(notify_api, mocker): + filename = get_letter_pdf_filename(reference='foo', crown=True) + + assert filename == '2017-12-05/NOTIFY.FOO.D.2.C.C.20171204173100.PDF' + + @pytest.mark.parametrize('personalisation', [{'name': 'test'}, None]) def test_get_letters_pdf_calls_notifications_template_preview_service_correctly( notify_api, mocker, client, sample_letter_template, personalisation): @@ -79,22 +102,28 @@ def test_get_letters_pdf_calculates_billing_units( assert billable_units == expected_billable_units -def test_create_letters_pdf_calls_upload_letters_pdf(mocker, sample_letter_notification): +def test_create_letters_pdf_calls_s3upload(mocker, sample_letter_notification): mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=(b'\x00\x01', '1')) - mock_s3 = mocker.patch('app.celery.tasks.s3.upload_letters_pdf') + mock_s3 = mocker.patch('app.celery.letters_pdf_tasks.s3upload') create_letters_pdf(sample_letter_notification.id) - mock_s3.assert_called_with( + filename = get_letter_pdf_filename( reference=sample_letter_notification.reference, - crown=sample_letter_notification.service.crown, - filedata=b'\x00\x01' + crown=sample_letter_notification.service.crown + ) + + mock_s3.assert_called_with( + bucket_name=current_app.config['LETTERS_PDF_BUCKET_NAME'], + file_location=filename, + filedata=b'\x00\x01', + region=current_app.config['AWS_REGION'] ) def test_create_letters_pdf_sets_billable_units(mocker, sample_letter_notification): mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=(b'\x00\x01', 1)) - mocker.patch('app.celery.tasks.s3.upload_letters_pdf') + mocker.patch('app.celery.letters_pdf_tasks.s3upload') create_letters_pdf(sample_letter_notification.id) noti = Notification.query.filter(Notification.reference == sample_letter_notification.reference).one() @@ -118,7 +147,7 @@ def test_create_letters_pdf_handles_request_errors(mocker, sample_letter_notific def test_create_letters_pdf_handles_s3_errors(mocker, sample_letter_notification): mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=(b'\x00\x01', 1)) - mock_s3 = mocker.patch('app.celery.tasks.s3.upload_letters_pdf', side_effect=ClientError({}, 'operation_name')) + mock_s3 = mocker.patch('app.celery.letters_pdf_tasks.s3upload', side_effect=ClientError({}, 'operation_name')) mock_retry = mocker.patch('app.celery.letters_pdf_tasks.create_letters_pdf.retry') create_letters_pdf(sample_letter_notification.id) @@ -137,7 +166,6 @@ def test_create_letters_pdf_sets_technical_failure_max_retries(mocker, sample_le assert mock_get_letters_pdf.called assert mock_retry.called - assert mock_update_noti.called mock_update_noti.assert_called_once_with(sample_letter_notification.id, 'technical-failure') From 04beff7d05811ac8f791819d36b560a7a1cc2ac7 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 24 Jan 2018 15:57:24 +0000 Subject: [PATCH 3/6] Return sorted ACK files - also fixes unit test failure due to random order of filenames --- app/celery/scheduled_tasks.py | 2 +- tests/app/celery/test_scheduled_tasks.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 6c27f8fb5..b461a7299 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -526,7 +526,7 @@ def letter_raise_alert_if_no_ack_file_for_zip(): deskpro_message = "Letter ack file does not contains all zip files sent. " \ "Missing ack for zip files: {}, " \ "pdf bucket: {}, subfolder: {}, " \ - "ack bucket: {}".format(str(zip_file_set - ack_content_set), + "ack bucket: {}".format(str(sorted(zip_file_set - ack_content_set)), current_app.config['LETTERS_PDF_BUCKET_NAME'], datetime.utcnow().strftime('%Y-%m-%d') + '/zips_sent', current_app.config['DVLA_RESPONSE_BUCKET_NAME']) diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index e4cf756b9..73c4b0af2 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -1159,7 +1159,7 @@ def test_letter_raise_alert_if_ack_files_not_match_zip_list(mocker, notify_db): deskpro_message = "Letter ack file does not contains all zip files sent. " \ "Missing ack for zip files: {}, " \ "pdf bucket: {}, subfolder: {}, " \ - "ack bucket: {}".format(str(set(['NOTIFY.20180111175009.ZIP', 'NOTIFY.20180111175010.ZIP'])), + "ack bucket: {}".format(str(['NOTIFY.20180111175009.ZIP', 'NOTIFY.20180111175010.ZIP']), current_app.config['LETTERS_PDF_BUCKET_NAME'], datetime.utcnow().strftime('%Y-%m-%d') + '/zips_sent', current_app.config['DVLA_RESPONSE_BUCKET_NAME']) From c14452d8b105aed61e6b6bd35b0bd3452fc8bb92 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 26 Jan 2018 13:52:13 +0000 Subject: [PATCH 4/6] Add create fake letter response file - used by letter functional tests to ensure that we can create the response file necessary to trigger the update to a delivered state --- app/celery/research_mode_tasks.py | 33 ++++++++++ tests/app/celery/test_research_mode_tasks.py | 68 +++++++++++++++++++- 2 files changed, 100 insertions(+), 1 deletion(-) diff --git a/app/celery/research_mode_tasks.py b/app/celery/research_mode_tasks.py index 509ee1681..ea9099509 100644 --- a/app/celery/research_mode_tasks.py +++ b/app/celery/research_mode_tasks.py @@ -1,8 +1,12 @@ +from datetime import datetime import json from flask import current_app from requests import request, RequestException, HTTPError +from notifications_utils.s3 import s3upload + +from app import notify_celery from app.models import SMS_TYPE from app.config import QueueNames from app.celery.process_ses_receipts_tasks import process_ses_results @@ -115,6 +119,35 @@ def firetext_callback(notification_id, to): } +@notify_celery.task(bind=True, name="create-fake-letter-response-file", max_retries=5, default_retry_delay=300) +def create_fake_letter_response_file(self, reference): + now = datetime.utcnow() + dvla_response_data = '{}|Sent|1|Sorted'.format(reference) + upload_file_name = 'NOTIFY.{}.RSP.TXT'.format(now.strftime('%Y%m%d%H%M%S')) + + s3upload( + filedata=dvla_response_data, + region=current_app.config['AWS_REGION'], + bucket_name=current_app.config['DVLA_RESPONSE_BUCKET_NAME'], + file_location=upload_file_name + ) + current_app.logger.info("Fake DVLA response file {}, content [{}], uploaded to {}, created at {}".format( + upload_file_name, dvla_response_data, current_app.config['DVLA_RESPONSE_BUCKET_NAME'], now)) + + # on development we can't trigger SNS callbacks so we need to manually hit the DVLA callback endpoint + if current_app.config['NOTIFY_ENVIRONMENT'] == 'development': + make_request('letter', 'dvla', _fake_sns_s3_callback(upload_file_name), None) + + +def _fake_sns_s3_callback(filename): + message_contents = '{"Records":[{"s3":{"object":{"key":"%s"}}}]}' % (filename) # noqa + return json.dumps({ + "Type": "Notification", + "MessageId": "some-message-id", + "Message": message_contents + }) + + def ses_notification_callback(reference): ses_message_body = { 'delivery': { diff --git a/tests/app/celery/test_research_mode_tasks.py b/tests/app/celery/test_research_mode_tasks.py index 82a3e21df..819e9a399 100644 --- a/tests/app/celery/test_research_mode_tasks.py +++ b/tests/app/celery/test_research_mode_tasks.py @@ -1,8 +1,10 @@ import uuid from unittest.mock import ANY +from flask import current_app, json +from freezegun import freeze_time import pytest -from flask import json +import requests_mock from app.config import QueueNames from app.celery.research_mode_tasks import ( @@ -11,7 +13,9 @@ from app.celery.research_mode_tasks import ( mmg_callback, firetext_callback, ses_notification_callback, + create_fake_letter_response_file, ) +from tests.conftest import set_config_values def test_make_mmg_callback(notify_api, rmock): @@ -102,3 +106,65 @@ def test_failure_firetext_callback(phone_number): 'time': '2016-03-10 14:17:00', 'reference': '1234' } + + +@freeze_time("2018-01-25 14:00:00") +def test_create_fake_letter_response_file_uploads_response_file_s3( + notify_api, mocker): + mock_s3upload = mocker.patch('app.celery.research_mode_tasks.s3upload') + filename = 'NOTIFY.20180125140000.RSP.TXT' + + with requests_mock.Mocker() as request_mock: + request_mock.post( + 'http://localhost:6011/notifications/letter/dvla', + content=b'{}', + status_code=200 + ) + + create_fake_letter_response_file('random-ref') + + mock_s3upload.assert_called_once_with( + filedata='random-ref|Sent|1|Sorted', + region=current_app.config['AWS_REGION'], + bucket_name=current_app.config['DVLA_RESPONSE_BUCKET_NAME'], + file_location=filename + ) + + +@freeze_time("2018-01-25 14:00:00") +def test_create_fake_letter_response_file_calls_dvla_callback_on_development( + notify_api, mocker): + mocker.patch('app.celery.research_mode_tasks.s3upload') + filename = 'NOTIFY.20180125140000.RSP.TXT' + + with set_config_values(notify_api, { + 'NOTIFY_ENVIRONMENT': 'development' + }): + with requests_mock.Mocker() as request_mock: + request_mock.post( + 'http://localhost:6011/notifications/letter/dvla', + content=b'{}', + status_code=200 + ) + + create_fake_letter_response_file('random-ref') + + assert request_mock.last_request.json() == { + "Type": "Notification", + "MessageId": "some-message-id", + "Message": '{"Records":[{"s3":{"object":{"key":"' + filename + '"}}}]}' + } + + +@freeze_time("2018-01-25 14:00:00") +def test_create_fake_letter_response_file_does_not_call_dvla_callback_on_preview( + notify_api, mocker): + mocker.patch('app.celery.research_mode_tasks.s3upload') + + with set_config_values(notify_api, { + 'NOTIFY_ENVIRONMENT': 'preview' + }): + with requests_mock.Mocker() as request_mock: + create_fake_letter_response_file('random-ref') + + assert request_mock.last_request is None From 377afc3ed4b826b3456796623a3e3cf931ba325c Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 26 Jan 2018 14:05:09 +0000 Subject: [PATCH 5/6] Update letter processing for letter pdfs in research mode - call create fake response file task if in research mode only on preview and development environments to not impact response files on staging and live --- app/celery/tasks.py | 19 +++++++--- tests/app/celery/test_tasks.py | 69 +++++++++++++++++++++++++++------- 2 files changed, 69 insertions(+), 19 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 8908496cf..7063b791a 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -29,8 +29,7 @@ from app import ( encryption ) from app.aws import s3 -from app.celery import provider_tasks -from app.celery import letters_pdf_tasks +from app.celery import provider_tasks, letters_pdf_tasks, research_mode_tasks from app.config import QueueNames from app.dao.inbound_sms_dao import dao_get_inbound_sms_by_id from app.dao.jobs_dao import ( @@ -46,6 +45,7 @@ from app.dao.notifications_dao import ( dao_update_notifications_by_reference, dao_get_last_notification_added_for_job_id, dao_get_notification_by_reference, + update_notification_status_by_reference, ) from app.dao.provider_details_dao import get_current_provider from app.dao.service_inbound_api_dao import get_service_inbound_api_for_service @@ -62,6 +62,7 @@ from app.models import ( JOB_STATUS_SENT_TO_DVLA, JOB_STATUS_ERROR, KEY_TYPE_NORMAL, LETTER_TYPE, + NOTIFICATION_CREATED, NOTIFICATION_DELIVERED, NOTIFICATION_SENDING, NOTIFICATION_TEMPORARY_FAILURE, @@ -304,6 +305,9 @@ def save_letter( template = dao_get_template_by_id(notification['template'], version=notification['template_version']) try: + # if we don't want to actually send the letter, then start it off in SENDING so we don't pick it up + status = NOTIFICATION_CREATED if not service.research_mode else NOTIFICATION_SENDING + saved_notification = persist_notification( template_id=notification['template'], template_version=notification['template_version'], @@ -318,7 +322,8 @@ def save_letter( job_row_number=notification['row_number'], notification_id=notification_id, reference=create_random_identifier(), - reply_to_text=template.get_reply_to_text() + reply_to_text=template.get_reply_to_text(), + status=status ) if service.has_permission('letters_as_pdf'): @@ -327,11 +332,13 @@ def save_letter( [str(saved_notification.id)], queue=QueueNames.CREATE_LETTERS_PDF ) - else: - update_letter_notifications_to_sent_to_dvla.apply_async( - kwargs={'notification_references': [saved_notification.reference]}, + elif current_app.config['NOTIFY_ENVIRONMENT'] in ['preview', 'development']: + research_mode_tasks.create_fake_letter_response_file.apply_async( + (saved_notification.reference,), queue=QueueNames.RESEARCH_MODE ) + else: + update_notification_status_by_reference(saved_notification.reference, 'delivered') current_app.logger.info("Letter {} created at {}".format(saved_notification.id, saved_notification.created_at)) except SQLAlchemyError as e: diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index fd87850ff..f3967cff9 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -68,6 +68,7 @@ from tests.app.db import ( create_reply_to_email, create_service_with_defined_sms_sender, ) +from tests.conftest import set_config_values class AnyStringWith(str): @@ -1149,13 +1150,15 @@ def test_save_sms_uses_sms_sender_reply_to_text(mocker, notify_db_session): assert persisted_notification.reply_to_text == '447123123123' -def test_save_letter_calls_update_noti_to_sent_task_with_letters_as_pdf_permission_in_research_mode( - mocker, notify_db_session, sample_letter_job): +@pytest.mark.parametrize('env', ['staging', 'live']) +def test_save_letter_sets_delivered_letters_as_pdf_permission_in_research_mode_in_staging_live( + notify_api, mocker, notify_db_session, sample_letter_job, env): sample_letter_job.service.research_mode = True + sample_reference = "this-is-random-in-real-life" service_permissions_dao.dao_add_service_permission(sample_letter_job.service.id, 'letters_as_pdf') - mock_update_letter_noti_sent = mocker.patch( - 'app.celery.tasks.update_letter_notifications_to_sent_to_dvla.apply_async') - mocker.patch('app.celery.tasks.create_random_identifier', return_value="this-is-random-in-real-life") + mock_create_fake_letter_response_file = mocker.patch( + 'app.celery.research_mode_tasks.create_fake_letter_response_file.apply_async') + mocker.patch('app.celery.tasks.create_random_identifier', return_value=sample_reference) personalisation = { 'addressline1': 'Foo', @@ -1171,15 +1174,55 @@ def test_save_letter_calls_update_noti_to_sent_task_with_letters_as_pdf_permissi ) notification_id = uuid.uuid4() - save_letter( - sample_letter_job.service_id, - notification_id, - encryption.encrypt(notification_json), - ) + with set_config_values(notify_api, { + 'NOTIFY_ENVIRONMENT': env + }): + save_letter( + sample_letter_job.service_id, + notification_id, + encryption.encrypt(notification_json), + ) - assert mock_update_letter_noti_sent.called - mock_update_letter_noti_sent.assert_called_once_with( - kwargs={'notification_references': ['this-is-random-in-real-life']}, + notification = Notification.query.filter(Notification.id == notification_id).one() + assert notification.status == 'delivered' + assert not mock_create_fake_letter_response_file.called + + +@pytest.mark.parametrize('env', ['development', 'preview']) +def test_save_letter_calls_create_fake_response_for_letters_as_pdf_permission_in_research_mode_on_development_preview( + notify_api, mocker, notify_db_session, sample_letter_job, env): + sample_letter_job.service.research_mode = True + sample_reference = "this-is-random-in-real-life" + service_permissions_dao.dao_add_service_permission(sample_letter_job.service.id, 'letters_as_pdf') + mock_create_fake_letter_response_file = mocker.patch( + 'app.celery.research_mode_tasks.create_fake_letter_response_file.apply_async') + mocker.patch('app.celery.tasks.create_random_identifier', return_value=sample_reference) + + personalisation = { + 'addressline1': 'Foo', + 'addressline2': 'Bar', + 'postcode': 'Flob', + } + notification_json = _notification_json( + template=sample_letter_job.template, + to='Foo', + personalisation=personalisation, + job_id=sample_letter_job.id, + row_number=1 + ) + notification_id = uuid.uuid4() + + with set_config_values(notify_api, { + 'NOTIFY_ENVIRONMENT': env + }): + save_letter( + sample_letter_job.service_id, + notification_id, + encryption.encrypt(notification_json), + ) + + mock_create_fake_letter_response_file.assert_called_once_with( + (sample_reference,), queue=QueueNames.RESEARCH_MODE ) From 5ef9b426bb4179c0bcce211257c0d876e9cc9955 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 26 Jan 2018 14:08:01 +0000 Subject: [PATCH 6/6] Update letter notification processing for letter pdfs in research mode - calls create fake response file to allow functional tests to run and trigger update of status to delivered only on preview and development so that FT response files don't pollute staging and live buckets --- app/v2/notifications/post_notifications.py | 24 +++- .../test_post_letter_notifications.py | 124 +++++++++++++++++- 2 files changed, 141 insertions(+), 7 deletions(-) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index bcb2b4b7e..a8c9893b7 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -6,6 +6,7 @@ from notifications_utils.recipients import try_validate_and_format_phone_number from app import api_user, authenticated_service from app.config import QueueNames +from app.dao.notifications_dao import update_notification_status_by_reference from app.models import ( SMS_TYPE, EMAIL_TYPE, @@ -14,9 +15,11 @@ from app.models import ( KEY_TYPE_TEST, KEY_TYPE_TEAM, NOTIFICATION_CREATED, - NOTIFICATION_SENDING + NOTIFICATION_SENDING, + NOTIFICATION_DELIVERED ) from app.celery.letters_pdf_tasks import create_letters_pdf +from app.celery.research_mode_tasks import create_fake_letter_response_file from app.celery.tasks import update_letter_notifications_to_sent_to_dvla from app.notifications.process_notifications import ( persist_notification, @@ -171,7 +174,7 @@ def process_letter_notification(*, letter_data, api_key, template, reply_to_text if api_key.key_type == KEY_TYPE_TEAM: raise BadRequestError(message='Cannot send letters with a team api key', status_code=403) - if api_key.service.restricted and api_key.key_type != KEY_TYPE_TEST: + if not api_key.service.research_mode and api_key.service.restricted and api_key.key_type != KEY_TYPE_TEST: raise BadRequestError(message='Cannot send letters when service is in trial mode', status_code=403) should_send = not (api_key.service.research_mode or api_key.key_type == KEY_TYPE_TEST) @@ -191,10 +194,19 @@ def process_letter_notification(*, letter_data, api_key, template, reply_to_text ) if api_key.service.has_permission('letters_as_pdf'): - create_letters_pdf.apply_async( - [str(notification.id)], - queue=QueueNames.CREATE_LETTERS_PDF - ) + if should_send: + create_letters_pdf.apply_async( + [str(notification.id)], + queue=QueueNames.CREATE_LETTERS_PDF + ) + elif (api_key.service.research_mode and + current_app.config['NOTIFY_ENVIRONMENT'] in ['preview', 'development']): + create_fake_letter_response_file.apply_async( + (notification.reference,), + queue=QueueNames.RESEARCH_MODE + ) + else: + update_notification_status_by_reference(notification.reference, NOTIFICATION_DELIVERED) return notification diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py index cda66f1c9..82728fb33 100644 --- a/tests/app/v2/notifications/test_post_letter_notifications.py +++ b/tests/app/v2/notifications/test_post_letter_notifications.py @@ -13,7 +13,7 @@ from app.models import KEY_TYPE_TEAM from app.models import KEY_TYPE_TEST from app.models import LETTER_TYPE from app.models import Notification -from app.models import NOTIFICATION_SENDING +from app.models import NOTIFICATION_SENDING, NOTIFICATION_DELIVERED from app.models import SMS_TYPE from app.schema_validation import validate from app.v2.errors import RateLimitError @@ -21,6 +21,7 @@ from app.v2.notifications.notification_schemas import post_letter_response from tests import create_authorization_header from tests.app.db import create_service, create_template, create_letter_contact +from tests.conftest import set_config_values test_address = { 'address_line_1': 'test 1', @@ -105,6 +106,127 @@ def test_post_letter_notification_for_letters_as_pdf_calls_celery_task(client, s fake_task.assert_called_once_with([str(notification.id)], queue=QueueNames.CREATE_LETTERS_PDF) +@pytest.mark.parametrize('env', [ + 'development', + 'preview', +]) +def test_post_letter_notification_for_letters_as_pdf_calls_create_fake_response_in_research_and_test_key_correct_env( + notify_api, client, sample_letter_template, mocker, env): + service_permissions_dao.dao_add_service_permission(sample_letter_template.service.id, 'letters_as_pdf') + sample_letter_template.service.research_mode = True + + data = { + 'template_id': str(sample_letter_template.id), + 'personalisation': { + 'address_line_1': 'Her Royal Highness Queen Elizabeth II', + 'address_line_2': 'Buckingham Palace', + 'address_line_3': 'London', + 'postcode': 'SW1 1AA', + 'name': 'Lizzie' + }, + 'reference': 'foo' + } + + fake_update_letter_noti_to_sent = mocker.patch( + 'app.celery.tasks.update_letter_notifications_to_sent_to_dvla.apply_async') + fake_create_letter_task = mocker.patch('app.celery.letters_pdf_tasks.create_letters_pdf.apply_async') + fake_create_dvla_response_task = mocker.patch( + 'app.celery.research_mode_tasks.create_fake_letter_response_file.apply_async') + + with set_config_values(notify_api, { + 'NOTIFY_ENVIRONMENT': env + }): + letter_request(client, data, service_id=sample_letter_template.service_id, key_type=KEY_TYPE_TEST) + + notification = Notification.query.one() + + assert fake_update_letter_noti_to_sent.called + assert not fake_create_letter_task.called + fake_create_dvla_response_task.assert_called_once_with((notification.reference,), queue=QueueNames.RESEARCH_MODE) + + +@pytest.mark.parametrize('env', [ + 'staging', + 'live', +]) +def test_post_letter_noti_for_letters_as_pdf_sets_status_delivered_in_research_and_test_key_incorrect_env( + notify_api, client, sample_letter_template, mocker, env): + service_permissions_dao.dao_add_service_permission(sample_letter_template.service.id, 'letters_as_pdf') + sample_letter_template.service.research_mode = True + + data = { + 'template_id': str(sample_letter_template.id), + 'personalisation': { + 'address_line_1': 'Her Royal Highness Queen Elizabeth II', + 'address_line_2': 'Buckingham Palace', + 'address_line_3': 'London', + 'postcode': 'SW1 1AA', + 'name': 'Lizzie' + }, + 'reference': 'foo' + } + + fake_update_letter_noti_to_sent = mocker.patch( + 'app.celery.tasks.update_letter_notifications_to_sent_to_dvla.apply_async') + fake_create_letter_task = mocker.patch('app.celery.letters_pdf_tasks.create_letters_pdf.apply_async') + fake_create_dvla_response_task = mocker.patch( + 'app.celery.research_mode_tasks.create_fake_letter_response_file.apply_async') + + with set_config_values(notify_api, { + 'NOTIFY_ENVIRONMENT': env + }): + letter_request(client, data, service_id=sample_letter_template.service_id, key_type=KEY_TYPE_TEST) + + notification = Notification.query.one() + + assert fake_update_letter_noti_to_sent.called + assert not fake_create_letter_task.called + assert not fake_create_dvla_response_task.called + assert notification.status == NOTIFICATION_DELIVERED + + +@pytest.mark.parametrize('env', [ + 'development', + 'preview', + 'staging', + 'live', +]) +def test_post_letter_noti_for_letters_as_pdf_sets_status_to_delivered_using_test_key_and_not_research_all_env( + notify_api, client, sample_letter_template, mocker, env): + service_permissions_dao.dao_add_service_permission(sample_letter_template.service.id, 'letters_as_pdf') + sample_letter_template.service.research_mode = False + + data = { + 'template_id': str(sample_letter_template.id), + 'personalisation': { + 'address_line_1': 'Her Royal Highness Queen Elizabeth II', + 'address_line_2': 'Buckingham Palace', + 'address_line_3': 'London', + 'postcode': 'SW1 1AA', + 'name': 'Lizzie' + }, + 'reference': 'foo' + } + + fake_update_letter_noti_to_sent = mocker.patch( + 'app.celery.tasks.update_letter_notifications_to_sent_to_dvla.apply_async') + fake_create_letter_task = mocker.patch('app.celery.letters_pdf_tasks.create_letters_pdf.apply_async') + fake_create_dvla_response_task = mocker.patch( + 'app.celery.research_mode_tasks.create_fake_letter_response_file.apply_async') + + with set_config_values(notify_api, { + 'NOTIFY_ENVIRONMENT': env + }): + letter_request(client, data, service_id=sample_letter_template.service_id, key_type=KEY_TYPE_TEST) + + notification = Notification.query.one() + + assert fake_update_letter_noti_to_sent.called + assert not fake_create_letter_task.called + assert not fake_create_dvla_response_task.called + assert notification.status == NOTIFICATION_DELIVERED + + def test_post_letter_notification_returns_400_and_missing_template( client, sample_service_full_permissions