diff --git a/app/main/forms.py b/app/main/forms.py index 8cfe5f497..ea34f8137 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -690,3 +690,13 @@ def get_placeholder_form_instance( return PlaceholderForm( placeholder_value=dict_to_populate_from.get(placeholder_name, '') ) + + +class SetSenderForm(Form): + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.sender.choices = kwargs['sender_choices'] + self.sender.label.text = kwargs['sender_label'] + + sender = RadioField() diff --git a/app/main/views/send.py b/app/main/views/send.py index e5fafd3b2..8307637c9 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -32,6 +32,7 @@ from app.main import main from app.main.forms import ( CsvUploadForm, ChooseTimeForm, + SetSenderForm, get_placeholder_form_instance ) from app.main.s3_client import ( @@ -92,7 +93,7 @@ def get_example_letter_address(key): @login_required @user_has_permissions('send_texts', 'send_emails', 'send_letters') def send_messages(service_id, template_id): - + session['sender_id'] = None db_template = service_api_client.get_service_template(service_id, template_id)['data'] if email_or_sms_not_enabled(db_template['template_type'], current_service['permissions']): @@ -166,6 +167,77 @@ def get_example_csv(service_id, template_id): } +@main.route("/services//send//set-sender", methods=['GET', 'POST']) +@login_required +@user_has_permissions('send_texts', 'send_emails', 'send_letters') +def set_sender(service_id, template_id): + session['sender_id'] = None + redirect_to_one_off = redirect( + url_for('.send_one_off', service_id=service_id, template_id=template_id) + ) + + template = service_api_client.get_service_template(service_id, template_id)['data'] + + if template['template_type'] != 'email': + return redirect_to_one_off + + sender_details = get_sender_details(service_id, template['template_type']) + if len(sender_details) <= 1: + return redirect_to_one_off + + sender_context = get_sender_context(sender_details, template['template_type']) + + form = SetSenderForm( + sender=sender_context['default_id'], + sender_choices=sender_context['value_and_label'], + sender_label=sender_context['description'] + ) + option_hints = {sender_context['default_id']: 'Default'} + + if form.validate_on_submit(): + session['sender_id'] = form.sender.data + return redirect(url_for('.send_one_off', + service_id=service_id, + template_id=template_id)) + + return render_template( + 'views/templates/set-sender.html', + form=form, + template_id=template_id, + sender_context={'title': sender_context['title'], 'description': sender_context['description']}, + option_hints=option_hints + ) + + +def get_sender_context(sender_details, template_type): + context = { + 'email': { + 'title': "Choose where to send replies", + 'description': "Select an email address that recipients can reply to", + 'field_name': 'email_address' + }, + 'letter': { + 'title': 'Choose sender address', + 'description': 'Select an address that recipients can reply to', + 'field_name': 'contact_block' + } + }[template_type] + + sender_format = context['field_name'] + + context['default_id'] = next(sender['id'] for sender in sender_details if sender['is_default']) + context['value_and_label'] = [(sender['id'], sender[sender_format]) for sender in sender_details] + return context + + +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 + }[template_type] + return api_call(service_id) + + @main.route("/services//send//test", endpoint='send_test') @main.route("/services//send//one-off", endpoint='send_one_off') @login_required @@ -176,6 +248,8 @@ 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': + session['sender_id'] = None if email_or_sms_not_enabled(db_template['template_type'], current_service['permissions']): return redirect(url_for( @@ -319,7 +393,6 @@ def send_test_step(service_id, template_id, step_index): ) else: skip_link = None - return render_template( 'views/send-test.html', page_title=get_send_test_page_title(template.template_type, get_help_argument()), @@ -707,13 +780,13 @@ def send_notification(service_id, template_id): service_id=service_id, template_id=template_id, )) - try: noti = notification_api_client.send_notification( service_id, template_id=template_id, recipient=session['recipient'], - personalisation=session['placeholders'] + personalisation=session['placeholders'], + sender_id=session['sender_id'] if 'sender_id' in session else None ) except HTTPError as exception: current_app.logger.info('Service {} could not send notification: "{}"'.format( @@ -724,6 +797,7 @@ def send_notification(service_id, template_id): session.pop('placeholders') session.pop('recipient') + session.pop('sender_id', None) return redirect(url_for( '.view_notification', diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 9daf1c1c5..19ac0f39f 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -581,6 +581,7 @@ def service_set_branding_and_org(service_id): organisations = organisations_client.get_organisations() form = ServiceBrandingOrg(branding_type=current_service.get('branding')) + # dynamically create org choices, including the null option form.organisation.choices = [('None', 'None')] + get_branding_as_value_and_label(organisations) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 237625d05..719d8ae40 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -79,7 +79,7 @@ def view_template(service_id, template_id): show_recipient=True, page_count=get_page_count_for_letter(template), ), - default_letter_contact_block_id=default_letter_contact_block_id + default_letter_contact_block_id=default_letter_contact_block_id, ) @@ -487,7 +487,7 @@ def delete_service_template(service_id, template_id): filetype='png', ), show_recipient=True, - ) + ), ) @@ -495,10 +495,12 @@ def delete_service_template(service_id, template_id): @login_required @user_has_permissions('manage_templates', admin_override=True) def confirm_redact_template(service_id, template_id): + template = service_api_client.get_service_template(service_id, template_id)['data'] + return render_template( 'views/templates/template.html', template=get_template( - service_api_client.get_service_template(service_id, template_id)['data'], + template, current_service, expand_emails=True, letter_preview_url=url_for( diff --git a/app/notify_client/notification_api_client.py b/app/notify_client/notification_api_client.py index d46e6caba..195d089ac 100644 --- a/app/notify_client/notification_api_client.py +++ b/app/notify_client/notification_api_client.py @@ -54,12 +54,14 @@ class NotificationApiClient(NotifyAdminAPIClient): params=params ) - def send_notification(self, service_id, *, template_id, recipient, personalisation): + def send_notification(self, service_id, *, template_id, recipient, personalisation, sender_id): data = { 'template_id': template_id, 'to': recipient, 'personalisation': personalisation, } + if 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/templates/views/templates/_template.html b/app/templates/views/templates/_template.html index 029ef5b49..e731b3309 100644 --- a/app/templates/views/templates/_template.html +++ b/app/templates/views/templates/_template.html @@ -13,7 +13,7 @@
- + {{ 'Print a test letter' if template.template_type == 'letter' else 'Send to one recipient' }}
diff --git a/app/templates/views/templates/set-sender.html b/app/templates/views/templates/set-sender.html new file mode 100644 index 000000000..363499dac --- /dev/null +++ b/app/templates/views/templates/set-sender.html @@ -0,0 +1,29 @@ +{% extends "withnav_template.html" %} +{% from "components/radios.html" import radios, branding_radios %} +{% from "components/page-footer.html" import page_footer %} + +{% block service_page_title %} + {{sender_context['title']}} +{% endblock %} + +{% block maincolumn_content %} + +

