Merge pull request #3020 from alphagov/org-view-of-service-pages

Allow organisation team members to see team and usage

app/main/views/dashboard.py

@@ -143,7 +143,7 @@ def template_usage(service_id):
 
 @main.route("/services/<service_id>/usage")
 @login_required
-@user_has_permissions('manage_service')
+@user_has_permissions('manage_service', allow_org_user=True)
 def usage(service_id):
     year, current_financial_year = requested_and_current_financial_year(request)
 

app/main/views/manage_users.py

@@ -31,7 +31,7 @@ from app.utils import is_gov_user, redact_mobile_number, user_has_permissions
 
 @main.route("/services/<service_id>/users")
 @login_required
-@user_has_permissions()
+@user_has_permissions(allow_org_user=True)
 def manage_users(service_id):
     return render_template(
         'views/manage-users.html',

app/main/views/organisations.py

@@ -70,13 +70,8 @@ def add_organisation():
 @login_required
 @user_has_permissions()
 def organisation_dashboard(org_id):
-    for service in current_organisation.live_services:
-        has_permission = current_user.has_permission_for_service(service['id'], 'view_activity')
-        service.update({'has_permission_to_view': has_permission})
-
     return render_template(
         'views/organisations/organisation/index.html',
-        organisation_services=current_organisation.live_services
     )
 
 
@@ -87,7 +82,6 @@ def organisation_trial_mode_services(org_id):
     return render_template(
         'views/organisations/organisation/trial-mode-services.html',
         search_form=SearchByNameForm(),
-        services=current_organisation.trial_services
     )
 
 

app/models/user.py

@@ -174,7 +174,7 @@ class User(JSONModel, UserMixin):
     def platform_admin(self):
         return self._platform_admin and not session.get('disable_platform_admin_view', False)
 
-    def has_permissions(self, *permissions, restrict_admin_usage=False):
+    def has_permissions(self, *permissions, restrict_admin_usage=False, allow_org_user=False):
         unknown_permissions = set(permissions) - all_permissions
         if unknown_permissions:
             raise TypeError('{} are not valid permissions'.format(list(unknown_permissions)))
@@ -193,11 +193,22 @@ class User(JSONModel, UserMixin):
             return True
 
         if org_id:
-            return org_id in self.organisation_ids
-        if not permissions:
-            return service_id in self.service_ids
-        if service_id:
-            return any(x in self._permissions.get(service_id, []) for x in permissions)
+            return self.belongs_to_organisation(org_id)
+
+        if not permissions and self.belongs_to_service(service_id):
+            return True
+
+        if any(
+            self.has_permission_for_service(service_id, permission)
+            for permission in permissions
+        ):
+            return True
+
+        from app.models.service import Service
+
+        return allow_org_user and self.belongs_to_organisation(
+            Service.from_id(service_id).organisation_id
+        )
 
     def has_permission_for_service(self, service_id, permission):
         return permission in self._permissions.get(service_id, [])

app/templates/main_nav.html

@@ -6,15 +6,17 @@
   {% if current_user.has_permissions('view_activity') %}
     <li><a href="{{ url_for('.service_dashboard', service_id=current_service.id) }}" {{ main_navigation.is_selected('dashboard') }}>Dashboard</a></li>
   {% endif %}
+  {% if current_user.has_permissions() %}
     <li><a href="{{ url_for('.choose_template', service_id=current_service.id) }}" {{ main_navigation.is_selected('templates') }}>Templates</a></li>
-  {% if not current_user.has_permissions('view_activity') %}
-    <li><a href="{{ url_for('.view_notifications', service_id=current_service.id, status='sending,delivered,failed') }}" {{ casework_navigation.is_selected('sent-messages') }}>Sent messages</a></li>
-    {% if current_service.has_jobs %}
-      <li><a href="{{ url_for('.view_jobs', service_id=current_service.id) }}" {{ casework_navigation.is_selected('uploaded-files') }}>Uploaded files</a></li>
+    {% if not current_user.has_permissions('view_activity') %}
+      <li><a href="{{ url_for('.view_notifications', service_id=current_service.id, status='sending,delivered,failed') }}" {{ casework_navigation.is_selected('sent-messages') }}>Sent messages</a></li>
+      {% if current_service.has_jobs %}
+        <li><a href="{{ url_for('.view_jobs', service_id=current_service.id) }}" {{ casework_navigation.is_selected('uploaded-files') }}>Uploaded files</a></li>
+      {% endif %}
     {% endif %}
   {% endif %}
     <li><a href="{{ url_for('.manage_users', service_id=current_service.id) }}" {{ main_navigation.is_selected('team-members') }}>Team members</a></li>
-  {% if current_user.has_permissions('manage_service') %}
+  {% if current_user.has_permissions('manage_service', allow_org_user=True) %}
     <li><a href="{{ url_for('.usage', service_id=current_service.id) }}" {{ main_navigation.is_selected('usage') }}>Usage</a></li>
   {% endif %}
   {% if current_user.has_permissions('manage_api_keys', 'manage_service') %}

app/templates/org_nav.html

@@ -1,6 +1,6 @@
 <nav class="navigation">
   <ul>
-  	<li><a href="{{ url_for('.organisation_dashboard', org_id=current_org.id) }}" {{ org_navigation.is_selected('dashboard') }}>Services</a></li>
+  	<li><a href="{{ url_for('.organisation_dashboard', org_id=current_org.id) }}" {{ org_navigation.is_selected('dashboard') }}>Usage</a></li>
     <li><a href="{{ url_for('.manage_org_users', org_id=current_org.id) }}" {{ org_navigation.is_selected('team-members') }}>Team members</a></li>
     {% if current_user.platform_admin %}
     <li><a href="{{ url_for('.organisation_settings', org_id=current_org.id) }}" {{ org_navigation.is_selected('settings') }}>Settings</a></li>

app/templates/views/organisations/organisation/index.html

@@ -1,22 +1,18 @@
 {% extends "org_template.html" %}
 
 {% block org_page_title %}
-  Services
+  Usage
 {% endblock %}
 
 {% block maincolumn_content %}
 
   <h1 class="heading-medium">
-    Services
+    Usage
   </h1>
   <ul>
-  {% for service in organisation_services %}
+  {% for service in current_org.live_services %}
     <li class="browse-list-item">
-      {% if service.has_permission_to_view or current_user.platform_admin %}
-        <a href="{{ url_for('main.service_dashboard', service_id=service.id) }}" class="browse-list-link">{{ service['name'] }}</a>
-      {% else %}
-        {{ service['name'] }}
-      {% endif %}
+      <a href="{{ url_for('main.usage', service_id=service.id) }}" class="browse-list-link">{{ service['name'] }}</a>
     </li>
   {% endfor %}
   </ul>

app/templates/views/organisations/organisation/trial-mode-services.html

@@ -12,7 +12,7 @@
   </h1>
   {{ live_search(target_selector='.browse-list-item', show=True, form=search_form, label='Search by name') }}
   <ul>
-  {% for service in services %}
+  {% for service in current_org.trial_services %}
     <li class="browse-list-item">
       <a href="{{ url_for('main.service_dashboard', service_id=service.id) }}" class="browse-list-link">{{ service['name'] }}</a>
     </li>

app/templates/views/organisations/organisation/users/invite-org-user.html

@@ -15,22 +15,20 @@
     "Invite a team member",
     back_link=url_for('.manage_org_users', org_id=current_org.id)
   ) }}
