From c29dd237027dcbe6bd2073aaa774d0f90039d75b Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Fri, 1 Jul 2016 14:06:32 +0100 Subject: [PATCH] Add sms sender to service to be used in sms templates in place of default numeric short code. If not present default short code is used. --- app/celery/provider_tasks.py | 30 +++++----- app/clients/sms/firetext.py | 4 +- app/clients/sms/mmg.py | 4 +- app/models.py | 2 +- app/schemas.py | 6 ++ .../versions/0036_service_sms_sender.py | 28 +++++++++ tests/app/celery/test_provider_tasks.py | 44 ++++++++++++-- tests/app/clients/test_firetext.py | 17 ++++++ tests/app/clients/test_mmg.py | 15 +++++ tests/app/service/test_rest.py | 57 +++++++++++++++++++ tests/conftest.py | 1 - 11 files changed, 181 insertions(+), 27 deletions(-) create mode 100644 migrations/versions/0036_service_sms_sender.py diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index e7aeea7ab..c4e896795 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -1,27 +1,24 @@ -import json - from datetime import datetime from monotonic import monotonic from flask import current_app -from app import notify_celery, statsd_client, encryption, clients +from app import notify_celery, statsd_client, clients 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.dao.templates_dao import dao_get_template_by_id + from app.celery.research_mode_tasks import send_sms_response -from notifications_utils.recipients import ( - validate_and_format_phone_number -) - -from app.dao.templates_dao import dao_get_template_by_id -from notifications_utils.template import ( - Template, - unlink_govuk_escaped -) +from notifications_utils.recipients import validate_and_format_phone_number +from notifications_utils.template import Template from app.models import SMS_TYPE @@ -57,12 +54,12 @@ def send_sms_to_provider(self, service_id, notification_id, encrypted_notificati 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( @@ -72,7 +69,8 @@ def send_sms_to_provider(self, service_id, notification_id, encrypted_notificati 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( diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index a0ca80f8c..938a86689 100644 --- a/app/clients/sms/firetext.py +++ b/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 diff --git a/app/clients/sms/mmg.py b/app/clients/sms/mmg.py index 46553ef57..2edd52cca 100644 --- a/app/clients/sms/mmg.py +++ b/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 } diff --git a/app/models.py b/app/models.py index cd6186e33..2a1b35e3b 100644 --- a/app/models.py +++ b/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' diff --git a/app/schemas.py b/app/schemas.py index 3e7741fa7..0cb9010e3 100644 --- a/app/schemas.py +++ b/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: diff --git a/migrations/versions/0036_service_sms_sender.py b/migrations/versions/0036_service_sms_sender.py new file mode 100644 index 000000000..7ad42f4ac --- /dev/null +++ b/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 ### diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index cce6458cf..756eecfe5 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -14,9 +14,7 @@ from app.clients.sms import SmsClientException from app.dao import notifications_dao, provider_details_dao from app.dao import provider_statistics_dao from app.models import Notification, NotificationStatistics, Job -from tests.app.conftest import ( - sample_notification -) +from tests.app.conftest import sample_notification def test_should_by_10_second_delay_as_default(): @@ -49,6 +47,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] @@ -100,7 +99,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() @@ -139,7 +139,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) @@ -289,3 +290,36 @@ def test_should_go_into_technical_error_if_exceeds_retries( job = Job.query.get(notification.job.id) assert job.notification_count == 1 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 + ) diff --git a/tests/app/clients/test_firetext.py b/tests/app/clients/test_firetext.py index 7804eaee9..ea8630913 100644 --- a/tests/app/clients/test_firetext.py +++ b/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' diff --git a/tests/app/clients/test_mmg.py b/tests/app/clients/test_mmg.py index aa5bb9f22..9410c29b0 100644 --- a/tests/app/clients/test_mmg.py +++ b/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' diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index f332ce495..5622b66df 100644 --- a/tests/app/service/test_rest.py +++ b/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']} diff --git a/tests/conftest.py b/tests/conftest.py index 04777f4fc..bf8a68565 100644 --- a/tests/conftest.py +++ b/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