From a358acfd052279fa0f2b06836fcbb36912d143df Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 7 Nov 2017 11:08:26 +0000 Subject: [PATCH 1/3] Remove references to computed service attribute `prefix_sms` is the real database column, which should be referred to from now on. --- app/main/views/service_settings.py | 5 ++--- app/templates/views/service-settings.html | 2 +- app/utils.py | 2 +- tests/__init__.py | 6 ++---- 4 files changed, 6 insertions(+), 9 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index ad5817fa1..effcea000 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -109,8 +109,7 @@ def service_settings(service_id): default_sms_sender=default_sms_sender, sms_sender_count=sms_sender_count, free_sms_fragment_limit=free_sms_fragment_limit, - prefix_sms_with_service_name=current_service['prefix_sms_with_service_name'], - + prefix_sms=current_service['prefix_sms'], ) @@ -477,7 +476,7 @@ def service_set_sms(service_id): def service_set_sms_prefix(service_id): form = SMSPrefixForm(enabled=( - 'on' if current_service['prefix_sms_with_service_name'] else 'off' + 'on' if current_service['prefix_sms'] else 'off' )) form.enabled.label.text = 'Start all text messages with ‘{}:’'.format(current_service['name']) diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index cebf16b43..07439ba83 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -94,7 +94,7 @@ {% call row() %} {{ text_field('Text messages start with service name') }} - {{ boolean_field(prefix_sms_with_service_name) }} + {{ boolean_field(prefix_sms) }} {{ edit_field('Change', url_for('.service_set_sms_prefix', service_id=current_service.id)) }} {% endcall %} diff --git a/app/utils.py b/app/utils.py index 3a69f4486..5b1052567 100644 --- a/app/utils.py +++ b/app/utils.py @@ -288,7 +288,7 @@ def get_template( return SMSPreviewTemplate( template, prefix=service['name'], - sender=not service['prefix_sms_with_service_name'], + sender=not service['prefix_sms'], show_recipient=show_recipient, redact_missing_personalisation=redact_missing_personalisation, ) diff --git a/tests/__init__.py b/tests/__init__.py index c8f7ec629..372d34f6c 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -56,7 +56,7 @@ def service_json( permissions=None, organisation_type='central', free_sms_fragment_limit=250000, - prefix_sms_with_service_name='Treat as None', + prefix_sms=True, ): if users is None: users = [] @@ -64,8 +64,6 @@ def service_json( permissions = ['email', 'sms'] if inbound_api is None: inbound_api = [] - if prefix_sms_with_service_name == 'Treat as None': - prefix_sms_with_service_name = (sms_sender == 'GOVUK') return { 'id': id_, 'name': name, @@ -86,7 +84,7 @@ def service_json( 'dvla_organisation': '001', 'permissions': permissions, 'inbound_api': inbound_api, - 'prefix_sms_with_service_name': prefix_sms_with_service_name, + 'prefix_sms': prefix_sms, } From 8d38215e86d232f1d8d3562936295a9581ffd383 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 16 Nov 2017 13:35:17 +0000 Subject: [PATCH 2/3] Remove confusing argument name Brings in new utils which changes the name of this argument to be closer to what the API uses. --- app/__init__.py | 4 ++-- app/utils.py | 3 ++- requirements.txt | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 93c2b2078..46e1dd8c9 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -24,7 +24,7 @@ from flask_wtf.csrf import CSRFError from functools import partial from notifications_python_client.errors import HTTPError -from notifications_utils import logging, request_id, formatters +from notifications_utils import logging, request_helper, formatters from notifications_utils.clients.statsd.statsd_client import StatsdClient from notifications_utils.recipients import ( validate_phone_number, @@ -93,7 +93,7 @@ def create_app(application): statsd_client.init_app(application) logging.init_app(application, statsd_client) init_csrf(application) - request_id.init_app(application) + request_helper.init_app(application) service_api_client.init_app(application) user_api_client.init_app(application) diff --git a/app/utils.py b/app/utils.py index 5b1052567..47fd1f6f8 100644 --- a/app/utils.py +++ b/app/utils.py @@ -288,7 +288,8 @@ def get_template( return SMSPreviewTemplate( template, prefix=service['name'], - sender=not service['prefix_sms'], + show_prefix=service['prefix_sms'], + sender=sms_sender, show_recipient=show_recipient, redact_missing_personalisation=redact_missing_personalisation, ) diff --git a/requirements.txt b/requirements.txt index 74229a332..2f60866a5 100644 --- a/requirements.txt +++ b/requirements.txt @@ -21,4 +21,4 @@ notifications-python-client==4.6.0 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@21.5.0#egg=notifications-utils==21.5.0 +git+https://github.com/alphagov/notifications-utils.git@23.0.0#egg=notifications-utils==23.0.0 From 7d1cf2169d31e0c58566d4e2f782472c3cf3ec10 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 16 Nov 2017 14:13:32 +0000 Subject: [PATCH 3/3] Show text message sender in send one-off flow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If you’ve chosen a text message sender then it’s good to see confirmation of your choice. This replicates what we do when you choose an email reply-to address. --- .../stylesheets/components/sms-message.scss | 6 +++ app/utils.py | 3 +- tests/app/main/views/test_send.py | 51 +++++++++++++++++++ 3 files changed, 59 insertions(+), 1 deletion(-) diff --git a/app/assets/stylesheets/components/sms-message.scss b/app/assets/stylesheets/components/sms-message.scss index f9ce3643b..054d6b797 100644 --- a/app/assets/stylesheets/components/sms-message.scss +++ b/app/assets/stylesheets/components/sms-message.scss @@ -48,6 +48,12 @@ $tail-angle: 20deg; } +.sms-message-sender { + @include copy-19; + color: $secondary-text-colour; + margin: 0 0 -10px 0; +} + .sms-message-recipient { @include copy-19; color: $secondary-text-colour; diff --git a/app/utils.py b/app/utils.py index 47fd1f6f8..ba19eddc7 100644 --- a/app/utils.py +++ b/app/utils.py @@ -272,7 +272,7 @@ def get_template( page_count=1, redact_missing_personalisation=False, email_reply_to=None, - sms_sender=None + sms_sender=None, ): if 'email' == template['template_type']: return EmailPreviewTemplate( @@ -290,6 +290,7 @@ def get_template( prefix=service['name'], show_prefix=service['prefix_sms'], sender=sms_sender, + show_sender=bool(sms_sender), show_recipient=show_recipient, redact_missing_personalisation=redact_missing_personalisation, ) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 969b75523..960b8ecac 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -2377,3 +2377,54 @@ def test_reply_to_is_previewed_if_chosen( assert 'test@example.com' in email_meta else: assert 'test@example.com' not in email_meta + + +@pytest.mark.parametrize('endpoint, extra_args', [ + ('main.check_messages', {'template_type': 'sms', 'upload_id': fake_uuid()}), + ('main.send_one_off_step', {'template_id': fake_uuid(), 'step_index': 0}), +]) +@pytest.mark.parametrize('sms_sender', [ + None, + fake_uuid(), +]) +def test_sms_sender_is_previewed( + client_request, + mocker, + mock_get_service_template, + mock_s3_download, + mock_get_users_by_service, + mock_get_detailed_service_for_today, + get_default_sms_sender, + endpoint, + extra_args, + sms_sender, +): + + mocker.patch('app.main.views.send.s3download', return_value=""" + phone number,date,thing + 7700900986,foo,bar + """) + + with client_request.session_transaction() as session: + session['recipient'] = '7700900986' + session['placeholders'] = {} + session['upload_data'] = { + 'original_file_name': 'example.csv', + 'template_id': fake_uuid(), + 'notification_count': 1, + 'valid': True + } + session['sender_id'] = sms_sender + + page = client_request.get( + endpoint, + service_id=SERVICE_ONE_ID, + **extra_args + ) + + sms_sender_on_page = page.select_one('.sms-message-sender') + + if sms_sender: + assert sms_sender_on_page.text.strip() == 'From: GOVUK' + else: + assert not sms_sender_on_page