From d49622c5dc4c7c9108667a8bb9c0c4aacdd00280 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 16 Nov 2018 13:41:55 +0000 Subject: [PATCH 1/3] Style folder hierarchy in headings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This makes the display of folders in the `

` look like the prototype. It alters the behaviour we’ve initially built here by only ever showing a maximum of two levels of hierarchy (the current folders and its parent). --- .../stylesheets/components/message.scss | 56 +++++++++++++++++++ app/models/service.py | 23 ++++---- app/templates/components/folder-path.html | 50 +++++++++++++++++ app/templates/views/templates/choose.html | 34 +++++------ tests/app/main/views/test_template_folders.py | 47 ++++++++++++++-- 5 files changed, 176 insertions(+), 34 deletions(-) create mode 100644 app/templates/components/folder-path.html diff --git a/app/assets/stylesheets/components/message.scss b/app/assets/stylesheets/components/message.scss index 55267d554..2e9b52180 100644 --- a/app/assets/stylesheets/components/message.scss +++ b/app/assets/stylesheets/components/message.scss @@ -55,3 +55,59 @@ } } + +.folder-heading { + + .column-main>.grid-row:first-child &.heading-medium { + margin-top: 18px; + } + + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; + + + a, + &-folder { + + display: inline-block; + vertical-align: top; + padding: 5px 0 0 60px; + background-repeat: no-repeat; + background-size: 38px auto; + background-position: 0 1px; + + } + + a { + + background-image: file-url('folder-blue.svg'); + max-width: 33%; + overflow: hidden; + text-overflow: ellipsis; + + } + + &-folder { + + padding-left: 0; + + &:first-child { + background-image: file-url('folder-black.svg'); + padding-left: $gutter * 2; + } + + } + + &-separator { + + display: inline-block; + vertical-align: top; + color: $secondary-text-colour; + padding: 5px 4px 0 5px; + font-weight: normal; + + } + + +} diff --git a/app/models/service.py b/app/models/service.py index 32de656ba..d91a12e5e 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -316,6 +316,12 @@ class Service(): ] def get_template_folder(self, folder_id): + if folder_id is None: + return { + 'id': None, + 'name': 'Templates', + 'parent_id': None, + } return self._get_by_id(self.all_template_folders, folder_id) def is_folder_visible(self, template_folder_id, template_type='all'): @@ -335,19 +341,16 @@ class Service(): return False def get_template_folder_path(self, template_folder_id): - if template_folder_id is None: - return [] - id_to_folder = {folder['id']: folder for folder in self.all_template_folders} + folder = self.get_template_folder(template_folder_id) - folder = id_to_folder[template_folder_id] - path = [folder] + if folder['id'] is None: + return [folder] - while folder['parent_id']: - folder = id_to_folder[folder['parent_id']] - path.append(folder) - - return list(reversed(path)) + return [ + self.get_template_folder(folder['parent_id']), + folder, + ] def get_template_folders_and_templates(self, template_type, template_folder_id): return ( diff --git a/app/templates/components/folder-path.html b/app/templates/components/folder-path.html new file mode 100644 index 000000000..ef747046b --- /dev/null +++ b/app/templates/components/folder-path.html @@ -0,0 +1,50 @@ +{% macro folder_path( + folders, + service_id, + template_type, + fallback_page_title=None, + show_fallback_page_title=False +) %} + {% if show_fallback_page_title %} +

+ {{ fallback_page_title }} +

+ {% else %} +

+ {% for folder in folders %} + {% if loop.last %} + {{ folder.name }} + {% else %} + {% if folder.id %} + {{ folder.name }} {{ folder_path_separator() }} + {% else %} + Templates {{ folder_path_separator() }} + {% endif %} + {% endif %} + {% endfor %} +

+ {% endif %} +{% endmacro %} + + +{% macro page_title_folder_path( + folders, + fallback_page_title=None, + show_fallback_page_title=False +) %} + {% if show_fallback_page_title %} + {{ fallback_page_title }} + {% else %} + {% for folder in folders|reverse %} + {{ folder.name }} + {% if not loop.last %} + – + {% endif %} + {% endfor %} + {% endif %} +{% endmacro %} + + +{% macro folder_path_separator() %} + / +{% endmacro %} diff --git a/app/templates/views/templates/choose.html b/app/templates/views/templates/choose.html index c3e519c6d..9b5e8fe6c 100644 --- a/app/templates/views/templates/choose.html +++ b/app/templates/views/templates/choose.html @@ -1,3 +1,4 @@ +{% from "components/folder-path.html" import folder_path, page_title_folder_path %} {% from "components/pill.html" import pill %} {% from "components/message-count-label.html" import message_count_label %} {% from "components/textbox.html" import textbox %} @@ -8,10 +9,11 @@ {% set page_title = 'Templates' %} {% block service_page_title %} - {{ page_title }} - {% for folder in template_folder_path %} - / {{ folder.name }} - {% endfor %} + {{ page_title_folder_path( + template_folder_path, + fallback_page_title=page_title, + show_fallback_page_title=not current_service.all_template_folders + ) }} {% endblock %} {% block maincolumn_content %} @@ -46,27 +48,19 @@
-

- {% if template_folder_path %} - {{ page_title }} - {% else %} - {{ page_title }} - {% endif %} - {% for folder in template_folder_path %} - / - {% if loop.last %} - {{ folder.name }} - {% else %} - {{ folder.name }} - {% endif %} - {% endfor %} -

+ {{ folder_path( + folders=template_folder_path, + service_id=current_service.id, + template_type=template_type, + fallback_page_title=page_title, + show_fallback_page_title=not current_service.all_template_folders + ) }}
{% if current_user.has_permissions('manage_templates') %}
Add new template - {% if can_manage_folders and template_folder_path %} + {% if can_manage_folders and current_template_folder_id %} Manage {% endif %}
diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index 8fe03eb91..f1dd42de0 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -99,10 +99,19 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare @pytest.mark.parametrize( - 'expected_page_title, extra_args, expected_nav_links, expected_items', + ( + 'expected_title_tag,' + 'expected_page_title,' + 'expected_parent_link_args,' + 'extra_args,' + 'expected_nav_links,' + 'expected_items' + ), [ ( + 'Templates – service one – GOV.UK Notify', 'Templates', + None, {}, ['Text message', 'Email', 'Letter'], [ @@ -117,7 +126,9 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare ] ), ( + 'Templates – service one – GOV.UK Notify', 'Templates', + None, {'template_type': 'sms'}, ['All', 'Email', 'Letter'], [ @@ -127,7 +138,9 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare ], ), ( + 'folder_one – Templates – service one – GOV.UK Notify', 'Templates / folder_one', + {'template_type': 'all'}, {'template_folder_id': PARENT_FOLDER_ID}, ['Text message', 'Email', 'Letter'], [ @@ -136,7 +149,9 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare ], ), ( + 'folder_one – Templates – service one – GOV.UK Notify', 'Templates / folder_one', + {'template_type': 'sms'}, {'template_type': 'sms', 'template_folder_id': PARENT_FOLDER_ID}, ['All', 'Email', 'Letter'], [ @@ -144,13 +159,17 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare ], ), ( + 'folder_one – Templates – service one – GOV.UK Notify', 'Templates / folder_one', + {'template_type': 'email'}, {'template_type': 'email', 'template_folder_id': PARENT_FOLDER_ID}, ['All', 'Text message', 'Letter'], [], ), ( - 'Templates / folder_one / folder_one_one', + 'folder_one_one – folder_one – service one – GOV.UK Notify', + 'folder_one / folder_one_one', + {'template_type': 'all', 'template_folder_id': PARENT_FOLDER_ID}, {'template_folder_id': CHILD_FOLDER_ID}, ['Text message', 'Email', 'Letter'], [ @@ -159,7 +178,9 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare ], ), ( - 'Templates / folder_one / folder_one_one / folder_one_one_one', + 'folder_one_one_one – folder_one_one – service one – GOV.UK Notify', + 'folder_one_one / folder_one_one_one', + {'template_type': 'all', 'template_folder_id': CHILD_FOLDER_ID}, {'template_folder_id': GRANDCHILD_FOLDER_ID}, ['Text message', 'Email', 'Letter'], [ @@ -175,7 +196,9 @@ def test_should_show_templates_folder_page( service_one, mocker, fake_uuid, + expected_title_tag, expected_page_title, + expected_parent_link_args, extra_args, expected_nav_links, expected_items, @@ -206,11 +229,23 @@ def test_should_show_templates_folder_page( page = client_request.get( 'main.choose_template', service_id=SERVICE_ONE_ID, + _test_page_title=False, **extra_args ) + assert normalize_spaces(page.select_one('title').text) == expected_title_tag assert normalize_spaces(page.select_one('h1').text) == expected_page_title + if expected_parent_link_args: + assert len(page.select('h1 a')) == 1 + assert page.select_one('h1 a')['href'] == url_for( + 'main.choose_template', + service_id=SERVICE_ONE_ID, + **expected_parent_link_args + ) + else: + assert page.select_one('h1 a') is None + links_in_page = page.select('.pill a') assert len(links_in_page) == len(expected_nav_links) @@ -312,7 +347,11 @@ def test_can_create_email_template_with_parent_folder( data['parent_folder_id']) -def test_get_manage_folder_page(client_request, service_one, mock_get_template_folders): +def test_get_manage_folder_page( + client_request, + service_one, + mock_get_template_folders, +): folder_id = str(uuid.uuid4()) mock_get_template_folders.return_value = [ {'id': folder_id, 'name': 'folder_two', 'parent_id': None}, From 738043c5c4a23ae051f7feeff04e03606a6a9bb0 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 16 Nov 2018 14:09:14 +0000 Subject: [PATCH 2/3] Show path on template and manage folder pages So the page headings stay consistent as you click around, and make it easy to get back where you came from. --- app/models/service.py | 6 +++++ app/templates/views/templates/choose.html | 2 +- .../templates/manage-template-folder.html | 25 ++++++++----------- app/templates/views/templates/template.html | 13 +++++++++- tests/__init__.py | 1 + tests/app/main/views/test_template_folders.py | 9 +++++-- tests/app/main/views/test_templates.py | 13 ++++++++++ 7 files changed, 51 insertions(+), 18 deletions(-) diff --git a/app/models/service.py b/app/models/service.py index d91a12e5e..dff5916e6 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -352,6 +352,12 @@ class Service(): folder, ] + def get_template_path(self, template): + return [ + self.get_template_folder(template['folder']), + template, + ] + def get_template_folders_and_templates(self, template_type, template_folder_id): return ( self.get_templates(template_type, template_folder_id) + diff --git a/app/templates/views/templates/choose.html b/app/templates/views/templates/choose.html index 9b5e8fe6c..980e2d6e1 100644 --- a/app/templates/views/templates/choose.html +++ b/app/templates/views/templates/choose.html @@ -46,7 +46,7 @@ {% else %} -
+
{{ folder_path( folders=template_folder_path, diff --git a/app/templates/views/templates/manage-template-folder.html b/app/templates/views/templates/manage-template-folder.html index 7066003ac..ae8990fd9 100644 --- a/app/templates/views/templates/manage-template-folder.html +++ b/app/templates/views/templates/manage-template-folder.html @@ -1,26 +1,24 @@ {% extends "withnav_template.html" %} +{% from "components/folder-path.html" import folder_path, page_title_folder_path %} {% from "components/textbox.html" import textbox %} {% from "components/page-footer.html" import page_footer %} {% from "components/form.html" import form_wrapper %} {% block service_page_title %} -Templates -{% for folder in template_folder_path %} - / - {{ folder.name }} -{% endfor %} - - Manage folder + {{ page_title_folder_path(template_folder_path) }} {% endblock %} {% block maincolumn_content %} -

- Templates - {% for folder in template_folder_path %} - / - {{ folder.name }} - {% endfor %} -

+
+
+ {{ folder_path( + folders=template_folder_path, + service_id=current_service.id, + template_type='all', + ) }} +
+
{% if not delete_folder %} {% call form_wrapper() %} @@ -33,7 +31,6 @@ Templates template_folder_id=template_folder_id ), delete_link_text="Delete this folder") }} - {% endcall %} {% else %} Back to manage folder page diff --git a/app/templates/views/templates/template.html b/app/templates/views/templates/template.html index 8bde511c6..5f61b25a5 100644 --- a/app/templates/views/templates/template.html +++ b/app/templates/views/templates/template.html @@ -1,5 +1,6 @@ {% extends "withnav_template.html" %} {% from "components/banner.html" import banner_wrapper %} +{% from "components/folder-path.html" import folder_path %} {% from "components/page-footer.html" import page_footer %} {% from "components/textbox.html" import textbox %} {% from "components/api-key.html" import api_key %} @@ -28,7 +29,17 @@ {% endcall %}
{% else %} -

{{ template.name }}

+
+
+ {{ folder_path( + folders=current_service.get_template_path(template._template), + service_id=current_service.id, + template_type='all', + fallback_page_title=template.name, + show_fallback_page_title=not current_service.all_template_folders + ) }} +
+
{% endif %}
diff --git a/tests/__init__.py b/tests/__init__.py index f12e111b8..cf96dd13d 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -229,6 +229,7 @@ def template_json(service_id, 'reply_to': reply_to, 'reply_to_text': reply_to_text, 'is_precompiled_letter': is_precompiled_letter, + 'folder': None, } if content is None: template['content'] = "template content" diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index f1dd42de0..506c80ef3 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -361,7 +361,11 @@ def test_get_manage_folder_page( page = client_request.get( 'main.manage_template_folder', service_id=service_one['id'], - template_folder_id=folder_id + template_folder_id=folder_id, + _test_page_title=False, + ) + assert normalize_spaces(page.select_one('title').text) == ( + 'folder_two – Templates – service one – GOV.UK Notify' ) assert page.select_one('input[name=name]') is not None delete_link = page.find('a', string="Delete this folder") @@ -431,7 +435,8 @@ def test_delete_template_folder_should_request_confirmation( ) page = client_request.get( 'main.delete_template_folder', service_id=service_one['id'], - template_folder_id=folder_id + template_folder_id=folder_id, + _test_page_title=False, ) assert normalize_spaces(page.select('.banner-dangerous')[0].text) == ( 'Are you sure you want to delete the ‘sacrifice’ folder? ' diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index fcfa62108..9531056a6 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -492,6 +492,7 @@ def test_user_with_only_send_and_view_redirected_to_one_off( def test_user_with_only_send_and_view_sees_letter_page( client_request, mock_get_service_templates, + mock_get_template_folders, mock_get_service_letter_template, single_letter_contact_block, mock_has_jobs, @@ -536,6 +537,7 @@ def test_user_with_only_send_and_view_sees_letter_page( def test_should_be_able_to_view_a_template_with_links( client, mock_get_service_template, + mock_get_template_folders, active_user_with_permissions, single_letter_contact_block, mocker, @@ -575,6 +577,7 @@ def test_should_be_able_to_view_a_template_with_links( def test_should_show_template_id_on_template_page( logged_in_client, mock_get_service_template, + mock_get_template_folders, service_one, fake_uuid, ): @@ -595,6 +598,7 @@ def test_should_show_sms_template_with_downgraded_unicode_characters( mocker, service_one, single_letter_contact_block, + mock_get_template_folders, fake_uuid, ): msg = 'here:\tare some “fancy quotes” and zero\u200Bwidth\u200Bspaces' @@ -618,6 +622,7 @@ def test_should_show_sms_template_with_downgraded_unicode_characters( def test_should_let_letter_contact_block_be_changed_for_the_template( mocker, mock_get_service_letter_template, + mock_get_template_folders, no_letter_contact_blocks, client_request, service_one, @@ -1264,6 +1269,7 @@ def test_should_redirect_when_saving_a_template_email( def test_should_show_delete_template_page_with_time_block( client_request, mock_get_service_template, + mock_get_template_folders, mocker, fake_uuid ): @@ -1294,6 +1300,7 @@ def test_should_show_delete_template_page_with_time_block( def test_should_show_delete_template_page_with_time_block_for_empty_notification( client_request, mock_get_service_template, + mock_get_template_folders, mocker, fake_uuid ): @@ -1323,6 +1330,7 @@ def test_should_show_delete_template_page_with_time_block_for_empty_notification def test_should_show_delete_template_page_with_never_used_block( client_request, mock_get_service_template, + mock_get_template_folders, fake_uuid, mocker, ): @@ -1390,6 +1398,7 @@ def test_should_show_page_for_a_deleted_template( api_user_active, mock_login, mock_get_service, + mock_get_template_folders, mock_get_deleted_template, single_letter_contact_block, mock_get_user, @@ -1430,6 +1439,7 @@ def test_route_permissions( api_user_active, service_one, mock_get_service_template, + mock_get_template_folders, mock_get_template_statistics_for_template, fake_uuid, ): @@ -1713,6 +1723,7 @@ def test_should_show_message_before_redacting_template( def test_should_show_redact_template( client_request, mock_get_service_template, + mock_get_template_folders, mock_redact_template, single_letter_contact_block, service_one, @@ -1737,6 +1748,7 @@ def test_should_show_hint_once_template_redacted( client_request, mocker, service_one, + mock_get_template_folders, fake_uuid, ): @@ -1756,6 +1768,7 @@ def test_should_not_show_redaction_stuff_for_letters( mocker, fake_uuid, mock_get_service_letter_template, + mock_get_template_folders, single_letter_contact_block, ): From d3e7557058412b2f9b5ab7a4ae42bdcbbc7b7a81 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 16 Nov 2018 15:22:51 +0000 Subject: [PATCH 3/3] Link current level in hierarchy from manage folder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Because that’s the page you come directly back from. --- app/assets/stylesheets/components/message.scss | 16 +++++++++++----- app/templates/components/folder-path.html | 9 +++++---- .../views/templates/manage-template-folder.html | 1 + 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/app/assets/stylesheets/components/message.scss b/app/assets/stylesheets/components/message.scss index 2e9b52180..7b46c6cd3 100644 --- a/app/assets/stylesheets/components/message.scss +++ b/app/assets/stylesheets/components/message.scss @@ -67,7 +67,7 @@ white-space: nowrap; - a, + a:first-child, &-folder { display: inline-block; @@ -81,10 +81,16 @@ a { - background-image: file-url('folder-blue.svg'); - max-width: 33%; - overflow: hidden; - text-overflow: ellipsis; + padding-top: 5px; + display: inline-block; + vertical-align: top; + + &:first-child { + background-image: file-url('folder-blue.svg'); + max-width: 33%; + overflow: hidden; + text-overflow: ellipsis; + } } diff --git a/app/templates/components/folder-path.html b/app/templates/components/folder-path.html index ef747046b..d342465b5 100644 --- a/app/templates/components/folder-path.html +++ b/app/templates/components/folder-path.html @@ -3,7 +3,8 @@ service_id, template_type, fallback_page_title=None, - show_fallback_page_title=False + show_fallback_page_title=False, + link_current_item=False ) %} {% if show_fallback_page_title %}

@@ -12,13 +13,13 @@ {% else %}

{% for folder in folders %} - {% if loop.last %} + {% if loop.last and not link_current_item %} {{ folder.name }} {% else %} {% if folder.id %} - {{ folder.name }} {{ folder_path_separator() }} + {{ folder.name }} {% if not loop.last %}{{ folder_path_separator() }}{% endif %} {% else %} - Templates {{ folder_path_separator() }} + Templates {% if not loop.last %}{{ folder_path_separator() }}{% endif %} {% endif %} {% endif %} {% endfor %} diff --git a/app/templates/views/templates/manage-template-folder.html b/app/templates/views/templates/manage-template-folder.html index ae8990fd9..b638e8479 100644 --- a/app/templates/views/templates/manage-template-folder.html +++ b/app/templates/views/templates/manage-template-folder.html @@ -16,6 +16,7 @@ folders=template_folder_path, service_id=current_service.id, template_type='all', + link_current_item=True ) }}