From d0d6421a9187d58d86f27a1df83d5f004a30e428 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 30 Apr 2019 16:07:05 +0100 Subject: [PATCH 1/2] Fix heading size on empty templates page This is a special case which I missed when doing the rationalisation in https://github.com/alphagov/notifications-admin/pull/2937 --- app/templates/views/templates/choose.html | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/app/templates/views/templates/choose.html b/app/templates/views/templates/choose.html index 465cd2fdd..f2834970d 100644 --- a/app/templates/views/templates/choose.html +++ b/app/templates/views/templates/choose.html @@ -4,6 +4,7 @@ {% from "components/textbox.html" import textbox %} {% from "components/live-search.html" import live_search %} {% from "components/form.html" import form_wrapper %} +{% from "components/page-footer.html" import page_header %} {% from "components/page-footer.html" import page_footer %} {% extends "withnav_template.html" %} @@ -11,18 +12,14 @@ {% set page_title = 'Templates' %} {% block service_page_title %} - {{ page_title_folder_path( - template_folder_path, - fallback_page_title=page_title, - show_fallback_page_title=not current_service.all_template_folders - ) }} + {{ page_title_folder_path(template_folder_path) }} {% endblock %} {% block maincolumn_content %} {% if (not current_service.all_templates) and (not current_service.all_template_folders) %} -

+

{{ page_title }}

{% if current_user.has_permissions('manage_templates') %} From 69b1ecb57cad5feb34d916ceecd1bcbd50126056 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 30 Apr 2019 16:32:20 +0100 Subject: [PATCH 2/2] =?UTF-8?q?Add=20folder-style=20navigation=20even=20if?= =?UTF-8?q?=20you=20don=E2=80=99t=20have=20folders?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/templates/components/folder-path.html | 64 +++++++------------ app/templates/views/templates/choose.html | 5 +- app/templates/views/templates/template.html | 8 +-- .../views/accounts/test_choose_accounts.py | 1 + tests/app/main/views/test_accept_invite.py | 2 +- tests/app/main/views/test_templates.py | 48 +++++++++----- 6 files changed, 63 insertions(+), 65 deletions(-) diff --git a/app/templates/components/folder-path.html b/app/templates/components/folder-path.html index 33f6cb52d..9c6e86065 100644 --- a/app/templates/components/folder-path.html +++ b/app/templates/components/folder-path.html @@ -3,38 +3,30 @@ service_id, template_type, current_user, - fallback_page_title=None, - show_fallback_page_title=False, link_current_item=False ) %} - {% if show_fallback_page_title %} -

- {{ fallback_page_title }} -

- {% else %} -

- {% for folder in folders %} - {% if loop.last and not link_current_item %} - {% if folder.template_type or not folder.id %} - {{ folder.name }} +

+ {% for folder in folders %} + {% if loop.last and not link_current_item %} + {% if folder.template_type or not folder.id %} + {{ folder.name }} + {% else %} + {{ folder.name }} + {% endif %} + {% else %} + {% if folder.id %} + {% if current_user.has_template_folder_permission(folder) %} + {{ folder.name }} {% else %} {{ folder.name }} {% endif %} {% else %} - {% if folder.id %} - {% if current_user.has_template_folder_permission(folder) %} - {{ folder.name }} - {% else %} - {{ folder.name }} - {% endif %} - {% else %} - Templates - {% endif %} - {% if not loop.last %}{{ folder_path_separator() }}{% endif %} + Templates {% endif %} - {% endfor %} -

