From 172f93e8411c8d6324274edfac95fbbfd421e614 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 3 Feb 2020 11:35:17 +0000 Subject: [PATCH 1/5] Refactor navigation logic Will be easier to change if the navigation shown to org users is separated from the navigation shown to regular users of a service. --- app/templates/main_nav.html | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/app/templates/main_nav.html b/app/templates/main_nav.html index 884a78441..242394636 100644 --- a/app/templates/main_nav.html +++ b/app/templates/main_nav.html @@ -3,10 +3,10 @@ {% else %} {% endif %} From a35931a33a47de385294042c75bd8cbe89d1671c Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 3 Feb 2020 11:35:29 +0000 Subject: [PATCH 2/5] Swap order of usage and team members for org users MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Org users, when looking at the page for their org, see: > Usage > Team members When they click into a service it switches to: > Team members > Usage This is jarring. It should stay consistent. I think it that _Usage_ then _Team members_ is the natural way of ordering the navigation at the organisation level, so let’s follow that through to the service level. This does mean that if someone is a member of both an organisation and a service that the nav will jump (because it’ll switch to the existing, service-level order of _Team members_ then _Usage_) but it’s going to jump anyway because you get all the extra navigation items when you’re a member of a service. --- app/templates/main_nav.html | 2 +- tests/app/main/test_permissions.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/templates/main_nav.html b/app/templates/main_nav.html index 242394636..92c05dd6c 100644 --- a/app/templates/main_nav.html +++ b/app/templates/main_nav.html @@ -29,8 +29,8 @@
  • API integration
    • {% endif %} {% elif current_user.has_permissions(allow_org_user=True) %} -
  • Team members
  • Usage
    • +
  • Team members
    • {% endif %} diff --git a/tests/app/main/test_permissions.py b/tests/app/main/test_permissions.py index 34462f5b4..bff68d66e 100644 --- a/tests/app/main/test_permissions.py +++ b/tests/app/main/test_permissions.py @@ -373,8 +373,8 @@ def test_service_navigation_for_org_user( assert [ item.text.strip() for item in page.select('nav.navigation a') ] == [ - 'Team members', 'Usage', + 'Team members', ] From b7b9e9747b124dfe82a1497f95921f1566a9eac6 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 3 Feb 2020 16:05:46 +0000 Subject: [PATCH 3/5] Add test for org user who is also a service user MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Org users should always be able to see the usage page, even if they don’t have the `manage_service` permission for the service. --- tests/app/main/test_permissions.py | 39 ++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/tests/app/main/test_permissions.py b/tests/app/main/test_permissions.py index bff68d66e..9fc7d1898 100644 --- a/tests/app/main/test_permissions.py +++ b/tests/app/main/test_permissions.py @@ -378,6 +378,45 @@ def test_service_navigation_for_org_user( ] +def test_org_users_can_always_see_usage_page( + client_request, + mocker, + active_caseworking_user, + mock_has_no_jobs, + 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, +): + active_caseworking_user['services'] = [SERVICE_ONE_ID] + active_caseworking_user['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(active_caseworking_user, 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') + ] == [ + 'Templates', + 'Sent messages', + 'Team members', + 'Usage', + ] + + def get_name_of_decorator_from_ast_node(node): if isinstance(node, ast.Name): return str(node.id) From cceb731596506bd08499a11b97fc7f925b81da10 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 3 Feb 2020 16:28:47 +0000 Subject: [PATCH 4/5] =?UTF-8?q?Rename=20test=20to=20be=20a=20more=20explic?= =?UTF-8?q?it=20about=20what=20it=E2=80=99s=20testing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s not just that they’re an org user, it’s that they’re also a service user. And the usage page is the special case. --- tests/app/main/test_permissions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/main/test_permissions.py b/tests/app/main/test_permissions.py index 9fc7d1898..09a6d7b1d 100644 --- a/tests/app/main/test_permissions.py +++ b/tests/app/main/test_permissions.py @@ -378,7 +378,7 @@ def test_service_navigation_for_org_user( ] -def test_org_users_can_always_see_usage_page( +def test_org_user_who_is_also_service_user_can_still_see_usage_page( client_request, mocker, active_caseworking_user, From 8a4bf1d07e61a920f91aa52fe9c17f51245cfd72 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 11 Feb 2020 11:08:35 +0000 Subject: [PATCH 5/5] Make test more robust MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now tests: - that the user doesn’t have the usage menu item when they’re not a member of the organisation (i.e. the counterfactual) - that the user still gets a 403 when they try to view the usage page if they’re not a member of the organisation --- tests/app/main/test_permissions.py | 48 +++++++++++++++++++++++------- 1 file changed, 37 insertions(+), 11 deletions(-) diff --git a/tests/app/main/test_permissions.py b/tests/app/main/test_permissions.py index 09a6d7b1d..4156001f2 100644 --- a/tests/app/main/test_permissions.py +++ b/tests/app/main/test_permissions.py @@ -378,7 +378,28 @@ def test_service_navigation_for_org_user( ] -def test_org_user_who_is_also_service_user_can_still_see_usage_page( +@pytest.mark.parametrize('user_organisations, expected_menu_items, expected_status', [ + ( + [], + ( + 'Templates', + 'Sent messages', + 'Team members', + ), + 403, + ), + ( + [ORGANISATION_ID], + ( + 'Templates', + 'Sent messages', + 'Team members', + 'Usage', + ), + 200, + ), +]) +def test_service_user_without_manage_service_permission_can_see_usage_page_when_org_user( client_request, mocker, active_caseworking_user, @@ -390,9 +411,14 @@ def test_org_user_who_is_also_service_user_can_still_see_usage_page( mock_get_invites_for_service, mock_get_users_by_service, mock_get_service_organisation, + mock_get_service_templates, + mock_get_template_folders, + user_organisations, + expected_status, + expected_menu_items, ): active_caseworking_user['services'] = [SERVICE_ONE_ID] - active_caseworking_user['organisations'] = [ORGANISATION_ID] + active_caseworking_user['organisations'] = user_organisations service = service_json( id_=SERVICE_ONE_ID, organisation_id=ORGANISATION_ID, @@ -402,19 +428,19 @@ def test_org_user_who_is_also_service_user_can_still_see_usage_page( return_value={'data': service} ) client_request.login(active_caseworking_user, service=service) - page = client_request.get( - 'main.usage', + 'main.choose_template', service_id=SERVICE_ONE_ID, ) - assert [ + assert tuple( item.text.strip() for item in page.select('nav.navigation a') - ] == [ - 'Templates', - 'Sent messages', - 'Team members', - 'Usage', - ] + ) == expected_menu_items + + client_request.get( + 'main.usage', + service_id=SERVICE_ONE_ID, + _expected_status=expected_status, + ) def get_name_of_decorator_from_ast_node(node):