From 66f66b95bfe8990a1f70bdc1bc08959af7001ff5 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 12 Nov 2018 11:45:47 +0000 Subject: [PATCH 1/9] Make a helper function for testing folders MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Splitting this out into a function makes it easier to see what’s going on in the tests. --- tests/app/main/views/test_template_folders.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index 3bca9dc07..74b0121f6 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -9,6 +9,14 @@ PARENT_FOLDER_ID = '7e979e79-d970-43a5-ac69-b625a8d147b0' CHILD_FOLDER_ID = '92ee1ee0-e4ee-4dcc-b1a7-a5da9ebcfa2b' +def _folder(name, folder_id=None, parent=None): + return { + 'name': name, + 'id': folder_id or str(uuid.uuid4()), + 'parent_id': parent, + } + + @pytest.mark.parametrize('parent_folder_id', [None, PARENT_FOLDER_ID]) def test_add_page_shows_option_for_folder( client_request, @@ -133,10 +141,10 @@ def test_should_show_templates_folder_page( ): mock_get_template_folders.return_value = [ - {'id': str(uuid.uuid4()), 'name': 'folder_two', 'parent_id': None}, - {'id': PARENT_FOLDER_ID, 'name': 'folder_one', 'parent_id': None}, - {'id': str(uuid.uuid4()), 'name': 'folder_one_two', 'parent_id': PARENT_FOLDER_ID}, - {'id': CHILD_FOLDER_ID, 'name': 'folder_one_one', 'parent_id': PARENT_FOLDER_ID}, + _folder('folder_two'), + _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), ] service_one['permissions'] += ['letter', 'edit_folders'] From 5db7025e12fd07283936a99566497835872024c5 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 12 Nov 2018 11:46:39 +0000 Subject: [PATCH 2/9] Make line breaks consistent Putting each item on its own line makes the diff cleaner in subsequent commits, and makes each pytest param consistent. --- tests/app/main/views/test_template_folders.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index 74b0121f6..d01dbad1b 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -110,13 +110,21 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare 'Templates', {'template_type': 'sms'}, ['All', 'Email', 'Letter'], - ['folder_one', 'folder_two', 'sms_template_one', 'sms_template_two'], + [ + 'folder_one', + 'folder_two', + 'sms_template_one', + 'sms_template_two', + ], ), ( 'Templates / folder_one', {'template_type': 'sms', 'template_folder_id': PARENT_FOLDER_ID}, ['All', 'Email', 'Letter'], - ['folder_one_one', 'folder_one_two'], + [ + 'folder_one_one', + 'folder_one_two', + ], ), ( 'Templates / folder_one / folder_one_one', From 084c020d0cdf57b7c81866ab5e892341ced9afd8 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 12 Nov 2018 11:50:53 +0000 Subject: [PATCH 3/9] Test for template hints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit extends the tests to not only look at the name of the displayed templates and folders, but also the hint text underneath. Because there are going to be more variations of this hint text in the future we need to make sure it’s tested. Doing this in its own commit so only the meaningful changes show up in the commits that actually change stuff. --- tests/app/main/views/test_template_folders.py | 40 +++++++++---------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index d01dbad1b..eb14ceb9a 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -89,21 +89,21 @@ 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_links', + 'expected_page_title, extra_args, expected_nav_links, expected_items', [ ( 'Templates', {}, ['Text message', 'Email', 'Letter'], [ - 'folder_one', - 'folder_two', - 'sms_template_one', - 'sms_template_two', - 'email_template_one', - 'email_template_two', - 'letter_template_one', - 'letter_template_two', + 'folder_one Folder containing templates', + 'folder_two Folder containing templates', + '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', ] ), ( @@ -111,10 +111,10 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare {'template_type': 'sms'}, ['All', 'Email', 'Letter'], [ - 'folder_one', - 'folder_two', - 'sms_template_one', - 'sms_template_two', + 'folder_one Folder containing templates', + 'folder_two Folder containing templates', + 'sms_template_one Text message template', + 'sms_template_two Text message template', ], ), ( @@ -122,8 +122,8 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare {'template_type': 'sms', 'template_folder_id': PARENT_FOLDER_ID}, ['All', 'Email', 'Letter'], [ - 'folder_one_one', - 'folder_one_two', + 'folder_one_one Folder containing templates', + 'folder_one_two Folder containing templates', ], ), ( @@ -145,7 +145,7 @@ def test_should_show_templates_folder_page( expected_page_title, extra_args, expected_nav_links, - expected_links, + expected_items, ): mock_get_template_folders.return_value = [ @@ -172,12 +172,12 @@ def test_should_show_templates_folder_page( for index, expected_link in enumerate(expected_nav_links): assert links_in_page[index].text.strip() == expected_link - page_links = page.select('.message-name a') + page_items = page.select('.template-list-item') - assert len(page_links) == len(expected_links) + assert len(page_items) == len(expected_items) - for index, expected_link in enumerate(expected_links): - assert page_links[index].text.strip() == expected_link + for index, expected_item in enumerate(expected_items): + assert normalize_spaces(page_items[index].text) == expected_item mock_get_service_templates.assert_called_once_with(SERVICE_ONE_ID) From 10fe78bb31b2d7dc095f43360943306bbd4ae6f5 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 12 Nov 2018 10:17:57 +0000 Subject: [PATCH 4/9] Only show relevant folders MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A folder is relevant if it, or any of its descents contain a template of the kind you’re looking for. If you’re not filtering by template type then you should see all folders. --- app/main/views/templates.py | 2 +- app/models/service.py | 25 ++++++++++++++++--- tests/app/main/views/test_template_folders.py | 4 --- 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 297bbba21..71949b9f3 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -126,7 +126,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_folders=current_service.get_template_folders(template_folder_id), + template_folders=current_service.get_template_folders(template_folder_id, template_type), templates=current_service.get_templates(template_type, template_folder_id), show_search_box=(len(current_service.get_templates(template_type)) > 7), show_template_nav=( diff --git a/app/models/service.py b/app/models/service.py index 7780347f4..f60151b38 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -298,12 +298,31 @@ class Service(): def all_template_folder_ids(self): return {folder['id'] for folder in self.all_template_folders} - def get_template_folders(self, parent_folder_id=None): + def get_template_folders(self, parent_folder_id=None, template_type='all'): return [ folder for folder in self.all_template_folders - if folder['parent_id'] == parent_folder_id + if ( + folder['parent_id'] == parent_folder_id and + self.show_folder(folder['id'], template_type) + ) ] + def show_folder(self, template_folder_id, template_type='all'): + + if template_type == 'all': + return True + + if self.get_templates(template_type, template_folder_id): + return True + + if any( + self.show_folder(child_folder['id'], template_type) + for child_folder in self.get_template_folders(template_folder_id, template_type) + ): + return True + + return False + def get_template_folder_path(self, template_folder_id): if template_folder_id is None: return [] @@ -322,7 +341,7 @@ class Service(): def get_template_folders_and_templates(self, template_type, template_folder_id): return ( self.get_templates(template_type, template_folder_id) + - self.get_template_folders(template_folder_id) + self.get_template_folders(template_folder_id, template_type) ) def move_to_folder(self, ids_to_move, move_to): diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index eb14ceb9a..c03de789f 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -111,8 +111,6 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare {'template_type': 'sms'}, ['All', 'Email', 'Letter'], [ - 'folder_one Folder containing templates', - 'folder_two Folder containing templates', 'sms_template_one Text message template', 'sms_template_two Text message template', ], @@ -122,8 +120,6 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare {'template_type': 'sms', 'template_folder_id': PARENT_FOLDER_ID}, ['All', 'Email', 'Letter'], [ - 'folder_one_one Folder containing templates', - 'folder_one_two Folder containing templates', ], ), ( From 012b560673f6a8794d3a3315cb95c15450f7d568 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 12 Nov 2018 12:39:14 +0000 Subject: [PATCH 5/9] Make a helper function for generating templates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Having this stuff split across various points in `conftest.py` was making my brain melt. This way I feel like it’s easier to see the input/output of the test. --- tests/app/main/views/test_template_folders.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index c03de789f..f8d2e0771 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -17,6 +17,14 @@ def _folder(name, folder_id=None, parent=None): } +def _template(template_type, name): + return { + 'id': str(uuid.uuid4()), + 'name': name, + 'template_type': template_type, + } + + @pytest.mark.parametrize('parent_folder_id', [None, PARENT_FOLDER_ID]) def test_add_page_shows_option_for_folder( client_request, @@ -150,6 +158,14 @@ def test_should_show_templates_folder_page( _folder('folder_one_two', parent=PARENT_FOLDER_ID), _folder('folder_one_one', CHILD_FOLDER_ID, parent=PARENT_FOLDER_ID), ] + mock_get_service_templates.return_value = [ + _template('sms', 'sms_template_one'), + _template('sms', 'sms_template_two'), + _template('email', 'email_template_one'), + _template('email', 'email_template_two'), + _template('letter', 'letter_template_one'), + _template('letter', 'letter_template_two'), + ] service_one['permissions'] += ['letter', 'edit_folders'] From 7fc5bf41b727aff11547fbaa37be7cdc0db59786 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 12 Nov 2018 12:52:41 +0000 Subject: [PATCH 6/9] Add a grandchild folder to the test cases This will make it easier to generate all the necessary permutations we need to test. --- tests/app/main/views/test_template_folders.py | 40 ++++++++++++++----- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index f8d2e0771..03664eff1 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -7,6 +7,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' def _folder(name, folder_id=None, parent=None): @@ -17,11 +18,12 @@ def _folder(name, folder_id=None, parent=None): } -def _template(template_type, name): +def _template(template_type, name, parent=None): return { 'id': str(uuid.uuid4()), 'name': name, 'template_type': template_type, + 'folder': parent, } @@ -119,6 +121,7 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare {'template_type': 'sms'}, ['All', 'Email', 'Letter'], [ + 'folder_one Folder containing templates', 'sms_template_one Text message template', 'sms_template_two Text message template', ], @@ -128,19 +131,29 @@ def test_post_add_template_folder_page(client_request, service_one, mocker, pare {'template_type': 'sms', 'template_folder_id': PARENT_FOLDER_ID}, ['All', 'Email', 'Letter'], [ + 'folder_one_one Folder containing templates', ], ), ( 'Templates / folder_one / folder_one_one', {'template_folder_id': CHILD_FOLDER_ID}, ['Text message', 'Email', 'Letter'], - [], + [ + 'folder_one_one_one Folder containing templates', + ], + ), + ( + 'Templates / folder_one / folder_one_one / folder_one_one_one', + {'template_folder_id': GRANDCHILD_FOLDER_ID}, + ['Text message', 'Email', 'Letter'], + [ + 'sms_template_nested Text message template', + ], ), ] ) def test_should_show_templates_folder_page( client_request, - mock_get_service_templates, mock_get_template_folders, mock_has_no_jobs, service_one, @@ -157,15 +170,20 @@ def test_should_show_templates_folder_page( _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), + _folder('folder_one_one_one', GRANDCHILD_FOLDER_ID, parent=CHILD_FOLDER_ID), ] - mock_get_service_templates.return_value = [ - _template('sms', 'sms_template_one'), - _template('sms', 'sms_template_two'), - _template('email', 'email_template_one'), - _template('email', 'email_template_two'), - _template('letter', 'letter_template_one'), - _template('letter', 'letter_template_two'), - ] + mock_get_service_templates = mocker.patch( + 'app.service_api_client.get_service_templates', + return_value={'data': [ + _template('sms', 'sms_template_one'), + _template('sms', 'sms_template_two'), + _template('email', 'email_template_one'), + _template('email', 'email_template_two'), + _template('letter', 'letter_template_one'), + _template('letter', 'letter_template_two'), + _template('sms', 'sms_template_nested', parent=GRANDCHILD_FOLDER_ID) + ]} + ) service_one['permissions'] += ['letter', 'edit_folders'] From 6c4b6774aa1aa9d4d0a2e5e222ef79cc097df0a9 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 12 Nov 2018 14:43:47 +0000 Subject: [PATCH 7/9] =?UTF-8?q?Label=20folders=20to=20show=20what=E2=80=99?= =?UTF-8?q?s=20in=20them?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It feels like a solid reckon that knowing what’s in a folder before you click on it will help you navigate around. However, what do you show in a folder if you’re filtering by template type? We think that: - if you’re not filtering you should see all folders, even empty ones - if you’re filtering you should only see folders that will get you to relevant templates This matches what happens when you filter templates, we don’t ‘grey out’ the non-email templates, we hide them completely. The logic then extends to how we describe the contents of a folder, ie we won’t count its subfolders if they don’t contain templates of the type we’re looking for. This means that however you see the folder described (eg ‘3 templates, 1 folder’) will match what you see when you click into it. --- .../components/message-count-label.html | 23 ++++++++++++++ .../views/templates/_template_list.html | 8 ++++- tests/app/main/views/test_template_folders.py | 30 ++++++++++++++----- 3 files changed, 53 insertions(+), 8 deletions(-) diff --git a/app/templates/components/message-count-label.html b/app/templates/components/message-count-label.html index 14b997665..0e5cfeedc 100644 --- a/app/templates/components/message-count-label.html +++ b/app/templates/components/message-count-label.html @@ -55,3 +55,26 @@ {%- endif -%} {%- endif %} {%- endmacro %} + + +{% macro folder_contents_count(number_of_folders, number_of_templates) %} + + {% if number_of_folders == number_of_templates == 0 %} + Empty + {% endif %} + + {% if number_of_templates == 1 %} + {{ number_of_templates }} template + {%- elif number_of_templates > 1 -%} + {{ number_of_templates }} templates + {%- endif -%} + + {%- if number_of_folders and number_of_templates %}, {% endif -%} + + {%- if number_of_folders == 1 -%} + {{ number_of_folders }} folder + {%- elif number_of_folders > 1 -%} + {{ number_of_folders }} folders + {% endif %} + +{%- endmacro %} diff --git a/app/templates/views/templates/_template_list.html b/app/templates/views/templates/_template_list.html index 96ecbff53..ca4b7c57a 100644 --- a/app/templates/views/templates/_template_list.html +++ b/app/templates/views/templates/_template_list.html @@ -1,4 +1,5 @@ {% from "components/checkbox.html" import unlabelled_checkbox %} +{% from "components/message-count-label.html" import folder_contents_count %}