- {% endif %} + {% if not loop.last %}{{ folder_path_separator() }}{% endif %} + {% endif %} + {% endfor %} + {% endmacro %} @@ -75,21 +67,13 @@ {% 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 %} +{% macro page_title_folder_path(folders) %} + {% for folder in folders|reverse %} + {{ folder.name }} + {% if not loop.last %} + – + {% endif %} + {% endfor %} {% endmacro %} diff --git a/app/templates/views/templates/choose.html b/app/templates/views/templates/choose.html index f2834970d..93d3bcd6f 100644 --- a/app/templates/views/templates/choose.html +++ b/app/templates/views/templates/choose.html @@ -4,7 +4,6 @@ {% from "components/textbox.html" import textbox %} {% from "components/live-search.html" import live_search %} {% from "components/form.html" import form_wrapper %} -{% from "components/page-footer.html" import page_header %} {% from "components/page-footer.html" import page_footer %} {% extends "withnav_template.html" %} @@ -50,9 +49,7 @@ folders=template_folder_path, service_id=current_service.id, template_type=template_type, - current_user=current_user, - fallback_page_title=page_title, - show_fallback_page_title=not current_service.all_template_folders + current_user=current_user ) }} {% if current_user.has_permissions('manage_templates') and current_template_folder_id and user_has_template_folder_permission %} diff --git a/app/templates/views/templates/template.html b/app/templates/views/templates/template.html index 82aa1486f..84832289a 100644 --- a/app/templates/views/templates/template.html +++ b/app/templates/views/templates/template.html @@ -1,12 +1,12 @@ {% extends "withnav_template.html" %} {% from "components/banner.html" import banner_wrapper %} -{% from "components/folder-path.html" import folder_path %} +{% from "components/folder-path.html" import folder_path, page_title_folder_path %} {% from "components/page-footer.html" import page_footer %} {% from "components/textbox.html" import textbox %} {% from "components/api-key.html" import api_key %} {% block service_page_title %} - {{ template.name }} + {{ page_title_folder_path(current_service.get_template_path(template._template)) }} {% endblock %} {% block maincolumn_content %} @@ -35,9 +35,7 @@ folders=current_service.get_template_path(template._template), service_id=current_service.id, template_type='all', - current_user=current_user, - fallback_page_title=template.name, - show_fallback_page_title=not current_service.all_template_folders + current_user=current_user ) }} diff --git a/tests/app/main/views/accounts/test_choose_accounts.py b/tests/app/main/views/accounts/test_choose_accounts.py index f9d928232..6d192b40b 100644 --- a/tests/app/main/views/accounts/test_choose_accounts.py +++ b/tests/app/main/views/accounts/test_choose_accounts.py @@ -176,6 +176,7 @@ def test_should_not_show_back_to_service_if_user_doesnt_belong_to_service( service_id=mock_get_service.return_value['id'], template_id=fake_uuid, _expected_status=expected_status, + _test_page_title=False, ) assert normalize_spaces( diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index 6f654f57e..50d64020c 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -163,7 +163,7 @@ def test_accepting_invite_removes_invite_from_session( token='thisisnotarealtoken', _follow_redirects=True, ) - assert normalize_spaces(page.h1.string) == landing_page_title + assert normalize_spaces(page.select_one('h1').text) == landing_page_title with client_request.session_transaction() as session: assert 'invited_user' not in session diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index 1cca93444..da0464a22 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -3,7 +3,6 @@ from functools import partial from unittest.mock import ANY, Mock import pytest -from bs4 import BeautifulSoup from flask import url_for from freezegun import freeze_time from notifications_python_client.errors import HTTPError @@ -426,8 +425,14 @@ def test_user_with_only_send_and_view_sees_letter_page( 'main.view_template', service_id=SERVICE_ONE_ID, template_id=fake_uuid, + _test_page_title=False, + ) + assert normalize_spaces(page.select_one('h1').text) == ( + 'Templates / Two week reminder' + ) + assert normalize_spaces(page.select_one('title').text) == ( + 'Two week reminder – Templates – service one – GOV.UK Notify' ) - assert page.select_one('h1').text.strip() == 'Two week reminder' @pytest.mark.parametrize('letter_branding, expected_link, expected_link_text', ( @@ -462,6 +467,7 @@ def test_letter_with_default_branding_has_add_logo_button( 'main.view_template', service_id=SERVICE_ONE_ID, template_id=TEMPLATE_ONE_ID, + _test_page_title=False, ) first_edit_link = page.select_one('.template-container a') @@ -493,6 +499,7 @@ def test_view_letter_template_displays_postage( 'main.view_template', service_id=SERVICE_ONE_ID, template_id=fake_uuid, + _test_page_title=False, ) assert normalize_spaces(page.select_one('.letter-postage').text) == expected_result @@ -508,6 +515,7 @@ def test_view_non_letter_template_does_not_display_postage( '.view_template', service_id=SERVICE_ONE_ID, template_id=fake_uuid, + _test_page_title=False, ) assert "Postage" not in page.text @@ -595,29 +603,33 @@ def test_edit_letter_templates_postage_updates_postage( ), ]) def test_should_be_able_to_view_a_template_with_links( - client, + client_request, mock_get_service_template, mock_get_template_folders, active_user_with_permissions, single_letter_contact_block, - mocker, - service_one, fake_uuid, permissions, links_to_be_shown, permissions_warning_to_be_shown, ): - active_user_with_permissions._permissions[service_one['id']] = permissions + ['view_activity'] - client.login(active_user_with_permissions, mocker, service_one) + active_user_with_permissions._permissions[SERVICE_ONE_ID] = permissions + ['view_activity'] + client_request.login(active_user_with_permissions) - response = client.get(url_for( + page = client_request.get( '.view_template', - service_id=service_one['id'], - template_id=fake_uuid - )) + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + _test_page_title=False, + ) + + assert normalize_spaces(page.select_one('h1').text) == ( + 'Templates / Two week reminder' + ) + assert normalize_spaces(page.select_one('title').text) == ( + 'Two week reminder – Templates – service one – GOV.UK Notify' + ) - assert response.status_code == 200 - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') links_in_page = page.select('.pill-separate-item') assert len(links_in_page) == len(links_to_be_shown) @@ -625,7 +637,7 @@ def test_should_be_able_to_view_a_template_with_links( for index, link_to_be_shown in enumerate(links_to_be_shown): assert links_in_page[index]['href'] == url_for( link_to_be_shown, - service_id=service_one['id'], + service_id=SERVICE_ONE_ID, template_id=fake_uuid, ) @@ -644,6 +656,7 @@ def test_should_show_template_id_on_template_page( '.view_template', service_id=SERVICE_ONE_ID, template_id=fake_uuid, + _test_page_title=False, ) assert page.select('.api-key-key')[0].text == fake_uuid @@ -668,6 +681,7 @@ def test_should_show_sms_template_with_downgraded_unicode_characters( '.view_template', service_id=SERVICE_ONE_ID, template_id=fake_uuid, + _test_page_title=False, ) assert rendered_msg in page.text @@ -688,7 +702,8 @@ def test_should_let_letter_contact_block_be_changed_for_the_template( page = client_request.get( 'main.view_template', service_id=SERVICE_ONE_ID, - template_id=fake_uuid + template_id=fake_uuid, + _test_page_title=False, ) assert page.find('a', {'class': 'edit-template-link-letter-contact'})['href'] == url_for( @@ -1814,6 +1829,7 @@ def test_should_show_page_for_a_deleted_template( '.view_template', service_id=SERVICE_ONE_ID, template_id=template_id, + _test_page_title=False, ) content = str(page) @@ -2138,6 +2154,7 @@ def test_should_show_hint_once_template_redacted( 'main.view_template', service_id=SERVICE_ONE_ID, template_id=fake_uuid, + _test_page_title=False, ) assert page.select('.hint')[0].text == 'Personalisation is hidden after sending' @@ -2158,6 +2175,7 @@ def test_should_not_show_redaction_stuff_for_letters( 'main.view_template', service_id=SERVICE_ONE_ID, template_id=fake_uuid, + _test_page_title=False, ) assert page.select('.hint') == []