Highlight selected navigation item

In research I’ve sometimes seen people click the wrong nav item. I
reckon that people’s concept of which pages live behind which navigation
items isn’t very strong.

We can reinforce this relationship by showing, for every page, which is
the corresponding nav item. The conventional way of doing this is either
with some kind of emphasis, typically colour or bold. I’ve gone for bold
because colour would be weird.

---

The implementation of this is quite loosely coupled to our application
code because:
- our application code is not well structured (eg we don’t make any use
  of blueprints)
- spreading this change across lots of files in our application would
  make it harder to test without actually hitting each endpoints; such
  tests would be slow and verbose

So I’ve gone for more of a meta approach. Rather than testing that each
endpoint has a specific navigation item selected, I’ve gone for
validating that:
- all endpoints being mapped to are real
- all endpoints have _a_ selected navigation item (or are specifically
  excluded)

This means that it’s impossible to add, change or remove an endpoint
without also updating which navigation item should be selected. And the
actual mapping is so declarative that it testing it would be redundant.

app/__init__.py

@@ -36,7 +36,7 @@ from notifications_utils.formatters import formatted_list
 from werkzeug.exceptions import abort
 from werkzeug.local import LocalProxy
 
-from app import proxy_fix
+from app import proxy_fix, navigation
 from app.config import configs
 from app.asset_fingerprinter import AssetFingerprinter
 from app.notify_client.service_api_client import ServiceAPIClient
@@ -171,6 +171,10 @@ def init_app(application):
     def _attach_current_user():
         return{'current_user': current_user}
 
+    @application.context_processor
+    def _nav_selected():
+        return {'nav_selected': navigation.nav_selected}
+
     @application.before_request
     def record_start_time():
         g.start = monotonic()

app/assets/stylesheets/components/navigation.scss

@@ -63,6 +63,15 @@
       color: $text-colour;
     }
 
+    &.selected {
+      @include bold-19;
+      position: relative;
+      // These two lines stop the width of the item jumping so much
+      // between selected and unselected states
+      left: -0.5px;
+      letter-spacing: -0.01em;
+    }
+
   }
 
 }
