Merge pull request #1364 from alphagov/put-sms-prefix-on-model

Work out SMS message prefixing in a way that is aware of multiple SMS senders

app/delivery/send_to_providers.py

@@ -47,11 +47,14 @@ def send_sms_to_provider(notification):
             "Starting sending SMS {} to provider at {}".format(notification.id, datetime.utcnow())
         )
         template_model = dao_get_template_by_id(notification.template_id, notification.template_version)
+
+        sender_has_been_customised = (not service.get_prefix_sms_with_service_name())
+
         template = SMSMessageTemplate(
             template_model.__dict__,
             values=notification.personalisation,
             prefix=service.name,
-            sender=service.sms_sender not in {None, current_app.config['FROM_NUMBER']}
+            sender=sender_has_been_customised,
         )
 
         if service.research_mode or notification.key_type == KEY_TYPE_TEST:

app/models.py

@@ -224,6 +224,7 @@ class Service(db.Model, Versioned):
     _reply_to_email_address = db.Column("reply_to_email_address", db.Text, index=False, unique=False, nullable=True)
     _letter_contact_block = db.Column('letter_contact_block', db.Text, index=False, unique=False, nullable=True)
     sms_sender = db.Column(db.String(11), nullable=False, default=lambda: current_app.config['FROM_NUMBER'])
+    prefix_sms = db.Column(db.Boolean, nullable=True)
     organisation_id = db.Column(UUID(as_uuid=True), db.ForeignKey('organisation.id'), index=True, nullable=True)
     free_sms_fragment_limit = db.Column(db.BigInteger, index=False, unique=False, nullable=True)
     organisation = db.relationship('Organisation')
@@ -280,6 +281,11 @@ class Service(db.Model, Versioned):
         default_letter_contact = [x for x in self.letter_contacts if x.is_default]
         return default_letter_contact[0].contact_block if default_letter_contact else None
 
+    def get_prefix_sms_with_service_name(self):
+        if self.prefix_sms is not None:
+            return self.prefix_sms
+        return self.get_default_sms_sender() == current_app.config['FROM_NUMBER']
+
 
 class AnnualBilling(db.Model):
     __tablename__ = "annual_billing"

app/schemas.py

@@ -188,6 +188,7 @@ class ServiceSchema(BaseSchema):
     reply_to_email_address = fields.Method(method_name="get_reply_to_email_address")
     sms_sender = fields.Method(method_name="get_sms_sender")
     letter_contact_block = fields.Method(method_name="get_letter_contact")
+    prefix_sms_with_service_name = fields.Method(method_name="get_prefix_sms_with_service_name")
 
     def service_permissions(self, service):
         return [p.permission for p in service.permissions]
@@ -201,6 +202,9 @@ class ServiceSchema(BaseSchema):
     def get_letter_contact(self, service):
         return service.get_default_letter_contact()
 
+    def get_prefix_sms_with_service_name(self, service):
+        return service.get_prefix_sms_with_service_name()
+
     class Meta:
         model = models.Service
         dump_only = ['reply_to_email_address', 'letter_contact_block']

migrations/versions/0132_add_sms_prefix_setting.py

@@ -0,0 +1,23 @@
+"""
+
+Revision ID: 0132_add_sms_prefix_setting
+Revises: 0131_user_auth_types
+Create Date: 2017-11-03 11:07:40.537006
+
+"""
+from alembic import op
+import sqlalchemy as sa
+from sqlalchemy.dialects import postgresql
+
+revision = '0132_add_sms_prefix_setting'
+down_revision = '0131_user_auth_types'
+
+
+def upgrade():
+    op.add_column('services', sa.Column('prefix_sms', sa.Boolean(), nullable=True))
+    op.add_column('services_history', sa.Column('prefix_sms', sa.Boolean(), nullable=True))
+
+
+def downgrade():
+    op.drop_column('services_history', 'prefix_sms')
+    op.drop_column('services', 'prefix_sms')

