From 0888bdf5d4c3f48d62530992684c99a73292616e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 13 Nov 2018 15:06:39 +0000 Subject: [PATCH 01/27] =?UTF-8?q?Put=20=E2=80=98copy=E2=80=99=20at=20end?= =?UTF-8?q?=20of=20new=20template=20name?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In multiple user research sessions we’ve noticed people edit the auto-generated template name to put something at the end of it. This is fiddly because of the quotes we put around the name: > Copy of ‘Exiting template’ It also means that if they keep our prefix then the template doesn’t sort alongside the one it’s replacing. This commit changes the name of copied templates to better match the behaviour our users are showing. Also adds a bit of auto numbering, just as a nice detail. --- app/main/views/templates.py | 16 +++++++++++- tests/app/main/views/test_templates.py | 35 +++++++++++++++++++++++++- 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 8c91f609b..f69a830a4 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -317,7 +317,7 @@ def copy_template(service_id, template_id): return add_service_template(service_id, template['template_type']) template['template_content'] = template['content'] - template['name'] = 'Copy of ‘{}’'.format(template['name']) + template['name'] = _get_template_copy_name(template, current_service.all_templates) form = form_objects[template['template_type']](**template) return render_template( @@ -329,6 +329,20 @@ def copy_template(service_id, template_id): ) +def _get_template_copy_name(template, existing_templates): + + template_names = [existing['name'] for existing in existing_templates] + + for index in reversed(range(1, 10)): + if '{} (copy {})'.format(template['name'], index) in template_names: + return '{} (copy {})'.format(template['name'], index + 1) + + if '{} (copy)'.format(template['name']) in template_names: + return '{} (copy 2)'.format(template['name']) + + return '{} (copy)'.format(template['name']) + + @main.route("/services//templates/action-blocked///") @login_required @user_has_permissions('manage_templates') diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index 1a57db7e5..6cd8a6160 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -785,11 +785,44 @@ def test_choose_a_template_to_copy( ) +@pytest.mark.parametrize('existing_template_names, expected_name', ( + ( + ['Two week reminder'], + 'Two week reminder (copy)' + ), + ( + ['Two week reminder (copy)'], + 'Two week reminder (copy 2)' + ), + ( + ['Two week reminder', 'Two week reminder (copy)'], + 'Two week reminder (copy 2)' + ), + ( + ['Two week reminder (copy 8)', 'Two week reminder (copy 9)'], + 'Two week reminder (copy 10)' + ), + ( + ['Two week reminder (copy)', 'Two week reminder (copy 9)'], + 'Two week reminder (copy 10)' + ), + ( + ['Two week reminder (copy)', 'Two week reminder (copy 10)'], + 'Two week reminder (copy 2)' + ), +)) def test_load_edit_template_with_copy_of_template( client_request, + mock_get_service_templates, mock_get_service_email_template, mock_get_non_empty_organisations_and_services_for_user, + existing_template_names, + expected_name, ): + mock_get_service_templates.side_effect = lambda service_id: {'data': [ + {'name': existing_template_name, 'template_type': 'sms'} + for existing_template_name in existing_template_names + ]} page = client_request.get( 'main.copy_template', service_id=SERVICE_ONE_ID, @@ -800,7 +833,7 @@ def test_load_edit_template_with_copy_of_template( assert page.select_one('form')['method'] == 'post' assert page.select_one('input')['value'] == ( - 'Copy of ‘Two week reminder’' + expected_name ) assert page.select_one('textarea').text == ( 'Your ((thing)) is due soon' From 13a962b6bd23948214ede93de2768d38538bb064 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 20 Nov 2018 10:37:33 +0000 Subject: [PATCH 02/27] =?UTF-8?q?Don=E2=80=99t=20hide=20the=20rename=20for?= =?UTF-8?q?m=20on=20delete?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s weird that this page changes when you click delete – it looks like you’re going to a different page. It should feel like you’re on the same page, just with the confirmation message. The problem we had before is that the rename form would `POST` to `…/delete`, which would then delete the template instead of updating its name. We can fix this by explicitly setting the `action` attribute on the rename form to always post to `…/manage`, even if the user is currently looking at the `…/delete` page. --- app/main/views/templates.py | 5 ++-- .../templates/manage-template-folder.html | 26 ++++++++----------- tests/app/main/views/test_template_folders.py | 19 +++++++++++--- 3 files changed, 29 insertions(+), 21 deletions(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 70a854143..3dc26d475 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -406,7 +406,9 @@ def manage_template_folder(service_id, template_folder_id): def delete_template_folder(service_id, template_folder_id): if not current_service.has_permission('edit_folders'): abort(403) - form = TemplateFolderForm() + form = TemplateFolderForm( + name=current_service.get_template_folder(template_folder_id)['name'] + ) template_folder_path = current_service.get_template_folder_path(template_folder_id) template_folder_name = template_folder_path[-1]["name"] @@ -450,7 +452,6 @@ def delete_template_folder(service_id, template_folder_id): current_service_id=current_service.id, template_folder_id=template_folder_id, template_type="all", - delete_folder=True ) diff --git a/app/templates/views/templates/manage-template-folder.html b/app/templates/views/templates/manage-template-folder.html index b638e8479..2b14dee9f 100644 --- a/app/templates/views/templates/manage-template-folder.html +++ b/app/templates/views/templates/manage-template-folder.html @@ -21,20 +21,16 @@ - {% if not delete_folder %} - {% call form_wrapper() %} - {{ textbox(form.name) }} - {{ page_footer( - 'Save', - delete_link=url_for( - '.delete_template_folder', - service_id=current_service_id, - template_folder_id=template_folder_id - ), - delete_link_text="Delete this folder") }} - {% endcall %} - {% else %} - Back to manage folder page - {% endif %} + {% call form_wrapper(action=url_for('main.manage_template_folder', service_id=current_service.id, template_folder_id=template_folder_id)) %} + {{ textbox(form.name) }} + {{ page_footer( + 'Save', + delete_link=url_for( + '.delete_template_folder', + service_id=current_service_id, + template_folder_id=template_folder_id + ), + delete_link_text="Delete this folder") }} + {% endcall %} {% endblock %} diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index 506c80ef3..da3a97b60 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -367,7 +367,7 @@ def test_get_manage_folder_page( 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 + assert page.select_one('input[name=name]')['value'] == 'folder_two' delete_link = page.find('a', string="Delete this folder") expected_delete_url = "/services/{}/templates/folders/{}/delete".format(service_one['id'], folder_id) @@ -443,9 +443,20 @@ def test_delete_template_folder_should_request_confirmation( 'Yes, delete' ) - assert len(page.select('label')) == 0 - assert len(page.select('button')) == 1 - assert "Back to manage folder page" in page.text + assert page.select_one('input[name=name]')['value'] == 'sacrifice' + + assert len(page.select('form')) == 2 + assert len(page.select('button')) == 2 + + assert 'action' not in page.select('form')[0] + assert page.select('form button')[0].text == 'Yes, delete' + + assert page.select('form')[1]['action'] == url_for( + 'main.manage_template_folder', + service_id=service_one['id'], + template_folder_id=folder_id, + ) + assert page.select('form button')[1].text == 'Save' def test_delete_template_folder_should_detect_non_empty_folder_on_get( From 4bccce4772bafcae1aa385b385a022951fb3be8d Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 20 Nov 2018 10:43:56 +0000 Subject: [PATCH 03/27] Refactor how we get template folder on delete page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s a bit obtuse to look it up from the `template_folder_path`, especially now that we have a specific method on the model that does the exact thing. --- app/main/views/templates.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 3dc26d475..2f8827b57 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -404,18 +404,18 @@ def manage_template_folder(service_id, template_folder_id): @login_required @user_has_permissions('manage_templates') def delete_template_folder(service_id, template_folder_id): + if not current_service.has_permission('edit_folders'): abort(403) - form = TemplateFolderForm( - name=current_service.get_template_folder(template_folder_id)['name'] - ) - template_folder_path = current_service.get_template_folder_path(template_folder_id) - template_folder_name = template_folder_path[-1]["name"] + + template_folder = current_service.get_template_folder(template_folder_id) + + form = TemplateFolderForm(name=template_folder['name']) if len(current_service.get_template_folders_and_templates( template_type="all", template_folder_id=template_folder_id )) > 0: - flash("You must empty this folder before you can delete it".format(template_folder_name), 'info') + flash("You must empty this folder before you can delete it".format(template_folder['name']), 'info') return redirect( url_for( '.choose_template', service_id=service_id, template_type="all", template_folder_id=template_folder_id @@ -432,7 +432,7 @@ def delete_template_folder(service_id, template_folder_id): except HTTPError as e: msg = "Folder is not empty" if e.status_code == 400 and msg in e.message: - flash("You must empty this folder before you can delete it".format(template_folder_name), 'info') + flash("You must empty this folder before you can delete it", 'info') return redirect( url_for( '.choose_template', @@ -444,11 +444,11 @@ def delete_template_folder(service_id, template_folder_id): else: abort(500, e) - flash("Are you sure you want to delete the ‘{}’ folder?".format(template_folder_name), 'delete') + flash("Are you sure you want to delete the ‘{}’ folder?".format(template_folder['name']), 'delete') return render_template( 'views/templates/manage-template-folder.html', form=form, - template_folder_path=template_folder_path, + template_folder_path=current_service.get_template_folder_path(template_folder_id), current_service_id=current_service.id, template_folder_id=template_folder_id, template_type="all", From 77529f419122f7a89ddab378d0320191d1b1b731 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 20 Nov 2018 10:44:09 +0000 Subject: [PATCH 04/27] Redirect to parent after deleting a folder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit You lose your place if, after deleting something that’s deeply nested, you return to the root level. --- app/main/views/templates.py | 2 +- tests/app/main/views/test_template_folders.py | 17 ++++++++++++----- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 2f8827b57..2ae0ba6ae 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -427,7 +427,7 @@ def delete_template_folder(service_id, template_folder_id): template_folder_api_client.delete_template_folder(current_service.id, template_folder_id) return redirect( - url_for('.choose_template', service_id=service_id) + url_for('.choose_template', service_id=service_id, template_folder_id=template_folder['parent_id']) ) except HTTPError as e: msg = "Folder is not empty" diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index da3a97b60..e0cd223c4 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -487,11 +487,15 @@ def test_delete_template_folder_should_detect_non_empty_folder_on_get( ) -def test_delete_folder(client_request, service_one, mock_get_template_folders, mocker): +@pytest.mark.parametrize('parent_folder_id', ( + None, + PARENT_FOLDER_ID, +)) +def test_delete_folder(client_request, service_one, mock_get_template_folders, mocker, parent_folder_id): mock_delete = mocker.patch('app.template_folder_api_client.delete_template_folder') folder_id = str(uuid.uuid4()) mock_get_template_folders.side_effect = [[ - {'id': folder_id, 'name': 'sacrifice', 'parent_id': None}, + {'id': folder_id, 'name': 'sacrifice', 'parent_id': parent_folder_id}, ], []] mocker.patch( 'app.models.service.Service.get_templates', @@ -503,9 +507,12 @@ def test_delete_folder(client_request, service_one, mock_get_template_folders, m 'main.delete_template_folder', service_id=service_one['id'], template_folder_id=folder_id, - _expected_redirect=url_for("main.choose_template", - service_id=service_one['id'], - _external=True) + _expected_redirect=url_for( + "main.choose_template", + service_id=service_one['id'], + template_folder_id=parent_folder_id, + _external=True, + ) ) mock_delete.assert_called_once_with(service_one['id'], folder_id) From d3fded694e9250cf8a36024fdc247f811fc3cb70 Mon Sep 17 00:00:00 2001 From: fidejoseph <40238656+fidejoseph@users.noreply.github.com> Date: Tue, 20 Nov 2018 13:22:22 +0000 Subject: [PATCH 05/27] Update email_domains.yml Whitelisting UK SBS --- app/email_domains.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/app/email_domains.yml b/app/email_domains.yml index 115926e42..dcdcbf782 100644 --- a/app/email_domains.yml +++ b/app/email_domains.yml @@ -40,3 +40,4 @@ - hscni.net - bi.team - networkrail.co.uk +- uksbs.co.uk From 7b3c04d0c2c6ebe8b5caef10e8980ac0775887f4 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 20 Nov 2018 14:13:02 +0000 Subject: [PATCH 06/27] Mark agreement signed by Northumberland and Lincolnshire --- app/domains.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/domains.yml b/app/domains.yml index 839e14aa4..71fafe603 100644 --- a/app/domains.yml +++ b/app/domains.yml @@ -2629,7 +2629,7 @@ lincoln.gov.uk: lincolnshire.gov.uk: owner: Lincolnshire County Council crown: false - agreement_signed: false + agreement_signed: true lincsbc.gov.uk: owner: East Lindsey District Council crown: false @@ -3246,7 +3246,7 @@ northtyneside.gov.uk: northumberland.gov.uk: owner: Northumberland County Council crown: false - agreement_signed: false + agreement_signed: true northwalesadoption.gov.uk: owner: Wrexham County Borough Council crown: false From 5d13c639b1559bd8c755176380e1f84484d558c9 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 20 Nov 2018 16:10:14 +0000 Subject: [PATCH 07/27] Fix logic around showing search box It was looking at the count of items at the root level (because it was passing `parent_folder_id=None` as an argument). This changes it to look at the total count of items for a service (which was the intended behaviour). --- app/models/service.py | 2 +- tests/app/main/views/test_templates.py | 28 ++++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/app/models/service.py b/app/models/service.py index 66d279430..8a04d79e5 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -369,7 +369,7 @@ class Service(): @property def count_of_templates_and_folders(self): - return len(self.get_template_folders_and_templates('all', None)) + return len(self.all_templates + self.all_template_folders) def move_to_folder(self, ids_to_move, move_to): diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index bfa8c511a..7fec6a7e7 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -20,6 +20,7 @@ from tests import ( from tests.app.main.views.test_template_folders import ( CHILD_FOLDER_ID, PARENT_FOLDER_ID, + _folder, ) from tests.conftest import ( SERVICE_ONE_ID, @@ -446,6 +447,33 @@ def test_should_show_live_search_if_list_of_templates_taller_than_screen( assert len(page.select(search['data-targets'])) == len(page.select('.message-name')) == 14 +def test_should_show_live_search_if_service_has_lots_of_folders( + client_request, + mock_get_template_folders, + mock_get_service_templates, # returns 4 templates +): + + mock_get_template_folders.return_value = [ + _folder('one', PARENT_FOLDER_ID), + _folder('two', None, parent=PARENT_FOLDER_ID), + _folder('three', None, parent=PARENT_FOLDER_ID), + _folder('four', None, parent=PARENT_FOLDER_ID), + ] + + page = client_request.get( + 'main.choose_template', + service_id=SERVICE_ONE_ID, + ) + + count_of_templates_and_folders = len(page.select('.message-name')) + count_of_folders = len(page.select('.template-list-folder')) + count_of_templates = count_of_templates_and_folders - count_of_folders + + assert len(page.select('.live-search')) == 1 + assert count_of_folders == 1 + assert count_of_templates == 4 + + def test_should_show_page_for_one_template( logged_in_client, mock_get_service_template, From 2f0abb9c7d0e8f85fe716ca6c92e483bcbf75ee1 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Tue, 20 Nov 2018 16:39:23 +0000 Subject: [PATCH 08/27] Rename staging CSV uploads bucket to match other environments --- app/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/config.py b/app/config.py index f446bd7ed..0325b4ce3 100644 --- a/app/config.py +++ b/app/config.py @@ -124,7 +124,7 @@ class Staging(Config): HTTP_PROTOCOL = 'https' HEADER_COLOUR = '#6F72AF' # $mauve STATSD_ENABLED = True - CSV_UPLOAD_BUCKET_NAME = 'staging-notify-csv-upload' + CSV_UPLOAD_BUCKET_NAME = 'staging-notifications-csv-upload' LOGO_UPLOAD_BUCKET_NAME = 'public-logos-staging' MOU_BUCKET_NAME = 'staging-notify.works-mou' NOTIFY_ENVIRONMENT = 'staging' From d55117e4d67c72199bc55efe3eb7d836e3b62e9e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 21 Nov 2018 12:33:28 +0000 Subject: [PATCH 09/27] Mark agreement signed by Inverclyde Council --- app/domains.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/domains.yml b/app/domains.yml index 71fafe603..914d930dd 100644 --- a/app/domains.yml +++ b/app/domains.yml @@ -2374,7 +2374,7 @@ internalauditscotland.gov.uk: inverclyde.gov.uk: owner: Inverclyde Council crown: false - agreement_signed: false + agreement_signed: true iow.gov.uk: owner: Isle of Wight Council crown: false From c7118a80e2aba3822c78b00a0d452ea89ab19336 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 20 Nov 2018 13:57:48 +0000 Subject: [PATCH 10/27] Stick email status to bottom of screen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We’ve moved away from using the expand/collapse pattern on the page where you click ‘send’. Instead we’re putting the send button in the sticky footer. So it’s a bit jarring to still have the expand/collapse on the page you see after you’ve sent an email. This commit replaces it with the sticky footer as well. This is only relevant for emails because: 1. Text messages are generally short enough to fit on the screen 2. We don’t show the status of letters because they don’t really change --- .../stylesheets/components/stick-at-top-when-scrolling.scss | 4 ++++ app/main/views/notifications.py | 1 + app/templates/views/notifications/notification.html | 6 ++++++ 3 files changed, 11 insertions(+) diff --git a/app/assets/stylesheets/components/stick-at-top-when-scrolling.scss b/app/assets/stylesheets/components/stick-at-top-when-scrolling.scss index ec75473d3..283c1aeec 100644 --- a/app/assets/stylesheets/components/stick-at-top-when-scrolling.scss +++ b/app/assets/stylesheets/components/stick-at-top-when-scrolling.scss @@ -50,6 +50,10 @@ margin-bottom: 0; } + .notification-status { + margin: 0; + } + } .content-fixed, diff --git a/app/main/views/notifications.py b/app/main/views/notifications.py index 03187cb24..d9ff30463 100644 --- a/app/main/views/notifications.py +++ b/app/main/views/notifications.py @@ -65,6 +65,7 @@ def view_notification(service_id, notification_id): notification_id=notification_id, filetype='png', ), + expand_emails=True, page_count=page_count, show_recipient=True, redact_missing_personalisation=True, diff --git a/app/templates/views/notifications/notification.html b/app/templates/views/notifications/notification.html index 15fbc194a..7fc689cac 100644 --- a/app/templates/views/notifications/notification.html +++ b/app/templates/views/notifications/notification.html @@ -60,7 +60,13 @@ {{ template|string }} {% if template.template_type != 'letter' %} + + {% if template.template_type == 'email' %}
{% endif %} + {{ ajax_block(partials, updates_url, 'status', finished=finished) }} + + {% if template.template_type == 'email' %}
{% endif %} + {% endif %} {% if current_user.has_permissions('send_messages') and current_user.has_permissions('view_activity') and template.template_type == 'sms' and can_receive_inbound %} From d98d04844a3685e0f7741091db58d10e76ee7a6c Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 22 Nov 2018 10:29:48 +0000 Subject: [PATCH 11/27] Mark agreement signed by Tower Hamlets council --- app/domains.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/domains.yml b/app/domains.yml index 914d930dd..9dd09b9b6 100644 --- a/app/domains.yml +++ b/app/domains.yml @@ -4659,7 +4659,7 @@ towcester-tc.gov.uk: towerhamlets.gov.uk: owner: Tower Hamlets London Borough Council crown: false - agreement_signed: false + agreement_signed: true towynkinmelbay-tc.gov.uk: owner: Towyn and Kinmel Bay Town Council crown: false From fb84dee4d9b8fd6f1126fa95666734aa53f4c2d0 Mon Sep 17 00:00:00 2001 From: Pete Herlihy Date: Thu, 22 Nov 2018 10:39:12 +0000 Subject: [PATCH 12/27] Adding wmfs.net (West Midlands Fire Service) to the signed MOU list. --- app/domains.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/domains.yml b/app/domains.yml index 9dd09b9b6..c2fd6610a 100644 --- a/app/domains.yml +++ b/app/domains.yml @@ -5070,6 +5070,10 @@ wixford-pc.gov.uk: owner: Stratford-on-Avon District Council crown: false agreement_signed: false +wmfs.net: + owner: West Midlands Fire Service + crown: false + agreement_signed: true woking.gov.uk: owner: Woking Borough Council crown: false From 8b84bf27e3ec47593a56a5bb2d997d25b0b9e8be Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 22 Nov 2018 10:41:47 +0000 Subject: [PATCH 13/27] =?UTF-8?q?Don=E2=80=99t=20wrap=20users=20onto=20mul?= =?UTF-8?q?tiple=20lines?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It looks cleaner to truncate instead. You can always see the full email address by clicking into ‘edit’. --- app/assets/stylesheets/views/users.scss | 13 +++++++++++++ app/templates/views/manage-users.html | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/app/assets/stylesheets/views/users.scss b/app/assets/stylesheets/views/users.scss index 4854de20f..efbf79c31 100644 --- a/app/assets/stylesheets/views/users.scss +++ b/app/assets/stylesheets/views/users.scss @@ -11,6 +11,19 @@ $item-top-padding: $gutter-half; border-top: 1px solid $border-colour; position: relative; + h3 { + + white-space: nowrap; + overflow: hidden; + text-overflow: ellipsis; + color: $secondary-text-colour; // So the ellipsis is grey + + .heading-small { + color: $black; + } + + } + &:last-child { border-bottom: 1px solid $border-colour; } diff --git a/app/templates/views/manage-users.html b/app/templates/views/manage-users.html index 577abcc64..5de9f9e95 100644 --- a/app/templates/views/manage-users.html +++ b/app/templates/views/manage-users.html @@ -33,7 +33,7 @@
{% for user in users %}
-

+

{%- if user.name -%} {{ user.name }}  {%- endif -%} From b317bd7a0be688ebac18d3680d5e2c906229b737 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 22 Nov 2018 12:16:32 +0000 Subject: [PATCH 14/27] Put download PDF in footer This makes its positioning consistent with the previous page in the one-off sending journey. It gives us more space to put information about the status of the letter above the preview of the letter. --- .../stick-at-top-when-scrolling.scss | 1 + .../views/notifications/notification.html | 22 ++++++++++--------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/app/assets/stylesheets/components/stick-at-top-when-scrolling.scss b/app/assets/stylesheets/components/stick-at-top-when-scrolling.scss index 283c1aeec..34bb42526 100644 --- a/app/assets/stylesheets/components/stick-at-top-when-scrolling.scss +++ b/app/assets/stylesheets/components/stick-at-top-when-scrolling.scss @@ -48,6 +48,7 @@ .page-footer { margin-bottom: 0; + min-height: 50px; } .notification-status { diff --git a/app/templates/views/notifications/notification.html b/app/templates/views/notifications/notification.html index 7fc689cac..d0e8c9613 100644 --- a/app/templates/views/notifications/notification.html +++ b/app/templates/views/notifications/notification.html @@ -51,22 +51,24 @@

Estimated delivery date: {{ estimated_letter_delivery_date|string|format_date_short }}

-

- Download as a PDF -

{% endif %} {% endif %} {{ template|string }} - {% if template.template_type != 'letter' %} - - {% if template.template_type == 'email' %}
{% endif %} - + {% if template.template_type == 'letter' %} +
+
+
 
+ Download as a PDF +
+
+ {% elif template.template_type == 'email' %} +
+ {{ ajax_block(partials, updates_url, 'status', finished=finished) }} +
+ {% elif template.template_type == 'sms' %} {{ ajax_block(partials, updates_url, 'status', finished=finished) }} - - {% if template.template_type == 'email' %}
{% endif %} - {% endif %} {% if current_user.has_permissions('send_messages') and current_user.has_permissions('view_activity') and template.template_type == 'sms' and can_receive_inbound %} From 741a8856fa1e2f74411555f97c5657a268d7550c Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 22 Nov 2018 12:17:11 +0000 Subject: [PATCH 15/27] =?UTF-8?q?Remove=20the=20word=20=E2=80=98printable?= =?UTF-8?q?=E2=80=99?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Don’t think it’s necessary. Makes things consistent with the sent letter page, which only says ‘Download as a PDF’. This inconsistency would be more glaring now these pieces of text appear in the same place, in adjacent steps of a journey. --- app/templates/views/check/ok.html | 2 +- app/templates/views/notifications/check.html | 2 +- tests/app/main/views/test_send.py | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/templates/views/check/ok.html b/app/templates/views/check/ok.html index b35e6a040..52804960e 100644 --- a/app/templates/views/check/ok.html +++ b/app/templates/views/check/ok.html @@ -39,7 +39,7 @@ {% if template.template_type != 'letter' or not request.args.from_test %} {% else %} - Download as a printable PDF + Download as a PDF {% endif %} Back diff --git a/app/templates/views/notifications/check.html b/app/templates/views/notifications/check.html index 8ab9434cc..21854b4b7 100644 --- a/app/templates/views/notifications/check.html +++ b/app/templates/views/notifications/check.html @@ -65,7 +65,7 @@ {% endif %} Back {% if template.template_type == 'letter' %} - Download as a printable PDF + Download as a PDF {% endif %}

diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 3325501c4..b8e9073f7 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -2803,7 +2803,7 @@ def test_one_off_letters_have_download_link( template_id=fake_uuid, filetype='pdf', ) - assert page.select_one('a[download]').text == 'Download as a printable PDF' + assert page.select_one('a[download]').text == 'Download as a PDF' def test_send_one_off_letter_errors_in_trial_mode( @@ -2848,7 +2848,7 @@ def test_send_one_off_letter_errors_in_trial_mode( assert not page.select('[type=submit]') assert page.select_one('.page-footer-back-link').text == 'Back' - assert page.select_one('a[download]').text == 'Download as a printable PDF' + assert page.select_one('a[download]').text == 'Download as a PDF' def test_check_messages_shows_over_max_row_error( From d8a0a192c3648b7f1b5ce09ac8640f470ec6c247 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 22 Nov 2018 17:24:00 +0000 Subject: [PATCH 16/27] Mark agreement signed by Plymouth City Council --- app/domains.yml | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/app/domains.yml b/app/domains.yml index c2fd6610a..a3ce9d7d5 100644 --- a/app/domains.yml +++ b/app/domains.yml @@ -3494,11 +3494,8 @@ pkc.gov.uk: plymouth.gov.uk: owner: Plymouth City Council crown: false - agreement_signed: false -plymouthmuseum.gov.uk: - owner: Plymouth City Council - crown: false - agreement_signed: false + agreement_signed: true +plymouthmuseum.gov.uk: plymouth.gov.uk pocklington.gov.uk: owner: Pocklington Town Council crown: false From 82d82076122025eab727e7fe6af1e0e29fbdf5cd Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 21 Nov 2018 09:53:29 +0000 Subject: [PATCH 17/27] Refactor template list logic into one place MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Jinja template for the ‘choose templates’ page is now pulling in data from a lot of diparate places in order to work out what to show. As we add more logic about what to show (in order to make the live search work) it’s going to get harder to have all this logic in the Jinja template. This commit refactors it back into Python where we have more language features for managing complex logic. It’s a bit weird to call this file a model, in that it’s dealing with some presentational logic, rather than just data. Conceptually it’s more like a view model[1]. 1. https://en.wikipedia.org/wiki/Model%E2%80%93view%E2%80%93viewmodel --- app/main/views/templates.py | 5 +- app/models/template_list.py | 120 ++++++++++++++++++ .../components/message-count-label.html | 23 ---- app/templates/views/templates/_move_to.html | 2 +- .../views/templates/_template_list.html | 50 +++----- app/templates/views/templates/choose.html | 24 +--- tests/app/main/views/test_template_folders.py | 15 ++- tests/app/main/views/test_templates.py | 2 +- 8 files changed, 159 insertions(+), 82 deletions(-) create mode 100644 app/models/template_list.py diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 6fc6e6dc9..fc49bbb08 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -29,6 +29,7 @@ from app.main.forms import ( ) from app.main.views.send import get_example_csv_rows, get_sender_details from app.models.service import Service +from app.models.template_list import TemplateList from app.template_previews import TemplatePreview, get_page_count_for_letter from app.utils import ( email_or_sms_not_enabled, @@ -126,9 +127,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=current_service.get_templates(template_type, template_folder_id), + template_list=TemplateList(current_service, template_type, template_folder_id), show_search_box=current_service.count_of_templates_and_folders > 7, show_template_nav=( current_service.has_multiple_template_types diff --git a/app/models/template_list.py b/app/models/template_list.py new file mode 100644 index 000000000..0b86ff1d3 --- /dev/null +++ b/app/models/template_list.py @@ -0,0 +1,120 @@ +class TemplateList(): + + def __init__( + self, + service, + template_type='all', + template_folder_id=None, + ): + self.service = service + self.template_type = template_type + self.template_folder_id = template_folder_id + + def __iter__(self): + for item in self.get_templates_and_folders( + self.template_type, self.template_folder_id, ancestors=[] + ): + yield item + + def get_templates_and_folders(self, template_type, template_folder_id, ancestors): + + for item in self.service.get_template_folders( + template_type, template_folder_id + ): + yield TemplateListFolder( + item, + folders=self.service.get_template_folders( + self.template_type, item['id'] + ), + templates=self.service.get_templates( + self.template_type, item['id'] + ), + ancestors=ancestors, + ) + for sub_item in self.get_templates_and_folders( + template_type, item['id'], ancestors + [item] + ): + yield sub_item + + for item in self.service.get_templates( + self.template_type, template_folder_id + ): + yield TemplateListTemplate( + item, + ancestors=ancestors, + ) + + @property + def templates_to_show(self): + return any(self) + + @property + def folder_is_empty(self): + return not any(self.get_templates_and_folders( + 'all', self.template_folder_id, [] + )) + + +class TemplateListItem(): + + def __init__( + self, + template_or_folder, + ancestors, + ): + self.id = template_or_folder['id'] + self.name = template_or_folder['name'] + self.ancestors = ancestors + + +class TemplateListTemplate(TemplateListItem): + + is_folder = False + + def __init__( + self, + template, + ancestors, + ): + super().__init__(template, ancestors) + self.hint = { + 'email': 'Email template', + 'sms': 'Text message template', + 'letter': 'Letter template', + }.get(template['template_type']) + + +class TemplateListFolder(TemplateListItem): + + is_folder = True + + def __init__( + self, + folder, + templates, + folders, + ancestors, + ): + super().__init__(folder, ancestors) + self.number_of_templates = len(templates) + self.number_of_folders = len(folders) + + @property + def _hint_parts(self): + + if self.number_of_folders == self.number_of_templates == 0: + yield 'Empty' + + if self.number_of_templates == 1: + yield '1 template' + elif self.number_of_templates > 1: + yield '{} templates'.format(self.number_of_templates) + + if self.number_of_folders == 1: + yield '1 folder' + elif self.number_of_folders > 1: + yield '{} folders'.format(self.number_of_folders) + + @property + def hint(self): + return ', '.join(self._hint_parts) diff --git a/app/templates/components/message-count-label.html b/app/templates/components/message-count-label.html index 0e5cfeedc..14b997665 100644 --- a/app/templates/components/message-count-label.html +++ b/app/templates/components/message-count-label.html @@ -55,26 +55,3 @@ {%- 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/_move_to.html b/app/templates/views/templates/_move_to.html index 4b9564c3d..10ab965a2 100644 --- a/app/templates/views/templates/_move_to.html +++ b/app/templates/views/templates/_move_to.html @@ -1,7 +1,7 @@ {% from "components/radios.html" import radios %} {% from "components/page-footer.html" import page_footer %} -{% if templates_and_folders_form.move_to.choices and (templates or template_folders) %} +{% if templates_and_folders_form.move_to.choices and template_list.templates_to_show %} {{ radios(templates_and_folders_form.move_to) }} {{ page_footer('Move selected', button_name='operation', button_value='move') }} {% endif %} diff --git a/app/templates/views/templates/_template_list.html b/app/templates/views/templates/_template_list.html index 3c1cb7555..53439904a 100644 --- a/app/templates/views/templates/_template_list.html +++ b/app/templates/views/templates/_template_list.html @@ -1,52 +1,38 @@ {% from "components/checkbox.html" import unlabelled_checkbox %} {% 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 %} +{% if not template_list.templates_to_show %}

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

{% else %}