From 5e1eac1f6f232ff7a3092defb2daa3fecaf86cb8 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 14 Dec 2017 17:05:36 +0000 Subject: [PATCH 1/9] Add test for manifest-delivery-base.yml - This should ensure that queue names defined in config.py / QueueNames are in the manifest-delivery-base.yml --- tests/test_manifest_delivery_base.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 tests/test_manifest_delivery_base.py diff --git a/tests/test_manifest_delivery_base.py b/tests/test_manifest_delivery_base.py new file mode 100644 index 000000000..265a1170b --- /dev/null +++ b/tests/test_manifest_delivery_base.py @@ -0,0 +1,23 @@ +import yaml + +from app.config import QueueNames + + +def test_queue_names_set_in_manifest_delivery_base_correctly(): + with open("manifest-delivery-base.yml", 'r') as stream: + search = ' -Q ' + yml_commands = [y['command'] for y in yaml.load(stream)['applications']] + + watched_queues = set() + for command in yml_commands: + start_of_queue_arg = command.find(search) + if start_of_queue_arg > 0: + start_of_queue_names = start_of_queue_arg + len(search) + queues = command[start_of_queue_names:].split(',') + watched_queues.update(queues) + + # ses-callbacks isn't used in api (only used in SNS lambda) + ignored_queues = {'ses-callbacks'} + watched_queues -= ignored_queues + + assert watched_queues == set(QueueNames.all_queues()) From 1b82afb6bbdacec152d33ae2e7000475b39e4293 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Mon, 18 Dec 2017 11:39:21 +0000 Subject: [PATCH 2/9] Change reply to address for invitation emails to be invitation sender If someone receives an invitation email for Notify the reply-to address of the email was the GOV.UK Notify email address. This has been changed to be the email address of the user who sent the invite. Pivotal story: https://www.pivotaltracker.com/story/show/153094646 --- app/invite/rest.py | 2 +- tests/app/invite/test_invite_rest.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/invite/rest.py b/app/invite/rest.py index 55cab2a30..ba263e1b3 100644 --- a/app/invite/rest.py +++ b/app/invite/rest.py @@ -42,7 +42,7 @@ def create_invited_user(service_id): notification_type=EMAIL_TYPE, api_key_id=None, key_type=KEY_TYPE_NORMAL, - reply_to_text=service.get_default_reply_to_email_address() + reply_to_text=invited_user.from_user.email_address ) send_notification_to_queue(saved_notification, False, queue=QueueNames.NOTIFY) diff --git a/tests/app/invite/test_invite_rest.py b/tests/app/invite/test_invite_rest.py index 1c4c6b10a..ebeeb3492 100644 --- a/tests/app/invite/test_invite_rest.py +++ b/tests/app/invite/test_invite_rest.py @@ -33,7 +33,7 @@ def test_create_invited_user(admin_request, sample_service, mocker, invitation_e assert json_resp['data']['id'] notification = Notification.query.first() - assert notification.reply_to_text == "notify@gov.uk" + assert notification.reply_to_text == invite_from.email_address mocked.assert_called_once_with([(str(notification.id))], queue="notify-internal-tasks") From 17c239bf3d1bb689979d4e4d0c955512edd463e5 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Fri, 15 Dec 2017 16:48:23 +0000 Subject: [PATCH 3/9] 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 4/9] 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 5/9] 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 6/9] 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 7/9] 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 8/9] 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 9/9] 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