tests/app/db.py

@@ -68,7 +68,8 @@ def create_service(
     research_mode=False,
     active=True,
     do_create_inbound_number=True,
-    email_from=None
+    email_from=None,
+    prefix_sms=None,
 ):
     service = Service(
         name=service_name,
@@ -77,6 +78,7 @@ def create_service(
         email_from=email_from if email_from else service_name.lower().replace(' ', '.'),
         created_by=user or create_user(email='{}@digital.cabinet-office.gov.uk'.format(uuid.uuid4())),
         sms_sender=sms_sender,
+        prefix_sms=prefix_sms,
     )
 
     dao_create_service(service, service.created_by, service_id, service_permissions=service_permissions)

tests/app/delivery/test_send_to_providers.py

@@ -722,6 +722,39 @@ def test_should_handle_sms_sender_and_prefix_message(
     )
 
 
+@pytest.mark.parametrize('sms_sender, prefix_setting, expected_content', [
+    ('foo', True, 'Sample service: bar'),
+    ('foo', False, 'bar'),
+    ('foo', None, 'bar'),
+    # 'testing' is the default SMS sender in unit tests
+    ('testing', None, 'Sample service: bar'),
+    ('testing', False, 'bar'),
+])
+def test_should_handle_sms_prefix_setting(
+    mocker,
+    sms_sender,
+    prefix_setting,
+    expected_content,
+    notify_db_session
+):
+    mocker.patch('app.mmg_client.send_sms')
+    mocker.patch('app.delivery.send_to_providers.create_initial_notification_statistic_tasks')
+    service = create_service(
+        sms_sender=sms_sender,
+        prefix_sms=prefix_setting,
+    )
+    template = create_template(service, content='bar')
+    notification = create_notification(template)
+
+    send_to_providers.send_sms_to_provider(notification)
+    mmg_client.send_sms.assert_called_once_with(
+        content=expected_content,
+        sender=ANY,
+        to=ANY,
+        reference=ANY,
+    )
+
+
 def test_should_use_inbound_number_as_sender_if_default_sms_sender(
         notify_db_session,
         mocker

tests/app/service/test_rest.py

@@ -156,6 +156,7 @@ def test_get_service_by_id(client, sample_service):
     assert json_resp['data']['branding'] == 'govuk'
     assert json_resp['data']['dvla_organisation'] == '001'
     assert json_resp['data']['sms_sender'] == current_app.config['FROM_NUMBER']
+    assert json_resp['data']['prefix_sms_with_service_name'] is True
 
 
 def test_get_service_by_id_returns_free_sms_limit(client, sample_service):
@@ -1561,6 +1562,36 @@ def test_set_sms_sender_for_service_rejects_null(client, sample_service):
     assert result['message'] == {'sms_sender': ['Field may not be null.']}
 
 
+@pytest.mark.parametrize('default_sms_sender, should_prefix', [
+    (None, True),  # None means use default
+    ('Foo', False),
+])
+def test_prefixing_messages_based_on_sms_sender(
+    client,
+    notify_db_session,
+    default_sms_sender,
+    should_prefix,
+):
+    service = create_service(
+        sms_sender=default_sms_sender or current_app.config['FROM_NUMBER']
+    )
+    create_service_sms_sender(
+        service=service,
+        sms_sender='ignored',
+        is_default=False,
+    )
+
+    result = client.get(
+        url_for(
+            'service.get_service_by_id',
+            service_id=service.id
+        ),
+        headers=[('Content-Type', 'application/json'), create_authorization_header()]
+    )
+    service = json.loads(result.get_data(as_text=True))['data']
+    assert service['prefix_sms_with_service_name'] == should_prefix
+
+
 @pytest.mark.parametrize('today_only,stats', [
     ('False', {'requested': 2, 'delivered': 1, 'failed': 0}),
     ('True', {'requested': 1, 'delivered': 0, 'failed': 0})