From ed5e73d5484943365b8d535a0adf9987f8861b7b Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 29 Jul 2020 13:42:20 +0100 Subject: [PATCH 1/3] Set postage for templated letters when the address is not from the united-kingdom. If the address is from the united-kingdom use the postage from the template. --- .../process_letter_notifications.py | 5 +++- app/v2/notifications/post_notifications.py | 9 +++++-- .../test_post_letter_notifications.py | 24 +++++++++++++++++++ 3 files changed, 35 insertions(+), 3 deletions(-) diff --git a/app/notifications/process_letter_notifications.py b/app/notifications/process_letter_notifications.py index 2af7f5f7a..7f9428786 100644 --- a/app/notifications/process_letter_notifications.py +++ b/app/notifications/process_letter_notifications.py @@ -14,6 +14,7 @@ def create_letter_notification( reply_to_text=None, billable_units=None, updated_at=None, + postage=None ): notification = persist_notification( template_id=template.id, @@ -33,7 +34,9 @@ def create_letter_notification( status=status, reply_to_text=reply_to_text, billable_units=billable_units, - postage=letter_data.get('postage'), + # letter_data.get('postage') is only set for precompiled letters + # letters from a template will pass in 'europe' or 'rest-of-world' if None then use postage from template + postage=postage or letter_data.get('postage'), updated_at=updated_at ) return notification diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 20bb50269..b3ff6e016 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -339,7 +339,7 @@ def process_letter_notification( template=template, reply_to_text=reply_to_text) - validate_address(service, letter_data) + postage = validate_address(service, letter_data) test_key = api_key.key_type == KEY_TYPE_TEST @@ -362,7 +362,8 @@ def process_letter_notification( api_key=api_key, status=status, reply_to_text=reply_to_text, - updated_at=updated_at + updated_at=updated_at, + postage=postage ) get_pdf_for_templated_letter.apply_async( @@ -413,6 +414,10 @@ def validate_address(service, letter_data): raise ValidationError( message='Address lines must not start with any of the following characters: @ ( ) = [ ] ” \\ / ,' ) + if address.postage == 'united-kingdom': + return None # use postage from template + else: + return address.postage def process_precompiled_letter_notifications(*, letter_data, api_key, service, template, reply_to_text): diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py index e3ea139a2..00deaa54d 100644 --- a/tests/app/v2/notifications/test_post_letter_notifications.py +++ b/tests/app/v2/notifications/test_post_letter_notifications.py @@ -174,6 +174,30 @@ def test_post_letter_notification_stores_country( 'Kronprinzenpalais\n' 'Germany' ) + assert notification.postage == 'europe' + + +def test_post_letter_notification_international_sets_rest_of_world( + client, notify_db_session, mocker + ): + service = create_service(service_permissions=[LETTER_TYPE, INTERNATIONAL_LETTERS]) + template = create_template(service, template_type="letter") + mocker.patch('app.celery.tasks.letters_pdf_tasks.get_pdf_for_templated_letter.apply_async') + data = { + 'template_id': str(template.id), + 'personalisation': { + 'address_line_1': 'Prince Harry', + 'address_line_2': 'Toronto', + 'address_line_5': 'Canada', + } + } + + resp_json = letter_request(client, data, service_id=service.id) + + assert validate(resp_json, post_letter_response) == resp_json + notification = Notification.query.one() + + assert notification.postage == 'rest-of-world' @pytest.mark.parametrize('permissions, personalisation, expected_error', ( From 10fe7d9fe808366d6908070eee6bdb5242eb6401 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 29 Jul 2020 14:52:18 +0100 Subject: [PATCH 2/3] Add postage for send-one-off letters. The postage is set to europe or rest-of-world for international letters, otherwise the template postage is used. Also set international for letters. --- app/celery/letters_pdf_tasks.py | 15 ++++-- app/notifications/process_notifications.py | 3 +- app/notifications/validators.py | 36 +++++++++++++- app/service/send_notification.py | 11 +++-- app/v2/notifications/post_notifications.py | 46 ++++-------------- tests/app/celery/test_letters_pdf_tasks.py | 48 +++++++++++++++++++ .../rest/test_send_notification.py | 27 +++++++++++ .../test_process_notification.py | 20 ++++++++ tests/app/notifications/test_validators.py | 24 +++++++++- .../service/test_send_one_off_notification.py | 19 +++++++- .../test_post_letter_notifications.py | 33 ++++++------- 11 files changed, 216 insertions(+), 66 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 61982e0b4..c269ec5a0 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -4,6 +4,7 @@ from base64 import urlsafe_b64encode from botocore.exceptions import ClientError as BotoClientError from flask import current_app +from notifications_utils.postal_address import PostalAddress from notifications_utils.statsd_decorators import statsd from notifications_utils.letter_timings import LETTER_PROCESSING_DEADLINE @@ -43,8 +44,8 @@ from app.models import ( NOTIFICATION_VALIDATION_FAILED, NOTIFICATION_VIRUS_SCAN_FAILED, POSTAGE_TYPES, - RESOLVE_POSTAGE_FOR_FILE_NAME -) + RESOLVE_POSTAGE_FOR_FILE_NAME, + INTERNATIONAL_POSTAGE_TYPES) from app.cronitor import cronitor @@ -393,8 +394,16 @@ def process_virus_scan_error(filename): def update_letter_pdf_status(reference, status, billable_units, recipient_address=None): - + postage = None + if recipient_address: + # fix allow_international_letters + postage = PostalAddress(raw_address=recipient_address.replace(',', '\n'), + allow_international_letters=True + ).postage + postage = postage if postage in INTERNATIONAL_POSTAGE_TYPES else None update_dict = {'status': status, 'billable_units': billable_units, 'updated_at': datetime.utcnow()} + if postage: + update_dict.update({'postage': postage, 'international': True}) if recipient_address: update_dict['to'] = recipient_address return dao_update_notifications_by_reference( diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index f6b14df18..5dedd4908 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -27,7 +27,7 @@ from app.models import ( LETTER_TYPE, NOTIFICATION_CREATED, Notification, -) + INTERNATIONAL_POSTAGE_TYPES) from app.dao.notifications_dao import ( dao_create_notification, dao_delete_notifications_by_id, @@ -148,6 +148,7 @@ def persist_notification( notification.normalised_to = format_email_address(notification.to) elif notification_type == LETTER_TYPE: notification.postage = postage or template_postage + notification.international = True if postage in INTERNATIONAL_POSTAGE_TYPES else False notification.normalised_to = ''.join(notification.to.split()).lower() # if simulated create a Notification model to return but do not persist the Notification to the dB diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 93992c580..e16440015 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -1,3 +1,4 @@ +from notifications_utils.postal_address import PostalAddress from sqlalchemy.orm.exc import NoResultFound from flask import current_app from notifications_utils import SMS_CHAR_COUNT_LIMIT @@ -14,9 +15,9 @@ from app.models import ( INTERNATIONAL_SMS_TYPE, SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, KEY_TYPE_TEST, KEY_TYPE_TEAM, ServicePermission, -) + INTERNATIONAL_LETTERS) from app.service.utils import service_allowed_to_send_to -from app.v2.errors import TooManyRequestsError, BadRequestError, RateLimitError +from app.v2.errors import TooManyRequestsError, BadRequestError, RateLimitError, ValidationError from app import redis_store from app.notifications.process_notifications import create_content_for_notification from app.utils import get_public_notify_type_text @@ -214,3 +215,34 @@ def check_service_letter_contact_id(service_id, letter_contact_id, notification_ message = 'letter_contact_id {} does not exist in database for service id {}'\ .format(letter_contact_id, service_id) raise BadRequestError(message=message) + + +def validate_address(service, letter_data): + address = PostalAddress.from_personalisation( + letter_data, + allow_international_letters=(INTERNATIONAL_LETTERS in str(service.permissions)), + ) + if not address.has_enough_lines: + raise ValidationError( + message=f'Address must be at least {PostalAddress.MIN_LINES} lines' + ) + if address.has_too_many_lines: + raise ValidationError( + message=f'Address must be no more than {PostalAddress.MAX_LINES} lines' + ) + if not address.has_valid_last_line: + if address.allow_international_letters: + raise ValidationError( + message=f'Last line of address must be a real UK postcode or another country' + ) + raise ValidationError( + message='Must be a real UK postcode' + ) + if address.has_invalid_characters: + raise ValidationError( + message='Address lines must not start with any of the following characters: @ ( ) = [ ] ” \\ / ,' + ) + if address.postage == 'united-kingdom': + return None # use postage from template + else: + return address.postage diff --git a/app/service/send_notification.py b/app/service/send_notification.py index 10ed42ab7..159d3bdbb 100644 --- a/app/service/send_notification.py +++ b/app/service/send_notification.py @@ -13,8 +13,8 @@ from app.notifications.validators import ( check_service_has_permission, check_service_over_daily_message_limit, validate_and_format_recipient, - validate_template -) + validate_template, + validate_address) from app.notifications.process_notifications import ( persist_notification, send_notification_to_queue @@ -75,7 +75,11 @@ def send_one_off_notification(service_id, post_data): notification_type=template.template_type, allow_whitelisted_recipients=False, ) - + postage = None + if template.template_type == LETTER_TYPE: + # Validate address and set postage to europe|rest-of-world if international letter, + # otherwise persist_notification with use template postage + postage = validate_address(service, personalisation) validate_created_by(service, post_data['created_by']) sender_id = post_data.get('sender_id', None) @@ -98,6 +102,7 @@ def send_one_off_notification(service_id, post_data): created_by_id=post_data['created_by'], reply_to_text=reply_to, reference=create_one_off_reference(template.template_type), + postage=postage ) queue_name = QueueNames.PRIORITY if template.process_type == PRIORITY else None diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index b3ff6e016..139702b45 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -5,7 +5,6 @@ from datetime import datetime from boto.exception import SQSError from flask import request, jsonify, current_app, abort -from notifications_utils.postal_address import PostalAddress from notifications_utils.recipients import try_validate_and_format_phone_number from gds_metrics import Histogram @@ -36,7 +35,6 @@ from app.models import ( NOTIFICATION_SENDING, NOTIFICATION_DELIVERED, NOTIFICATION_PENDING_VIRUS_CHECK, - INTERNATIONAL_LETTERS, Notification) from app.notifications.process_letter_notifications import ( create_letter_notification @@ -51,15 +49,18 @@ from app.notifications.validators import ( check_service_email_reply_to_id, check_service_has_permission, check_service_sms_sender_id, + validate_address, validate_and_format_recipient, validate_template, ) from app.schema_validation import validate -from app.v2.errors import BadRequestError, ValidationError -from app.v2.notifications import v2_notification_blueprint +from app.v2.errors import BadRequestError from app.v2.notifications.create_response import ( - create_post_sms_response_from_notification, create_post_email_response_from_notification, - create_post_letter_response_from_notification) + create_post_email_response_from_notification, + create_post_sms_response_from_notification, + create_post_letter_response_from_notification +) +from app.v2.notifications import v2_notification_blueprint from app.v2.notifications.notification_schemas import ( post_sms_request, post_email_request, @@ -339,7 +340,7 @@ def process_letter_notification( template=template, reply_to_text=reply_to_text) - postage = validate_address(service, letter_data) + postage = validate_address(service, letter_data['personalisation']) test_key = api_key.key_type == KEY_TYPE_TEST @@ -389,37 +390,6 @@ def process_letter_notification( return resp -def validate_address(service, letter_data): - address = PostalAddress.from_personalisation( - letter_data['personalisation'], - allow_international_letters=(INTERNATIONAL_LETTERS in service.permissions), - ) - if not address.has_enough_lines: - raise ValidationError( - message=f'Address must be at least {PostalAddress.MIN_LINES} lines' - ) - if address.has_too_many_lines: - raise ValidationError( - message=f'Address must be no more than {PostalAddress.MAX_LINES} lines' - ) - if not address.has_valid_last_line: - if address.allow_international_letters: - raise ValidationError( - message=f'Last line of address must be a real UK postcode or another country' - ) - raise ValidationError( - message='Must be a real UK postcode' - ) - if address.has_invalid_characters: - raise ValidationError( - message='Address lines must not start with any of the following characters: @ ( ) = [ ] ” \\ / ,' - ) - if address.postage == 'united-kingdom': - return None # use postage from template - else: - return address.postage - - def process_precompiled_letter_notifications(*, letter_data, api_key, service, template, reply_to_text): try: status = NOTIFICATION_PENDING_VIRUS_CHECK diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 4b331f279..02e48980d 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -646,6 +646,54 @@ def test_process_sanitised_letter_with_valid_letter( assert file_contents == 'sanitised_pdf_content' +@mock_s3 +@pytest.mark.parametrize('address, expected_postage, expected_international', + [('Lady Lou, 123 Main Street, SW1 1AA', 'second', False), + ('Lady Lou, 123 Main Street, France', 'europe', True), + ('Lady Lou, 123 Main Street, New Zealand', 'rest-of-world', True), + ]) +def test_process_sanitised_letter_sets_postage_international( + sample_letter_notification, + expected_postage, + expected_international, + address +): + filename = 'NOTIFY.{}'.format(sample_letter_notification.reference) + + scan_bucket_name = current_app.config['LETTERS_SCAN_BUCKET_NAME'] + template_preview_bucket_name = current_app.config['LETTER_SANITISE_BUCKET_NAME'] + destination_bucket_name = current_app.config['LETTERS_PDF_BUCKET_NAME'] + conn = boto3.resource('s3', region_name='eu-west-1') + conn.create_bucket(Bucket=scan_bucket_name) + conn.create_bucket(Bucket=template_preview_bucket_name) + conn.create_bucket(Bucket=destination_bucket_name) + + s3 = boto3.client('s3', region_name='eu-west-1') + s3.put_object(Bucket=scan_bucket_name, Key=filename, Body=b'original_pdf_content') + s3.put_object(Bucket=template_preview_bucket_name, Key=filename, Body=b'sanitised_pdf_content') + + sample_letter_notification.status = NOTIFICATION_PENDING_VIRUS_CHECK + sample_letter_notification.billable_units = 1 + sample_letter_notification.created_at = datetime(2018, 7, 1, 12) + + encrypted_data = encryption.encrypt({ + 'page_count': 2, + 'message': None, + 'invalid_pages': None, + 'validation_status': 'passed', + 'filename': filename, + 'notification_id': str(sample_letter_notification.id), + 'address': address + }) + process_sanitised_letter(encrypted_data) + + assert sample_letter_notification.status == 'created' + assert sample_letter_notification.billable_units == 1 + assert sample_letter_notification.to == address + assert sample_letter_notification.postage == expected_postage + assert sample_letter_notification.international == expected_international + + @mock_s3 @pytest.mark.parametrize('key_type', [KEY_TYPE_NORMAL, KEY_TYPE_TEST]) def test_process_sanitised_letter_with_invalid_letter(sample_letter_notification, key_type): diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index 92c31f555..4426f0b83 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -18,6 +18,7 @@ from app.dao.services_dao import dao_update_service from app.dao.api_key_dao import save_model_api_key from app.errors import InvalidRequest from app.models import Template +from app.service.send_notification import send_one_off_notification from app.v2.errors import RateLimitError from tests import create_authorization_header @@ -1214,3 +1215,29 @@ def test_post_notification_should_set_reply_to_text(client, sample_service, mock notifications = Notification.query.all() assert len(notifications) == 1 assert notifications[0].reply_to_text == expected_reply_to + + +@pytest.mark.parametrize('last_line_of_address, expected_postage, expected_international', + [('France', 'europe', True), + ('Canada', 'rest-of-world', True), + ('SW1 1AA', 'second', False)]) +def test_send_notification_should_send_international_letters( + sample_letter_template, mocker, last_line_of_address, expected_postage, expected_international +): + deliver_mock = mocker.patch('app.celery.tasks.letters_pdf_tasks.get_pdf_for_templated_letter.apply_async') + data = { + 'template_id': sample_letter_template.id, + 'personalisation': { + 'address_line_1': 'Jane', + 'address_line_2': 'Rue Vert', + 'address_line_3': last_line_of_address + }, + 'to': 'Jane', + 'created_by': sample_letter_template.service.created_by_id + } + + notification_id = send_one_off_notification(sample_letter_template.service_id, data) + assert deliver_mock.called + notification = Notification.query.get(notification_id['id']) + assert notification.postage == expected_postage + assert notification.international == expected_international diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 0fe703b6a..b300b6fdc 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -541,3 +541,23 @@ def test_persist_notification_with_billable_units_stores_correct_info( persisted_notification = Notification.query.all()[0] assert persisted_notification.billable_units == 3 + + +@pytest.mark.parametrize('postage', ['europe', 'rest-of-world']) +def test_persist_notification_for_international_letter(sample_letter_template, postage): + notification = persist_notification( + template_id=sample_letter_template.id, + template_version=sample_letter_template.version, + recipient="123 Main Street", + service=sample_letter_template.service, + personalisation=None, + notification_type=sample_letter_template.template_type, + api_key_id=None, + key_type="normal", + billable_units=3, + postage=postage, + template_postage='second' + ) + persisted_notification = Notification.query.get(notification.id) + assert persisted_notification.postage == postage + assert persisted_notification.international diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index 4e6748917..403390658 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -5,7 +5,12 @@ from notifications_utils import SMS_CHAR_COUNT_LIMIT import app from app.dao import templates_dao -from app.models import SMS_TYPE, EMAIL_TYPE, LETTER_TYPE +from app.models import ( + EMAIL_TYPE, + INTERNATIONAL_LETTERS, + LETTER_TYPE, + SMS_TYPE, +) from app.notifications.process_notifications import create_content_for_notification from app.notifications.validators import ( check_content_char_count, @@ -20,6 +25,7 @@ from app.notifications.validators import ( check_service_letter_contact_id, check_reply_to, service_can_send_to_recipient, + validate_address, validate_and_format_recipient, validate_template, ) @@ -619,3 +625,19 @@ def test_check_if_service_can_send_files_by_email_passes_if_contact_link_set(sam service_contact_link=sample_service.contact_link, service_id=sample_service.id ) + + +@pytest.mark.parametrize('key, address_line_3, expected_postage', + [('address_line_3', 'SW1 1AA', None), + ('address_line_5', 'CANADA', 'rest-of-world'), + ('address_line_3', 'GERMANY', 'europe') + ]) +def test_validate_address(notify_db_session, key, address_line_3, expected_postage): + service = create_service(service_permissions=[LETTER_TYPE, INTERNATIONAL_LETTERS]) + data = { + 'address_line_1': 'Prince Harry', + 'address_line_2': 'Toronto', + key: address_line_3, + } + postage = validate_address(service, data) + assert postage == expected_postage diff --git a/tests/app/service/test_send_one_off_notification.py b/tests/app/service/test_send_one_off_notification.py index 0b8e74cd4..6588001eb 100644 --- a/tests/app/service/test_send_one_off_notification.py +++ b/tests/app/service/test_send_one_off_notification.py @@ -100,6 +100,7 @@ def test_send_one_off_notification_calls_persist_correctly_for_sms( created_by_id=str(service.created_by_id), reply_to_text='testing', reference=None, + postage=None ) @@ -161,6 +162,7 @@ def test_send_one_off_notification_calls_persist_correctly_for_email( created_by_id=str(service.created_by_id), reply_to_text=None, reference=None, + postage=None ) @@ -188,8 +190,8 @@ def test_send_one_off_notification_calls_persist_correctly_for_letter( 'to': 'First Last', 'personalisation': { 'name': 'foo', - 'address line 1': 'First Last', - 'address line 2': '1 Example Street', + 'address_line_1': 'First Last', + 'address_line_2': '1 Example Street', 'postcode': 'SW1A 1AA', }, 'created_by': str(service.created_by_id) @@ -210,6 +212,7 @@ def test_send_one_off_notification_calls_persist_correctly_for_letter( created_by_id=str(service.created_by_id), reply_to_text=None, reference='this-is-random-in-real-life', + postage=None ) @@ -362,6 +365,12 @@ def test_send_one_off_letter_notification_should_use_template_reply_to_text(samp data = { 'to': 'user@example.com', 'template_id': str(sample_letter_template.id), + 'personalisation': { + 'name': 'foo', + 'address_line_1': 'First Last', + 'address_line_2': '1 Example Street', + 'address_line_3': 'SW1A 1AA', + }, 'created_by': str(sample_letter_template.service.created_by_id) } @@ -383,6 +392,12 @@ def test_send_one_off_letter_should_not_make_pdf_in_research_mode(sample_letter_ data = { 'to': 'A. Name', 'template_id': str(sample_letter_template.id), + 'personalisation': { + 'name': 'foo', + 'address_line_1': 'First Last', + 'address_line_2': '1 Example Street', + 'address_line_3': 'SW1A 1AA', + }, 'created_by': str(sample_letter_template.service.created_by_id) } diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py index 00deaa54d..297292758 100644 --- a/tests/app/v2/notifications/test_post_letter_notifications.py +++ b/tests/app/v2/notifications/test_post_letter_notifications.py @@ -175,29 +175,30 @@ def test_post_letter_notification_stores_country( 'Germany' ) assert notification.postage == 'europe' + assert notification.international def test_post_letter_notification_international_sets_rest_of_world( - client, notify_db_session, mocker - ): - service = create_service(service_permissions=[LETTER_TYPE, INTERNATIONAL_LETTERS]) - template = create_template(service, template_type="letter") - mocker.patch('app.celery.tasks.letters_pdf_tasks.get_pdf_for_templated_letter.apply_async') - data = { - 'template_id': str(template.id), - 'personalisation': { - 'address_line_1': 'Prince Harry', - 'address_line_2': 'Toronto', - 'address_line_5': 'Canada', - } + client, notify_db_session, mocker +): + service = create_service(service_permissions=[LETTER_TYPE, INTERNATIONAL_LETTERS]) + template = create_template(service, template_type="letter") + mocker.patch('app.celery.tasks.letters_pdf_tasks.get_pdf_for_templated_letter.apply_async') + data = { + 'template_id': str(template.id), + 'personalisation': { + 'address_line_1': 'Prince Harry', + 'address_line_2': 'Toronto', + 'address_line_5': 'Canada', } + } - resp_json = letter_request(client, data, service_id=service.id) + resp_json = letter_request(client, data, service_id=service.id) - assert validate(resp_json, post_letter_response) == resp_json - notification = Notification.query.one() + assert validate(resp_json, post_letter_response) == resp_json + notification = Notification.query.one() - assert notification.postage == 'rest-of-world' + assert notification.postage == 'rest-of-world' @pytest.mark.parametrize('permissions, personalisation, expected_error', ( From 4a9f9e4b17204177d1ad58d7f9f74cc62b5e6de0 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 4 Aug 2020 15:41:42 +0100 Subject: [PATCH 3/3] Remove the template_postage parameter for persist_notification It was confusing to have 2 differnt postage parameters. --- app/celery/tasks.py | 2 +- app/notifications/process_letter_notifications.py | 5 ++--- app/notifications/process_notifications.py | 5 ++--- app/notifications/rest.py | 2 +- app/service/send_notification.py | 6 +++--- tests/app/notifications/test_process_notification.py | 6 +----- tests/app/service/test_send_one_off_notification.py | 5 +---- 7 files changed, 11 insertions(+), 20 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 5444eb03e..6bad3643a 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -354,7 +354,7 @@ def save_letter( saved_notification = persist_notification( template_id=notification['template'], template_version=notification['template_version'], - template_postage=template.postage, + postage=template.postage, recipient=recipient, service=service, personalisation=notification['personalisation'], diff --git a/app/notifications/process_letter_notifications.py b/app/notifications/process_letter_notifications.py index 7f9428786..3cce47433 100644 --- a/app/notifications/process_letter_notifications.py +++ b/app/notifications/process_letter_notifications.py @@ -19,7 +19,6 @@ def create_letter_notification( notification = persist_notification( template_id=template.id, template_version=template.version, - template_postage=template.postage, # we only accept addresses_with_underscores from the API (from CSV we also accept dashes, spaces etc) recipient=PostalAddress.from_personalisation(letter_data['personalisation']).normalised, service=service, @@ -34,9 +33,9 @@ def create_letter_notification( status=status, reply_to_text=reply_to_text, billable_units=billable_units, - # letter_data.get('postage') is only set for precompiled letters + # letter_data.get('postage') is only set for precompiled letters (if international it is set after sanitise) # letters from a template will pass in 'europe' or 'rest-of-world' if None then use postage from template - postage=postage or letter_data.get('postage'), + postage=postage or letter_data.get('postage') or template.postage, updated_at=updated_at ) return notification diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 5dedd4908..2b649997a 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -107,7 +107,6 @@ def persist_notification( reply_to_text=None, billable_units=None, postage=None, - template_postage=None, document_download_count=None, updated_at=None ): @@ -147,8 +146,8 @@ def persist_notification( elif notification_type == EMAIL_TYPE: notification.normalised_to = format_email_address(notification.to) elif notification_type == LETTER_TYPE: - notification.postage = postage or template_postage - notification.international = True if postage in INTERNATIONAL_POSTAGE_TYPES else False + notification.postage = postage + notification.international = postage in INTERNATIONAL_POSTAGE_TYPES notification.normalised_to = ''.join(notification.to.split()).lower() # if simulated create a Notification model to return but do not persist the Notification to the dB diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 81e70f6e2..e6425b794 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -120,7 +120,7 @@ def send_notification(notification_type): simulated = simulated_recipient(notification_form['to'], notification_type) notification_model = persist_notification(template_id=template.id, template_version=template.version, - template_postage=template.postage, + postage=template.postage, recipient=request.get_json()['to'], service=authenticated_service, personalisation=notification_form.get('personalisation', None), diff --git a/app/service/send_notification.py b/app/service/send_notification.py index 159d3bdbb..ac97b2c27 100644 --- a/app/service/send_notification.py +++ b/app/service/send_notification.py @@ -80,6 +80,8 @@ def send_one_off_notification(service_id, post_data): # Validate address and set postage to europe|rest-of-world if international letter, # otherwise persist_notification with use template postage postage = validate_address(service, personalisation) + if not postage: + postage = template.postage validate_created_by(service, post_data['created_by']) sender_id = post_data.get('sender_id', None) @@ -92,7 +94,6 @@ def send_one_off_notification(service_id, post_data): notification = persist_notification( template_id=template.id, template_version=template.version, - template_postage=template.postage, recipient=post_data['to'], service=service, personalisation=personalisation, @@ -178,7 +179,6 @@ def send_pdf_letter_notification(service_id, post_data): notification_id=post_data['file_id'], template_id=template.id, template_version=template.version, - template_postage=template.postage, recipient=urllib.parse.unquote(post_data['recipient_address']), service=service, personalisation=personalisation, @@ -189,7 +189,7 @@ def send_pdf_letter_notification(service_id, post_data): client_reference=post_data['filename'], created_by_id=post_data['created_by'], billable_units=billable_units, - postage=post_data['postage'], + postage=post_data['postage'] or template.postage, ) upload_filename = get_letter_pdf_filename( diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index b300b6fdc..ead2edfb8 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -489,8 +489,7 @@ def test_persist_email_notification_stores_normalised_email( [ ("second", "first", "second"), ("first", "first", "first"), - ("first", "second", "first"), - (None, "second", "second") + ("first", "second", "first") ] ) def test_persist_letter_notification_finds_correct_postage( @@ -506,7 +505,6 @@ def test_persist_letter_notification_finds_correct_postage( persist_notification( template_id=template.id, template_version=template.version, - template_postage=template.postage, recipient="Jane Doe, 10 Downing Street, London", service=sample_service_full_permissions, personalisation=None, @@ -536,7 +534,6 @@ def test_persist_notification_with_billable_units_stores_correct_info( api_key_id=None, key_type="normal", billable_units=3, - template_postage=template.postage ) persisted_notification = Notification.query.all()[0] @@ -556,7 +553,6 @@ def test_persist_notification_for_international_letter(sample_letter_template, p key_type="normal", billable_units=3, postage=postage, - template_postage='second' ) persisted_notification = Notification.query.get(notification.id) assert persisted_notification.postage == postage diff --git a/tests/app/service/test_send_one_off_notification.py b/tests/app/service/test_send_one_off_notification.py index 6588001eb..784f6211c 100644 --- a/tests/app/service/test_send_one_off_notification.py +++ b/tests/app/service/test_send_one_off_notification.py @@ -90,7 +90,6 @@ def test_send_one_off_notification_calls_persist_correctly_for_sms( persist_mock.assert_called_once_with( template_id=template.id, template_version=template.version, - template_postage=None, recipient=post_data['to'], service=template.service, personalisation={'name': 'foo'}, @@ -152,7 +151,6 @@ def test_send_one_off_notification_calls_persist_correctly_for_email( persist_mock.assert_called_once_with( template_id=template.id, template_version=template.version, - template_postage=None, recipient=post_data['to'], service=template.service, personalisation={'name': 'foo'}, @@ -202,7 +200,6 @@ def test_send_one_off_notification_calls_persist_correctly_for_letter( persist_mock.assert_called_once_with( template_id=template.id, template_version=template.version, - template_postage='first', recipient=post_data['to'], service=template.service, personalisation=post_data['personalisation'], @@ -212,7 +209,7 @@ def test_send_one_off_notification_calls_persist_correctly_for_letter( created_by_id=str(service.created_by_id), reply_to_text=None, reference='this-is-random-in-real-life', - postage=None + postage='first' )