From 8a7525a809baf4df0a4ce179cd9c80b0f17fb746 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 25 Apr 2018 11:11:37 +0100 Subject: [PATCH] Highlight selected item in proposition navigation It is standard practice when using GOV.UK template to highlight the selected navigation item in the propositional navigation (black bar) by colouring it blue. This commit adds a new subclass of `Navigation` with the mapping needed to decide which pages belong to which item in the navigation (or none at all). --- app/__init__.py | 9 +- app/navigation.py | 225 +++++++++++++++++++++++++++++- app/templates/admin_template.html | 16 +-- tests/app/test_navigation.py | 112 ++++++++++++--- 4 files changed, 328 insertions(+), 34 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 0eb444f53..d064906f8 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -39,7 +39,7 @@ from werkzeug.local import LocalProxy from app import proxy_fix from app.config import configs from app.asset_fingerprinter import AssetFingerprinter -from app.navigation import MainNavigation +from app.navigation import HeaderNavigation, MainNavigation from app.notify_client.service_api_client import ServiceAPIClient from app.notify_client.api_key_api_client import ApiKeyApiClient from app.notify_client.invite_api_client import InviteApiClient @@ -90,7 +90,10 @@ current_service = LocalProxy(partial(_lookup_req_object, 'service')) # The current organisation attached to the request stack. current_organisation = LocalProxy(partial(_lookup_req_object, 'organisation')) -main_navigation = MainNavigation() +navigation = { + 'main_navigation': MainNavigation(), + 'header_navigation': HeaderNavigation(), +} def create_app(application): @@ -176,7 +179,7 @@ def init_app(application): @application.context_processor def _nav_selected(): - return {'main_navigation': main_navigation} + return navigation @application.before_request def record_start_time(): diff --git a/app/navigation.py b/app/navigation.py index 78bd6b102..6440d5357 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -7,6 +7,7 @@ class Navigation: mapping = {} exclude = {} + selected_attribute = "class=selected" def __init__(self): self.mapping = { @@ -30,10 +31,232 @@ class Navigation: def is_selected(self, navigation_item): if request.endpoint in self.mapping[navigation_item]: - return "class=selected" + return self.selected_attribute return '' +class HeaderNavigation(Navigation): + + selected_attribute = "class=active" + + mapping = { + 'support': { + 'bat_phone', + 'feedback', + 'old_feedback', + 'old_submit_feedback', + 'support', + 'thanks', + 'triage', + }, + 'features': { + 'features', + 'roadmap', + 'security', + 'terms', + 'using_notify', + }, + 'pricing': { + 'pricing', + }, + 'documentation': { + 'documentation', + 'integration_testing', + }, + 'user-profile': { + 'user_profile', + 'user_profile_email', + 'user_profile_email_authenticate', + 'user_profile_email_confirm', + 'user_profile_mobile_number', + 'user_profile_mobile_number_authenticate', + 'user_profile_mobile_number_confirm', + 'user_profile_name', + 'user_profile_password', + }, + 'platform-admin': { + 'add_organisation', + 'create_email_branding', + 'email_branding', + 'live_services', + 'organisations', + 'platform_admin', + 'suspend_service', + 'trial_services', + 'update_email_branding', + 'view_provider', + 'view_providers', + }, + 'sign-in': { + 'sign_in', + 'two_factor', + 'two_factor_email', + 'two_factor_email_sent', + 'verify', + 'verify_email', + 'verify_mobile', + }, + } + + exclude = { + 'accept_invite', + 'accept_org_invite', + 'action_blocked', + 'add_service', + 'add_service_template', + 'add_template_by_type', + 'agreement', + 'api_callbacks', + 'api_documentation', + 'api_integration', + 'api_keys', + 'archive_service', + 'callbacks', + 'cancel_invited_org_user', + 'cancel_invited_user', + 'cancel_job', + 'check_and_resend_text_code', + 'check_and_resend_verification_code', + 'check_messages', + 'check_messages_preview', + 'check_notification', + 'choose_account', + 'choose_service', + 'choose_template', + 'confirm_edit_organisation_name', + 'confirm_redact_template', + 'conversation', + 'conversation_reply', + 'conversation_reply_with_template', + 'conversation_updates', + 'cookies', + 'create_api_key', + 'delete_service_template', + 'delivery_and_failure', + 'delivery_status_callback', + 'design_content', + 'download_agreement', + 'download_notifications_csv', + 'edit_organisation_name', + 'edit_provider', + 'edit_service_template', + 'edit_user_org_permissions', + 'edit_user_permissions', + 'email_not_received', + 'email_template', + 'error', + 'forgot_password', + 'get_example_csv', + 'get_notifications_as_json', + 'go_to_dashboard_after_tour', + 'inbound_sms_admin', + 'inbox', + 'inbox_download', + 'inbox_updates', + 'index', + 'information_risk_management', + 'information_security', + 'invite_org_user', + 'invite_user', + 'letter_jobs', + 'link_service_to_organisation', + 'manage_org_users', + 'manage_users', + 'monthly', + 'new_password', + 'old_integration_testing', + 'old_roadmap', + 'old_service_dashboard', + 'old_terms', + 'old_using_notify', + 'organisation_dashboard', + 'organisation_settings', + 'received_text_messages_callback', + 'redact_template', + 'register', + 'register_from_invite', + 'register_from_org_invite', + 'registration_continue', + 'remove_user_from_organisation', + 'remove_user_from_service', + 'request_to_go_live', + 'resend_email_link', + 'resend_email_verification', + 'resume_service', + 'revoke_api_key', + 'send_messages', + 'send_notification', + 'send_one_off', + 'send_one_off_step', + 'send_test', + 'send_test_preview', + 'send_test_step', + 'service_add_email_reply_to', + 'service_add_letter_contact', + 'service_add_sms_sender', + 'service_dashboard', + 'service_dashboard_updates', + 'service_edit_email_reply_to', + 'service_edit_letter_contact', + 'service_edit_sms_sender', + 'service_email_reply_to', + 'service_letter_contact_details', + 'service_name_change', + 'service_name_change_confirm', + 'service_set_auth_type', + 'service_set_email', + 'service_set_email_branding', + 'service_set_inbound_number', + 'service_set_inbound_sms', + 'service_set_international_sms', + 'service_set_letter_contact_block', + 'service_set_letters', + 'service_set_reply_to_email', + 'service_set_sms', + 'service_set_sms_prefix', + 'service_settings', + 'service_sms_senders', + 'service_switch_can_send_email', + 'service_switch_can_send_precompiled_letter', + 'service_switch_can_send_sms', + 'service_switch_email_auth', + 'service_switch_live', + 'service_switch_research_mode', + 'services_or_dashboard', + 'set_free_sms_allowance', + 'set_letter_branding', + 'set_organisation_type', + 'set_sender', + 'set_template_sender', + 'show_accounts_or_dashboard', + 'sign_out', + 'start_job', + 'start_tour', + 'styleguide', + 'submit_request_to_go_live', + 'temp_service_history', + 'template_history', + 'template_usage', + 'trial_mode', + 'usage', + 'view_job', + 'view_job_csv', + 'view_job_updates', + 'view_jobs', + 'view_letter_notification_as_preview', + 'view_letter_template_preview', + 'view_notification', + 'view_notification_updates', + 'view_notifications', + 'view_notifications_csv', + 'view_template', + 'view_template_version', + 'view_template_version_preview', + 'view_template_versions', + 'whitelist', + } + + class MainNavigation(Navigation): mapping = { diff --git a/app/templates/admin_template.html b/app/templates/admin_template.html index 6595057e7..d8c1c8ca7 100644 --- a/app/templates/admin_template.html +++ b/app/templates/admin_template.html @@ -40,19 +40,19 @@ Menu diff --git a/tests/app/test_navigation.py b/tests/app/test_navigation.py index acbfcb6de..4388a9719 100644 --- a/tests/app/test_navigation.py +++ b/tests/app/test_navigation.py @@ -1,38 +1,91 @@ import pytest from tests.conftest import SERVICE_ONE_ID, app_ -from app.navigation import MainNavigation +from app.navigation import HeaderNavigation, MainNavigation all_endpoints = [ rule.endpoint for rule in next(app_(None)).url_map.iter_rules() ] -def test_navigation_items_are_properly_defined(): - for endpoint in MainNavigation().endpoints_with_navigation: - assert endpoint in all_endpoints - assert endpoint not in MainNavigation().endpoints_without_navigation - assert MainNavigation().endpoints_with_navigation.count(endpoint) == 1 - - -def test_excluded_navigation_items_are_properly_defined(): - for endpoint in MainNavigation().endpoints_without_navigation: - assert endpoint in all_endpoints - assert endpoint not in MainNavigation().endpoints_with_navigation - assert MainNavigation().endpoints_without_navigation.count(endpoint) == 1 - - -def test_all_endpoints_are_covered(): - for endpoint in all_endpoints: - assert endpoint in ( - MainNavigation().endpoints_with_navigation + - MainNavigation().endpoints_without_navigation +@pytest.mark.parametrize('navigation_instance', [ + MainNavigation(), + HeaderNavigation(), +]) +def test_navigation_items_are_properly_defined(navigation_instance): + for endpoint in navigation_instance.endpoints_with_navigation: + assert ( + endpoint in all_endpoints + ), '{} is not a real endpoint (in {}.mapping)'.format( + endpoint, + type(navigation_instance).__name__ + ) + assert ( + endpoint not in navigation_instance.endpoints_without_navigation + ), '{} is listed in {}.mapping and {}.exclude'.format( + endpoint, + type(navigation_instance).__name__, + type(navigation_instance).__name__, + ) + assert ( + navigation_instance.endpoints_with_navigation.count(endpoint) == 1 + ), '{} found more than once in {}.mapping'.format( + endpoint, + type(navigation_instance).__name__ ) +@pytest.mark.parametrize('navigation_instance', [ + MainNavigation(), + HeaderNavigation(), +]) +def test_excluded_navigation_items_are_properly_defined(navigation_instance): + for endpoint in navigation_instance.endpoints_without_navigation: + assert ( + endpoint in all_endpoints + ), '{} is not a real endpoint (in {}.exclude)'.format( + endpoint, + type(navigation_instance).__name__ + ) + assert ( + endpoint not in navigation_instance.endpoints_with_navigation + ), '{} is listed in {}.exclude and {}.mapping'.format( + endpoint, + type(navigation_instance).__name__, + type(navigation_instance).__name__, + ) + assert ( + navigation_instance.endpoints_without_navigation.count(endpoint) == 1 + ), '{} found more than once in {}.exclude'.format( + endpoint, + type(navigation_instance).__name__ + ) + + +@pytest.mark.parametrize('navigation_instance', [ + MainNavigation(), + HeaderNavigation(), +]) +def test_all_endpoints_are_covered(navigation_instance): + for endpoint in all_endpoints: + assert endpoint in ( + navigation_instance.endpoints_with_navigation + + navigation_instance.endpoints_without_navigation + ), '{} is not listed or excluded in {}'.format( + endpoint, + type(navigation_instance).__name__ + ) + + +@pytest.mark.parametrize('navigation_instance', [ + MainNavigation(), + HeaderNavigation(), +]) @pytest.mark.xfail(raises=KeyError) -def test_raises_on_invalid_navigation_item(client_request): - MainNavigation().is_selected('foo') +def test_raises_on_invalid_navigation_item( + client_request, navigation_instance +): + navigation_instance.is_selected('foo') @pytest.mark.parametrize('endpoint, selected_nav_item', [ @@ -51,3 +104,18 @@ def test_a_page_should_nave_selected_navigation_item( selected_nav_items = page.select('.navigation a.selected') assert len(selected_nav_items) == 1 assert selected_nav_items[0].text.strip() == selected_nav_item + + +@pytest.mark.parametrize('endpoint, selected_nav_item', [ + ('main.documentation', 'Documentation'), + ('main.support', 'Support'), +]) +def test_a_page_should_nave_selected_header_navigation_item( + client_request, + endpoint, + selected_nav_item, +): + page = client_request.get(endpoint, service_id=SERVICE_ONE_ID) + selected_nav_items = page.select('#proposition-links a.active') + assert len(selected_nav_items) == 1 + assert selected_nav_items[0].text.strip() == selected_nav_item