From 75d99ea3977aa06bd2e4863159c259cbd280b2ee Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 23 Nov 2017 14:55:49 +0000 Subject: [PATCH] [WIP] --- app/models.py | 2 + app/notifications/process_notifications.py | 6 ++- app/notifications/validators.py | 2 +- app/v2/notifications/post_notifications.py | 32 ++++++++++---- ...o.py => 0144_add_notification_reply_to.py} | 0 tests/app/notifications/test_validators.py | 2 +- .../notifications/test_post_notifications.py | 42 +++++++++++++++++-- 7 files changed, 70 insertions(+), 16 deletions(-) rename migrations/versions/{0145_add_notification_reply_to.py => 0144_add_notification_reply_to.py} (100%) diff --git a/app/models.py b/app/models.py index a1322cf75..a95c09a6f 100644 --- a/app/models.py +++ b/app/models.py @@ -978,6 +978,8 @@ class Notification(db.Model): created_by = db.relationship('User') created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), nullable=True) + reply_to_text = db.Column(db.String, nullable=True) + __table_args__ = ( db.ForeignKeyConstraint( ['template_id', 'template_version'], diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 76e18ebf4..c2d2f1a52 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -54,7 +54,8 @@ def persist_notification( notification_id=None, simulated=False, created_by_id=None, - status=NOTIFICATION_CREATED + status=NOTIFICATION_CREATED, + reply_to_text=None ): notification_created_at = created_at or datetime.utcnow() if not notification_id: @@ -76,7 +77,8 @@ def persist_notification( client_reference=client_reference, reference=reference, created_by_id=created_by_id, - status=status + status=status, + reply_to_text=reply_to_text, ) if notification_type == SMS_TYPE: diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 5aee7c55b..9a17ab69d 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -142,7 +142,7 @@ def check_service_email_reply_to_id(service_id, reply_to_id, notification_type): message = 'email_reply_to_id is not a valid option for {} notification'.format(notification_type) raise BadRequestError(message=message) try: - dao_get_reply_to_by_id(service_id, reply_to_id) + return dao_get_reply_to_by_id(service_id, reply_to_id).email_address except NoResultFound: message = 'email_reply_to_id {} does not exist in database for service id {}'\ .format(reply_to_id, service_id) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 90cd2997f..fa53176ea 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -63,15 +63,12 @@ def post_notification(notification_type): check_service_has_permission(notification_type, authenticated_service.permissions) scheduled_for = form.get("scheduled_for", None) - service_email_reply_to_id = form.get("email_reply_to_id", None) - service_sms_sender_id = form.get("sms_sender_id", None) check_service_can_schedule_notification(authenticated_service.permissions, scheduled_for) check_rate_limiting(authenticated_service, api_user) - check_service_email_reply_to_id(str(authenticated_service.id), service_email_reply_to_id, notification_type) - sms_sender = check_service_sms_sender_id(str(authenticated_service.id), service_sms_sender_id, notification_type) + reply_to = get_reply_to_text(notification_type, form) template, template_with_content = validate_template( form['template_id'], @@ -92,13 +89,14 @@ def post_notification(notification_type): notification_type=notification_type, api_key=api_user, template=template, - service=authenticated_service + service=authenticated_service, + reply_to_text=reply_to ) if notification_type == SMS_TYPE: create_resp_partial = functools.partial( create_post_sms_response_from_notification, - from_number=sms_sender or authenticated_service.get_default_sms_sender() + from_number=reply_to ) elif notification_type == EMAIL_TYPE: create_resp_partial = functools.partial( @@ -121,7 +119,7 @@ def post_notification(notification_type): return jsonify(resp), 201 -def process_sms_or_email_notification(*, form, notification_type, api_key, template, service): +def process_sms_or_email_notification(*, form, notification_type, api_key, template, service, reply_to_text=None): form_send_to = form['email_address'] if notification_type == EMAIL_TYPE else form['phone_number'] send_to = validate_and_format_recipient(send_to=form_send_to, @@ -142,7 +140,8 @@ def process_sms_or_email_notification(*, form, notification_type, api_key, templ api_key_id=api_key.id, key_type=api_key.key_type, client_reference=form.get('reference', None), - simulated=simulated + simulated=simulated, + reply_to_text=reply_to_text ) persist_sender_to_notification_mapping(form, notification) @@ -194,3 +193,20 @@ def process_letter_notification(*, letter_data, api_key, template): ) return notification + + +def get_reply_to_text(notification_type, form): + service_email_reply_to_id = form.get("email_reply_to_id", None) + service_sms_sender_id = form.get("sms_sender_id", None) + + reply_to = check_service_email_reply_to_id( + str(authenticated_service.id), service_email_reply_to_id, notification_type + ) + reply_to = check_service_sms_sender_id(str(authenticated_service.id), service_sms_sender_id, notification_type) + + if not reply_to: + if notification_type == EMAIL_TYPE: + reply_to = authenticated_service.get_default_reply_to_email_address() + elif notification_type == SMS_TYPE: + reply_to = authenticated_service.get_default_sms_sender() + return reply_to diff --git a/migrations/versions/0145_add_notification_reply_to.py b/migrations/versions/0144_add_notification_reply_to.py similarity index 100% rename from migrations/versions/0145_add_notification_reply_to.py rename to migrations/versions/0144_add_notification_reply_to.py diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index a20251790..7e4d8bfa9 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -338,7 +338,7 @@ def test_check_service_email_reply_to_id_where_reply_to_id_is_none(notification_ def test_check_service_email_reply_to_where_email_reply_to_is_found(sample_service): reply_to_address = create_reply_to_email(sample_service, "test@test.com") - assert check_service_email_reply_to_id(sample_service.id, reply_to_address.id, EMAIL_TYPE) is None + assert check_service_email_reply_to_id(sample_service.id, reply_to_address.id, EMAIL_TYPE) == "test@test.com" def test_check_service_email_reply_to_id_where_service_id_is_not_found(sample_service, fake_uuid): diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index a6ed18ff4..6fc197e42 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -3,14 +3,14 @@ import uuid import pytest from freezegun import freeze_time -from app.dao.service_sms_sender_dao import update_existing_sms_sender_with_inbound_number +from app.dao.service_sms_sender_dao import dao_update_service_sms_sender from app.models import ( NotificationEmailReplyTo, ScheduledNotification, SCHEDULE_NOTIFICATIONS, EMAIL_TYPE, SMS_TYPE, - NotificationSmsSender, ServiceSmsSender + NotificationSmsSender ) from flask import json, current_app @@ -31,7 +31,7 @@ from tests.app.db import ( create_service, create_template, create_reply_to_email, - create_service_sms_sender, create_notification, create_inbound_number, + create_service_sms_sender, create_notification, create_service_with_inbound_number ) @@ -96,13 +96,13 @@ def test_post_sms_notification_uses_inbound_number_as_sender(client, notify_db_s notification_id = notifications[0].id assert resp_json['id'] == str(notification_id) assert resp_json['content']['from_number'] == '1' + assert notifications[0].reply_to_text == '1' mocked.assert_called_once_with([str(notification_id)], queue='send-sms-tasks') def test_post_sms_notification_returns_201_with_sms_sender_id( client, sample_template_with_placeholders, mocker ): - sms_sender = create_service_sms_sender(service=sample_template_with_placeholders.service, sms_sender='123456') mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') data = { @@ -125,9 +125,40 @@ def test_post_sms_notification_returns_201_with_sms_sender_id( assert str(notification_to_sms_sender[0].notification_id) == resp_json['id'] assert resp_json['content']['from_number'] == sms_sender.sms_sender assert notification_to_sms_sender[0].service_sms_sender_id == sms_sender.id + notifications = Notification.query.all() + assert len(notifications) == 1 + assert notifications[0].reply_to_text == sms_sender.sms_sender mocked.assert_called_once_with([resp_json['id']], queue='send-sms-tasks') +def test_notification_reply_to_text_is_original_value_if_sender_is_changed_after_post_notification( + client, sample_template, mocker +): + sms_sender = create_service_sms_sender(service=sample_template.service, sms_sender='123456', is_default=False) + mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + data = { + 'phone_number': '+447700900855', + 'template_id': str(sample_template.id), + 'sms_sender_id': str(sms_sender.id) + } + auth_header = create_authorization_header(service_id=sample_template.service_id) + + response = client.post( + path='/v2/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + dao_update_service_sms_sender(service_id=sample_template.service_id, + service_sms_sender_id=sms_sender.id, + is_default=sms_sender.is_default, + sms_sender='updated') + + assert response.status_code == 201 + notifications = Notification.query.all() + assert len(notifications) == 1 + assert notifications[0].reply_to_text == '123456' + + @pytest.mark.parametrize("notification_type, key_send_to, send_to", [("sms", "phone_number", "+447700900855"), ("email", "email_address", "sample@email.com")]) @@ -225,6 +256,7 @@ def test_post_email_notification_returns_201(client, sample_email_template_with_ assert resp_json['id'] == str(notification.id) assert resp_json['reference'] == reference assert notification.reference is None + assert notification.reply_to_text is None assert resp_json['content']['body'] == sample_email_template_with_placeholders.content \ .replace('((name))', 'Bob').replace('GOV.UK', u'GOV.\u200bUK') assert resp_json['content']['subject'] == sample_email_template_with_placeholders.subject \ @@ -590,6 +622,7 @@ def test_post_email_notification_with_valid_reply_to_id_returns_201(client, samp resp_json = json.loads(response.get_data(as_text=True)) assert validate(resp_json, post_email_response) == resp_json notification = Notification.query.first() + assert notification.reply_to_text == 'test@test.com' assert resp_json['id'] == str(notification.id) assert mocked.called @@ -637,6 +670,7 @@ def test_post_email_notification_with_valid_reply_to_id_returns_201(client, samp assert email_reply_to.notification_id == notification.id assert email_reply_to.service_email_reply_to_id == reply_to_email.id + assert notification.reply_to_text == reply_to_email.email_address def test_persist_sender_to_notification_mapping_for_email(notify_db_session):