From 92b605833ff825094cc57b88e7f629a364d529a9 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 3 Nov 2017 09:54:04 +0000 Subject: [PATCH 1/3] Add prefix SMS with service name to service model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the service is sending messages from GOVUK, then its messages should be prefixed with the service name. Right now this logic is: - worked out separately in the admin app and API - isn’t aware of multiple senders This commit moves the logic to one place (the service model). It does this in a slightly naive way, in that it only looks at the default sender, not the actual sender of the message. In the future this will go away because we’ll move it to being a setting that’s controlled independently of the service name. But this is the first step towards that. fixup! Add prefix SMS with service name to service model --- app/models.py | 3 +++ app/schemas.py | 4 ++++ tests/app/service/test_rest.py | 31 +++++++++++++++++++++++++++++++ 3 files changed, 38 insertions(+) diff --git a/app/models.py b/app/models.py index 76bf0edfd..1c571ad2c 100644 --- a/app/models.py +++ b/app/models.py @@ -280,6 +280,9 @@ 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 prefix_sms_with_service_name(self): + return self.get_default_sms_sender() == current_app.config['FROM_NUMBER'] + class AnnualBilling(db.Model): __tablename__ = "annual_billing" diff --git a/app/schemas.py b/app/schemas.py index a143e3bbb..3916c27bb 100644 --- a/app/schemas.py +++ b/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.prefix_sms_with_service_name() + class Meta: model = models.Service dump_only = ['reply_to_email_address', 'letter_contact_block'] diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index e317f7a24..6da52540d 100644 --- a/tests/app/service/test_rest.py +++ b/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}) From e1239bea06358d2ee71d0bf341dc7c7a404aa3c0 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 3 Nov 2017 10:31:28 +0000 Subject: [PATCH 2/3] Use model to work out whether to prefix message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `service.sms_sender` has been deprecated; we should be looking at which of the service’s SMS senders is default to work out if the message has been sent from GOVUK or not (and if it has, then prefix the message with the service name). The arguments to `SMSMessageTemplate` are _super_ badly named – `sender` isn’t really used as a string, it’s a boolean that effectively means ‘is this a custom sender (`True`) or the platform default (`False`)’. We should rename it once this bug is fixed. --- app/delivery/send_to_providers.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 324c21582..74316dc08 100644 --- a/app/delivery/send_to_providers.py +++ b/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.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: From cccffdd2acb443f1dd267df166abba0d48cc58c7 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 3 Nov 2017 11:39:25 +0000 Subject: [PATCH 3/3] Add a column for a service to set SMS prefixing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In future changes, services will be able to control whether their text messages will be prefixed with the name of their service. This commit: - adds a column to store the value of that setting - makes the service model take notice of it, if it were to have a value set It doesn’t: - provide a way of setting the value of this column Currently the column can have three values: - `None` – ignore it (this is what all current services will start as) and continue to determine whether to prefix messages by looking at the sender - `True` – always the service name to the start of text messages - `False` – never add the service name to the start of text messages In the future we’ll migrate all services to be either `True` or `False`, the `None` will go away and all services will have direct control over the setting. --- app/delivery/send_to_providers.py | 2 +- app/models.py | 5 ++- app/schemas.py | 2 +- .../versions/0132_add_sms_prefix_setting.py | 23 +++++++++++++ tests/app/db.py | 4 ++- tests/app/delivery/test_send_to_providers.py | 33 +++++++++++++++++++ 6 files changed, 65 insertions(+), 4 deletions(-) create mode 100644 migrations/versions/0132_add_sms_prefix_setting.py diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 74316dc08..ab56a7e05 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -48,7 +48,7 @@ def send_sms_to_provider(notification): ) template_model = dao_get_template_by_id(notification.template_id, notification.template_version) - sender_has_been_customised = (not service.prefix_sms_with_service_name()) + sender_has_been_customised = (not service.get_prefix_sms_with_service_name()) template = SMSMessageTemplate( template_model.__dict__, diff --git a/app/models.py b/app/models.py index 1c571ad2c..6037bd26d 100644 --- a/app/models.py +++ b/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,7 +281,9 @@ 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 prefix_sms_with_service_name(self): + 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'] diff --git a/app/schemas.py b/app/schemas.py index 3916c27bb..76d91b23d 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -203,7 +203,7 @@ class ServiceSchema(BaseSchema): return service.get_default_letter_contact() def get_prefix_sms_with_service_name(self, service): - return service.prefix_sms_with_service_name() + return service.get_prefix_sms_with_service_name() class Meta: model = models.Service diff --git a/migrations/versions/0132_add_sms_prefix_setting.py b/migrations/versions/0132_add_sms_prefix_setting.py new file mode 100644 index 000000000..e461a008c --- /dev/null +++ b/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') diff --git a/tests/app/db.py b/tests/app/db.py index 4be3a3799..c070e4dca 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -67,7 +67,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, @@ -76,6 +77,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) diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 2d6a10cdf..9831091d1 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/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