{{sender_context['title']}}

+
+
+
+ {{ radios( + form.sender, + option_hints=option_hints + ) + }} + {{ page_footer( + 'Continue', + back_link=url_for('.view_template', service_id=current_service.id, template_id=template_id), + back_link_text='Back to template' + ) }} +
+
+
+ +{% endblock %} diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 5cd50a5d2..f91473141 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -26,6 +26,12 @@ from tests.conftest import ( SERVICE_ONE_ID, mock_get_service, mock_get_live_service, + multiple_reply_to_email_addresses, + multiple_letter_contact_blocks, + multiple_sms_senders, + no_reply_to_email_addresses, + no_letter_contact_blocks, + no_sms_senders ) template_types = ['email', 'sms'] @@ -35,6 +41,101 @@ test_spreadsheet_files = glob(path.join('tests', 'spreadsheet_files', '*')) test_non_spreadsheet_files = glob(path.join('tests', 'non_spreadsheet_files', '*')) +@pytest.mark.parametrize('template_mock, sender_data, expected_title, expected_description', [ + ( + mock_get_service_email_template, + multiple_reply_to_email_addresses, + 'Choose where to send replies', + 'Select an email address that recipients can reply to' + ), +]) +def test_show_correct_title_and_description_for_sender_type( + client_request, + service_one, + fake_uuid, + template_mock, + sender_data, + expected_title, + expected_description, + mocker +): + template_mock(mocker) + sender_data(mocker) + + page = client_request.get( + '.set_sender', + service_id=service_one['id'], + template_id=fake_uuid + ) + + assert page.select_one('h1').text == expected_title + assert normalize_spaces(page.select_one('legend').text) == expected_description + + +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 +): + 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" + assert not page.select('.multiple-choice input')[1].has_attr('checked') + assert not page.select('.multiple-choice input')[2].has_attr('checked') + + +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 +): + response = logged_in_client.post( + url_for('.set_sender', service_id=service_one['id'], template_id=fake_uuid), + data={'sender': '1234'} + ) + + with logged_in_client.session_transaction() as session: + assert session['sender_id'] == '1234' + + +@pytest.mark.parametrize('template_mock, sender_data', [ + ( + mock_get_service_email_template, + no_reply_to_email_addresses, + ), +]) +def test_set_sender_redirects_if_no_sender_data( + logged_in_client, + service_one, + fake_uuid, + template_mock, + sender_data, + mocker +): + template_mock(mocker) + sender_data(mocker) + response = logged_in_client.get( + url_for('.set_sender', service_id=service_one['id'], template_id=fake_uuid) + ) + assert response.status_code == 302 + expected_url = url_for( + '.send_one_off', + service_id=service_one['id'], + template_id=fake_uuid, + _external=True, + ) + assert response.location == expected_url + + def test_that_test_files_exist(): assert len(test_spreadsheet_files) == 8 assert len(test_non_spreadsheet_files) == 6 @@ -1965,7 +2066,8 @@ def test_send_notification_submits_data( service_one['id'], template_id=fake_uuid, recipient='07700900001', - personalisation={'a': 'b'} + personalisation={'a': 'b'}, + sender_id=None ) diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index 7241348c1..13f87c697 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -116,11 +116,11 @@ def test_should_show_page_for_one_template( ), ( ['send_texts', 'send_emails', 'send_letters'], - ['.send_messages', '.send_one_off'] + ['.send_messages', '.set_sender'] ), ( ['send_texts', 'send_emails', 'send_letters', 'manage_templates'], - ['.send_messages', '.send_one_off', '.edit_service_template'] + ['.send_messages', '.set_sender', '.edit_service_template'] ), ]) def test_should_be_able_to_view_a_template_with_links( @@ -335,6 +335,30 @@ 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_letter_template, 302), +]) +def test_should_redirect_to_one_off_if_template_type_is_not_email( + logged_in_client, + active_user_with_permissions, + multiple_reply_to_email_addresses, + service_one, + fake_uuid, + mocker, + fixture, + expected_status_code +): + fixture(mocker) + + page = logged_in_client.get( + url_for('.set_sender', service_id=service_one['id'], template_id=fake_uuid) + ) + + assert page.status_code == expected_status_code + + def test_should_redirect_when_saving_a_template( logged_in_client, active_user_with_permissions, diff --git a/tests/app/notify_client/test_notification_client.py b/tests/app/notify_client/test_notification_client.py index 4f7ddd5da..3cf09346c 100644 --- a/tests/app/notify_client/test_notification_client.py +++ b/tests/app/notify_client/test_notification_client.py @@ -42,7 +42,13 @@ def test_client_gets_notifications_for_service_and_job_by_page(mocker, arguments def test_send_notification(mocker, logged_in_client, active_user_with_permissions): mock_post = mocker.patch('app.notify_client.notification_api_client.NotificationApiClient.post') - NotificationApiClient().send_notification('foo', template_id='bar', recipient='07700900001', personalisation=None) + NotificationApiClient().send_notification( + 'foo', + template_id='bar', + recipient='07700900001', + personalisation=None, + sender_id=None + ) mock_post.assert_called_once_with( url='/service/foo/send-notification', data={ diff --git a/tests/conftest.py b/tests/conftest.py index 895332d43..c58dad210 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -270,6 +270,92 @@ def mock_update_letter_contact(mocker): return mocker.patch('app.service_api_client.update_letter_contact', side_effect=_update_letter_contact) +@pytest.fixture(scope='function') +def multiple_sms_senders(mocker): + def _get(service_id): + return [ + { + 'id': '1234', + 'service_id': service_id, + 'sms_sender': 'Example', + 'is_default': True, + 'created_at': datetime.utcnow(), + 'updated_at': None + }, { + 'id': '5678', + 'service_id': service_id, + 'sms_sender': 'Example 2', + 'is_default': False, + 'created_at': datetime.utcnow(), + 'updated_at': None + }, { + 'id': '9457', + 'service_id': service_id, + 'sms_sender': 'Example 3', + 'is_default': False, + 'created_at': datetime.utcnow(), + '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): + return [] + + return mocker.patch('app.service_api_client.get_sms_senders', side_effect=_get) + + +@pytest.fixture(scope='function') +def single_sms_sender(mocker): + def _get(service_id): + return [ + { + 'id': '1234', + 'service_id': service_id, + 'sms_sender': 'Example', + 'is_default': True, + 'created_at': datetime.utcnow(), + 'updated_at': None + } + ] + + return mocker.patch('app.service_api_client.get_sms_senders', side_effect=_get) + + +@pytest.fixture(scope='function') +def get_default_sms_sender(mocker): + def _get(service_id, sms_sender_id): + return { + 'id': '1234', + 'service_id': service_id, + 'sms_sender': 'Example', + 'is_default': True, + 'created_at': datetime.utcnow(), + 'updated_at': None + } + + return mocker.patch('app.service_api_client.get_sms_sender', side_effect=_get) + + +@pytest.fixture(scope='function') +def get_non_default_sms_sender(mocker): + def _get(service_id, sms_sender_id): + return { + 'id': '1234', + 'service_id': service_id, + 'service_id': service_id, + 'is_default': False, + 'created_at': datetime.utcnow(), + 'updated_at': None + } + + return mocker.patch('app.service_api_client.get_sms_sender', side_effect=_get) + + @pytest.fixture(scope='function') def fake_uuid(): return sample_uuid() @@ -2014,7 +2100,7 @@ def mock_get_notification( @pytest.fixture def mock_send_notification(mocker, fake_uuid): def _send_notification( - service_id, *, template_id, recipient, personalisation + service_id, *, template_id, recipient, personalisation, sender_id ): return {'id': fake_uuid}