From 29830da5e650ddda0ee79ff9fc242e6d4ab6a701 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 30 Nov 2018 17:31:41 +0000 Subject: [PATCH 1/4] Scope live search on team page to name and email MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s not useful for the search to also be picking up ‘Send messages’ or ‘Signs in with an email link’. --- app/templates/views/manage-users.html | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/templates/views/manage-users.html b/app/templates/views/manage-users.html index 5de9f9e95..8793ae8e3 100644 --- a/app/templates/views/manage-users.html +++ b/app/templates/views/manage-users.html @@ -35,17 +35,17 @@

{%- if user.name -%} - {{ user.name }}  + {{ user.name }}  {%- endif -%} {%- if user.status == 'pending' -%} - {{ user.email_address }} (invited) + {{ user.email_address }} (invited) {%- elif user.status == 'cancelled' -%} - {{ user.email_address }} (cancelled invite) + {{ user.email_address }} (cancelled invite) {%- elif user.id == current_user.id -%} - (you) + (you) {% else %} - {{ user.email_address }} + {{ user.email_address }} {% endif %}

From 538a06c0bf11846367364428487d140a2cd0c62e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 30 Nov 2018 17:39:39 +0000 Subject: [PATCH 2/4] Refactor filtering out accepted invites to client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit None of our model or view layer code should need to know about accepted invites. We don’t use them anywhere because once an invite is accepted that person is now a user. Putting this logic in the client means that: - none of the code calling the client needs to care about accepted invites - it’s easier to (if we want) update the API code to not return accepted invites --- app/main/views/manage_users.py | 8 ++++---- app/notify_client/invite_api_client.py | 22 ++++++++++------------ tests/app/main/views/test_manage_users.py | 12 ++++-------- tests/conftest.py | 4 ++-- 4 files changed, 20 insertions(+), 26 deletions(-) diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 4c0642ead..156206667 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -19,10 +19,10 @@ from app.utils import user_has_permissions @user_has_permissions() def manage_users(service_id): users = sorted( - user_api_client.get_users_for_service(service_id=service_id) + [ - invite for invite in invite_api_client.get_invites_for_service(service_id=service_id) - if invite.status != 'accepted' - ], + ( + user_api_client.get_users_for_service(service_id=service_id) + + invite_api_client.get_invites_for_service(service_id=service_id) + ), key=lambda user: user.email_address, ) diff --git a/app/notify_client/invite_api_client.py b/app/notify_client/invite_api_client.py index 9ea677157..b16f8841c 100644 --- a/app/notify_client/invite_api_client.py +++ b/app/notify_client/invite_api_client.py @@ -28,11 +28,16 @@ class InviteApiClient(NotifyAdminAPIClient): return InvitedUser(**resp['data']) def get_invites_for_service(self, service_id): - endpoint = '/service/{}/invite'.format(service_id) - resp = self.get(endpoint) - invites = resp['data'] - invited_users = self._get_invited_users(invites) - return invited_users + return [ + InvitedUser(**invite) + for invite in self._get_invites_for_service(service_id) + if invite['status'] != 'accepted' + ] + + def _get_invites_for_service(self, service_id): + return self.get( + '/service/{}/invite'.format(service_id) + )['data'] def check_token(self, token): resp = self.get(url='/invite/service/{}'.format(token)) @@ -51,12 +56,5 @@ class InviteApiClient(NotifyAdminAPIClient): self.post(url='/service/{0}/invite/{1}'.format(service_id, invited_user_id), data=data) - def _get_invited_users(self, invites): - invited_users = [] - for invite in invites: - invited_user = InvitedUser(**invite) - invited_users.append(invited_user) - return invited_users - invite_api_client = InviteApiClient() diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index e3a7b7590..5cf45990d 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -1,4 +1,5 @@ import copy +import uuid import pytest from bs4 import BeautifulSoup @@ -641,24 +642,19 @@ def test_manage_users_shows_invited_user( def test_manage_users_does_not_show_accepted_invite( - logged_in_client, + client_request, mocker, active_user_with_permissions, sample_invite, ): - import uuid invited_user_id = uuid.uuid4() sample_invite['id'] = invited_user_id sample_invite['status'] = 'accepted' - data = [InvitedUser(**sample_invite)] - service = create_sample_service(active_user_with_permissions) mocker.patch('app.user_api_client.get_users_for_service', return_value=[active_user_with_permissions]) - mocker.patch('app.invite_api_client.get_invites_for_service', return_value=data) + mocker.patch('app.invite_api_client._get_invites_for_service', return_value=[sample_invite]) - response = logged_in_client.get(url_for('main.manage_users', service_id=service['id'])) + page = client_request.get('main.manage_users', service_id=SERVICE_ONE_ID) - assert response.status_code == 200 - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert page.h1.string.strip() == 'Team members' user_lists = page.find_all('div', {'class': 'user-list'}) assert len(user_lists) == 1 diff --git a/tests/conftest.py b/tests/conftest.py index 0d964d14b..799115ec7 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2174,10 +2174,10 @@ def mock_get_invites_for_service(mocker, service_one, sample_invite): for i in range(0, 5): invite = copy.copy(sample_invite) invite['email_address'] = 'user_{}@testnotify.gov.uk'.format(i) - data.append(InvitedUser(**invite)) + data.append(invite) return data - return mocker.patch('app.invite_api_client.get_invites_for_service', side_effect=_get_invites) + return mocker.patch('app.invite_api_client._get_invites_for_service', side_effect=_get_invites) @pytest.fixture(scope='function') From 88070280974d2ee42100860d400ccebd49e8fc28 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 3 Dec 2018 10:59:36 +0000 Subject: [PATCH 3/4] Refactor team members into the model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follows the pattern we’re using elsewhere of having less logic in the view methods. --- app/main/views/manage_users.py | 12 ++---------- app/models/service.py | 11 +++++++++++ 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 156206667..cbbff87c8 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -18,19 +18,11 @@ from app.utils import user_has_permissions @login_required @user_has_permissions() def manage_users(service_id): - users = sorted( - ( - user_api_client.get_users_for_service(service_id=service_id) + - invite_api_client.get_invites_for_service(service_id=service_id) - ), - key=lambda user: user.email_address, - ) - return render_template( 'views/manage-users.html', - users=users, + users=current_service.team_members, current_user=current_user, - show_search_box=(len(users) > 7), + show_search_box=(len(current_service.team_members) > 7), form=SearchUsersForm(), permissions=permissions, ) diff --git a/app/models/service.py b/app/models/service.py index 8a04d79e5..97faf5554 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -6,6 +6,7 @@ from app.notify_client.api_key_api_client import api_key_api_client from app.notify_client.billing_api_client import billing_api_client from app.notify_client.email_branding_client import email_branding_client from app.notify_client.inbound_number_client import inbound_number_client +from app.notify_client.invite_api_client import invite_api_client from app.notify_client.job_api_client import job_api_client from app.notify_client.organisations_api_client import organisations_client from app.notify_client.service_api_client import service_api_client @@ -98,6 +99,16 @@ class Service(): def has_jobs(self): return job_api_client.has_jobs(self.id) + @cached_property + def team_members(self): + return list(sorted( + ( + invite_api_client.get_invites_for_service(service_id=self.id) + + user_api_client.get_users_for_service(service_id=self.id) + ), + key=lambda user: user.email_address, + )) + @cached_property def has_team_members(self): return user_api_client.get_count_of_users_with_permission( From eb7f421716acb734d8662cc0d3b3fad769580687 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 5 Dec 2018 16:39:15 +0000 Subject: [PATCH 4/4] Remove redundant `list` call (`sorted` returns a list) --- app/models/service.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/service.py b/app/models/service.py index 97faf5554..ccfed3ff7 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -101,13 +101,13 @@ class Service(): @cached_property def team_members(self): - return list(sorted( + return sorted( ( invite_api_client.get_invites_for_service(service_id=self.id) + user_api_client.get_users_for_service(service_id=self.id) ), key=lambda user: user.email_address, - )) + ) @cached_property def has_team_members(self):