@@ -99,7 +108,7 @@
 
   &__item--active {
     @include bold-16;
-    
+
     a:link, a:visited {
       color: $text-colour;
     }

app/navigation.py

@@ -0,0 +1,238 @@
+from itertools import chain
+
+from flask import request
+
+mapping = {
+    'dashboard': {
+        'conversation',
+        'inbox',
+        'monthly',
+        'service_dashboard',
+        'template_usage',
+        'view_job',
+        'view_jobs',
+        'view_notification',
+        'view_notifications',
+    },
+    'templates': {
+        'action_blocked',
+        'add_service_template',
+        'add_template_by_type',
+        'check_messages',
+        'check_notification',
+        'choose_template',
+        'confirm_redact_template',
+        'conversation_reply',
+        'delete_service_template',
+        'edit_service_template',
+        'send_messages',
+        'send_one_off',
+        'send_one_off_step',
+        'send_test',
+        'send_test_preview',
+        'send_test_step',
+        'set_sender',
+        'set_template_sender',
+        'view_template',
+        'view_template_version',
+        'view_template_versions',
+    },
+    'team-members': {
+        'edit_user_permissions',
+        'invite_user',
+        'manage_users',
+        'remove_user_from_service',
+    },
+    'usage': {
+        'usage',
+    },
+    'settings': {
+        'link_service_to_organisation',
+        'request_to_go_live',
+        'service_add_email_reply_to',
+        'service_add_letter_contact',
+        'service_add_sms_sender',
+        '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',
+        'set_free_sms_allowance',
+        'set_letter_branding',
+        'set_organisation_type',
+        'submit_request_to_go_live',
+    },
+    'api-integration': {
+        'api_callbacks',
+        'api_documentation',
+        'api_integration',
+        'api_keys',
+        'create_api_key',
+        'delivery_status_callback',
+        'received_text_messages_callback',
+        'revoke_api_key',
+        'whitelist',
+    },
+}
+
+exclude = {
+    'accept_invite',
+    'accept_org_invite',
+    'add_organisation',
+    'add_service',
+    'agreement',
+    'archive_service',
+    'bat_phone',
+    'callbacks',
+    'cancel_invited_org_user',
+    'cancel_invited_user',
+    'cancel_job',
+    'check_and_resend_text_code',
+    'check_and_resend_verification_code',
+    'check_messages_preview',
+    'choose_account',
+    'choose_service',
+    'confirm_edit_organisation_name',
+    'conversation_reply_with_template',
+    'conversation_updates',
+    'cookies',
+    'create_email_branding',
+    'delivery_and_failure',
+    'design_content',
+    'documentation',
+    'download_agreement',
+    'download_notifications_csv',
+    'edit_organisation_name',
+    'edit_provider',
+    'edit_user_org_permissions',
+    'email_branding',
+    'email_not_received',
+    'email_template',
+    'error',
+    'features',
+    'feedback',
+    'forgot_password',
+    'get_example_csv',
+    'get_notifications_as_json',
+    'go_to_dashboard_after_tour',
+    'inbound_sms_admin',
+    'inbox_download',
+    'inbox_updates',
+    'index',
+    'information_risk_management',
+    'information_security',
+    'integration_testing',
+    'invite_org_user',
+    'letter_jobs',
+    'live_services',
+    'manage_org_users',
+    'new_password',
+    'old_feedback',
+    'old_integration_testing',
+    'old_roadmap',
+    'old_service_dashboard',
+    'old_submit_feedback',
+    'old_terms',
+    'old_using_notify',
+    'organisation_dashboard',
+    'organisation_settings',
+    'organisations',
+    'platform_admin',
+    'pricing',
+    'redact_template',
+    'register',
+    'register_from_invite',
+    'register_from_org_invite',
+    'registration_continue',
+    'remove_user_from_organisation',
+    'resend_email_link',
+    'resend_email_verification',
+    'resume_service',
+    'roadmap',
+    'security',
+    'send_notification',
+    'service_dashboard_updates',
+    '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',
+    'show_accounts_or_dashboard',
+    'sign_in',
+    'sign_out',
+    'start_job',
+    'start_tour',
+    'styleguide',
+    'support',
+    'suspend_service',
+    'temp_service_history',
+    'template_history',
+    'terms',
+    'thanks',
+    'triage',
+    'trial_mode',
+    'trial_services',
+    'two_factor',
+    'two_factor_email',
+    'two_factor_email_sent',
+    'update_email_branding',
+    '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',
+    'using_notify',
+    'verify',
+    'verify_email',
+    'verify_mobile',
+    'view_job_csv',
+    'view_job_updates',
+    'view_letter_notification_as_preview',
+    'view_letter_template_preview',
+    'view_notification_updates',
+    'view_notifications_csv',
+    'view_provider',
+    'view_providers',
+    'view_template_version_preview',
+}
+
+mapping = {
+    navigation: {
+        'main.{}'.format(endpoint) for endpoint in endpoints
+    } for navigation, endpoints in mapping.items()
+}
+
+endpoints_with_navigation = tuple(chain.from_iterable((
+    endpoints for navigation_item, endpoints in mapping.items()
+)))
+
+endpoints_without_navigation = tuple(
+    'main.{}'.format(endpoint) for endpoint in exclude
+) + ('static', 'status.show_status')
+
+
+def nav_selected(navigation_item):
+    if request.endpoint in mapping[navigation_item]:
+        return "class=selected"
+    return ''

app/templates/main_nav.html

@@ -43,19 +43,19 @@
 {% else %}
 <nav class="navigation">
   <ul>
-    <li><a href="{{ url_for('.service_dashboard', service_id=current_service.id) }}">Dashboard</a></li>
+    <li><a href="{{ url_for('.service_dashboard', service_id=current_service.id) }}" {{ nav_selected('dashboard') }}>Dashboard</a></li>
   {% if current_user.has_permissions('view_activity') %}
-    <li><a href="{{ url_for('.choose_template', service_id=current_service.id) }}">Templates</a></li>
+    <li><a href="{{ url_for('.choose_template', service_id=current_service.id) }}" {{ nav_selected('templates') }}>Templates</a></li>
   {% endif %}
-  <li><a href="{{ url_for('.manage_users', service_id=current_service.id) }}">Team members</a></li>
+  <li><a href="{{ url_for('.manage_users', service_id=current_service.id) }}" {{ nav_selected('team-members') }}>Team members</a></li>
   {% if current_user.has_permissions('manage_service') %}
-    <li><a href="{{ url_for('.usage', service_id=current_service.id) }}">Usage</a></li>
+    <li><a href="{{ url_for('.usage', service_id=current_service.id) }}" {{ nav_selected('usage') }}>Usage</a></li>
   {% endif %}
   {% if current_user.has_permissions('manage_api_keys', 'manage_service') %}
-    <li><a href="{{ url_for('.service_settings', service_id=current_service.id) }}">Settings</a></li>
+    <li><a href="{{ url_for('.service_settings', service_id=current_service.id) }}" {{ nav_selected('settings') }}>Settings</a></li>
   {% endif %}
   {% if current_user.has_permissions('manage_api_keys') %}
-    <li><a href="{{ url_for('.api_integration', service_id=current_service.id) }}">API integration</a></li>
+    <li><a href="{{ url_for('.api_integration', service_id=current_service.id) }}" {{ nav_selected('api-integration') }}>API integration</a></li>
   {% endif %}
   </ul>
 </nav>

tests/app/test_navigation.py

@@ -0,0 +1,53 @@
+import pytest
+from tests.conftest import SERVICE_ONE_ID, app_
+
+from app import navigation
+
+all_endpoints = [
+    rule.endpoint for rule in next(app_(None)).url_map.iter_rules()
+]
+
+
+def test_navigation_items_are_properly_defined():
+    for endpoint in navigation.endpoints_with_navigation:
+        assert endpoint in all_endpoints
+        assert endpoint not in navigation.endpoints_without_navigation
+        assert navigation.endpoints_with_navigation.count(endpoint) == 1
+
+
+def test_excluded_navigation_items_are_properly_defined():
+    for endpoint in navigation.endpoints_without_navigation:
+        assert endpoint in all_endpoints
+        assert endpoint not in navigation.endpoints_with_navigation
+        assert navigation.endpoints_without_navigation.count(endpoint) == 1
+
+
+def test_all_endpoints_are_covered():
+    for endpoint in all_endpoints:
+        assert endpoint in (
+            navigation.endpoints_with_navigation +
+            navigation.endpoints_without_navigation
+        )
+
+
+@pytest.mark.xfail(raises=KeyError)
+def test_raises_on_invalid_navigation_item(client_request):
+    navigation.nav_selected('foo')
+
+
+@pytest.mark.parametrize('endpoint, selected_nav_item', [
+    ('main.choose_template', 'Templates'),
+    ('main.manage_users', 'Team members'),
+])
+def test_a_page_should_nave_selected_navigation_item(
+    client_request,
+    mock_get_service_templates,
+    mock_get_users_by_service,
+    mock_get_invites_for_service,
+    endpoint,
+    selected_nav_item,
+):
+    page = client_request.get(endpoint, service_id=SERVICE_ONE_ID)
+    selected_nav_items = page.select('.navigation a.selected')
+    assert len(selected_nav_items) == 1
+    assert selected_nav_items[0].text.strip() == selected_nav_item