diff --git a/app/main/forms.py b/app/main/forms.py index f75fe1f41..ae5fd6004 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -780,6 +780,10 @@ class SMSTemplateForm(BaseTemplateForm): OnlySMSCharacters()(None, field) +class BroadcastTemplateForm(SMSTemplateForm): + pass + + class LetterAddressForm(StripWhitespaceForm): def __init__(self, *args, allow_international_letters=False, **kwargs): @@ -1644,7 +1648,7 @@ class TemplateAndFoldersSelectionForm(Form): self, all_template_folders, template_list, - allow_adding_letter_template, + available_template_types, allow_adding_copy_of_template, *args, **kwargs @@ -1652,6 +1656,8 @@ class TemplateAndFoldersSelectionForm(Form): super().__init__(*args, **kwargs) + self.available_template_types = available_template_types + self.templates_and_folders.choices = template_list.as_id_and_name self.op = None @@ -1664,12 +1670,25 @@ class TemplateAndFoldersSelectionForm(Form): ] self.add_template_by_template_type.choices = list(filter(None, [ + # We want to show email and text message to everyone, + # whether or not the service has them switched on. The + # option to add letter or broadcast templates should only + # be shown to services which have that permission ('email', 'Email'), ('sms', 'Text message'), - ('letter', 'Letter') if allow_adding_letter_template else None, + ('letter', 'Letter') if 'letter' in available_template_types else None, + ('broadcast', 'Broadcast') if 'broadcast' in available_template_types else None, ('copy-existing', 'Copy an existing template') if allow_adding_copy_of_template else None, ])) + @property + def trying_to_add_unavailable_template_type(self): + return all(( + self.is_add_template_op, + self.add_template_by_template_type.data, + self.add_template_by_template_type.data not in self.available_template_types, + )) + def is_selected(self, template_folder_id): return template_folder_id in (self.templates_and_folders.data or []) diff --git a/app/main/views/send.py b/app/main/views/send.py index 1f6cdcef0..a878a0bc3 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -53,7 +53,6 @@ from app.template_previews import TemplatePreview, get_page_count_for_letter from app.utils import ( PermanentRedirect, Spreadsheet, - email_or_sms_not_enabled, get_errors_for_csv, get_help_argument, get_template, @@ -128,7 +127,7 @@ def send_messages(service_id, template_id): elif db_template['template_type'] == 'sms': sms_sender = get_sms_sender_from_session() - if email_or_sms_not_enabled(db_template['template_type'], current_service.permissions): + if db_template['template_type'] not in current_service.available_template_types: return redirect(url_for( '.action_blocked', service_id=service_id, @@ -302,8 +301,11 @@ def send_test(service_id, template_id): db_template = current_service.get_template_with_user_permission_or_403(template_id, current_user) if db_template['template_type'] == 'letter': session['sender_id'] = None + return redirect( + url_for('.send_one_off_letter_address', service_id=service_id, template_id=template_id) + ) - if email_or_sms_not_enabled(db_template['template_type'], current_service.permissions): + if db_template['template_type'] not in current_service.available_template_types: return redirect(url_for( '.action_blocked', service_id=service_id, @@ -311,11 +313,6 @@ def send_test(service_id, template_id): return_to='view_template', template_id=template_id)) - if db_template['template_type'] == 'letter': - return redirect( - url_for('.send_one_off_letter_address', service_id=service_id, template_id=template_id) - ) - return redirect(url_for( { 'main.send_test': '.send_test_step', diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 7b2e36046..fcd394f4e 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -16,6 +16,7 @@ from app import ( ) from app.main import main, no_cookie from app.main.forms import ( + BroadcastTemplateForm, EmailTemplateForm, LetterTemplateForm, LetterTemplatePostageForm, @@ -30,7 +31,6 @@ from app.models.service import Service from app.models.template_list import TemplateList, TemplateLists from app.template_previews import TemplatePreview, get_page_count_for_letter from app.utils import ( - email_or_sms_not_enabled, get_template, should_skip_template_page, user_has_permissions, @@ -40,7 +40,8 @@ from app.utils import ( form_objects = { 'email': EmailTemplateForm, 'sms': SMSTemplateForm, - 'letter': LetterTemplateForm + 'letter': LetterTemplateForm, + 'broadcast': BroadcastTemplateForm, } @@ -122,7 +123,7 @@ def choose_template(service_id, template_type='all', template_folder_id=None): all_template_folders=current_service.get_user_template_folders(current_user), template_list=template_list, template_type=template_type, - allow_adding_letter_template=current_service.has_permission('letter'), + available_template_types=current_service.available_template_types, allow_adding_copy_of_template=( current_service.all_templates or len(current_user.service_ids) > 1 ), @@ -136,6 +137,13 @@ def choose_template(service_id, template_type='all', template_folder_id=None): return process_folder_management_form(templates_and_folders_form, template_folder_id) except HTTPError as e: flash(e.message) + elif templates_and_folders_form.trying_to_add_unavailable_template_type: + return redirect(url_for( + '.action_blocked', + service_id=current_service.id, + notification_type=templates_and_folders_form.add_template_by_template_type.data, + return_to='add_new_template', + )) if 'templates_and_folders' in templates_and_folders_form.errors: flash('Select at least one template or folder') @@ -199,6 +207,7 @@ def get_template_nav_label(value): 'sms': 'Text message', 'email': 'Email', 'letter': 'Letter', + 'broadcast': 'Broadcast', }[value] @@ -310,20 +319,12 @@ def _add_template_by_type(template_type, template_folder_id): template_id=blank_letter['data']['id'], )) - if email_or_sms_not_enabled(template_type, current_service.permissions): - return redirect(url_for( - '.action_blocked', - service_id=current_service.id, - notification_type=template_type, - return_to='add_new_template', - )) - else: - return redirect(url_for( - '.add_service_template', - service_id=current_service.id, - template_type=template_type, - template_folder_id=template_folder_id, - )) + return redirect(url_for( + '.add_service_template', + service_id=current_service.id, + template_type=template_type, + template_folder_id=template_folder_id, + )) @main.route("/services//templates/copy") @@ -437,7 +438,7 @@ def action_blocked(service_id, notification_type, return_to, template_id=None): service_id=service_id, notification_type=notification_type, back_link=back_link(), - ) + ), 403 @main.route("/services//templates/folders//manage", methods=['GET', 'POST']) @@ -527,8 +528,14 @@ def delete_template_folder(service_id, template_folder_id): @user_has_permissions('manage_templates') def add_service_template(service_id, template_type, template_folder_id=None): - if not current_service.has_permission('letter') and template_type == 'letter': - abort(403) + if template_type not in current_service.available_template_types: + return redirect(url_for( + '.action_blocked', + service_id=service_id, + notification_type=template_type, + template_folder_id=template_folder_id, + return_to='templates', + )) form = form_objects[template_type]() if form.validate_on_submit(): @@ -558,22 +565,13 @@ def add_service_template(service_id, template_type, template_folder_id=None): url_for('.view_template', service_id=service_id, template_id=new_template['data']['id']) ) - if email_or_sms_not_enabled(template_type, current_service.permissions): - return redirect(url_for( - '.action_blocked', - service_id=service_id, - notification_type=template_type, - template_folder_id=template_folder_id, - return_to='templates', - )) - else: - return render_template( - 'views/edit-{}-template.html'.format(template_type), - form=form, - template_type=template_type, - template_folder_id=template_folder_id, - heading_action='New', - ) + return render_template( + 'views/edit-{}-template.html'.format(template_type), + form=form, + template_type=template_type, + template_folder_id=template_folder_id, + heading_action='New', + ) def abort_403_if_not_admin_user(): @@ -642,7 +640,7 @@ def edit_service_template(service_id, template_id): template_id=template_id )) - if email_or_sms_not_enabled(template['template_type'], current_service.permissions): + if template['template_type'] not in current_service.available_template_types: return redirect(url_for( '.action_blocked', service_id=service_id, diff --git a/app/models/service.py b/app/models/service.py index abb830443..602357d38 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -54,6 +54,7 @@ class Service(JSONModel): 'email', 'sms', 'letter', + 'broadcast', ) ALL_PERMISSIONS = TEMPLATE_TYPES + ( diff --git a/app/templates/components/message-count-label.html b/app/templates/components/message-count-label.html index 14b997665..a4a9078de 100644 --- a/app/templates/components/message-count-label.html +++ b/app/templates/components/message-count-label.html @@ -24,6 +24,12 @@ {%- else -%} letters {%- endif -%} + {%- elif template_type == 'broadcast' -%} + {%- if count == 1 -%} + broadcast + {%- else -%} + broadcasts + {%- endif -%} {%- endif %} {{ suffix }} {%- endmacro %} diff --git a/app/templates/views/edit-broadcast-template.html b/app/templates/views/edit-broadcast-template.html new file mode 100644 index 000000000..5b97e9a6f --- /dev/null +++ b/app/templates/views/edit-broadcast-template.html @@ -0,0 +1,31 @@ +{% extends "withnav_template.html" %} +{% from "components/textbox.html" import textbox %} +{% from "components/page-header.html" import page_header %} +{% from "components/page-footer.html" import sticky_page_footer %} +{% from "components/form.html" import form_wrapper %} + +{% block service_page_title %} + {{ heading_action }} broadcast template +{% endblock %} + +{% block maincolumn_content %} + + {{ page_header( + '{} broadcast template'.format(heading_action), + back_link=url_for('main.view_template', service_id=current_service.id, template_id=template.id) if template else url_for('main.choose_template', service_id=current_service.id, template_folder_id=template_folder_id) + ) }} + + {% call form_wrapper() %} +
+
+ {{ textbox(form.name, width='1-1', hint='Your recipients will not see this') }} +
+
+ {{ textbox(form.template_content, highlight_placeholders=True, width='1-1', rows=5) }} + {{ sticky_page_footer('Save') }} +
+
+ {% endcall %} + + +{% endblock %} diff --git a/app/url_converters.py b/app/url_converters.py index 847543b96..59fd31685 100644 --- a/app/url_converters.py +++ b/app/url_converters.py @@ -5,11 +5,12 @@ from app.models.feedback import ( PROBLEM_TICKET_TYPE, QUESTION_TICKET_TYPE, ) +from app.models.service import Service class TemplateTypeConverter(BaseConverter): - regex = '(?:email|sms|letter)' + regex = '(?:{})'.format('|'.join(Service.TEMPLATE_TYPES)) class TicketTypeConverter(BaseConverter): diff --git a/app/utils.py b/app/utils.py index 01f85de3b..370ff57fd 100644 --- a/app/utils.py +++ b/app/utils.py @@ -461,10 +461,6 @@ def get_time_left(created_at, service_data_retention_days=7): ) -def email_or_sms_not_enabled(template_type, permissions): - return (template_type in ['email', 'sms']) and (template_type not in permissions) - - def get_logo_cdn_domain(): parsed_uri = urlparse(current_app.config['ADMIN_BASE_URL']) diff --git a/tests/app/main/views/test_letters.py b/tests/app/main/views/test_letters.py index b5edfe596..b6048a578 100644 --- a/tests/app/main/views/test_letters.py +++ b/tests/app/main/views/test_letters.py @@ -14,7 +14,8 @@ letters_urls = [ ([], 403) ]) def test_letters_access_restricted( - platform_admin_client, + client_request, + platform_admin_user, mocker, permissions, response_code, @@ -23,12 +24,12 @@ def test_letters_access_restricted( service_one, ): service_one['permissions'] = permissions - - mocker.patch('app.service_api_client.get_service', return_value={"data": service_one}) - - response = platform_admin_client.get(url(service_id=service_one['id'])) - - assert response.status_code == response_code + client_request.login(platform_admin_user) + client_request.get_url( + url(service_id=service_one['id']), + _follow_redirects=True, + _expected_status=response_code, + ) @pytest.mark.parametrize('url', letters_urls) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 7e682d5b4..3d9975846 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -280,6 +280,7 @@ def test_should_not_allow_files_to_be_uploaded_without_the_correct_permission( service_id=SERVICE_ONE_ID, template_id=template_id, _follow_redirects=True, + _expected_status=403, ) assert page.select('main p')[0].text.strip() == "Sending text messages has been disabled for your service." @@ -312,10 +313,12 @@ def test_example_spreadsheet( def test_example_spreadsheet_for_letters( client_request, + service_one, mocker, mock_get_service_letter_template_with_placeholders, fake_uuid, ): + service_one['permissions'] += ['letter'] mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=1) page = client_request.get( @@ -600,6 +603,7 @@ def test_upload_csv_file_with_bad_postal_address_shows_check_page_with_errors( mock_get_jobs, fake_uuid, ): + service_one['permissions'] += ['letter'] mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=9) mocker.patch( 'app.main.views.send.s3download', @@ -658,7 +662,7 @@ def test_upload_csv_file_with_international_letters_permission_shows_appropriate mock_get_jobs, fake_uuid, ): - service_one['permissions'] += ['international_letters'] + service_one['permissions'] += ['letter', 'international_letters'] mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=9) mocker.patch( 'app.main.views.send.s3download', @@ -1328,6 +1332,7 @@ def test_send_one_off_does_not_send_without_the_correct_permissions( service_id=SERVICE_ONE_ID, template_id=template_id, _follow_redirects=True, + _expected_status=403, ) assert page.select('main p')[0].text.strip() == "Sending text messages has been disabled for your service." diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index 02fd8365d..d65795284 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -1241,9 +1241,9 @@ def test_cant_copy_template_from_non_member_service( assert mock_get_service_email_template.call_args_list == [] -@pytest.mark.parametrize('endpoint, data, expected_error', ( +@pytest.mark.parametrize('service_permissions, data, expected_error', ( ( - 'main.choose_template', + ['letter'], { 'operation': 'add-new-template', 'add_template_by_template_type': 'email', @@ -1251,30 +1251,47 @@ def test_cant_copy_template_from_non_member_service( "Sending emails has been disabled for your service." ), ( - 'main.choose_template', + ['email'], { 'operation': 'add-new-template', 'add_template_by_template_type': 'sms', }, "Sending text messages has been disabled for your service." ), + ( + ['sms'], + { + 'operation': 'add-new-template', + 'add_template_by_template_type': 'letter', + }, + "Sending letters has been disabled for your service." + ), + ( + ['letter'], + { + 'operation': 'add-new-template', + 'add_template_by_template_type': 'broadcast', + }, + "Sending broadcasts has been disabled for your service." + ), )) def test_should_not_allow_creation_of_template_through_form_without_correct_permission( client_request, service_one, mock_get_service_templates, mock_get_template_folders, - endpoint, + service_permissions, data, expected_error, fake_uuid, ): - service_one['permissions'] = [] + service_one['permissions'] = service_permissions page = client_request.post( - endpoint, + 'main.choose_template', service_id=SERVICE_ONE_ID, _data=data, _follow_redirects=True, + _expected_status=403, ) assert normalize_spaces(page.select('main p')[0].text) == expected_error assert page.select(".govuk-back-link")[0].text == "Back" @@ -1284,24 +1301,31 @@ def test_should_not_allow_creation_of_template_through_form_without_correct_perm ) -@pytest.mark.parametrize('type_of_template', ['email', 'sms']) +@pytest.mark.parametrize('method', ('get', 'post')) +@pytest.mark.parametrize('type_of_template, expected_error', [ + ('email', 'Sending emails has been disabled for your service.'), + ('sms', 'Sending text messages has been disabled for your service.'), + ('letter', 'Sending letters has been disabled for your service.'), + ('broadcast', 'Sending broadcasts has been disabled for your service.'), +]) def test_should_not_allow_creation_of_a_template_without_correct_permission( client_request, service_one, mocker, + method, type_of_template, + expected_error, ): service_one['permissions'] = [] - template_description = {'sms': 'text messages', 'email': 'emails'} - page = client_request.get( + page = getattr(client_request, method)( '.add_service_template', service_id=SERVICE_ONE_ID, template_type=type_of_template, _follow_redirects=True, + _expected_status=403, ) - assert page.select('main p')[0].text.strip() == \ - "Sending {} has been disabled for your service.".format(template_description[type_of_template]) + assert page.select('main p')[0].text.strip() == expected_error assert page.select(".govuk-back-link")[0].text == "Back" assert page.select(".govuk-back-link")[0]['href'] == url_for( '.choose_template', @@ -1419,6 +1443,7 @@ def test_should_not_allow_template_edits_without_correct_permission( service_id=SERVICE_ONE_ID, template_id=fake_uuid, _follow_redirects=True, + _expected_status=403, ) assert page.select('main p')[0].text.strip() == "Sending text messages has been disabled for your service." @@ -1934,15 +1959,20 @@ def test_can_create_email_template_with_emoji( assert mock_create_service_template.called is True -def test_should_not_create_sms_template_with_emoji( +@pytest.mark.parametrize('template_type', ( + 'sms', 'broadcast' +)) +def test_should_not_create_sms_or_broadcast_template_with_emoji( client_request, service_one, mock_create_service_template, + template_type, ): + service_one['permissions'] += [template_type] page = client_request.post( '.add_service_template', service_id=SERVICE_ONE_ID, - template_type='sms', + template_type=template_type, _data={ 'name': "new name", 'template_content': "here are some noodles 🍜", @@ -1956,12 +1986,18 @@ def test_should_not_create_sms_template_with_emoji( assert mock_create_service_template.called is False +@pytest.mark.parametrize('template_type', ( + 'sms', 'broadcast' +)) def test_should_not_update_sms_template_with_emoji( client_request, + service_one, mock_get_service_template, mock_update_service_template, fake_uuid, + template_type, ): + service_one['permissions'] += [template_type] page = client_request.post( '.edit_service_template', service_id=SERVICE_ONE_ID, @@ -1971,7 +2007,7 @@ def test_should_not_update_sms_template_with_emoji( 'name': "new name", 'template_content': "here's a burger 🍔", 'service': SERVICE_ONE_ID, - 'template_type': 'sms', + 'template_type': template_type, 'process_type': 'normal' }, _expected_status=200, @@ -1980,10 +2016,17 @@ def test_should_not_update_sms_template_with_emoji( assert mock_update_service_template.called is False -def test_should_create_sms_template_without_downgrading_unicode_characters( +@pytest.mark.parametrize('template_type', ( + 'sms', 'broadcast' +)) +def test_should_create_sms_or_broadcast_template_without_downgrading_unicode_characters( client_request, - mock_create_service_template + service_one, + mock_create_service_template, + template_type, ): + service_one['permissions'] += [template_type] + msg = 'here:\tare some “fancy quotes” and non\u200Bbreaking\u200Bspaces' client_request.post( @@ -1993,7 +2036,7 @@ def test_should_create_sms_template_without_downgrading_unicode_characters( _data={ 'name': "new name", 'template_content': msg, - 'template_type': 'sms', + 'template_type': template_type, 'service': SERVICE_ONE_ID, 'process_type': 'normal' },