From 17c239bf3d1bb689979d4e4d0c955512edd463e5 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Fri, 15 Dec 2017 16:48:23 +0000 Subject: [PATCH 01/12] Remove accidentally committed app/__ file --- app/__ | 0 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 app/__ diff --git a/app/__ b/app/__ deleted file mode 100644 index e69de29bb..000000000 From 509441f1d9d31e12a6ada22f2fc1ed13825d332f Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Fri, 15 Dec 2017 16:51:40 +0000 Subject: [PATCH 02/12] Add validators for service_letter_contact_id and reply_to Validators check that service_letter_contact_id belongs to the same service as the notification/template. Generic reply_to validator calls the correct function for the given type (for either notification or template). It can be used by the template API endpoints to verify that given reply_to ID has the same service_id as the template itself. The original approach was to create a DB foreign key constraint, but this caused issues with the `version_class` decorator saving related Service objects without creating a history record. --- app/notifications/validators.py | 22 ++++++++- tests/app/notifications/test_validators.py | 55 ++++++++++++++++++++-- 2 files changed, 73 insertions(+), 4 deletions(-) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 176241f1e..5daaaaa08 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -10,7 +10,7 @@ from notifications_utils.clients.redis import rate_limit_cache_key, daily_limit_ from app.dao import services_dao, templates_dao from app.dao.service_sms_sender_dao import dao_get_service_sms_senders_by_id from app.models import ( - INTERNATIONAL_SMS_TYPE, SMS_TYPE, EMAIL_TYPE, + INTERNATIONAL_SMS_TYPE, SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, KEY_TYPE_TEST, KEY_TYPE_TEAM, SCHEDULE_NOTIFICATIONS ) from app.service.utils import service_allowed_to_send_to @@ -20,6 +20,7 @@ from app import redis_store from app.notifications.process_notifications import create_content_for_notification from app.utils import get_public_notify_type_text from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_id +from app.dao.service_letter_contact_dao import dao_get_letter_contact_by_id def check_service_over_api_rate_limit(service, api_key): @@ -141,6 +142,15 @@ def validate_template(template_id, personalisation, service, notification_type): return template, template_with_content +def check_reply_to(service_id, reply_to_id, type_): + if type_ == EMAIL_TYPE: + return check_service_email_reply_to_id(service_id, reply_to_id, type_) + elif type_ == SMS_TYPE: + return check_service_sms_sender_id(service_id, reply_to_id, type_) + elif type_ == LETTER_TYPE: + return check_service_letter_contact_id(service_id, reply_to_id, type_) + + def check_service_email_reply_to_id(service_id, reply_to_id, notification_type): if reply_to_id: try: @@ -159,3 +169,13 @@ def check_service_sms_sender_id(service_id, sms_sender_id, notification_type): message = 'sms_sender_id {} does not exist in database for service id {}'\ .format(sms_sender_id, service_id) raise BadRequestError(message=message) + + +def check_service_letter_contact_id(service_id, letter_contact_id, notification_type): + if letter_contact_id: + try: + return dao_get_letter_contact_by_id(service_id, letter_contact_id).contact_block + except NoResultFound: + message = 'letter_contact_id {} does not exist in database for service id {}'\ + .format(letter_contact_id, service_id) + raise BadRequestError(message=message) diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index 1c9fbb579..36286ed70 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -3,7 +3,7 @@ from freezegun import freeze_time from flask import current_app import app -from app.models import INTERNATIONAL_SMS_TYPE, SMS_TYPE, EMAIL_TYPE +from app.models import INTERNATIONAL_SMS_TYPE, SMS_TYPE, EMAIL_TYPE, LETTER_TYPE from app.notifications.validators import ( check_service_over_daily_message_limit, check_template_is_for_notification_type, @@ -13,7 +13,10 @@ from app.notifications.validators import ( check_service_over_api_rate_limit, validate_and_format_recipient, check_service_email_reply_to_id, - check_service_sms_sender_id) + check_service_sms_sender_id, + check_service_letter_contact_id, + check_reply_to, +) from app.v2.errors import ( BadRequestError, @@ -26,7 +29,7 @@ from tests.app.conftest import ( sample_service as create_service, sample_service_whitelist, sample_api_key) -from tests.app.db import create_reply_to_email, create_service_sms_sender +from tests.app.db import create_reply_to_email, create_service_sms_sender, create_letter_contact # all of these tests should have redis enabled (except where we specifically disable it) @@ -402,3 +405,49 @@ def test_check_service_sms_sender_id_where_sms_sender_is_not_found(sample_servic assert e.value.status_code == 400 assert e.value.message == 'sms_sender_id {} does not exist in database for service id {}' \ .format(fake_uuid, sample_service.id) + + +def test_check_service_letter_contact_id_where_letter_contact_id_is_none(): + assert check_service_letter_contact_id(None, None, 'letter') is None + + +def test_check_service_letter_contact_id_where_letter_contact_id_is_found(sample_service): + letter_contact = create_letter_contact(service=sample_service, contact_block='123456') + assert check_service_letter_contact_id(sample_service.id, letter_contact.id, LETTER_TYPE) == '123456' + + +def test_check_service_letter_contact_id_where_service_id_is_not_found(sample_service, fake_uuid): + letter_contact = create_letter_contact(service=sample_service, contact_block='123456') + with pytest.raises(BadRequestError) as e: + check_service_letter_contact_id(fake_uuid, letter_contact.id, LETTER_TYPE) + assert e.value.status_code == 400 + assert e.value.message == 'letter_contact_id {} does not exist in database for service id {}' \ + .format(letter_contact.id, fake_uuid) + + +def test_check_service_letter_contact_id_where_letter_contact_is_not_found(sample_service, fake_uuid): + with pytest.raises(BadRequestError) as e: + check_service_letter_contact_id(sample_service.id, fake_uuid, LETTER_TYPE) + assert e.value.status_code == 400 + assert e.value.message == 'letter_contact_id {} does not exist in database for service id {}' \ + .format(fake_uuid, sample_service.id) + + +@pytest.mark.parametrize('notification_type', ['sms', 'email', 'letter']) +def test_check_reply_to_with_empty_reply_to(sample_service, notification_type): + assert check_reply_to(sample_service.id, None, notification_type) is None + + +def test_check_reply_to_email_type(sample_service): + reply_to_address = create_reply_to_email(sample_service, "test@test.com") + assert check_reply_to(sample_service.id, reply_to_address.id, EMAIL_TYPE) == 'test@test.com' + + +def test_check_reply_to_sms_type(sample_service): + sms_sender = create_service_sms_sender(service=sample_service, sms_sender='123456') + assert check_reply_to(sample_service.id, sms_sender.id, SMS_TYPE) == '123456' + + +def test_check_reply_to_letter_type(sample_service): + letter_contact = create_letter_contact(service=sample_service, contact_block='123456') + assert check_reply_to(sample_service.id, letter_contact.id, LETTER_TYPE) == '123456' From da247680a420b0af0de71ee6d44454b2f2e38b16 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Fri, 15 Dec 2017 17:06:11 +0000 Subject: [PATCH 03/12] Validate that template reply_to belongs to template's service Checks that email/sms/letter reply to object has the same service_id as the template it's being attached to, to make sure it's not possible to retrieve data about return addresses for other services. --- app/template/rest.py | 7 ++++- tests/app/template/test_rest.py | 45 +++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/app/template/rest.py b/app/template/rest.py index 1f869fb5d..3f03ece60 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -16,7 +16,7 @@ from app.dao.templates_dao import ( from notifications_utils.template import SMSMessageTemplate from app.dao.services_dao import dao_fetch_service_by_id from app.models import SMS_TYPE -from app.notifications.validators import service_has_permission +from app.notifications.validators import service_has_permission, check_reply_to from app.schemas import (template_schema, template_history_schema) from app.errors import ( register_errors, @@ -58,6 +58,8 @@ def create_template(service_id): errors = {'content': [message]} raise InvalidRequest(errors, status_code=400) + check_reply_to(service_id, new_template.reply_to, new_template.template_type) + dao_create_template(new_template) return jsonify(data=template_schema.dump(new_template).data), 201 @@ -93,6 +95,9 @@ def update_template(service_id, template_id): message = 'Content has a character count greater than the limit of {}'.format(char_count_limit) errors = {'content': [message]} raise InvalidRequest(errors, status_code=400) + + check_reply_to(service_id, update_dict.reply_to, fetched_template.template_type) + dao_update_template(update_dict) return jsonify(data=template_schema.dump(update_dict).data), 200 diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index f754baa24..8f32458f1 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -588,6 +588,28 @@ def test_create_a_template_with_reply_to(admin_request, sample_user): assert sorted(json_resp['data']) == sorted(template_schema.dump(template).data) +def test_create_a_template_with_foreign_service_reply_to(admin_request, sample_user): + service = create_service(service_permissions=['letter']) + service2 = create_service(service_name='test service', email_from='test@example.com', + service_permissions=['letter']) + letter_contact = create_letter_contact(service2, "Edinburgh, ED1 1AA") + data = { + 'name': 'my template', + 'subject': 'subject', + 'template_type': 'letter', + 'content': 'template content', + 'service': str(service.id), + 'created_by': str(sample_user.id), + 'reply_to': str(letter_contact.id), + } + + json_resp = admin_request.post('template.create_template', service_id=service.id, _data=data, _expected_status=400) + + assert json_resp['message'] == "letter_contact_id {} does not exist in database for service id {}".format( + str(letter_contact.id), str(service.id) + ) + + def test_get_template_reply_to(client, sample_letter_template): auth_header = create_authorization_header() letter_contact = create_letter_contact(sample_letter_template.service, "Edinburgh, ED1 1AA") @@ -621,6 +643,29 @@ def test_update_template_reply_to(client, sample_letter_template): assert template.reply_to == letter_contact.id +def test_update_template_with_foreign_service_reply_to(client, sample_letter_template): + auth_header = create_authorization_header() + + service2 = create_service(service_name='test service', email_from='test@example.com', + service_permissions=['letter']) + letter_contact = create_letter_contact(service2, "Edinburgh, ED1 1AA") + + data = { + 'reply_to': str(letter_contact.id), + } + + resp = client.post('/service/{}/template/{}'.format(sample_letter_template.service_id, sample_letter_template.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert resp.status_code == 400, resp.get_data(as_text=True) + json_resp = json.loads(resp.get_data(as_text=True)) + + assert json_resp['message'] == "letter_contact_id {} does not exist in database for service id {}".format( + str(letter_contact.id), str(sample_letter_template.service_id) + ) + + def test_update_redact_template(admin_request, sample_template): assert sample_template.redact_personalisation is False From 8afe9aced76c9f04829df21f57bbbb7518aced8c Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Fri, 15 Dec 2017 17:08:01 +0000 Subject: [PATCH 04/12] Allow setting reply_to when creating a test template --- tests/app/db.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/app/db.py b/tests/app/db.py index 717b1aa2b..97791ceb3 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -123,7 +123,8 @@ def create_template( template_name=None, subject='Template subject', content='Dear Sir/Madam, Hello. Yours Truly, The Government.', - template_id=None + template_id=None, + reply_to=None ): data = { 'name': template_name or '{} Template Name'.format(template_type), @@ -131,6 +132,7 @@ def create_template( 'content': content, 'service': service, 'created_by': service.created_by, + 'reply_to': reply_to, } if template_type != SMS_TYPE: data['subject'] = subject From 3b0790f9504110c6924994cdfebda3401d1c7715 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Fri, 15 Dec 2017 17:10:45 +0000 Subject: [PATCH 05/12] Add Template.get_reply_to_text helper method Returns either template's reply_to text if set or the related field from the default service record. Return value can be used as default for `Notification.reply_to_text` when per-notification value is not provided. --- app/models.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/app/models.py b/app/models.py index 2f196c7a5..469bfff4a 100644 --- a/app/models.py +++ b/app/models.py @@ -607,6 +607,10 @@ class TemplateBase(db.Model): def service_letter_contact_id(cls): return db.Column(UUID(as_uuid=True), db.ForeignKey('service_letter_contacts.id'), nullable=True) + @declared_attr + def service_letter_contact(cls): + return db.relationship('ServiceLetterContact', viewonly=True) + @property def reply_to(self): if self.template_type == LETTER_TYPE: @@ -623,6 +627,19 @@ class TemplateBase(db.Model): else: raise ValueError('Unable to set sender for {} template'.format(self.template_type)) + def get_reply_to_text(self): + if self.template_type == LETTER_TYPE: + if self.service_letter_contact_id is not None: + return self.service_letter_contact.contact_block + else: + return self.service.get_default_letter_contact() + elif self.template_type == EMAIL_TYPE: + return self.service.get_default_reply_to_email_address() + elif self.template_type == SMS_TYPE: + return self.service.get_default_sms_sender() + else: + return None + def _as_utils_template(self): if self.template_type == EMAIL_TYPE: return PlainTextEmailTemplate( From a98e5247b8d153611607824068ae4711bfa36421 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Fri, 15 Dec 2017 17:13:55 +0000 Subject: [PATCH 06/12] Update notificaiton API endpoints to use template's reply_to Sets the reply_to_text on notification from the template value if it's set. --- app/service/send_notification.py | 27 +++++++++---------- app/v2/notifications/post_notifications.py | 18 ++++++------- .../service/test_send_one_off_notification.py | 23 +++++++++++++++- .../notifications/test_post_notifications.py | 24 +++++++++++++++++ 4 files changed, 67 insertions(+), 25 deletions(-) diff --git a/app/service/send_notification.py b/app/service/send_notification.py index 634edc5ff..53cbb2b6c 100644 --- a/app/service/send_notification.py +++ b/app/service/send_notification.py @@ -15,7 +15,6 @@ from app.models import ( PRIORITY, SMS_TYPE, EMAIL_TYPE, - LETTER_TYPE ) from app.dao.services_dao import dao_fetch_service_by_id from app.dao.templates_dao import dao_get_template_by_id_and_service_id @@ -56,7 +55,12 @@ def send_one_off_notification(service_id, post_data): validate_created_by(service, post_data['created_by']) sender_id = post_data.get('sender_id', None) - reply_to = get_reply_to_text(notification_type=template.template_type, sender_id=sender_id, service=service) + reply_to = get_reply_to_text( + notification_type=template.template_type, + sender_id=sender_id, + service=service, + template=template + ) notification = persist_notification( template_id=template.id, template_version=template.version, @@ -80,21 +84,14 @@ def send_one_off_notification(service_id, post_data): return {'id': str(notification.id)} -def get_reply_to_text(notification_type, sender_id, service): +def get_reply_to_text(notification_type, sender_id, service, template): reply_to = None - if notification_type == EMAIL_TYPE: - if sender_id: + if sender_id: + if notification_type == EMAIL_TYPE: reply_to = dao_get_reply_to_by_id(service.id, sender_id).email_address - else: - service.get_default_reply_to_email_address() - - elif notification_type == SMS_TYPE: - if sender_id: + elif notification_type == SMS_TYPE: reply_to = dao_get_service_sms_senders_by_id(service.id, sender_id).sms_sender - else: - reply_to = service.get_default_sms_sender() - - elif notification_type == LETTER_TYPE: - reply_to = service.get_default_letter_contact() + else: + reply_to = template.get_reply_to_text() return reply_to diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index ed9d67c7f..5466be234 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -70,8 +70,6 @@ def post_notification(notification_type): check_rate_limiting(authenticated_service, api_user) - reply_to = get_reply_to_text(notification_type, form) - template, template_with_content = validate_template( form['template_id'], form.get('personalisation', {}), @@ -79,11 +77,14 @@ def post_notification(notification_type): notification_type, ) + reply_to = get_reply_to_text(notification_type, form, template) + if notification_type == LETTER_TYPE: notification = process_letter_notification( letter_data=form, api_key=api_user, template=template, + reply_to_text=reply_to ) else: notification = process_sms_or_email_notification( @@ -164,7 +165,7 @@ def process_sms_or_email_notification(*, form, notification_type, api_key, templ return notification -def process_letter_notification(*, letter_data, api_key, template): +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) @@ -175,12 +176,11 @@ def process_letter_notification(*, letter_data, api_key, template): # 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 should_send else NOTIFICATION_SENDING - letter_contact_block = api_key.service.get_default_letter_contact() notification = create_letter_notification(letter_data=letter_data, template=template, api_key=api_key, status=status, - reply_to_text=letter_contact_block) + reply_to_text=reply_to_text) if not should_send: update_letter_notifications_to_sent_to_dvla.apply_async( @@ -198,21 +198,21 @@ def process_letter_notification(*, letter_data, api_key, template): @statsd(namespace="performance-testing") -def get_reply_to_text(notification_type, form): +def get_reply_to_text(notification_type, form, template): reply_to = None if notification_type == EMAIL_TYPE: service_email_reply_to_id = form.get("email_reply_to_id", None) reply_to = check_service_email_reply_to_id( str(authenticated_service.id), service_email_reply_to_id, notification_type - ) or authenticated_service.get_default_reply_to_email_address() + ) or template.get_reply_to_text() elif notification_type == SMS_TYPE: service_sms_sender_id = form.get("sms_sender_id", None) reply_to = check_service_sms_sender_id( str(authenticated_service.id), service_sms_sender_id, notification_type - ) or authenticated_service.get_default_sms_sender() + ) or template.get_reply_to_text() elif notification_type == LETTER_TYPE: - reply_to = authenticated_service.get_default_letter_contact() + reply_to = template.get_reply_to_text() return reply_to diff --git a/tests/app/service/test_send_one_off_notification.py b/tests/app/service/test_send_one_off_notification.py index 6a9a681ff..e64102ec1 100644 --- a/tests/app/service/test_send_one_off_notification.py +++ b/tests/app/service/test_send_one_off_notification.py @@ -18,6 +18,7 @@ from app.models import ( from tests.app.db import ( create_user, create_reply_to_email, + create_letter_contact, create_service, create_template ) @@ -217,7 +218,27 @@ def test_send_one_off_notification_should_add_email_reply_to_text_for_notificati research_mode=False, queue=None ) - notification.reply_to_text == reply_to_email.email_address + assert notification.reply_to_text == reply_to_email.email_address + + +def test_send_one_off_letter_notification_should_use_template_reply_to_text(sample_letter_template, celery_mock): + letter_contact = create_letter_contact(sample_letter_template.service, "Edinburgh, ED1 1AA", is_default=False) + sample_letter_template.reply_to = str(letter_contact.id) + + data = { + 'to': 'user@example.com', + 'template_id': str(sample_letter_template.id), + 'created_by': str(sample_letter_template.service.created_by_id) + } + + notification_id = send_one_off_notification(service_id=sample_letter_template.service.id, post_data=data) + notification = Notification.query.get(notification_id['id']) + celery_mock.assert_called_once_with( + notification=notification, + research_mode=False, + queue=None + ) + assert notification.reply_to_text == "Edinburgh, ED1 1AA" def test_send_one_off_notification_should_throw_exception_if_reply_to_id_doesnot_exist( diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index aa1bb70f6..cba00ff9b 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -28,6 +28,7 @@ from tests.app.db import ( create_service, create_template, create_reply_to_email, + create_letter_contact, create_service_sms_sender, create_service_with_inbound_number ) @@ -639,3 +640,26 @@ def test_post_email_notification_with_invalid_reply_to_id_returns_400(client, sa assert 'email_reply_to_id {} does not exist in database for service id {}'. \ format(fake_uuid, sample_email_template.service_id) in resp_json['errors'][0]['message'] assert 'BadRequestError' in resp_json['errors'][0]['error'] + + +def test_letter_notification_should_use_template_reply_to(client, sample_letter_template): + letter_contact = create_letter_contact(sample_letter_template.service, "Edinburgh, ED1 1AA", is_default=False) + sample_letter_template.reply_to = str(letter_contact.id) + + data = { + 'template_id': str(sample_letter_template.id), + 'personalisation': { + 'address_line_1': '123', + 'address_line_2': '234', + 'postcode': 'W1A1AA', + } + } + response = client.post("v2/notifications/letter", + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), + create_authorization_header(service_id=sample_letter_template.service.id)] + ) + assert response.status_code == 201, response.get_data(as_text=True) + notifications = Notification.query.all() + assert len(notifications) == 1 + assert notifications[0].reply_to_text == "Edinburgh, ED1 1AA" From 87b56567b2f74a1759b33553d0b71f7c799890cb Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Fri, 15 Dec 2017 17:15:31 +0000 Subject: [PATCH 07/12] Set job notifications reply_to_text from the template.reply_to When creating notification objects from the job sets the reply_to_text from template's reply_to if it's present. Otherwise uses the service default. --- app/celery/tasks.py | 11 ++++++--- tests/app/celery/test_tasks.py | 41 ++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 3 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 7460f3b90..7b5717b66 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -203,6 +203,7 @@ def save_sms(self, key_type=KEY_TYPE_NORMAL): notification = encryption.decrypt(encrypted_notification) service = dao_fetch_service_by_id(service_id) + template = dao_get_template_by_id(notification['template'], version=notification['template_version']) if not service_allowed_to_send_to(notification['to'], service, key_type): current_app.logger.info( @@ -224,7 +225,7 @@ def save_sms(self, job_id=notification.get('job', None), job_row_number=notification.get('row_number', None), notification_id=notification_id, - reply_to_text=service.get_default_sms_sender() + reply_to_text=template.get_reply_to_text() ) provider_tasks.deliver_sms.apply_async( @@ -252,7 +253,9 @@ def save_email(self, api_key_id=None, key_type=KEY_TYPE_NORMAL): notification = encryption.decrypt(encrypted_notification) + service = dao_fetch_service_by_id(service_id) + template = dao_get_template_by_id(notification['template'], version=notification['template_version']) if not service_allowed_to_send_to(notification['to'], service, key_type): current_app.logger.info("Email {} failed as restricted service".format(notification_id)) @@ -272,7 +275,7 @@ def save_email(self, job_id=notification.get('job', None), job_row_number=notification.get('row_number', None), notification_id=notification_id, - reply_to_text=service.get_default_reply_to_email_address() + reply_to_text=template.get_reply_to_text() ) provider_tasks.deliver_email.apply_async( @@ -299,6 +302,8 @@ def save_letter( recipient = notification['personalisation']['addressline1'] service = dao_fetch_service_by_id(service_id) + template = dao_get_template_by_id(notification['template'], version=notification['template_version']) + try: saved_notification = persist_notification( template_id=notification['template'], @@ -314,7 +319,7 @@ def save_letter( job_row_number=notification['row_number'], notification_id=notification_id, reference=create_random_identifier(), - reply_to_text=service.get_default_letter_contact() + reply_to_text=template.get_reply_to_text() ) if service.has_permission('letters_as_pdf'): diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index a5837b3a2..882fc5b9f 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1040,6 +1040,47 @@ def test_save_letter_saves_letter_to_database(mocker, notify_db_session): assert notification_db.reply_to_text == "Address contact" +def test_save_letter_uses_template_reply_to_text(mocker, notify_db_session): + service = create_service() + create_letter_contact(service=service, contact_block="Address contact", is_default=True) + template_contact = create_letter_contact( + service=service, + contact_block="Template address contact", + is_default=False + ) + template = create_template( + service=service, + template_type=LETTER_TYPE, + reply_to=template_contact.id + ) + + job = create_job(template=template) + + mocker.patch('app.celery.tasks.create_random_identifier', return_value="this-is-random-in-real-life") + + personalisation = { + 'addressline1': 'Foo', + 'addressline2': 'Bar', + 'postcode': 'Flob', + } + notification_json = _notification_json( + template=job.template, + to='Foo', + personalisation=personalisation, + job_id=job.id, + row_number=1 + ) + + save_letter( + job.service_id, + uuid.uuid4(), + encryption.encrypt(notification_json), + ) + + notification_db = Notification.query.one() + assert notification_db.reply_to_text == "Template address contact" + + 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): sample_letter_job.service.research_mode = True From 441651bbd1f4482d5a7394f142ca12181f0a9652 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 18 Dec 2017 16:12:17 +0000 Subject: [PATCH 08/12] Add get_count_of_letters_to_process to notifications_dao - will get the letter notifications from day before >= letter processing deadline (17:30) - letters_as_pdf permission is required in the service --- app/config.py | 11 ++- app/dao/notifications_dao.py | 31 +++++++ .../notification_dao/test_notification_dao.py | 89 +++++++++++++++++++ 3 files changed, 130 insertions(+), 1 deletion(-) diff --git a/app/config.py b/app/config.py index 0c4b477c6..f92355352 100644 --- a/app/config.py +++ b/app/config.py @@ -1,4 +1,4 @@ -from datetime import timedelta +from datetime import timedelta, time import os import json @@ -32,6 +32,7 @@ class QueueNames(object): PROCESS_FTP = 'process-ftp-tasks' CREATE_LETTERS_PDF = 'create-letters-pdf-tasks' CALLBACKS = 'service-callbacks' + LETTERS = 'letter-tasks' @staticmethod def all_queues(): @@ -48,6 +49,7 @@ class QueueNames(object): QueueNames.NOTIFY, QueueNames.CREATE_LETTERS_PDF, QueueNames.CALLBACKS, + QueueNames.LETTERS, ] @@ -238,6 +240,11 @@ class Config(object): 'schedule': crontab(hour=17, minute=30), 'options': {'queue': QueueNames.PERIODIC} }, + 'run-letter-pdfs': { + 'task': 'run-letter-pdfs', + 'schedule': crontab(hour=17, minute=50), + 'options': {'queue': QueueNames.PERIODIC} + }, 'run-letter-api-notifications': { 'task': 'run-letter-api-notifications', 'schedule': crontab(hour=17, minute=40), @@ -313,6 +320,8 @@ class Config(object): TEMPLATE_PREVIEW_API_HOST = os.environ.get('TEMPLATE_PREVIEW_API_HOST', 'http://localhost:6013') TEMPLATE_PREVIEW_API_KEY = os.environ.get('TEMPLATE_PREVIEW_API_KEY', 'my-secret-key') + LETTER_PROCESSING_DEADLINE = time(17, 30) + ###################### # Config overrides ### diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 29b49ba7c..fc6d2b32e 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -584,3 +584,34 @@ def dao_get_last_notification_added_for_job_id(job_id): ).first() return last_notification_added + + +def dao_get_count_of_letters_to_process_for_date(date_to_process=date.today()): + """ + Returns a count of letter notifications for services with letters_as_pdf permission set + to be processed today if no argument passed in otherwise will return the count for + the date passed in. + Records processed today = yesterday 17:30 to today 17:29:59 + + Note - services without letters_as_pdf permission will be ignored + """ + day_before = date_to_process - timedelta(days=1) + letter_deadline_time = current_app.config.get('LETTER_PROCESSING_DEADLINE') + + start_datetime = datetime.combine(day_before, letter_deadline_time) + end_datetime = start_datetime + timedelta(days=1) + + count_of_letters_to_process_for_date = Notification.query.join( + Service + ).filter( + Notification.created_at >= start_datetime, + Notification.created_at < end_datetime, + Notification.notification_type == LETTER_TYPE, + Notification.status == NOTIFICATION_CREATED, + Notification.key_type != KEY_TYPE_TEST, + Service.permissions.any( + ServicePermission.permission == 'letters_as_pdf' + ) + ).count() + + return count_of_letters_to_process_for_date diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 8af596199..7d5083307 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -10,6 +10,7 @@ from app.dao.notifications_dao import ( dao_create_notification, dao_created_scheduled_notification, dao_delete_notifications_and_history_by_id, + dao_get_count_of_letters_to_process_for_date, dao_get_last_notification_added_for_job_id, dao_get_last_template_usage, dao_get_notifications_by_to_field, @@ -33,6 +34,7 @@ from app.dao.notifications_dao import ( dao_get_notifications_by_references ) from app.dao.services_dao import dao_update_service +from app.dao.service_permissions_dao import dao_add_service_permission from app.models import ( Job, Notification, @@ -2001,3 +2003,90 @@ def test_dao_get_notifications_by_reference(sample_template): assert len(notifications) == 2 assert notifications[0].id in [notification_1.id, notification_2.id] assert notifications[1].id in [notification_1.id, notification_2.id] + + +@freeze_time("2017-12-18 17:50") +def test_dao_get_count_of_letters_to_process_for_today(sample_letter_template): + dao_add_service_permission(sample_letter_template.service.id, 'letters_as_pdf') + + # expected + create_notification(template=sample_letter_template, created_at='2017-12-17 17:30:00') + create_notification(template=sample_letter_template, created_at='2017-12-18 17:29:59') + + # not expected + create_notification(template=sample_letter_template, created_at='2017-12-17 17:29:59') + create_notification(template=sample_letter_template, created_at='2017-12-18 17:30:00') + + count_for_date = dao_get_count_of_letters_to_process_for_date() + + assert count_for_date == 2 + + +@freeze_time("2017-12-18 17:50") +def test_dao_get_count_of_letters_to_process_for_date_in_past(sample_letter_template): + dao_add_service_permission(sample_letter_template.service.id, 'letters_as_pdf') + + # expected + create_notification(template=sample_letter_template, created_at='2017-12-15 17:29:59') + + # not expected + create_notification(template=sample_letter_template, created_at='2017-12-15 17:30:00') + create_notification(template=sample_letter_template, created_at='2017-12-18 17:29:00') + + count_for_date = dao_get_count_of_letters_to_process_for_date(date(2017, 12, 15)) + + assert count_for_date == 1 + + +@freeze_time("2017-12-18 17:50") +def test_dao_get_count_of_letters_to_process_for_date_in_future_does_not_raise_error(sample_letter_template): + dao_add_service_permission(sample_letter_template.service.id, 'letters_as_pdf') + + # not expected + create_notification(template=sample_letter_template, created_at='2017-12-18 17:30:00') + create_notification(template=sample_letter_template, created_at='2017-12-19 17:29:59') + + count_for_date = dao_get_count_of_letters_to_process_for_date(date(2017, 12, 20)) + + assert count_for_date == 0 + + +def test_dao_get_count_of_letters_to_process_for_today_without_notis_does_not_raise_error(sample_letter_template): + count_for_date = dao_get_count_of_letters_to_process_for_date() + + assert count_for_date == 0 + + +@freeze_time("2017-12-18 17:50") +def test_dao_get_count_of_letters_to_process_for_date_ignores_service_not_letters_as_pdf( + sample_letter_template, sample_template): + # not expected + create_notification(template=sample_letter_template, created_at='2017-12-18 17:29:00') + + dao_add_service_permission(sample_template.service.id, 'letters_as_pdf') + sample_template.template_type = 'letter' + + # expected + create_notification(template=sample_template, created_at='2017-12-18 17:29:00') + create_notification(template=sample_template, created_at='2017-12-18 17:29:10') + + count_for_date = dao_get_count_of_letters_to_process_for_date() + + assert 'letters_as_pdf' not in [p.permission for p in sample_letter_template.service.permissions] + assert count_for_date == 2 + + +@freeze_time("2017-12-18 17:50") +def test_dao_get_count_of_letters_to_process_for_date_ignores_test_keys(sample_letter_template): + dao_add_service_permission(sample_letter_template.service.id, 'letters_as_pdf') + + # not expected + create_notification(template=sample_letter_template, key_type=KEY_TYPE_TEST, created_at='2017-12-18 17:29:00') + + # expected + create_notification(template=sample_letter_template, created_at='2017-12-18 17:29:10') + create_notification(template=sample_letter_template, created_at='2017-12-18 17:29:20') + + count_for_date = dao_get_count_of_letters_to_process_for_date() + + assert count_for_date == 2 From 88f3c17463247fe4d06edb1abdc07346880c7662 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 18 Dec 2017 16:19:27 +0000 Subject: [PATCH 09/12] Trigger collate-letter-pdfs-for-day task if notis to process --- app/celery/scheduled_tasks.py | 16 ++++++++++++ tests/app/celery/test_scheduled_tasks.py | 31 ++++++++++++++++++++++++ 2 files changed, 47 insertions(+) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 7b28b0fb4..c773fe739 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -1,4 +1,5 @@ from datetime import ( + date, datetime, timedelta ) @@ -33,6 +34,7 @@ from app.dao.notifications_dao import ( dao_timeout_notifications, is_delivery_slow_for_provider, delete_notifications_created_more_than_a_week_ago_by_type, + dao_get_count_of_letters_to_process_for_date, dao_get_scheduled_notifications, set_scheduled_notification_to_processed, dao_set_created_live_letter_api_notifications_to_pending, @@ -355,6 +357,20 @@ def run_letter_jobs(): current_app.logger.info("Queued {} ready letter job ids onto {}".format(len(job_ids), QueueNames.PROCESS_FTP)) +@notify_celery.task(name="run-letter-pdfs") +@statsd(namespace="tasks") +def run_letter_pdfs(): + letter_pdfs_count = dao_get_count_of_letters_to_process_for_date() + if letter_pdfs_count: + notify_celery.send_task( + name='collate-letter-pdfs-for-day', + args=(date.today().strftime("%Y-%m-%d"),), + queue=QueueNames.LETTERS + ) + current_app.logger.info("{} letter pdfs to be process by {} task".format( + letter_pdfs_count, 'collate-letter-pdfs-for-day')) + + @notify_celery.task(name="run-letter-api-notifications") @statsd(namespace="tasks") def run_letter_api_notifications(): diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index bf1e242f3..33909e4c4 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -24,6 +24,7 @@ from app.celery.scheduled_tasks import ( remove_transformed_dvla_files, run_scheduled_jobs, run_letter_jobs, + run_letter_pdfs, run_letter_api_notifications, populate_monthly_billing, s3, @@ -43,6 +44,7 @@ from app.dao.provider_details_dao import ( dao_update_provider_details, get_current_provider ) +from app.dao.service_permissions_dao import dao_add_service_permission from app.models import ( MonthlyBilling, NotificationHistory, @@ -713,6 +715,35 @@ def test_run_letter_jobs(client, mocker, sample_letter_template): queue=QueueNames.PROCESS_FTP) +@freeze_time("2017-12-18 17:50") +def test_run_letter_pdfs(client, mocker, sample_letter_template): + dao_add_service_permission(sample_letter_template.service.id, 'letters_as_pdf') + + create_notification(template=sample_letter_template, created_at='2017-12-17 17:30:00') + create_notification(template=sample_letter_template, created_at='2017-12-18 17:29:59') + + mock_celery = mocker.patch("app.celery.tasks.notify_celery.send_task") + + run_letter_pdfs() + + mock_celery.assert_called_once_with(name='collate-letter-pdfs-for-day', + args=('2017-12-18',), + queue=QueueNames.LETTERS) + + +@freeze_time("2017-12-18 17:50") +def test_run_letter_pdfs_send_task_not_called_if_no_notis_for_day(client, mocker, sample_letter_template): + dao_add_service_permission(sample_letter_template.service.id, 'letters_as_pdf') + + create_notification(template=sample_letter_template, created_at='2017-12-15 17:30:00') + + mock_celery = mocker.patch("app.celery.tasks.notify_celery.send_task") + + run_letter_pdfs() + + assert not mock_celery.called + + def test_run_letter_jobs_does_nothing_if_no_ready_jobs(client, mocker, sample_letter_template): create_job(sample_letter_template, job_status=JOB_STATUS_IN_PROGRESS) create_job(sample_letter_template, job_status=JOB_STATUS_SENT_TO_DVLA) From 3a4b8673bc654b18a507171bd011ba6840ef64fa Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 18 Dec 2017 16:20:57 +0000 Subject: [PATCH 10/12] Update upload_letters_pdf to use config setting for letter processing deadline --- app/aws/s3.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index 5380544c8..5ffb24068 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -1,4 +1,4 @@ -from datetime import datetime, timedelta, time +from datetime import datetime, timedelta from flask import current_app @@ -85,7 +85,7 @@ def upload_letters_pdf(reference, crown, filedata): now = datetime.utcnow() print_datetime = now - if now.time() > time(17, 30): + 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( From b70bf1b541dc7b93db5637da620ec8236985524b Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 18 Dec 2017 16:22:22 +0000 Subject: [PATCH 11/12] Add letter tasks to manifest and queue name to config test --- manifest-delivery-base.yml | 2 +- tests/app/test_config.py | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/manifest-delivery-base.yml b/manifest-delivery-base.yml index 8838ad40b..26a12ea0e 100644 --- a/manifest-delivery-base.yml +++ b/manifest-delivery-base.yml @@ -51,7 +51,7 @@ applications: NOTIFY_APP_NAME: delivery-worker-priority - name: notify-delivery-worker - command: scripts/run_app_paas.sh celery -A run_celery.notify_celery worker --loglevel=INFO --concurrency=11 -Q job-tasks,retry-tasks,notify-internal-tasks,create-letters-pdf-tasks + command: scripts/run_app_paas.sh celery -A run_celery.notify_celery worker --loglevel=INFO --concurrency=11 -Q job-tasks,retry-tasks,notify-internal-tasks,create-letters-pdf-tasks,letter-tasks env: NOTIFY_APP_NAME: delivery-worker diff --git a/tests/app/test_config.py b/tests/app/test_config.py index 2f643ba5c..4e17390de 100644 --- a/tests/app/test_config.py +++ b/tests/app/test_config.py @@ -63,7 +63,7 @@ def test_cloudfoundry_config_has_different_defaults(): def test_queue_names_all_queues_correct(): # Need to ensure that all_queues() only returns queue names used in API queues = QueueNames.all_queues() - assert len(queues) == 12 + assert len(queues) == 13 assert set([ QueueNames.PRIORITY, QueueNames.PERIODIC, @@ -76,5 +76,6 @@ def test_queue_names_all_queues_correct(): QueueNames.RETRY, QueueNames.NOTIFY, QueueNames.CREATE_LETTERS_PDF, - QueueNames.CALLBACKS + QueueNames.CALLBACKS, + QueueNames.LETTERS, ]) == set(queues) From 8103540261205654e1174ee4904d15c946bfee02 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 19 Dec 2017 13:18:56 +0000 Subject: [PATCH 12/12] Renamed run-letter-pdfs to trigger-letter-pdfs-for-day - also set optional date_to_process argument for dao_get_count_of_letters_to_process_for_date to None, so it's set in the code instead --- app/celery/scheduled_tasks.py | 4 ++-- app/config.py | 4 ++-- app/dao/notifications_dao.py | 5 ++++- tests/app/celery/test_scheduled_tasks.py | 11 ++++++----- 4 files changed, 14 insertions(+), 10 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index c773fe739..5bac860a6 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -357,9 +357,9 @@ def run_letter_jobs(): current_app.logger.info("Queued {} ready letter job ids onto {}".format(len(job_ids), QueueNames.PROCESS_FTP)) -@notify_celery.task(name="run-letter-pdfs") +@notify_celery.task(name="trigger-letter-pdfs-for-day") @statsd(namespace="tasks") -def run_letter_pdfs(): +def trigger_letter_pdfs_for_day(): letter_pdfs_count = dao_get_count_of_letters_to_process_for_date() if letter_pdfs_count: notify_celery.send_task( diff --git a/app/config.py b/app/config.py index f92355352..295adab1c 100644 --- a/app/config.py +++ b/app/config.py @@ -240,8 +240,8 @@ class Config(object): 'schedule': crontab(hour=17, minute=30), 'options': {'queue': QueueNames.PERIODIC} }, - 'run-letter-pdfs': { - 'task': 'run-letter-pdfs', + 'trigger-letter-pdfs-for-day': { + 'task': 'trigger-letter-pdfs-for-day', 'schedule': crontab(hour=17, minute=50), 'options': {'queue': QueueNames.PERIODIC} }, diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index fc6d2b32e..0d1c6e72a 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -586,7 +586,7 @@ def dao_get_last_notification_added_for_job_id(job_id): return last_notification_added -def dao_get_count_of_letters_to_process_for_date(date_to_process=date.today()): +def dao_get_count_of_letters_to_process_for_date(date_to_process=None): """ Returns a count of letter notifications for services with letters_as_pdf permission set to be processed today if no argument passed in otherwise will return the count for @@ -595,6 +595,9 @@ def dao_get_count_of_letters_to_process_for_date(date_to_process=date.today()): Note - services without letters_as_pdf permission will be ignored """ + if date_to_process is None: + date_to_process = date.today() + day_before = date_to_process - timedelta(days=1) letter_deadline_time = current_app.config.get('LETTER_PROCESSING_DEADLINE') diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 33909e4c4..699f1fa9e 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -24,7 +24,7 @@ from app.celery.scheduled_tasks import ( remove_transformed_dvla_files, run_scheduled_jobs, run_letter_jobs, - run_letter_pdfs, + trigger_letter_pdfs_for_day, run_letter_api_notifications, populate_monthly_billing, s3, @@ -716,7 +716,7 @@ def test_run_letter_jobs(client, mocker, sample_letter_template): @freeze_time("2017-12-18 17:50") -def test_run_letter_pdfs(client, mocker, sample_letter_template): +def test_trigger_letter_pdfs_for_day(client, mocker, sample_letter_template): dao_add_service_permission(sample_letter_template.service.id, 'letters_as_pdf') create_notification(template=sample_letter_template, created_at='2017-12-17 17:30:00') @@ -724,7 +724,7 @@ def test_run_letter_pdfs(client, mocker, sample_letter_template): mock_celery = mocker.patch("app.celery.tasks.notify_celery.send_task") - run_letter_pdfs() + trigger_letter_pdfs_for_day() mock_celery.assert_called_once_with(name='collate-letter-pdfs-for-day', args=('2017-12-18',), @@ -732,14 +732,15 @@ def test_run_letter_pdfs(client, mocker, sample_letter_template): @freeze_time("2017-12-18 17:50") -def test_run_letter_pdfs_send_task_not_called_if_no_notis_for_day(client, mocker, sample_letter_template): +def test_trigger_letter_pdfs_for_day_send_task_not_called_if_no_notis_for_day( + client, mocker, sample_letter_template): dao_add_service_permission(sample_letter_template.service.id, 'letters_as_pdf') create_notification(template=sample_letter_template, created_at='2017-12-15 17:30:00') mock_celery = mocker.patch("app.celery.tasks.notify_celery.send_task") - run_letter_pdfs() + trigger_letter_pdfs_for_day() assert not mock_celery.called