From 2ea921952f505747c69058f0e44ef2e37f492a88 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 2 Nov 2017 12:07:46 +0000 Subject: [PATCH 1/4] This PR adds the ability to select a text message sender if more than one exist for the service. --- app/main/views/send.py | 48 ++++++++++++++++---- app/notify_client/notification_api_client.py | 1 + app/utils.py | 3 +- tests/app/main/views/test_send.py | 46 +++++++++++++++++-- tests/app/main/views/test_templates.py | 5 +- 5 files changed, 88 insertions(+), 15 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 5c8555970..63863f0f7 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -178,7 +178,7 @@ def set_sender(service_id, template_id): template = service_api_client.get_service_template(service_id, template_id)['data'] - if template['template_type'] != 'email': + if template['template_type'] == 'letter': return redirect_to_one_off sender_details = get_sender_details(service_id, template['template_type']) @@ -220,6 +220,11 @@ def get_sender_context(sender_details, template_type): 'title': 'Choose sender address', 'description': 'Select an address that recipients can reply to', 'field_name': 'contact_block' + }, + 'sms': { + 'title': 'Chose text message sender', + 'description': 'Select a text message sender that the recipients can reply to', + 'field_name': 'sms_sender' } }[template_type] @@ -233,7 +238,8 @@ def get_sender_context(sender_details, template_type): def get_sender_details(service_id, template_type): api_call = { 'email': service_api_client.get_reply_to_email_addresses, - 'letter': service_api_client.get_letter_contacts + 'letter': service_api_client.get_letter_contacts, + 'sms': service_api_client.get_sms_senders }[template_type] return api_call(service_id) @@ -248,7 +254,7 @@ def send_test(service_id, template_id): session['send_test_letter_page_count'] = None db_template = service_api_client.get_service_template(service_id, template_id)['data'] - if db_template['template_type'] != 'email': + if db_template['template_type'] == 'letter': session['sender_id'] = None if email_or_sms_not_enabled(db_template['template_type'], current_service['permissions']): @@ -311,7 +317,12 @@ def send_test_step(service_id, template_id, step_index): if not session.get('send_test_letter_page_count'): session['send_test_letter_page_count'] = get_page_count_for_letter(db_template) - + email_reply_to = None + sms_sender = None + if db_template['template_type'] == 'email': + email_reply_to = get_email_reply_to_address_from_session(service_id) + elif db_template['template_type'] == 'sms': + sms_sender = get_sms_sender_from_session(service_id) template = get_template( db_template, current_service, @@ -324,7 +335,8 @@ def send_test_step(service_id, template_id, step_index): filetype='png', ), page_count=session['send_test_letter_page_count'], - email_reply_to=get_email_reply_to_address_from_session(service_id), + email_reply_to=email_reply_to, + sms_sender=sms_sender ) placeholders = fields_to_fill_in( @@ -449,6 +461,12 @@ def _check_messages(service_id, template_type, upload_id, letters_as_pdf=False): remaining_messages = (current_service['message_limit'] - sum(stat['requested'] for stat in statistics.values())) contents = s3download(service_id, upload_id) + email_reply_to = None + sms_sender = None + if template_type == 'email': + email_reply_to = get_email_reply_to_address_from_session(service_id) + elif template_type == 'sms': + sms_sender = get_sms_sender_from_session(service_id) template = get_template( service_api_client.get_service_template( service_id, @@ -463,7 +481,8 @@ def _check_messages(service_id, template_type, upload_id, letters_as_pdf=False): upload_id=upload_id, filetype='png', ) if not letters_as_pdf else None, - email_reply_to=get_email_reply_to_address_from_session(service_id), + email_reply_to=email_reply_to, + sms_sender=sms_sender ) recipients = RecipientCSV( contents, @@ -723,12 +742,18 @@ def check_notification(service_id, template_id): def _check_notification(service_id, template_id, exception=None): db_template = service_api_client.get_service_template(service_id, template_id)['data'] - + email_reply_to = None + sms_sender = None + if db_template['template_type'] == 'email': + email_reply_to = get_email_reply_to_address_from_session(service_id) + elif db_template['template_type'] == 'sms': + sms_sender = get_sms_sender_from_session(service_id) template = get_template( db_template, current_service, show_recipient=True, - email_reply_to=get_email_reply_to_address_from_session(service_id), + email_reply_to=email_reply_to, + sms_sender=sms_sender ) # go back to start of process @@ -814,3 +839,10 @@ def get_email_reply_to_address_from_session(service_id): return service_api_client.get_reply_to_email_address( service_id, session['sender_id'] )['email_address'] + + +def get_sms_sender_from_session(service_id): + if session.get('sender_id'): + return service_api_client.get_sms_sender( + service_id=service_id, sms_sender_id=session['sender_id'] + )['sms_sender'] diff --git a/app/notify_client/notification_api_client.py b/app/notify_client/notification_api_client.py index 195d089ac..d9ce5a8d8 100644 --- a/app/notify_client/notification_api_client.py +++ b/app/notify_client/notification_api_client.py @@ -61,6 +61,7 @@ class NotificationApiClient(NotifyAdminAPIClient): 'personalisation': personalisation, } if sender_id: + print(sender_id) data['sender_id'] = sender_id data = _attach_current_user(data) return self.post(url='/service/{}/send-notification'.format(service_id), data=data) diff --git a/app/utils.py b/app/utils.py index d7ad0ce15..f9ce51bd8 100644 --- a/app/utils.py +++ b/app/utils.py @@ -272,6 +272,7 @@ def get_template( page_count=1, redact_missing_personalisation=False, email_reply_to=None, + sms_sender=None ): if 'email' == template['template_type']: return EmailPreviewTemplate( @@ -287,7 +288,7 @@ def get_template( return SMSPreviewTemplate( template, prefix=service['name'], - sender=(service['sms_sender'] not in {'GOVUK', None}), + sender=sms_sender if sms_sender else (service['sms_sender'] not in {'GOVUK', None}), 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 82c1a0ef5..2880743a4 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -27,6 +27,8 @@ from tests.conftest import ( mock_get_live_service, multiple_reply_to_email_addresses, no_reply_to_email_addresses, + multiple_sms_senders, + no_sms_senders ) template_types = ['email', 'sms'] @@ -43,6 +45,12 @@ test_non_spreadsheet_files = glob(path.join('tests', 'non_spreadsheet_files', '* 'Choose where to send replies', 'Select an email address that recipients can reply to' ), + ( + mock_get_service_template, + multiple_sms_senders, + 'Chose text message sender', + 'Select a text message sender that the recipients can reply to' + ) ]) def test_show_correct_title_and_description_for_sender_type( client_request, @@ -67,13 +75,26 @@ def test_show_correct_title_and_description_for_sender_type( assert normalize_spaces(page.select_one('legend').text) == expected_description +@pytest.mark.parametrize('template_mock, sender_data', [ + ( + mock_get_service_email_template, + multiple_reply_to_email_addresses, + ), + ( + mock_get_service_template, + multiple_sms_senders + ) +]) def test_default_sender_is_checked_and_has_hint( client_request, service_one, fake_uuid, - mock_get_service_email_template, - multiple_reply_to_email_addresses + template_mock, + sender_data, + mocker ): + template_mock(mocker) + sender_data(mocker) page = client_request.get( '.set_sender', service_id=service_one['id'], @@ -86,13 +107,26 @@ def test_default_sender_is_checked_and_has_hint( assert not page.select('.multiple-choice input')[2].has_attr('checked') +@pytest.mark.parametrize('template_mock, sender_data', [ + ( + mock_get_service_email_template, + multiple_reply_to_email_addresses, + ), + ( + mock_get_service_template, + multiple_sms_senders + ) +]) def test_sender_session_is_present_after_selected( logged_in_client, service_one, fake_uuid, - mock_get_service_email_template, - multiple_reply_to_email_addresses + template_mock, + sender_data, + mocker ): + template_mock(mocker) + sender_data(mocker) logged_in_client.post( url_for('.set_sender', service_id=service_one['id'], template_id=fake_uuid), data={'sender': '1234'} @@ -107,6 +141,10 @@ def test_sender_session_is_present_after_selected( mock_get_service_email_template, no_reply_to_email_addresses, ), + ( + mock_get_service_template, + no_sms_senders + ) ]) def test_set_sender_redirects_if_no_sender_data( logged_in_client, diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index 8a11e6283..cc3974357 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -335,13 +335,14 @@ def test_should_not_allow_creation_of_a_template_without_correct_permission( @pytest.mark.parametrize('fixture, expected_status_code', [ (mock_get_service_email_template, 200), - (mock_get_service_template, 302), + (mock_get_service_template, 200), (mock_get_service_letter_template, 302), ]) -def test_should_redirect_to_one_off_if_template_type_is_not_email( +def test_should_redirect_to_one_off_if_template_type_is_letter( logged_in_client, active_user_with_permissions, multiple_reply_to_email_addresses, + multiple_sms_senders, service_one, fake_uuid, mocker, From 83bfc5088427d6dbb6ed68ab1ff8c84c8caab30c Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 2 Nov 2017 14:58:14 +0000 Subject: [PATCH 2/4] Added a hint for Receives replies --- app/main/views/send.py | 6 +++++- app/notify_client/notification_api_client.py | 1 - tests/app/main/views/test_send.py | 19 +++++++++++++++++++ 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 63863f0f7..0db544410 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -192,7 +192,8 @@ def set_sender(service_id, template_id): sender_choices=sender_context['value_and_label'], sender_label=sender_context['description'] ) - option_hints = {sender_context['default_id']: 'Default'} + option_hints = {sender_context['default_id']: 'Default', + sender_context['receives_text_message']: 'Receives replies'} if form.validate_on_submit(): session['sender_id'] = form.sender.data @@ -231,6 +232,9 @@ def get_sender_context(sender_details, template_type): sender_format = context['field_name'] context['default_id'] = next(sender['id'] for sender in sender_details if sender['is_default']) + if template_type == 'sms': + context['receives_text_message'] = next( + sender['id'] for sender in sender_details if sender['inbound_number_id']) context['value_and_label'] = [(sender['id'], sender[sender_format]) for sender in sender_details] return context diff --git a/app/notify_client/notification_api_client.py b/app/notify_client/notification_api_client.py index d9ce5a8d8..195d089ac 100644 --- a/app/notify_client/notification_api_client.py +++ b/app/notify_client/notification_api_client.py @@ -61,7 +61,6 @@ class NotificationApiClient(NotifyAdminAPIClient): 'personalisation': personalisation, } if sender_id: - print(sender_id) data['sender_id'] = sender_id data = _attach_current_user(data) return self.post(url='/service/{}/send-notification'.format(service_id), data=data) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 2880743a4..55128afaa 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -107,6 +107,25 @@ def test_default_sender_is_checked_and_has_hint( assert not page.select('.multiple-choice input')[2].has_attr('checked') +def test_sms_sender_is_has_receives_replies_hint( + client_request, + service_one, + fake_uuid, + mock_get_service_template, + multiple_sms_senders +): + page = client_request.get( + '.set_sender', + service_id=service_one['id'], + template_id=fake_uuid + ) + + assert page.select('.multiple-choice input')[0].has_attr('checked') + assert normalize_spaces(page.select_one('.multiple-choice label .block-label-hint').text) == "Receives replies" + assert not page.select('.multiple-choice input')[1].has_attr('checked') + assert not page.select('.multiple-choice input')[2].has_attr('checked') + + @pytest.mark.parametrize('template_mock, sender_data', [ ( mock_get_service_email_template, From ff22c83b1d884909e0ed5c6df5495520ffabece0 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 2 Nov 2017 15:48:19 +0000 Subject: [PATCH 3/4] Added a hint to show default and receives text messages --- app/main/views/send.py | 16 +++-- app/main/views/service_settings.py | 2 +- tests/app/main/views/test_send.py | 34 +++++++++-- tests/app/main/views/test_service_settings.py | 2 +- tests/conftest.py | 60 +++++++++++++++++++ 5 files changed, 104 insertions(+), 10 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 0db544410..7835c9955 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -192,8 +192,12 @@ def set_sender(service_id, template_id): sender_choices=sender_context['value_and_label'], sender_label=sender_context['description'] ) - option_hints = {sender_context['default_id']: 'Default', - sender_context['receives_text_message']: 'Receives replies'} + option_hints = {sender_context['default_id']: '(Default)', + } + if sender_context.get('receives_text_message', None): + option_hints.update({sender_context['receives_text_message']: '(Receives replies)'}) + if sender_context.get('default_and_receives', None): + option_hints = {sender_context['default_and_receives']: '(Default and receives replies)'} if form.validate_on_submit(): session['sender_id'] = form.sender.data @@ -233,8 +237,12 @@ def get_sender_context(sender_details, template_type): context['default_id'] = next(sender['id'] for sender in sender_details if sender['is_default']) if template_type == 'sms': - context['receives_text_message'] = next( - sender['id'] for sender in sender_details if sender['inbound_number_id']) + inbound = [sender['id'] for sender in sender_details if sender['inbound_number_id']] + if inbound: + context['receives_text_message'] = next(iter(inbound)) + if context['default_id'] == context.get('receives_text_message', None): + context['default_and_receives'] = context['default_id'] + context['value_and_label'] = [(sender['id'], sender[sender_format]) for sender in sender_details] return context diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 4c2cef034..9446b69ff 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -590,7 +590,7 @@ def service_sms_senders(service_id): if sender['is_default']: hints += ["default"] if sender['inbound_number_id']: - hints += ["recieves replies"] + hints += ["receives replies"] if hints: sender['hint'] = "(" + " and ".join(hints) + ")" diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 55128afaa..0d40fdac9 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -28,7 +28,9 @@ from tests.conftest import ( multiple_reply_to_email_addresses, no_reply_to_email_addresses, multiple_sms_senders, - no_sms_senders + no_sms_senders, + multiple_sms_senders_with_diff_default, + multiple_sms_senders_no_inbound ) template_types = ['email', 'sms'] @@ -82,7 +84,11 @@ def test_show_correct_title_and_description_for_sender_type( ), ( mock_get_service_template, - multiple_sms_senders + multiple_sms_senders_with_diff_default + ), + ( + mock_get_service_template, + multiple_sms_senders_no_inbound ) ]) def test_default_sender_is_checked_and_has_hint( @@ -102,7 +108,26 @@ def test_default_sender_is_checked_and_has_hint( ) assert page.select('.multiple-choice input')[0].has_attr('checked') - assert normalize_spaces(page.select_one('.multiple-choice label .block-label-hint').text) == "Default" + assert normalize_spaces(page.select_one('.multiple-choice label .block-label-hint').text) == "(Default)" + assert not page.select('.multiple-choice input')[1].has_attr('checked') + + +def test_default_inbound_sender_is_checked_and_has_hint_with_default_and_receives_text( + client_request, + service_one, + fake_uuid, + mock_get_service_template, + multiple_sms_senders +): + page = client_request.get( + '.set_sender', + service_id=service_one['id'], + template_id=fake_uuid + ) + + assert page.select('.multiple-choice input')[0].has_attr('checked') + assert normalize_spaces( + page.select_one('.multiple-choice label .block-label-hint').text) == "(Default and receives replies)" assert not page.select('.multiple-choice input')[1].has_attr('checked') assert not page.select('.multiple-choice input')[2].has_attr('checked') @@ -121,7 +146,8 @@ def test_sms_sender_is_has_receives_replies_hint( ) assert page.select('.multiple-choice input')[0].has_attr('checked') - assert normalize_spaces(page.select_one('.multiple-choice label .block-label-hint').text) == "Receives replies" + assert normalize_spaces( + page.select_one('.multiple-choice label .block-label-hint').text) == "(Default and receives replies)" assert not page.select('.multiple-choice input')[1].has_attr('checked') assert not page.select('.multiple-choice input')[2].has_attr('checked') diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 3a7cc9884..998b32243 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -761,7 +761,7 @@ def test_api_ids_dont_show_on_option_pages_with_a_single_sender( ), ( 'main.service_sms_senders', multiple_sms_senders, - 'Example (default and recieves replies) Change 1234', + 'Example (default and receives replies) Change 1234', 'Example 2 Change 5678', 'Example 3 Change 9457' ), diff --git a/tests/conftest.py b/tests/conftest.py index d3a2a1a7d..34c0f6cec 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -304,6 +304,66 @@ def multiple_sms_senders(mocker): return mocker.patch('app.service_api_client.get_sms_senders', side_effect=_get) +@pytest.fixture(scope='function') +def multiple_sms_senders_with_diff_default(mocker): + def _get(service_id): + return [ + { + 'id': '1234', + 'service_id': service_id, + 'sms_sender': 'Example', + 'is_default': True, + 'created_at': datetime.utcnow(), + 'inbound_number_id': None, + 'updated_at': None + }, { + 'id': '5678', + 'service_id': service_id, + 'sms_sender': 'Example 2', + 'is_default': False, + 'created_at': datetime.utcnow(), + 'inbound_number_id': None, + 'updated_at': None + }, { + 'id': '9457', + 'service_id': service_id, + 'sms_sender': 'Example 3', + 'is_default': False, + 'created_at': datetime.utcnow(), + 'inbound_number_id': '12354', + 'updated_at': None + } + ] + + return mocker.patch('app.service_api_client.get_sms_senders', side_effect=_get) + + +@pytest.fixture(scope='function') +def multiple_sms_senders_no_inbound(mocker): + def _get(service_id): + return [ + { + 'id': '1234', + 'service_id': service_id, + 'sms_sender': 'Example', + 'is_default': True, + 'created_at': datetime.utcnow(), + 'inbound_number_id': None, + 'updated_at': None + }, { + 'id': '5678', + 'service_id': service_id, + 'sms_sender': 'Example 2', + 'is_default': False, + 'created_at': datetime.utcnow(), + 'inbound_number_id': None, + 'updated_at': None + } + ] + + return mocker.patch('app.service_api_client.get_sms_senders', side_effect=_get) + + @pytest.fixture(scope='function') def no_sms_senders(mocker): def _get(service_id): From 3e10cfb16528c94225d7f7e94f57119e047fd422 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 2 Nov 2017 16:30:49 +0000 Subject: [PATCH 4/4] Rename test --- tests/app/main/views/test_send.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 0d40fdac9..5d99b6d27 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -132,7 +132,7 @@ def test_default_inbound_sender_is_checked_and_has_hint_with_default_and_receive assert not page.select('.multiple-choice input')[2].has_attr('checked') -def test_sms_sender_is_has_receives_replies_hint( +def test_sms_sender_has_receives_replies_hint( client_request, service_one, fake_uuid,