From 72159ff6e33d4d87fc57ce3cf5e79dc33b63c0f5 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 19 Nov 2018 16:26:27 +0000 Subject: [PATCH 1/4] Fix search box show/hide logic The search box should only be shown if there are too many items to quickly scan on the page. This number is judged to be 7; see the original PR for more context: https://github.com/alphagov/notifications-admin/pull/1195 This commit makes that logic work in the world of folders, by explicitly counting the templates shown on the screen, not all the templates a service has (which may be spread across multiple folders). --- app/main/views/templates.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 70a854143..c11231006 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -121,14 +121,16 @@ def choose_template(service_id, template_type='all', template_folder_id=None): ) return redirect(request.url) + templates_on_page = current_service.get_templates(template_type, template_folder_id) + return render_template( 'views/templates/choose.html', current_template_folder_id=template_folder_id, can_manage_folders=can_manage_folders(), template_folder_path=current_service.get_template_folder_path(template_folder_id), template_folders=current_service.get_template_folders(template_type, template_folder_id), - templates=current_service.get_templates(template_type, template_folder_id), - show_search_box=(len(current_service.get_templates(template_type)) > 7), + templates=templates_on_page, + show_search_box=(len(templates_on_page) > 7), show_template_nav=( current_service.has_multiple_template_types and (len(current_service.all_templates) > 2) From 32c36bf70bc29a1d29d38ef7274c5fce49aabb20 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 19 Nov 2018 16:52:21 +0000 Subject: [PATCH 2/4] Make the empty folder state more useful MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently if there’s nothing in a folder you just get an empty page. This looks a bit broken, or like the page hasn’t finished loading. This commit adds a message to the page to show that it’s intentionally blank. The message is contextual based on type of template, because there might be templates in the current folder, even if you can’t see them at the moment (because you’re filtering). --- .../stylesheets/components/message.scss | 5 ++ app/models/service.py | 5 +- .../views/templates/_template_list.html | 90 ++++++++++--------- app/templates/views/templates/choose.html | 14 ++- tests/app/main/views/test_template_folders.py | 32 ++++++- 5 files changed, 98 insertions(+), 48 deletions(-) diff --git a/app/assets/stylesheets/components/message.scss b/app/assets/stylesheets/components/message.scss index 7b46c6cd3..ec44dd8a7 100644 --- a/app/assets/stylesheets/components/message.scss +++ b/app/assets/stylesheets/components/message.scss @@ -54,6 +54,11 @@ } + &-empty { + color: $secondary-text-colour; + padding: $gutter-half 0 $gutter-one-third 0; + } + } .folder-heading { diff --git a/app/models/service.py b/app/models/service.py index dff5916e6..7192b9e30 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -133,7 +133,10 @@ class Service(): @property def has_templates(self): - return len(self.all_templates) > 0 + return bool(self.all_templates) + + def has_folders(self): + return bool(self.all_template_folders) @property def has_multiple_template_types(self): diff --git a/app/templates/views/templates/_template_list.html b/app/templates/views/templates/_template_list.html index d92627d76..cf41d3d31 100644 --- a/app/templates/views/templates/_template_list.html +++ b/app/templates/views/templates/_template_list.html @@ -1,44 +1,50 @@ {% from "components/checkbox.html" import unlabelled_checkbox %} -{% from "components/message-count-label.html" import folder_contents_count %} +{% from "components/message-count-label.html" import folder_contents_count, message_count_label %} - +{% if service_has_templates_or_folders and not templates and not template_folders %} +

+ No {{ message_count_label(1, template_type, suffix='') }} templates in this folder +

