Merge pull request #2541 from alphagov/choose-new-template

Let users add new template from the choose page

app/main/forms.py

@@ -707,12 +707,21 @@ class ServicePostageForm(StripWhitespaceForm):
 
 class FieldWithNoneOption():
 
-    # This needs to match the data the browser will post from
-    # <input value='None'>
-    NONE_OPTION_VALUE = 'None'
+    # This is a special value that is specific to our forms. This is
+    # more expicit than casting `None` to a string `'None'` which can
+    # have unexpected edge cases
+    NONE_OPTION_VALUE = '__NONE__'
 
+    # When receiving Python data, eg when instantiating the form object
+    # we want to convert that data to our special value, so that it gets
+    # recognised as being one of the valid choices
+    def process_data(self, value):
+        self.data = self.NONE_OPTION_VALUE if value is None else value
+
+    # After validation we want to convert it back to a Python `None` for
+    # use elsewhere, eg posting to the API
     def post_validate(self, form, validation_stopped):
-        if self.data == self.NONE_OPTION_VALUE:
+        if self.data == self.NONE_OPTION_VALUE and not validation_stopped:
             self.data = None
 
 
@@ -1174,6 +1183,8 @@ class TemplateAndFoldersSelectionForm(Form):
         all_template_folders,
         template_list,
         current_folder_id,
+        allow_adding_letter_template,
+        allow_adding_copy_of_template,
         *args,
         **kwargs
     ):
@@ -1183,21 +1194,32 @@ class TemplateAndFoldersSelectionForm(Form):
         self.templates_and_folders.choices = template_list.as_id_and_name
 
         self.op = None
-        self.is_move_op = self.is_add_op = False
+        self.is_move_op = self.is_add_folder_op = self.is_add_template_op = False
+
+        if current_folder_id is None:
+            current_folder_id = RadioFieldWithNoneOption.NONE_OPTION_VALUE
 
         self.move_to.choices = [
             (item['id'], item['name'])
             for item in ([self.ALL_TEMPLATES_FOLDER] + all_template_folders)
-            if item['id'] != str(current_folder_id)
+            if item['id'] != current_folder_id
         ]
 
+        self.add_template_by_template_type.choices = list(filter(None, [
+            ('email', 'Email template'),
+            ('sms', 'Text message template'),
+            ('letter', 'Letter template') if allow_adding_letter_template else None,
+            ('copy-existing', 'Copy of an existing template') if allow_adding_copy_of_template else None,
+        ]))
+
     def validate(self):
         self.op = request.form.get('operation')
 
         self.is_move_op = self.op in {'move_to_existing_folder', 'move_to_new_folder'}
-        self.is_add_op = self.op in {'add_new_folder', 'move_to_new_folder'}
+        self.is_add_folder_op = self.op in {'add_new_folder', 'move_to_new_folder'}
+        self.is_add_template_op = self.op in {'add_template'}
 
-        if not (self.is_add_op or self.is_move_op):
+        if not (self.is_add_folder_op or self.is_move_op or self.is_add_template_op):
             return False
 
         return super().validate()
@@ -1218,3 +1240,8 @@ class TemplateAndFoldersSelectionForm(Form):
     ])
     add_new_folder_name = StringField('Folder name', validators=[required_for_ops('add_new_folder')])
     move_to_new_folder_name = StringField('Folder name', validators=[required_for_ops('move_to_new_folder')])
+
+    add_template_by_template_type = RadioFieldWithNoneOption('Add new', validators=[
+        Optional(),
+        required_for_ops('add_template')
+    ])

app/main/views/templates.py

@@ -116,6 +116,11 @@ def choose_template(service_id, template_type='all', template_folder_id=None):
         template_list=template_list,
         template_type=template_type,
         current_folder_id=template_folder_id,
+        allow_adding_letter_template=current_service.has_permission('letter'),
+        allow_adding_copy_of_template=(
+            current_service.all_templates or
+            len(user_api_client.get_service_ids_for_user(current_user)) > 1
+        ),
     )
 
     if request.method == 'POST' and templates_and_folders_form.validate_on_submit():
@@ -144,7 +149,13 @@ def choose_template(service_id, template_type='all', template_folder_id=None):
 def process_folder_management_form(form, current_folder_id):
     new_folder_id = None
 