-  <div class="grid-row">
-    {% call form_wrapper(class="column-three-quarters") %}
-	    {{ textbox(form.email_address, width='1-1', safe_error_message=True) }}
+  {% call form_wrapper() %}
+    {{ textbox(form.email_address, width='1-1', safe_error_message=True) }}
 
-	    <div class="bottom-gutter">
-			  <p class="form-label">
-			    All team members can see:
-			  </p>
-			  <ul class="list list-bullet">
-			    <li>dashboard</li>
-			    <li>who the other team members are</li>
-			  </ul>
-			</div>
+    <div class="bottom-gutter">
+		  <p class="form-label">
+		    {{ current_org.name }} team members can:
+		  </p>
+		  <ul class="list list-bullet">
+		    <li>see usage for and team members for each service</li>
+        <li>invite other team members</li>
+		  </ul>
+		</div>
 
-	    {{ page_footer('Send invitation email') }}
-	  {% endcall %}
-  </div>
+    {{ page_footer('Send invitation email') }}
+  {% endcall %}
 
 {% endblock %}

tests/app/main/test_permissions.py

@@ -8,6 +8,13 @@ from app.models.roles_and_permissions import (
     translate_permissions_from_db_to_admin_roles,
 )
 from app.utils import user_has_permissions
+from tests import service_json
+from tests.conftest import (
+    ORGANISATION_ID,
+    ORGANISATION_TWO_ID,
+    SERVICE_ONE_ID,
+    SERVICE_TWO_ID,
+)
 
 
 def _test_permissions(
@@ -37,6 +44,7 @@ def _test_permissions(
 def test_user_has_permissions_on_endpoint_fail(
     client,
     mocker,
+    mock_get_service,
 ):
     user = _user_with_permissions()
     mocker.patch('app.user_api_client.get_user', return_value=user)
@@ -116,6 +124,7 @@ def test_platform_admin_user_can_not_access_page(
     client,
     platform_admin_user,
     mocker,
+    mock_get_service,
 ):
     mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user)
     _test_permissions(
@@ -254,3 +263,109 @@ def test_translate_permissions_from_admin_roles_to_db():
     roles = ['send_messages', 'manage_templates', 'some_unknown_permission']
     db_perms = translate_permissions_from_admin_roles_to_db(roles)
     assert db_perms == {'send_texts', 'send_emails', 'send_letters', 'manage_templates', 'some_unknown_permission'}
+
+
+@pytest.mark.parametrize(
+    'user_services, user_organisations, expected_status, organisation_checked',
+    (
+        ([SERVICE_ONE_ID], [], 200, False),
+        ([SERVICE_ONE_ID, SERVICE_TWO_ID], [], 200, False),
+        ([], [ORGANISATION_ID], 200, True),
+        ([SERVICE_ONE_ID], [ORGANISATION_ID], 200, False),
+        ([], [], 403, True),
+        ([SERVICE_TWO_ID], [], 403, True),
+        ([SERVICE_TWO_ID], [ORGANISATION_ID], 200, True),
+        ([SERVICE_ONE_ID, SERVICE_TWO_ID], [ORGANISATION_ID], 200, False),
+        ([], [ORGANISATION_TWO_ID], 403, True),
+        ([], [ORGANISATION_ID, ORGANISATION_TWO_ID], 200, True),
+    )
+)
+def test_services_pages_that_org_users_are_allowed_to_see(
+    client_request,
+    mocker,
+    api_user_active,
+    mock_get_usage,
+    mock_get_billable_units,
+    mock_get_free_sms_fragment_limit,
+    mock_get_service,
+    mock_get_invites_for_service,
+    mock_get_users_by_service,
+    mock_get_template_folders,
+    mock_get_service_organisation,
+    mock_has_jobs,
+    user_services,
+    user_organisations,
+    expected_status,
+    organisation_checked,
+):
+    api_user_active['services'] = user_services
+    api_user_active['organisations'] = user_organisations
+    api_user_active['permissions'] = {
+        service_id: ['manage_service']
+        for service_id in user_services
+    }
+    service = service_json(
+        name='SERVICE WITH ORG',
+        id_=SERVICE_ONE_ID,
+        users=[api_user_active['id']],
+        organisation_id=ORGANISATION_ID,
+    )
+
+    mock_get_service = mocker.patch(
+        'app.notify_client.service_api_client.service_api_client.get_service',
+        return_value={'data': service}
+    )
+    client_request.login(
+        api_user_active,
+        service=service if SERVICE_ONE_ID in user_services else None,
+    )
+
+    endpoints = (
+        'main.usage',
+        'main.manage_users',
+    )
+
+    for endpoint in endpoints:
+        client_request.get(
+            endpoint,
+            service_id=SERVICE_ONE_ID,
+            _expected_status=expected_status,
+        )
+
+    assert mock_get_service.called is organisation_checked
+
+
+def test_service_navigation_for_org_user(
+    client_request,
+    mocker,
+    api_user_active,
+    mock_get_usage,
+    mock_get_billable_units,
+    mock_get_free_sms_fragment_limit,
+    mock_get_service,
+    mock_get_invites_for_service,
+    mock_get_users_by_service,
+    mock_get_service_organisation,
+):
+    api_user_active['services'] = []
+    api_user_active['organisations'] = [ORGANISATION_ID]
+    service = service_json(
+        id_=SERVICE_ONE_ID,
+        organisation_id=ORGANISATION_ID,
+    )
+    mocker.patch(
+        'app.service_api_client.get_service',
+        return_value={'data': service}
+    )
+    client_request.login(api_user_active, service=service)
+
+    page = client_request.get(
+        'main.usage',
+        service_id=SERVICE_ONE_ID,
+    )
+    assert [
+        item.text.strip() for item in page.select('nav.navigation a')
+    ] == [
+        'Team members',
+        'Usage',
+    ]

tests/app/main/views/organisations/test_organisation.py

@@ -9,6 +9,7 @@ from tests import organisation_json, service_json
 from tests.conftest import (
     ORGANISATION_ID,
     SERVICE_ONE_ID,
+    SERVICE_TWO_ID,
     active_user_with_permissions,
     normalize_spaces,
     platform_admin_user,
@@ -71,7 +72,7 @@ def test_view_organisation_shows_the_correct_organisation(
         org_id=ORGANISATION_ID,
     )
 
-    assert normalize_spaces(page.select_one('h1').text) == 'Services'
+    assert normalize_spaces(page.select_one('h1').text) == 'Usage'
 
 
 def test_create_new_organisation(
@@ -104,11 +105,11 @@ def test_organisation_services_shows_live_services_only(
     mocker.patch(
         'app.organisations_client.get_organisation_services',
         return_value=[
-            service_json(id_='1', name='1', restricted=False, active=True),  # live
+            service_json(id_=SERVICE_ONE_ID, name='1', restricted=False, active=True),  # live
             service_json(id_='2', name='2', restricted=True, active=True),  # trial
             service_json(id_='3', name='3', restricted=True, active=False),  # trial, now archived
             service_json(id_='4', name='4', restricted=False, active=False),  # was live, now archived
-            service_json(id_=SERVICE_ONE_ID, name='5', restricted=False, active=True),  # live, member of
+            service_json(id_=SERVICE_TWO_ID, name='5', restricted=False, active=True),  # live, member of
         ]
     )
 
@@ -120,8 +121,8 @@ def test_organisation_services_shows_live_services_only(
 
     assert normalize_spaces(services[0].text) == '1'
     assert normalize_spaces(services[1].text) == '5'
-    assert services[0].find('a') is None
-    assert services[1].find('a')['href'] == url_for('main.service_dashboard', service_id=SERVICE_ONE_ID)
+    assert services[0].find('a')['href'] == url_for('main.usage', service_id=SERVICE_ONE_ID)
+    assert services[1].find('a')['href'] == url_for('main.usage', service_id=SERVICE_TWO_ID)
 
 
 def test_organisation_trial_mode_services_shows_all_non_live_services(

tests/app/test_navigation.py

@@ -130,7 +130,7 @@ def test_a_page_should_nave_selected_header_navigation_item(
 
 
 @pytest.mark.parametrize('endpoint, selected_nav_item', [
-    ('main.organisation_dashboard', 'Services'),
+    ('main.organisation_dashboard', 'Usage'),
     ('main.manage_org_users', 'Team members'),
 ])
 def test_a_page_should_nave_selected_org_navigation_item(

tests/conftest.py

@@ -691,6 +691,7 @@ def mock_update_service_raise_httperror_duplicate_name(mocker):
 SERVICE_ONE_ID = "596364a0-858e-42c8-9062-a8fe822260eb"
 SERVICE_TWO_ID = "147ad62a-2951-4fa1-9ca0-093cd1a52c52"
 ORGANISATION_ID = "c011fa40-4cbe-4524-b415-dde2f421bd9c"
+ORGANISATION_TWO_ID = "d9b5be73-0b36-4210-9d89-8f1a5c2fef26"
 TEMPLATE_ONE_ID = "b22d7d94-2197-4a7d-a8e7-fd5f9770bf48"
 USER_ONE_ID = "7b395b52-c6c1-469c-9d61-54166461c1ab"
 
@@ -1393,6 +1394,7 @@ def active_user_no_api_key_permission(fake_uuid):
         'auth_type': 'sms_auth',
         'organisations': [],
         'current_session_id': None,
+        'services': [SERVICE_ONE_ID],
     }
 
 
@@ -1415,6 +1417,8 @@ def active_user_no_settings_permission(fake_uuid):
         'platform_admin': False,
         'auth_type': 'sms_auth',
         'current_session_id': None,
+        'services': [SERVICE_ONE_ID],
+        'organisations': [],
     }
 
 
@@ -1432,6 +1436,7 @@ def api_user_locked(fake_uuid):
                  'organisations': [],
                  'current_session_id': None,
                  'platform_admin': False,
+                 'services': [],
                  }
     return user_data
 
@@ -1451,6 +1456,7 @@ def api_user_request_password_reset(fake_uuid):
                  'organisations': [],
                  'current_session_id': None,
                  'platform_admin': False,
+                 'services': [],
                  }
     return user_data
 
@@ -1470,6 +1476,7 @@ def api_user_changed_password(fake_uuid):
                  'organisations': [],
                  'current_session_id': None,
                  'platform_admin': False,
+                 'services': [],
                  }
     return user_data
 
@@ -2148,7 +2155,7 @@ def mock_no_inbound_number_for_service(mocker):
 
 @pytest.fixture(scope='function')
 def mock_has_permissions(mocker):
-    def _has_permission(*permissions, restrict_admin_usage=False):
+    def _has_permission(*permissions, restrict_admin_usage=False, allow_org_user=False):
         return True
 
     return mocker.patch(