+{% else %} + +{% endif %} diff --git a/app/templates/views/templates/choose.html b/app/templates/views/templates/choose.html index 4d3e6877e..4e8145c1c 100644 --- a/app/templates/views/templates/choose.html +++ b/app/templates/views/templates/choose.html @@ -77,7 +77,12 @@ {% if can_manage_folders %}
- {% with templates=templates, template_folders=template_folders %} + {% with + templates=templates, + template_folders=template_folders, + service_has_templates_or_folders=(current_service.has_templates or current_service.has_folders), + template_type=template_type + %} {% include 'views/templates/_template_list.html' %} {% endwith %} {% with templates=templates, template_folders=template_folders, templates_and_folders_form=templates_and_folders_form %} @@ -85,7 +90,12 @@ {% endwith %}
{% else %} - {% with templates=templates, template_folders=template_folders %} + {% with + templates=templates, + template_folders=template_folders, + service_has_templates_or_folders=(current_service.has_templates or current_service.has_folders), + template_type=template_type + %} {% include 'views/templates/_template_list.html' %} {% endwith %} {% endif %} diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index 506c80ef3..44ba3c543 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -8,6 +8,7 @@ from tests.conftest import SERVICE_ONE_ID, normalize_spaces PARENT_FOLDER_ID = '7e979e79-d970-43a5-ac69-b625a8d147b0' CHILD_FOLDER_ID = '92ee1ee0-e4ee-4dcc-b1a7-a5da9ebcfa2b' GRANDCHILD_FOLDER_ID = 'fafe723f-1d39-4a10-865f-e551e03d8886' +FOLDER_TWO_ID = 'bbbb222b-2b22-2b22-222b-b222b22b2222' def _folder(name, folder_id=None, parent=None): @@ -105,7 +106,8 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare 'expected_parent_link_args,' 'extra_args,' 'expected_nav_links,' - 'expected_items' + 'expected_items, ' + 'expected_empty_message ' ), [ ( @@ -123,7 +125,8 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare 'email_template_two Email template', 'letter_template_one Letter template', 'letter_template_two Letter template', - ] + ], + None, ), ( 'Templates – service one – GOV.UK Notify', @@ -136,6 +139,7 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare 'sms_template_one Text message template', 'sms_template_two Text message template', ], + None, ), ( 'folder_one – Templates – service one – GOV.UK Notify', @@ -147,6 +151,7 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare 'folder_one_one 1 template, 1 folder', 'folder_one_two Empty', ], + None, ), ( 'folder_one – Templates – service one – GOV.UK Notify', @@ -157,6 +162,7 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare [ 'folder_one_one 1 folder', ], + None, ), ( 'folder_one – Templates – service one – GOV.UK Notify', @@ -165,6 +171,7 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare {'template_type': 'email', 'template_folder_id': PARENT_FOLDER_ID}, ['All', 'Text message', 'Letter'], [], + 'No email templates in this folder', ), ( 'folder_one_one – folder_one – service one – GOV.UK Notify', @@ -176,6 +183,7 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare 'folder_one_one_one 1 template', 'letter_template_nested Letter template', ], + None, ), ( 'folder_one_one_one – folder_one_one – service one – GOV.UK Notify', @@ -186,6 +194,16 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare [ 'sms_template_nested Text message template', ], + None, + ), + ( + 'folder_two – Templates – service one – GOV.UK Notify', + 'Templates / folder_two', + {'template_type': 'all'}, + {'template_folder_id': FOLDER_TWO_ID}, + ['Text message', 'Email', 'Letter'], + [], + 'No templates in this folder', ), ] ) @@ -202,9 +220,10 @@ def test_should_show_templates_folder_page( extra_args, expected_nav_links, expected_items, + expected_empty_message, ): mock_get_template_folders.return_value = [ - _folder('folder_two'), + _folder('folder_two', FOLDER_TWO_ID), _folder('folder_one', PARENT_FOLDER_ID), _folder('folder_one_two', parent=PARENT_FOLDER_ID), _folder('folder_one_one', CHILD_FOLDER_ID, parent=PARENT_FOLDER_ID), @@ -260,6 +279,13 @@ def test_should_show_templates_folder_page( for index, expected_item in enumerate(expected_items): assert normalize_spaces(page_items[index].text) == expected_item + if expected_empty_message: + assert normalize_spaces(page.select_one('.template-list-empty').text) == ( + expected_empty_message + ) + else: + assert not page.select('.template-list-empty') + mock_get_service_templates.assert_called_once_with(SERVICE_ONE_ID) From 4bb7519d9ef41112ba177c5a33b606656fc1f37b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 20 Nov 2018 12:13:50 +0000 Subject: [PATCH 3/4] Refine empty folder message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the folder is completely empty, always say ‘This folder is empty’, so we don’t suggest that you can click around and find other things. If the current view is filtered, but there are templates available in other filters, say ‘There are no text message templates in this folder’ to make it clear why you can’t see anything. --- app/main/views/templates.py | 1 + app/templates/views/templates/_template_list.html | 6 +++++- app/templates/views/templates/choose.html | 6 ++++-- tests/app/main/views/test_template_folders.py | 13 +++++++++++-- 4 files changed, 21 insertions(+), 5 deletions(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index c11231006..7e4164a9b 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -128,6 +128,7 @@ def choose_template(service_id, template_type='all', template_folder_id=None): current_template_folder_id=template_folder_id, can_manage_folders=can_manage_folders(), template_folder_path=current_service.get_template_folder_path(template_folder_id), + template_folder_has_contents=current_service.get_template_folders_and_templates('all', template_folder_id), template_folders=current_service.get_template_folders(template_type, template_folder_id), templates=templates_on_page, show_search_box=(len(templates_on_page) > 7), diff --git a/app/templates/views/templates/_template_list.html b/app/templates/views/templates/_template_list.html index cf41d3d31..3c1cb7555 100644 --- a/app/templates/views/templates/_template_list.html +++ b/app/templates/views/templates/_template_list.html @@ -3,7 +3,11 @@ {% if service_has_templates_or_folders and not templates and not template_folders %}

- No {{ message_count_label(1, template_type, suffix='') }} templates in this folder + {% if template_folder_has_contents %} + There are no {{ message_count_label(1, template_type, suffix='') }} templates in this folder + {% else %} + This folder is empty + {% endif %}

{% else %}