-    if form.is_add_op:
+    if form.is_add_template_op:
+        return _add_template_by_type(
+            form.add_template_by_template_type.data,
+            current_folder_id,
+        )
+
+    if form.is_add_folder_op:
         new_folder_id = template_folder_api_client.create_template_folder(
             current_service.id,
             name=form.get_folder_name(),
@@ -247,56 +258,60 @@ def add_template_by_type(service_id, template_folder_id=None):
 
     form = ChooseTemplateType(
         include_letters=current_service.has_permission('letter'),
-        include_copy=any((
-            service_api_client.count_service_templates(service_id) > 0,
-            len(user_api_client.get_service_ids_for_user(current_user)) > 1,
-        )),
+        include_copy=(
+            current_service.all_templates or
+            len(user_api_client.get_service_ids_for_user(current_user)) > 1
+        ),
         include_folder=current_service.has_permission('edit_folders')
     )
 
     if form.validate_on_submit():
-
-        if form.template_type.data == 'copy-existing':
-            return redirect(url_for(
-                '.choose_template_to_copy',
-                service_id=service_id,
-            ))
-
-        if form.template_type.data == 'letter':
-            blank_letter = service_api_client.create_service_template(
-                'Untitled',
-                'letter',
-                'Body',
-                service_id,
-                'Main heading',
-                'normal',
-                template_folder_id
-            )
-            return redirect(url_for(
-                '.view_template',
-                service_id=service_id,
-                template_id=blank_letter['data']['id'],
-            ))
-
-        if email_or_sms_not_enabled(form.template_type.data, current_service.permissions):
-            return redirect(url_for(
-                '.action_blocked',
-                service_id=service_id,
-                notification_type=form.template_type.data,
-                return_to='add_new_template',
-                template_id='0'
-            ))
-        else:
-            return redirect(url_for(
-                '.add_service_template',
-                service_id=service_id,
-                template_type=form.template_type.data,
-                template_folder_id=template_folder_id,
-            ))
+        return _add_template_by_type(form.template_type.data, template_folder_id)
 
     return render_template('views/templates/add.html', form=form)
 
 
+def _add_template_by_type(template_type, template_folder_id):
+
+    if template_type == 'copy-existing':
+        return redirect(url_for(
+            '.choose_template_to_copy',
+            service_id=current_service.id,
+        ))
+
+    if template_type == 'letter':
+        blank_letter = service_api_client.create_service_template(
+            'Untitled',
+            'letter',
+            'Body',
+            current_service.id,
+            'Main heading',
+            'normal',
+            template_folder_id
+        )
+        return redirect(url_for(
+            '.view_template',
+            service_id=current_service.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',
+            template_id='0'
+        ))
+    else:
+        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/<service_id>/templates/copy")
 @login_required
 @user_has_permissions('manage_templates')

app/templates/views/templates/_move_to.html

@@ -1,19 +1,20 @@
 {% from "components/radios.html" import radios %}
 {% from "components/page-footer.html" import page_footer %}
 
-{% if templates_and_folders_form.move_to.choices and template_list.templates_to_show %}
   <button type="submit" name="operation" value="unknown" hidden></button>
-  <div id="move_to_folder_radios">
-    {{ radios(templates_and_folders_form.move_to) }}
-    {{ page_footer('Move', button_name='operation', button_value='move_to_existing_folder') }}
-  </div>
-  <div id="move_to_new_folder_form">
-    <fieldset>
-      <legend class="visuallyhidden">Move to a new folder</legend>
-      {{ textbox(templates_and_folders_form.move_to_new_folder_name) }}
-      {{ page_footer('Move to a new folder', button_name='operation', button_value='move_to_new_folder') }}
-    </fieldset>
-  </div>
+  {% if templates_and_folders_form.move_to.choices and template_list.templates_to_show %}
+    <div id="move_to_folder_radios">
+      {{ radios(templates_and_folders_form.move_to) }}
+      {{ page_footer('Move', button_name='operation', button_value='move_to_existing_folder') }}
+    </div>
+    <div id="move_to_new_folder_form">
+      <fieldset>
+        <legend class="visuallyhidden">Move to a new folder</legend>
+        {{ textbox(templates_and_folders_form.move_to_new_folder_name) }}
+        {{ page_footer('Move to a new folder', button_name='operation', button_value='move_to_new_folder') }}
+      </fieldset>
+    </div>
+  {% endif %}
   <div id="add_new_folder_form">
     <fieldset>
       <legend class="visuallyhidden">Add a new folder</legend>
@@ -21,4 +22,10 @@
       {{ page_footer('New folder', button_name='operation', button_value='add_new_folder') }}
     </fieldset>
   </div>
-{% endif %}
+  <div id="add_new_template_form">
+    <fieldset>
+      <legend class="visuallyhidden">Add a new template</legend>
+      {{ radios(templates_and_folders_form.add_template_by_template_type) }}
+      {{ page_footer('Continue', button_name='operation', button_value='add_template') }}
+    </fieldset>
+  </div>

tests/app/main/views/test_service_settings.py

@@ -2188,12 +2188,12 @@ def test_set_postage_saves(
 
 @pytest.mark.parametrize('current_branding, expected_values, expected_labels', [
     (None, [
-        'None', '1', '2', '3', '4', '5',
+        '__NONE__', '1', '2', '3', '4', '5',
     ], [
         'GOV.UK', 'org 1', 'org 2', 'org 3', 'org 4', 'org 5'
     ]),
     ('5', [
-        '5', 'None', '1', '2', '3', '4',
+        '5', '__NONE__', '1', '2', '3', '4',
     ], [
         'org 5', 'GOV.UK', 'org 1', 'org 2', 'org 3', 'org 4',
     ]),
@@ -2314,7 +2314,8 @@ def test_should_preview_email_branding(
 
 @pytest.mark.parametrize('posted_value, submitted_value', (
     ('1', '1'),
-    ('None', None),
+    ('__NONE__', None),
+    pytest.param('None', None, marks=pytest.mark.xfail(raises=AssertionError)),
 ))
 def test_should_set_branding_and_organisations(
     logged_in_platform_admin_client,

tests/app/main/views/test_template_folders.py

@@ -785,7 +785,7 @@ def test_should_show_radios_and_buttons_for_move_destination_if_correct_permissi
         'main.choose_template',
         service_id=SERVICE_ONE_ID,
     )
-    radios = page.select('input[type=radio]')
+    radios = page.select('#move_to_folder_radios input[type=radio]')
     radio_div = page.find('div', {'id': 'move_to_folder_radios'})
     assert radios == page.select('input[name=move_to]')
 
@@ -799,7 +799,8 @@ def test_should_show_radios_and_buttons_for_move_destination_if_correct_permissi
         'unknown',
         'move_to_existing_folder',
         'move_to_new_folder',
-        'add_new_folder'
+        'add_new_folder',
+        'add_template',
     }
 
 
@@ -900,7 +901,7 @@ def test_should_be_able_to_move_a_sub_item(
         template_folder_id=PARENT_FOLDER_ID,
         _data={
             'operation': 'move_to_existing_folder',
-            'move_to': 'None',
+            'move_to': '__NONE__',
             'templates_and_folders': [GRANDCHILD_FOLDER_ID],
         },
         _expected_status=302,
@@ -928,13 +929,36 @@ def test_should_be_able_to_move_a_sub_item(
         'move_to_new_folder_name': 'foo',
         'move_to': PARENT_FOLDER_ID
     },
+    # move to existing, but no templates to move
+    {
+        'operation': 'move_to_existing_folder',
+        'templates_and_folders': [],
+        'move_to_new_folder_name': '',
+        'move_to': PARENT_FOLDER_ID
+    },
     # move to new, but nothing selected to move
     {
         'operation': 'move_to_new_folder',
         'templates_and_folders': [],
         'move_to_new_folder_name': 'foo',
         'move_to': None
-    }
+    },
+    # add a new template, but also select move destination
+    {
+        'operation': 'add_template',
+        'templates_and_folders': [],
+        'move_to_new_folder_name': '',
+        'move_to': PARENT_FOLDER_ID,
+        'add_template_by_template_type': 'email',
+    },
+    # add a new template, but also move to root folder
+    {
+        'operation': 'add_template',
+        'templates_and_folders': [],
+        'move_to_new_folder_name': '',
+        'move_to': '__NONE__',
+        'add_template_by_template_type': 'email',
+    },
 ])
 def test_no_action_if_user_fills_in_ambiguous_fields(
     client_request,
@@ -945,9 +969,14 @@ def test_no_action_if_user_fills_in_ambiguous_fields(
     mock_create_template_folder,
     data,
 ):
-    service_one['permissions'] += ['edit_folders']
+    service_one['permissions'] += ['edit_folders', 'letter']
 
-    client_request.post(
+    mock_get_template_folders.return_value = [
+        {'id': PARENT_FOLDER_ID, 'name': 'parent folder', 'parent_id': None},
+        {'id': FOLDER_TWO_ID, 'name': 'folder_two', 'parent_id': None},
+    ]
+
+    page = client_request.post(
         'main.choose_template',
         service_id=SERVICE_ONE_ID,
         _data=data,
@@ -958,6 +987,26 @@ def test_no_action_if_user_fills_in_ambiguous_fields(
     assert mock_move_to_template_folder.called is False
     assert mock_create_template_folder.called is False
 
+    assert page.select_one('button[value={}]'.format(data['operation']))
+
+    assert [
+        'email',
+        'sms',
+        'letter',
+        'copy-existing',
+    ] == [
+        radio['value']
+        for radio in page.select('#add_new_template_form input[type=radio]')
+    ]
+
+    assert [
+        FOLDER_TWO_ID,
+        PARENT_FOLDER_ID,
+    ] == [
+        radio['value']
+        for radio in page.select('#move_to_folder_radios input[type=radio]')
+    ]
+
 
 def test_new_folder_is_created_if_only_new_folder_is_filled_out(
     client_request,

tests/app/main/views/test_templates.py

@@ -21,6 +21,7 @@ from tests.conftest import (
     SERVICE_ONE_ID,
     SERVICE_TWO_ID,
     TEMPLATE_ONE_ID,
+    ElementNotFound,
     active_caseworking_user,
     active_user_view_permissions,
     mock_get_service_email_template,
@@ -40,6 +41,7 @@ from tests.conftest import single_letter_contact_block
 def test_should_show_empty_page_when_no_templates(
     client_request,
     service_one,
+    mock_get_organisations_and_services_for_user,
     mock_get_service_templates_when_no_templates_exist,
     mock_get_template_folders,
     extra_permissions,
@@ -128,6 +130,7 @@ def test_should_show_empty_page_when_no_templates(
 )
 def test_should_show_page_for_choosing_a_template(
     client_request,
+    mock_get_organisations_and_services_for_user,
     mock_get_service_templates,
     mock_get_template_folders,
     mock_has_no_jobs,
@@ -242,6 +245,63 @@ def test_should_show_live_search_if_service_has_lots_of_folders(
     assert count_of_templates == 4
 
 
+@pytest.mark.parametrize('extra_permissions, expected_values, expected_labels', (
+    pytest.param(['edit_folders'], [
+        'email',
+        'sms',
+        'copy-existing',
+    ], [
+        'Email template',
+        'Text message template',
+        'Copy of an existing template',
+    ]),
+    pytest.param(['edit_folders', 'letter'], [
+        'email',
+        'sms',
+        'letter',
+        'copy-existing',
+    ], [
+        'Email template',
+        'Text message template',
+        'Letter template',
+        'Copy of an existing template',
+    ]),
+    pytest.param(
+        [], [], [],
+        marks=pytest.mark.xfail(raises=ElementNotFound)
+    ),
+))
+def test_should_show_new_template_choices_if_service_has_folder_permission(
+    client_request,
+    service_one,
+    mock_get_organisations_and_services_for_user,
+    mock_get_service_templates,
+    mock_get_template_folders,
+    extra_permissions,
+    expected_values,
+    expected_labels,
+):
+    service_one['permissions'] += extra_permissions
+
+    page = client_request.get(
+        'main.choose_template',
+        service_id=SERVICE_ONE_ID,
+    )
+
+    if not page.select('#add_new_template_form'):
+        raise ElementNotFound()
+
+    assert normalize_spaces(page.select_one('#add_new_template_form fieldset fieldset legend').text) == (
+        'Add new'
+    )
+    assert [
+        choice['value'] for choice in page.select('#add_new_template_form input[type=radio]')
+    ] == expected_values
+    assert [
+        normalize_spaces(choice.text) for choice in page.select('#add_new_template_form label')
+    ] == expected_labels
+
+
 def test_should_show_page_for_one_template(
     logged_in_client,
     mock_get_service_template,
@@ -551,15 +611,35 @@ def test_dont_show_preview_letter_templates_for_bad_filetype(
     assert mock_get_service_template.called is False
 
 
+@pytest.mark.parametrize('endpoint, data', (
+    ('main.add_template_by_type', {
+        'template_type': 'copy-existing'
+    }),
+    ('main.choose_template', {
+        'operation': 'add_template',
+        'add_template_by_template_type': 'copy-existing'
+    }),
+))
 def test_choosing_to_copy_redirects(
     client_request,
+    service_one,
     mock_get_service_templates,
+    mock_get_template_folders,
     mock_get_organisations_and_services_for_user,
+    endpoint,
+    data,
 ):
+    service_one['permissions'] += ['edit_folders']
     client_request.post(
-        'main.add_template_by_type',
+        endpoint,
         service_id=SERVICE_ONE_ID,
-        _data={'template_type': 'copy-existing'}
+        _data=data,
+        _expected_status=302,
+        _expected_redirect=url_for(
+            'main.choose_template_to_copy',
+            service_id=SERVICE_ONE_ID,
+            _external=True,
+        ),
     )
 
 
@@ -693,29 +773,56 @@ def test_cant_copy_template_from_non_member_service(
     assert mock_get_service_email_template.call_args_list == []
 
 
-@pytest.mark.parametrize('type_of_template', ['email', 'sms'])
+@pytest.mark.parametrize('endpoint, data, expected_error', (
+    (
+        'main.add_template_by_type',
+        {
+            'template_type': 'email',
+        },
+        "Sending emails has been disabled for your service."
+    ),
+    (
+        'main.add_template_by_type',
+        {
+            'template_type': 'sms',
+        },
+        "Sending text messages has been disabled for your service."
+    ),
+    (
+        'main.choose_template',
+        {
+            'operation': 'add_template',
+            'add_template_by_template_type': 'email',
+        },
+        "Sending emails has been disabled for your service."
+    ),
+    (
+        'main.choose_template',
+        {
+            'operation': 'add_template',
+            'add_template_by_template_type': 'sms',
+        },
+        "Sending text messages has been disabled for your service."
+    ),
+))
 def test_should_not_allow_creation_of_template_through_form_without_correct_permission(
-    logged_in_client,
+    client_request,
     service_one,
-    mocker,
     mock_get_service_templates,
+    mock_get_template_folders,
     mock_get_organisations_and_services_for_user,
-    type_of_template,
+    endpoint,
+    data,
+    expected_error,
 ):
-    service_one['permissions'] = []
-    template_description = {'sms': 'text messages', 'email': 'emails'}
-
-    response = logged_in_client.post(url_for(
-        '.add_template_by_type',
-        service_id=service_one['id']),
-        data={'template_type': type_of_template},
-        follow_redirects=True)
-
-    page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
-
-    assert response.status_code == 200
-    assert page.select('main p')[0].text.strip() == \
-        "Sending {} has been disabled for your service.".format(template_description[type_of_template])
+    service_one['permissions'] = ['edit_folders']
+    page = client_request.post(
+        endpoint,
+        service_id=SERVICE_ONE_ID,
+        _data=data,
+        _follow_redirects=True,
+    )
+    assert normalize_spaces(page.select('main p')[0].text) == expected_error
     assert page.select(".page-footer-back-link")[0].text == "Back to add new template"
     assert page.select(".page-footer-back-link")[0]['href'] == url_for(
         '.add_template_by_type',

tests/conftest.py

@@ -32,6 +32,10 @@ from . import (
 )
 
 
+class ElementNotFound(Exception):
+    pass
+
+
 @pytest.fixture
 def app_(request):
     app = Flask('app')