From 5ac6efc5809bc7517d8d7b0e609e81b9ca4ce718 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 4 May 2022 16:40:29 +0100 Subject: [PATCH 1/3] Refactor logic out of Jinja before making more complicated To keep the conditionals in the Jinja template more readable, this commit moves the logic into a method on the model, where it can be split over multiple statements and lines. --- app/models/user.py | 10 ++++++++++ app/templates/views/manage-users.html | 2 +- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/app/models/user.py b/app/models/user.py index ce7e41842..74745f3c8 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -440,6 +440,13 @@ class User(JSONModel, UserMixin): def complete_webauthn_login_attempt(self, is_successful=True): return user_api_client.complete_webauthn_login_attempt(self.id, is_successful) + def is_editable_by(self, other_user): + if other_user == self: + return False + if self.state == 'active': + return True + return False + class InvitedUser(JSONModel): @@ -575,6 +582,9 @@ class InvitedUser(JSONModel): # only used on the manage users page to display the count, so okay to not be fully fledged for now return [{'id': x} for x in self.folder_permissions] + def is_editable_by(self, other): + return False + class InvitedOrgUser(JSONModel): diff --git a/app/templates/views/manage-users.html b/app/templates/views/manage-users.html index b17469d1c..a8d3daac7 100644 --- a/app/templates/views/manage-users.html +++ b/app/templates/views/manage-users.html @@ -73,7 +73,7 @@ {% if current_user.has_permissions('manage_service') %} {% if user.status == 'pending' %} Cancel invitation for {{ user.email_address }} - {% elif user.state == 'active' and current_user.id != user.id %} + {% elif user.is_editable_by(current_user) %} Change details for {{ user.name }} {{ user.email_address }} {% endif %} {% endif %} From c5d4bfd8efffdfbb59bd4489ec4d8e18b2fad39c Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 4 May 2022 16:44:59 +0100 Subject: [PATCH 2/3] Refactor to avoid direct string comparison Direct string comparison in multiple places is prone to typos. It also means that a consumer of the class needs to know that whether a user is pending or active is held in the `state` property, which is an implementation detail. --- app/models/user.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/models/user.py b/app/models/user.py index 74745f3c8..aee4fe9b2 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -151,7 +151,7 @@ class User(JSONModel, UserMixin): return session.get('current_session_id') != self.current_session_id def activate(self): - if self.state == 'pending': + if self.is_pending: user_data = user_api_client.activate_user(self.id) return self.__class__(user_data['data']) else: @@ -192,6 +192,10 @@ class User(JSONModel, UserMixin): def is_active(self): return self.state == 'active' + @property + def is_pending(self): + return self.state == 'pending' + @property def is_gov_user(self): return is_gov_user(self.email_address) @@ -443,7 +447,7 @@ class User(JSONModel, UserMixin): def is_editable_by(self, other_user): if other_user == self: return False - if self.state == 'active': + if self.is_active: return True return False From c6dc0d513e0a59d376becad0e2e273a347ca8849 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 4 May 2022 16:46:00 +0100 Subject: [PATCH 3/3] Allow editing of pending users MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At the moment if a user is pending we don’t show the ‘change’ link. This is unhelpful because: - there’s no way to remove this user - there’s no way to change their phone number, if the reason that they are still pending is because they’ve been unable to receive the two factor code at the number they first provided --- app/models/user.py | 2 +- tests/app/main/views/test_manage_users.py | 39 +++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/app/models/user.py b/app/models/user.py index aee4fe9b2..9cfa07869 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -447,7 +447,7 @@ class User(JSONModel, UserMixin): def is_editable_by(self, other_user): if other_user == self: return False - if self.is_active: + if self.is_active or self.is_pending: return True return False diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 14005bc28..cf57eef0d 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -151,6 +151,45 @@ def test_should_show_overview_page( mock_get_users.assert_called_once_with(SERVICE_ONE_ID) +@pytest.mark.parametrize('state', ( + 'active', 'pending', +)) +def test_should_show_change_details_link( + client_request, + mocker, + mock_get_invites_for_service, + mock_get_template_folders, + service_one, + active_user_with_permissions, + active_caseworking_user, + state, +): + current_user = active_user_with_permissions + + other_user = active_caseworking_user + other_user['id'] = uuid.uuid4() + other_user['email_address'] = 'zzzzzzz@example.gov.uk' + other_user['state'] = state + + mocker.patch('app.user_api_client.get_user', return_value=current_user) + mocker.patch('app.models.user.Users.client_method', return_value=[ + current_user, + other_user, + ]) + + page = client_request.get('main.manage_users', service_id=SERVICE_ONE_ID) + link = page.select('.user-list-item')[-1].select_one('a') + + assert normalize_spaces(link.text) == ( + 'Change details for Test User zzzzzzz@example.gov.uk' + ) + assert link['href'] == url_for( + '.edit_user_permissions', + service_id=SERVICE_ONE_ID, + user_id=other_user['id'], + ) + + @pytest.mark.parametrize('number_of_users', ( pytest.param(7, marks=pytest.mark.xfail), pytest.param(8),