Merge pull request #498 from alphagov/set-sms-sender-api

Set sms sender on service

app/celery/provider_tasks.py

@@ -4,10 +4,14 @@ from flask import current_app
 from app import notify_celery, statsd_client, clients, create_uuid
 from app.clients.email import EmailClientException
 from app.clients.sms import SmsClientException
+
 from app.dao.notifications_dao import (
     update_provider_stats,
     get_notification_by_id,
-    dao_update_notification, update_notification_status_by_id)
+    dao_update_notification,
+    update_notification_status_by_id
+)
+
 from app.dao.provider_details_dao import get_provider_details_by_notification_type
 from app.dao.services_dao import dao_fetch_service_by_id
 from app.celery.research_mode_tasks import send_sms_response, send_email_response
@@ -54,12 +58,12 @@ def send_sms_to_provider(self, service_id, notification_id):
     provider = provider_to_use(SMS_TYPE, notification_id)
     notification = get_notification_by_id(notification_id)
     if notification.status == 'created':
+        template_model = dao_get_template_by_id(notification.template_id, notification.template_version)
         template = Template(
-            dao_get_template_by_id(notification.template_id, notification.template_version).__dict__,
+            template_model.__dict__,
             values={} if not notification.personalisation else notification.personalisation,
             prefix=service.name
         )
