From a0f6f9b1379ef7865df7d6ea61888f2141006b2f Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 13 Nov 2019 12:45:26 +0000 Subject: [PATCH 1/2] Add a specific route for template_type=all `all` is not a real template type, so for links to template folders that apply to all template types we have a URL that looks like: ``` /services//templates/folders/ ``` However Flask only generates this url when `url_for` is called with `template_type=None`. If called with `template_type=all` then Flask will generate a URL like ``` /services//templates/all/folders/ ``` However attempting to load this URL will now 404, since `all` is not a template type recognised by the regex introduced in https://github.com/alphagov/notifications-admin/pull/3176 It would be nice to not have URLs with `all` in them at all, but since people might have bookmarked them we need to support them indefinitely. Also considered but decided against adding `all` to the set of template types because it might cause other problems, for example attempting to create a new template with a type of `all` would never work. --- app/main/views/templates.py | 1 + tests/app/main/test_permissions.py | 2 +- tests/app/main/views/test_template_folders.py | 11 +++++++++++ 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index a3827a4b5..320869ebd 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -108,6 +108,7 @@ def start_tour(service_id, template_id): @main.route("/services//templates", methods=['GET', 'POST']) @main.route("/services//templates/folders/", methods=['GET', 'POST']) @main.route("/services//templates/", methods=['GET', 'POST']) +@main.route("/services//templates/all/folders/", methods=['GET', 'POST']) @main.route("/services//templates//folders/", methods=['GET', 'POST']) @user_has_permissions() def choose_template(service_id, template_type='all', template_folder_id=None): diff --git a/tests/app/main/test_permissions.py b/tests/app/main/test_permissions.py index a98617722..5303c5f9c 100644 --- a/tests/app/main/test_permissions.py +++ b/tests/app/main/test_permissions.py @@ -422,7 +422,7 @@ def format_decorators(decorators, indent=8): def test_code_to_extract_decorators_works_with_known_examples(): assert ( 'templates.choose_template', - ['main.route', 'main.route', 'main.route', 'main.route', 'user_has_permissions'], + ['main.route', 'main.route', 'main.route', 'main.route', 'main.route', 'user_has_permissions'], ) in list( get_routes_and_decorators(SERVICE_ID_ARGUMENT) ) diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index c93e70662..2e1c1217f 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -268,6 +268,17 @@ def _folder(name, folder_id=None, parent=None, users_with_permission=None): [], 'This folder is empty', ), + ( + 'folder_two – Templates – service one – GOV.UK Notify', + 'Templates folder_two', + [{'template_type': 'all'}], + {'template_folder_id': FOLDER_TWO_ID, 'template_type': 'all'}, + ['Email', 'Text message', 'Letter'], + [], + [], + [], + 'This folder is empty', + ), ] ) def test_should_show_templates_folder_page( From 91283c99b94a8c39c60270e3782724384d0cb98a Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 13 Nov 2019 13:38:02 +0000 Subject: [PATCH 2/2] Add a specific route for template_type=all `all` is not a real template type, so for links to template folders that apply to all template types we have a URL that looks like: ``` /services//templates ``` However Flask only generates this url when `url_for` is called with `template_type=None`. If called with `template_type=all` then Flask will generate a URL like ``` /services//templates/all ``` However attempting to load this URL will now 404, since `all` is not a template type recognised by the regex introduced in https://github.com/alphagov/notifications-admin/pull/3176 It would be nice to not have URLs with `all` in them at all, but since people might have bookmarked them we need to support them indefinitely. Also considered but decided against adding `all` to the set of template types because it might cause other problems, for example attempting to create a new template with a type of `all` would never work. --- app/main/views/templates.py | 1 + tests/app/main/test_permissions.py | 2 +- tests/app/main/views/test_template_folders.py | 48 +++++++++++++++++++ 3 files changed, 50 insertions(+), 1 deletion(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 320869ebd..16e6192cc 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -106,6 +106,7 @@ def start_tour(service_id, template_id): @main.route("/services//templates", methods=['GET', 'POST']) +@main.route("/services//templates/all", methods=['GET', 'POST']) @main.route("/services//templates/folders/", methods=['GET', 'POST']) @main.route("/services//templates/", methods=['GET', 'POST']) @main.route("/services//templates/all/folders/", methods=['GET', 'POST']) diff --git a/tests/app/main/test_permissions.py b/tests/app/main/test_permissions.py index 5303c5f9c..34462f5b4 100644 --- a/tests/app/main/test_permissions.py +++ b/tests/app/main/test_permissions.py @@ -422,7 +422,7 @@ def format_decorators(decorators, indent=8): def test_code_to_extract_decorators_works_with_known_examples(): assert ( 'templates.choose_template', - ['main.route', 'main.route', 'main.route', 'main.route', 'main.route', 'user_has_permissions'], + ['main.route', 'main.route', 'main.route', 'main.route', 'main.route', 'main.route', 'user_has_permissions'], ) in list( get_routes_and_decorators(SERVICE_ID_ARGUMENT) ) diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index 2e1c1217f..391292ae5 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -95,6 +95,54 @@ def _folder(name, folder_id=None, parent=None, users_with_permission=None): ], None, ), + ( + 'Templates – service one – GOV.UK Notify', + 'Templates', + [], + {'template_type': 'all'}, + ['Email', 'Text message', 'Letter'], + [ + 'folder_one 2 folders', + 'folder_one folder_one_one 1 template, 1 folder', + 'folder_one folder_one_one folder_one_one_one 1 template', + 'folder_one folder_one_one folder_one_one_one sms_template_nested Text message template', + 'folder_one folder_one_one letter_template_nested Letter template', + 'folder_one folder_one_two Empty', + 'folder_two Empty', + 'sms_template_one Text message template', + 'sms_template_two Text message template', + 'email_template_one Email template', + 'email_template_two Email template', + 'letter_template_one Letter template', + 'letter_template_two Letter template', + ], + [ + 'folder_one 2 folders', + 'folder_two Empty', + 'sms_template_one Text message template', + 'sms_template_two Text message template', + 'email_template_one Email template', + 'email_template_two Email template', + 'letter_template_one Letter template', + 'letter_template_two Letter template', + ], + [ + 'folder_one', + 'folder_one_one', + 'folder_one_one_one', + 'sms_template_nested', + 'letter_template_nested', + 'folder_one_two', + 'folder_two', + 'sms_template_one', + 'sms_template_two', + 'email_template_one', + 'email_template_two', + 'letter_template_one', + 'letter_template_two', + ], + None, + ), ( 'Templates – service one – GOV.UK Notify', 'Templates',