Merge pull request #1505 from alphagov/set-sms-sender-to-international-format

Set SMS sender to international format

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
 )
@@ -366,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),
@@ -636,7 +640,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
 

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()
 

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()

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

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
 ):

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
 ):