-
         try:
             if service.research_mode:
                 send_sms_response.apply_async(
@@ -69,7 +73,8 @@ def send_sms_to_provider(self, service_id, notification_id):
                 provider.send_sms(
                     to=validate_and_format_phone_number(notification.to),
                     content=template.replaced,
-                    reference=str(notification_id)
+                    reference=str(notification_id),
+                    sender=service.sms_sender
                 )
 
                 update_provider_stats(

app/clients/sms/firetext.py

@@ -67,11 +67,11 @@ class FiretextClient(SmsClient):
     def get_name(self):
         return self.name
 
-    def send_sms(self, to, content, reference):
+    def send_sms(self, to, content, reference, sender=None):
 
         data = {
             "apiKey": self.api_key,
-            "from": self.from_number,
+            "from": self.from_number if sender is None else sender,
             "to": to.replace('+', ''),
             "message": content,
             "reference": reference

app/clients/sms/mmg.py

@@ -67,12 +67,12 @@ class MMGClient(SmsClient):
     def get_name(self):
         return self.name
 
-    def send_sms(self, to, content, reference, multi=True):
+    def send_sms(self, to, content, reference, multi=True, sender=None):
         data = {
             "reqType": "BULK",
             "MSISDN": to,
             "msg": content,
-            "sender": self.from_number,
+            "sender":  self.from_number if sender is None else sender,
             "cid": reference,
             "multi": multi
         }

app/models.py

@@ -103,6 +103,7 @@ class Service(db.Model, Versioned):
     created_by = db.relationship('User')
     created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False)
     reply_to_email_address = db.Column(db.Text, index=False, unique=False, nullable=True)
+    sms_sender = db.Column(db.String(11), nullable=True)
 
 
 class ApiKey(db.Model, Versioned):
@@ -203,7 +204,6 @@ class Template(db.Model, Versioned):
     created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False)
     created_by = db.relationship('User')
 
-
 MMG_PROVIDER = "mmg"
 FIRETEXT_PROVIDER = "firetext"
 SES_PROVIDER = 'ses'

app/schemas.py

@@ -1,3 +1,4 @@
+import re
 from datetime import (
     datetime,
     date
@@ -106,6 +107,11 @@ class ServiceSchema(BaseSchema):
         exclude = ("updated_at", "created_at", "api_keys", "templates", "jobs", 'old_id')
         strict = True
 
+    @validates('sms_sender')
+    def validate_sms_sender(self, value):
+        if value and not re.match('^[a-zA-Z0-9\s]+$', value):
+            raise ValidationError('Only alphanumeric characters allowed')
+
 
 class NotificationModelSchema(BaseSchema):
     class Meta:

migrations/versions/0036_service_sms_sender.py

@@ -0,0 +1,28 @@
+"""empty message
+
+Revision ID: 0036_service_sms_sender
+Revises: 0035_notification_type
+Create Date: 2016-06-30 14:55:33.811696
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = '0036_service_sms_sender'
+down_revision = '0035_notification_type'
+
+from alembic import op
+import sqlalchemy as sa
+
+
+def upgrade():
+    ### commands auto generated by Alembic - please adjust! ###
+    op.add_column('services', sa.Column('sms_sender', sa.String(length=11), nullable=True))
+    op.add_column('services_history', sa.Column('sms_sender', sa.String(length=11), nullable=True))
+    ### end Alembic commands ###
+
+
+def downgrade():
+    ### commands auto generated by Alembic - please adjust! ###
+    op.drop_column('services_history', 'sms_sender')
+    op.drop_column('services', 'sms_sender')
+    ### end Alembic commands ###

tests/app/celery/test_provider_tasks.py

@@ -17,9 +17,7 @@ from app.dao import notifications_dao, provider_details_dao
 from app.dao import provider_statistics_dao
 from app.dao.provider_statistics_dao import get_provider_statistics
 from app.models import Notification, NotificationStatistics, Job
-from tests.app.conftest import (
+from tests.app.conftest import sample_notification
-    sample_notification
-)
 
 
 def test_should_by_10_second_delay_as_default():
@@ -52,6 +50,7 @@ def test_should_by_240_minute_delay_on_retry_two():
 
 def test_should_return_highest_priority_active_provider(notify_db, notify_db_session):
     providers = provider_details_dao.get_provider_details_by_notification_type('sms')
+
     first = providers[0]
     second = providers[1]
 
@@ -103,7 +102,8 @@ def test_should_send_personalised_template_to_correct_sms_provider_and_persist(
     mmg_client.send_sms.assert_called_once_with(
         to=format_phone_number(validate_phone_number("+447234123123")),
         content="Sample service: Hello Jo",
-        reference=str(db_notification.id)
+        reference=str(db_notification.id),
+        sender=None
     )
     notification = Notification.query.filter_by(id=db_notification.id).one()
 
@@ -142,7 +142,8 @@ def test_send_sms_should_use_template_version_from_notification_not_latest(
     mmg_client.send_sms.assert_called_once_with(
         to=format_phone_number(validate_phone_number("+447234123123")),
         content="Sample service: This is a template",
-        reference=str(db_notification.id)
+        reference=str(db_notification.id),
+        sender=None
     )
 
     persisted_notification = notifications_dao.get_notification(sample_template.service_id, db_notification.id)
@@ -294,6 +295,39 @@ def test_should_go_into_technical_error_if_exceeds_retries(
     assert job.notifications_failed == 1
 
 
+def test_should_send_sms_sender_from_service_if_present(
+        notify_db,
+        notify_db_session,
+        sample_service,
+        sample_template,
+        mocker):
+    db_notification = sample_notification(notify_db, notify_db_session, template=sample_template,
+                                          to_field="+447234123123",
+                                          status='created')
+
+    sample_service.sms_sender = 'elevenchars'
+    notify_db.session.add(sample_service)
+    notify_db.session.commit()
+
+    mocker.patch('app.mmg_client.send_sms')
+    mocker.patch('app.mmg_client.get_name', return_value="mmg")
+    mocker.patch('app.statsd_client.incr')
+    mocker.patch('app.statsd_client.timing_with_dates')
+    mocker.patch('app.statsd_client.timing')
+
+    send_sms_to_provider(
+        db_notification.service_id,
+        db_notification.id
+    )
+
+    mmg_client.send_sms.assert_called_once_with(
+        to=format_phone_number(validate_phone_number("+447234123123")),
+        content="Sample service: This is a template",
+        reference=str(db_notification.id),
+        sender=sample_service.sms_sender
+    )
+
+
 def test_send_email_to_provider_should_call_research_mode_task_response_task_if_research_mode(
         notify_db,
         notify_db_session,

tests/app/clients/test_firetext.py

@@ -94,3 +94,20 @@ def test_send_sms_raises_if_firetext_rejects(mocker, mock_firetext_client):
 
     assert exc.value.code == 1
     assert exc.value.description == 'Some kind of error'
+
+
+def test_send_sms_override_configured_shortcode_with_sender(mocker, mock_firetext_client):
+    to = '+447234567890'
+    content = 'my message'
+    reference = 'my reference'
+    response_dict = {
+        'code': 0,
+    }
+    sender = 'fromservice'
+
+    with requests_mock.Mocker() as request_mock:
+        request_mock.post('https://www.firetext.co.uk/api/sendsms/json', json=response_dict, status_code=200)
+        mock_firetext_client.send_sms(to, content, reference, sender=sender)
+
+    request_args = parse_qs(request_mock.request_history[0].text)
+    assert request_args['from'][0] == 'fromservice'

tests/app/clients/test_mmg.py

@@ -77,3 +77,18 @@ def test_send_sms_raises_if_mmg_rejects(mocker, mock_mmg_client):
 
     assert exc.value.code == 206
     assert exc.value.description == 'Some kind of error'
+
+
+def test_send_sms_override_configured_shortcode_with_sender(mocker, mock_mmg_client):
+    to = '+447234567890'
+    content = 'my message'
+    reference = 'my reference'
+    response_dict = {'Reference': 12345678}
+    sender = 'fromservice'
+
+    with requests_mock.Mocker() as request_mock:
+        request_mock.post('https://www.mmgrp.co.uk/API/json/api.php', json=response_dict, status_code=200)
+        mock_mmg_client.send_sms(to, content, reference, sender=sender)
+
+    request_args = request_mock.request_history[0].json()
+    assert request_args['sender'] == 'fromservice'

tests/app/service/test_rest.py

@@ -1029,3 +1029,60 @@ def test_get_all_notifications_for_service_in_order(notify_api, notify_db, notif
         assert resp['notifications'][1]['to'] == notification_2.to
         assert resp['notifications'][2]['to'] == notification_1.to
         assert response.status_code == 200
+
+
+def test_set_sms_sender_for_service(notify_api, sample_service):
+    with notify_api.test_request_context():
+        with notify_api.test_client() as client:
+            auth_header = create_authorization_header()
+            resp = client.get(
+                '/service/{}'.format(sample_service.id),
+                headers=[auth_header]
+            )
+            json_resp = json.loads(resp.get_data(as_text=True))
+            assert resp.status_code == 200
+            assert json_resp['data']['name'] == sample_service.name
+
+            data = {
+                'sms_sender': 'elevenchars',
+            }
+
+            auth_header = create_authorization_header()
+
+            resp = client.post(
+                '/service/{}'.format(sample_service.id),
+                data=json.dumps(data),
+                headers=[('Content-Type', 'application/json'), auth_header]
+            )
+            result = json.loads(resp.get_data(as_text=True))
+            assert resp.status_code == 200
+            assert result['data']['sms_sender'] == 'elevenchars'
+
+
+def test_set_sms_sender_for_service_rejects_invalid_characters(notify_api, sample_service):
+    with notify_api.test_request_context():
+        with notify_api.test_client() as client:
+            auth_header = create_authorization_header()
+            resp = client.get(
+                '/service/{}'.format(sample_service.id),
+                headers=[auth_header]
+            )
+            json_resp = json.loads(resp.get_data(as_text=True))
+            assert resp.status_code == 200
+            assert json_resp['data']['name'] == sample_service.name
+
+            data = {
+                'sms_sender': 'invalid####',
+            }
+
+            auth_header = create_authorization_header()
+
+            resp = client.post(
+                '/service/{}'.format(sample_service.id),
+                data=json.dumps(data),
+                headers=[('Content-Type', 'application/json'), auth_header]
+            )
+            result = json.loads(resp.get_data(as_text=True))
+            assert resp.status_code == 400
+            assert result['result'] == 'error'
+            assert result['message'] == {'sms_sender': ['Only alphanumeric characters allowed']}

tests/conftest.py

@@ -7,7 +7,6 @@ from alembic.command import upgrade
 from alembic.config import Config
 from flask.ext.migrate import Migrate, MigrateCommand
 from flask.ext.script import Manager
-from sqlalchemy.schema import MetaData
 
 from app import create_app, db