From 545ff7dda60e83f00f907f2d53a5a7710db449ed Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Mon, 18 Dec 2017 16:14:09 +0000 Subject: [PATCH 1/3] Normalize outbound SMS number for job notifications Replaces 0 with 44 in UK SMS sender number to make it possible to respond to the SMS from an international number. --- app/models.py | 3 ++- tests/app/celery/test_tasks.py | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/app/models.py b/app/models.py index 469bfff4a..6331c9076 100644 --- a/app/models.py +++ b/app/models.py @@ -14,6 +14,7 @@ from sqlalchemy import UniqueConstraint, CheckConstraint from notifications_utils.recipients import ( validate_email_address, validate_phone_number, + try_validate_and_format_phone_number, InvalidPhoneError, InvalidEmailError ) @@ -636,7 +637,7 @@ class TemplateBase(db.Model): 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() + return try_validate_and_format_phone_number(self.service.get_default_sms_sender()) else: return None diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 882fc5b9f..18f1fa9e5 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1081,6 +1081,25 @@ def test_save_letter_uses_template_reply_to_text(mocker, notify_db_session): assert notification_db.reply_to_text == "Template address contact" +def test_save_sms_uses_sms_sender_reply_to_text(mocker, notify_db_session): + service = create_service_with_defined_sms_sender(sms_sender_value='07123123123') + template = create_template(service=service) + + notification = _notification_json(template, to="07700 900205") + mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + + notification_id = uuid.uuid4() + save_sms( + service.id, + notification_id, + encryption.encrypt(notification), + key_type=KEY_TYPE_TEST + ) + + persisted_notification = Notification.query.one() + assert persisted_notification.reply_to_text == '447123123123' + + def test_save_letter_calls_update_noti_to_sent_task_with_letters_as_pdf_permission_in_research_mode( mocker, notify_db_session, sample_letter_job): sample_letter_job.service.research_mode = True From d5b6bd85fec7769ae8a25976b1a85f051d94ba70 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Mon, 18 Dec 2017 16:16:33 +0000 Subject: [PATCH 2/3] Normalize outbound SMS sender number in one off notifications --- app/models.py | 3 ++ app/service/send_notification.py | 2 +- .../service/test_send_one_off_notification.py | 54 +++++++++++++++++++ 3 files changed, 58 insertions(+), 1 deletion(-) diff --git a/app/models.py b/app/models.py index 6331c9076..214979611 100644 --- a/app/models.py +++ b/app/models.py @@ -367,6 +367,9 @@ class ServiceSmsSender(db.Model): created_at = db.Column(db.DateTime, default=datetime.datetime.utcnow, nullable=False) updated_at = db.Column(db.DateTime, nullable=True, onupdate=datetime.datetime.utcnow) + def get_reply_to_text(self): + return try_validate_and_format_phone_number(self.sms_sender) + def serialize(self): return { "id": str(self.id), diff --git a/app/service/send_notification.py b/app/service/send_notification.py index 53cbb2b6c..2b9be3de7 100644 --- a/app/service/send_notification.py +++ b/app/service/send_notification.py @@ -90,7 +90,7 @@ def get_reply_to_text(notification_type, sender_id, service, template): if notification_type == EMAIL_TYPE: reply_to = dao_get_reply_to_by_id(service.id, sender_id).email_address elif notification_type == SMS_TYPE: - reply_to = dao_get_service_sms_senders_by_id(service.id, sender_id).sms_sender + reply_to = dao_get_service_sms_senders_by_id(service.id, sender_id).get_reply_to_text() else: reply_to = template.get_reply_to_text() diff --git a/tests/app/service/test_send_one_off_notification.py b/tests/app/service/test_send_one_off_notification.py index e64102ec1..cbbe8f7dd 100644 --- a/tests/app/service/test_send_one_off_notification.py +++ b/tests/app/service/test_send_one_off_notification.py @@ -19,6 +19,7 @@ from tests.app.db import ( create_user, create_reply_to_email, create_letter_contact, + create_service_sms_sender, create_service, create_template ) @@ -238,9 +239,62 @@ def test_send_one_off_letter_notification_should_use_template_reply_to_text(samp research_mode=False, queue=None ) + assert notification.reply_to_text == "Edinburgh, ED1 1AA" +def test_send_one_off_sms_notification_should_use_sms_sender_reply_to_text(sample_service, celery_mock): + template = create_template(service=sample_service, template_type=SMS_TYPE) + sms_sender = create_service_sms_sender( + service=sample_service, + sms_sender='07123123123', + is_default=False + ) + + data = { + 'to': '07111111111', + 'template_id': str(template.id), + 'created_by': str(sample_service.created_by_id), + 'sender_id': str(sms_sender.id), + } + + notification_id = send_one_off_notification(service_id=sample_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 == "447123123123" + + +def test_send_one_off_sms_notification_should_use_default_service_reply_to_text(sample_service, celery_mock): + template = create_template(service=sample_service, template_type=SMS_TYPE) + sample_service.service_sms_senders[0].is_default = False + create_service_sms_sender( + service=sample_service, + sms_sender='07123123456', + is_default=True + ) + + data = { + 'to': '07111111111', + 'template_id': str(template.id), + 'created_by': str(sample_service.created_by_id), + } + + notification_id = send_one_off_notification(service_id=sample_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 == "447123123456" + + def test_send_one_off_notification_should_throw_exception_if_reply_to_id_doesnot_exist( sample_email_template ): From 7df9fdf42ae56dddda48bb348218b2b871e64da3 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Mon, 18 Dec 2017 16:17:20 +0000 Subject: [PATCH 3/3] Normalize outbound SMS sender number in notifications API Some of the tests don't set up a service sms_sender, so we need to check if it's None before trying to format the number. --- app/v2/notifications/post_notifications.py | 10 +++- .../notifications/test_post_notifications.py | 55 +++++++++++++++++++ 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 5466be234..bcb2b4b7e 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -2,6 +2,8 @@ import functools from flask import request, jsonify, current_app, abort +from notifications_utils.recipients import try_validate_and_format_phone_number + from app import api_user, authenticated_service from app.config import QueueNames from app.models import ( @@ -208,9 +210,13 @@ def get_reply_to_text(notification_type, form, template): elif notification_type == SMS_TYPE: service_sms_sender_id = form.get("sms_sender_id", None) - reply_to = check_service_sms_sender_id( + sms_sender_id = check_service_sms_sender_id( str(authenticated_service.id), service_sms_sender_id, notification_type - ) or template.get_reply_to_text() + ) + if sms_sender_id: + reply_to = try_validate_and_format_phone_number(sms_sender_id) + else: + reply_to = template.get_reply_to_text() elif notification_type == LETTER_TYPE: reply_to = template.get_reply_to_text() diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index cba00ff9b..df669d258 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -98,6 +98,34 @@ def test_post_sms_notification_uses_inbound_number_as_sender(client, notify_db_s mocked.assert_called_once_with([str(notification_id)], queue='send-sms-tasks') +def test_post_sms_notification_uses_inbound_number_reply_to_as_sender(client, notify_db_session, mocker): + service = create_service_with_inbound_number(inbound_number='07123123123') + + template = create_template(service=service, content="Hello (( Name))\nYour thing is due soon") + mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + data = { + 'phone_number': '+447700900855', + 'template_id': str(template.id), + 'personalisation': {' Name': 'Jo'} + } + auth_header = create_authorization_header(service_id=service.id) + + response = client.post( + path='/v2/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + assert response.status_code == 201 + resp_json = json.loads(response.get_data(as_text=True)) + assert validate(resp_json, post_sms_response) == resp_json + notifications = Notification.query.all() + assert len(notifications) == 1 + notification_id = notifications[0].id + assert resp_json['id'] == str(notification_id) + assert resp_json['content']['from_number'] == '447123123123' + assert notifications[0].reply_to_text == '447123123123' + 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 ): @@ -125,6 +153,33 @@ def test_post_sms_notification_returns_201_with_sms_sender_id( mocked.assert_called_once_with([resp_json['id']], queue='send-sms-tasks') +def test_post_sms_notification_uses_sms_sender_id_reply_to( + client, sample_template_with_placeholders, mocker +): + sms_sender = create_service_sms_sender(service=sample_template_with_placeholders.service, sms_sender='07123123123') + mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + data = { + 'phone_number': '+447700900855', + 'template_id': str(sample_template_with_placeholders.id), + 'personalisation': {' Name': 'Jo'}, + 'sms_sender_id': str(sms_sender.id) + } + auth_header = create_authorization_header(service_id=sample_template_with_placeholders.service_id) + + response = client.post( + path='/v2/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + assert response.status_code == 201 + resp_json = json.loads(response.get_data(as_text=True)) + assert validate(resp_json, post_sms_response) == resp_json + assert resp_json['content']['from_number'] == '447123123123' + notifications = Notification.query.all() + assert len(notifications) == 1 + assert notifications[0].reply_to_text == '447123123123' + 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 ):