From 5158377b2e9e5f0183d1630b33ca94637372b30b Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 19 Feb 2019 15:35:51 +0000 Subject: [PATCH 1/6] Add a get view and template that enable changing team members email --- app/main/views/manage_users.py | 18 ++++++++++++- app/templates/views/manage-users.html | 2 ++ .../views/manage-users/edit-user-email.html | 27 +++++++++++++++++++ tests/app/main/views/test_manage_users.py | 20 ++++++++++++++ 4 files changed, 66 insertions(+), 1 deletion(-) create mode 100644 app/templates/views/manage-users/edit-user-email.html diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index cbbff87c8..a24339fb7 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -9,7 +9,7 @@ from app import ( user_api_client, ) from app.main import main -from app.main.forms import InviteUserForm, PermissionsForm, SearchUsersForm +from app.main.forms import InviteUserForm, PermissionsForm, SearchUsersForm, ChangeEmailForm from app.models.user import permissions from app.utils import user_has_permissions @@ -122,6 +122,22 @@ def remove_user_from_service(service_id, user_id): ) +@main.route("/services//users//edit-email", methods=['GET']) +@login_required +@user_has_permissions('manage_service') +def edit_user_email(service_id, user_id): + user = user_api_client.get_user(user_id) + user_email = user.email_address + form = ChangeEmailForm(user_api_client.is_email_already_in_use(user_email), email_address=user_email) + + return render_template( + 'views/manage-users/edit-user-email.html', + user=user, + form=form, + service_id=service_id + ) + + @main.route("/services//cancel-invited-user/", methods=['GET']) @user_has_permissions('manage_service') def cancel_invited_user(service_id, invited_user_id): diff --git a/app/templates/views/manage-users.html b/app/templates/views/manage-users.html index da3157c5a..4c65a08a8 100644 --- a/app/templates/views/manage-users.html +++ b/app/templates/views/manage-users.html @@ -64,6 +64,8 @@ Cancel invitation {% elif user.state == 'active' and current_user.id != user.id %} Edit permissions +
+ Edit user email {% endif %} {% endif %} diff --git a/app/templates/views/manage-users/edit-user-email.html b/app/templates/views/manage-users/edit-user-email.html new file mode 100644 index 000000000..35164d463 --- /dev/null +++ b/app/templates/views/manage-users/edit-user-email.html @@ -0,0 +1,27 @@ +{% extends "withoutnav_template.html" %} +{% from "components/textbox.html" import textbox %} +{% from "components/page-footer.html" import page_footer %} +{% from "components/form.html" import form_wrapper %} + +{% block per_page_title %} + Edit user email +{% endblock %} + +{% block maincolumn_content %} + +

Edit user email

+

for {{ user.name }}

+
+
+ {% call form_wrapper() %} + {{ textbox(form.email_address) }} + {{ page_footer( + 'Save', + back_link=url_for('.manage_users', service_id=service_id), + back_link_text="Back to team members" + ) }} + {% endcall %} +
+
+ +{% endblock %} diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 5cf45990d..b6b9d9206 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -769,3 +769,23 @@ def test_can_invite_user_as_platform_admin( response = logged_in_client.get(url_for('main.manage_users', service_id=service_one['id'])) resp_text = response.get_data(as_text=True) assert url_for('.invite_user', service_id=service_one['id']) in resp_text + + +def test_edit_user_email_page( + client_request, + active_user_with_permissions, + service_one, + mocker +): + user = active_user_with_permissions + test_user = mocker.patch('app.user_api_client.get_user', return_value=user) + + page = client_request.get( + 'main.edit_user_email', + service_id=service_one['id'], + user_id=test_user.id + ) + + assert page.find('h1').text == "Edit user email" + assert page.select('p[id=user_name]')[0].text == "for " + user.name + assert page.select('input[type=email]')[0].attrs["value"] == user.email_address From 3c9c918963ce78f48979df45bef727acba191da8 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 19 Feb 2019 18:01:01 +0000 Subject: [PATCH 2/6] Redirect to confirmation page --- app/main/views/manage_users.py | 42 +++++++++++++++++++++-- tests/app/main/views/test_manage_users.py | 21 ++++++++++++ 2 files changed, 60 insertions(+), 3 deletions(-) diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index a24339fb7..fb6a38b87 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -1,4 +1,4 @@ -from flask import abort, flash, redirect, render_template, request, url_for +from flask import abort, flash, redirect, render_template, request, url_for, session from flask_login import current_user, login_required from notifications_python_client.errors import HTTPError @@ -122,13 +122,21 @@ def remove_user_from_service(service_id, user_id): ) -@main.route("/services//users//edit-email", methods=['GET']) +@main.route("/services//users//edit-email", methods=['GET', 'POST']) @login_required @user_has_permissions('manage_service') def edit_user_email(service_id, user_id): user = user_api_client.get_user(user_id) user_email = user.email_address - form = ChangeEmailForm(user_api_client.is_email_already_in_use(user_email), email_address=user_email) + + def _is_email_already_in_use(email): + return user_api_client.is_email_already_in_use(email) + + form = ChangeEmailForm(_is_email_already_in_use, email_address=user_email) + if form.validate_on_submit(): + session['team_member_email_change'] = form.email_address.data + + return redirect(url_for('.confirm_edit_user_email', user_id=user.id, service_id=service_id)) return render_template( 'views/manage-users/edit-user-email.html', @@ -138,6 +146,34 @@ def edit_user_email(service_id, user_id): ) +@main.route("/services//users//edit-email/confirm", methods=['GET', 'POST']) +@login_required +@user_has_permissions('manage_service') +def confirm_edit_user_email(service_id, user_id): + user = user_api_client.get_user(user_id) + new_email = session['team_member_email_change'] + if request.method == 'POST': + try: + user_api_client.update_user_attribute(user_id, email_address=new_email) + except HTTPError as e: + if e.status_code == 403: + flash("You don't have permission to edit users emails for this service", 'info') + return redirect(url_for( + '.manage_users', + service_id=service_id)) + else: + abort(500, e) + + return redirect(url_for( + '.manage_users', + service_id=service_id + )) + return render_template( + 'views/manage-users/confirm-edit-user-email.html', + user=user, + service_id=service_id + ) + @main.route("/services//cancel-invited-user/", methods=['GET']) @user_has_permissions('manage_service') def cancel_invited_user(service_id, invited_user_id): diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index b6b9d9206..1d2a52710 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -789,3 +789,24 @@ def test_edit_user_email_page( assert page.find('h1').text == "Edit user email" assert page.select('p[id=user_name]')[0].text == "for " + user.name assert page.select('input[type=email]')[0].attrs["value"] == user.email_address + + +def test_edit_user_email_redirects_to_confirmation( + logged_in_client, + active_user_with_permissions, + service_one, + mocker, + mock_get_user, +): + response = logged_in_client.post( + url_for( + 'main.edit_user_email', + service_id=service_one['id'], + user_id=active_user_with_permissions.id)) + assert response.status_code == 302 + assert response.location == url_for( + 'main.confirm_edit_user_email', + service_id=service_one['id'], + user_id=active_user_with_permissions.id, + _external=True + ) From 446a17d80175a43d4bd9d9428956c3cb13865e50 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Wed, 20 Feb 2019 11:50:34 +0000 Subject: [PATCH 3/6] Confirm edit user email changes user email --- app/main/views/manage_users.py | 21 ++++++-- app/navigation.py | 8 +++ .../manage-users/confirm-edit-user-email.html | 27 ++++++++++ .../views/manage-users/edit-user-email.html | 6 +-- tests/app/main/views/test_manage_users.py | 50 ++++++++++++++++++- 5 files changed, 104 insertions(+), 8 deletions(-) create mode 100644 app/templates/views/manage-users/confirm-edit-user-email.html diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index fb6a38b87..35b6431a2 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -1,4 +1,12 @@ -from flask import abort, flash, redirect, render_template, request, url_for, session +from flask import ( + abort, + flash, + redirect, + render_template, + request, + session, + url_for, +) from flask_login import current_user, login_required from notifications_python_client.errors import HTTPError @@ -9,7 +17,12 @@ from app import ( user_api_client, ) from app.main import main -from app.main.forms import InviteUserForm, PermissionsForm, SearchUsersForm, ChangeEmailForm +from app.main.forms import ( + ChangeEmailForm, + InviteUserForm, + PermissionsForm, + SearchUsersForm, +) from app.models.user import permissions from app.utils import user_has_permissions @@ -171,9 +184,11 @@ def confirm_edit_user_email(service_id, user_id): return render_template( 'views/manage-users/confirm-edit-user-email.html', user=user, - service_id=service_id + service_id=service_id, + new_email=new_email ) + @main.route("/services//cancel-invited-user/", methods=['GET']) @user_has_permissions('manage_service') def cancel_invited_user(service_id, invited_user_id): diff --git a/app/navigation.py b/app/navigation.py index e5dbca7ab..e019589f4 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -135,6 +135,7 @@ class HeaderNavigation(Navigation): 'choose_template', 'choose_template_to_copy', 'confirm_edit_organisation_name', + 'confirm_edit_user_email', 'confirm_redact_template', 'conversation', 'conversation_reply', @@ -157,6 +158,7 @@ class HeaderNavigation(Navigation): 'edit_service_template', 'edit_template_postage', 'edit_user_org_permissions', + 'edit_user_email', 'edit_user_permissions', 'email_not_received', 'email_template', @@ -326,6 +328,8 @@ class MainNavigation(Navigation): 'view_template_versions', }, 'team-members': { + 'confirm_edit_user_email', + 'edit_user_email', 'edit_user_permissions', 'invite_user', 'manage_users', @@ -583,6 +587,7 @@ class CaseworkNavigation(Navigation): 'choose_template_to_copy', 'clear_cache', 'confirm_edit_organisation_name', + 'confirm_edit_user_email', 'confirm_redact_template', 'conversation', 'conversation_reply', @@ -607,6 +612,7 @@ class CaseworkNavigation(Navigation): 'edit_provider', 'edit_service_template', 'edit_template_postage', + 'edit_user_email', 'edit_user_org_permissions', 'edit_user_permissions', 'email_branding', @@ -822,6 +828,7 @@ class OrgNavigation(Navigation): 'choose_template', 'choose_template_to_copy', 'clear_cache', + 'confirm_edit_user_email', 'confirm_redact_template', 'conversation', 'conversation_reply', @@ -845,6 +852,7 @@ class OrgNavigation(Navigation): 'edit_provider', 'edit_service_template', 'edit_template_postage', + 'edit_user_email', 'edit_user_permissions', 'email_branding', 'email_not_received', diff --git a/app/templates/views/manage-users/confirm-edit-user-email.html b/app/templates/views/manage-users/confirm-edit-user-email.html new file mode 100644 index 000000000..f852d5de8 --- /dev/null +++ b/app/templates/views/manage-users/confirm-edit-user-email.html @@ -0,0 +1,27 @@ +{% extends "org_template.html" %} +{% from "components/page-footer.html" import page_footer %} +{% from "components/form.html" import form_wrapper %} + +{% block per_page_title %} + Confirm changes to user email +{% endblock %} + +{% block maincolumn_content %} + +

Confirm changes to user email

+ +
+
+ + {% call form_wrapper() %} +

Email address for {{ user.name }} will be changed from "{{ user.email_address }}" to "{{ new_email }}".

+ {{ page_footer( + 'Confirm', + destructive=destructive, + back_link=url_for('.edit_user_email', service_id=service_id, user_id=user.id) + ) }} + {% endcall %} +
+
+ +{% endblock %} diff --git a/app/templates/views/manage-users/edit-user-email.html b/app/templates/views/manage-users/edit-user-email.html index 35164d463..cc1074e3e 100644 --- a/app/templates/views/manage-users/edit-user-email.html +++ b/app/templates/views/manage-users/edit-user-email.html @@ -1,15 +1,15 @@ -{% extends "withoutnav_template.html" %} +{% extends "org_template.html" %} {% from "components/textbox.html" import textbox %} {% from "components/page-footer.html" import page_footer %} {% from "components/form.html" import form_wrapper %} {% block per_page_title %} - Edit user email + Edit user email address {% endblock %} {% block maincolumn_content %} -

Edit user email

+

Edit user email address

for {{ user.name }}

diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 1d2a52710..7356da657 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -39,7 +39,7 @@ from tests.conftest import service_one as create_sample_service 'Can’t Add and edit templates ' 'Can’t Manage settings, team and usage ' 'Can’t Manage API integration ' - 'Edit permissions' + 'Edit permissions Edit user email' ) ), ( @@ -786,7 +786,7 @@ def test_edit_user_email_page( user_id=test_user.id ) - assert page.find('h1').text == "Edit user email" + assert page.find('h1').text == "Edit user email address" assert page.select('p[id=user_name]')[0].text == "for " + user.name assert page.select('input[type=email]')[0].attrs["value"] == user.email_address @@ -810,3 +810,49 @@ def test_edit_user_email_redirects_to_confirmation( user_id=active_user_with_permissions.id, _external=True ) + + +def test_confirm_edit_user_email_page( + logged_in_client, + active_user_with_permissions, + service_one, + mocker, + mock_get_user, +): + new_email = 'new_email@gov.uk' + with logged_in_client.session_transaction() as session: + session['team_member_email_change'] = new_email + response = logged_in_client.get(url_for( + 'main.confirm_edit_user_email', + service_id=service_one['id'], + user_id=active_user_with_permissions.id + )) + + assert 'Confirm changes to user email' in response.get_data(as_text=True) + assert 'Email address for {} will be changed from "{}" to "{}".'.format( + active_user_with_permissions.name, active_user_with_permissions.email_address, new_email + ) in response.get_data(as_text=True) + assert 'Confirm' in response.get_data(as_text=True) + assert response.status_code == 200 + + +def test_confirm_edit_user_email_changes_user_email( + logged_in_client, + active_user_with_permissions, + service_one, + mocker, + mock_get_user, + mock_update_user_attribute +): + new_email = 'new_email@gov.uk' + with logged_in_client.session_transaction() as session: + session['team_member_email_change'] = new_email + response = logged_in_client.post( + url_for( + 'main.confirm_edit_user_email', + service_id=service_one['id'], + user_id=active_user_with_permissions.id)) + assert response.status_code == 302 + assert response.location == url_for( + 'main.manage_users', service_id=service_one['id'], _external=True) + mock_update_user_attribute.assert_called_once_with(active_user_with_permissions.id, email_address=new_email) From 4faf44b5c51afdd82d2e7cc4a60ed7b12c0fc795 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Wed, 20 Feb 2019 17:34:58 +0000 Subject: [PATCH 4/6] Content changes following consultation with content and design pros :) --- app/templates/views/edit-user-permissions.html | 2 +- app/templates/views/manage-users.html | 4 +--- .../manage-users/confirm-edit-user-email.html | 15 +++++++++------ .../views/manage-users/edit-user-email.html | 12 ++++++------ tests/app/main/views/test_manage_users.py | 18 +++++++++++------- 5 files changed, 28 insertions(+), 23 deletions(-) diff --git a/app/templates/views/edit-user-permissions.html b/app/templates/views/edit-user-permissions.html index 4a59b6363..4bd585d96 100644 --- a/app/templates/views/edit-user-permissions.html +++ b/app/templates/views/edit-user-permissions.html @@ -14,7 +14,7 @@

- {{ user.email_address }} + {{ user.email_address }} Change

diff --git a/app/templates/views/manage-users.html b/app/templates/views/manage-users.html index 4c65a08a8..645f67f1d 100644 --- a/app/templates/views/manage-users.html +++ b/app/templates/views/manage-users.html @@ -63,9 +63,7 @@ {% if user.status == 'pending' %} Cancel invitation {% elif user.state == 'active' and current_user.id != user.id %} - Edit permissions -
- Edit user email + Edit team member {% endif %} {% endif %} diff --git a/app/templates/views/manage-users/confirm-edit-user-email.html b/app/templates/views/manage-users/confirm-edit-user-email.html index f852d5de8..322d470fe 100644 --- a/app/templates/views/manage-users/confirm-edit-user-email.html +++ b/app/templates/views/manage-users/confirm-edit-user-email.html @@ -1,20 +1,23 @@ -{% extends "org_template.html" %} +{% extends "withnav_template.html" %} {% from "components/page-footer.html" import page_footer %} {% from "components/form.html" import form_wrapper %} {% block per_page_title %} - Confirm changes to user email + Confirm change of email address {% endblock %} {% block maincolumn_content %} -

Confirm changes to user email

+

Confirm change of email address

-
- +
{% call form_wrapper() %} -

Email address for {{ user.name }} will be changed from "{{ user.email_address }}" to "{{ new_email }}".

+

New email address:

+
+

{{ new_email }}

+
+

We will send {{ user.name }} an email to tell them about the change.

{{ page_footer( 'Confirm', destructive=destructive, diff --git a/app/templates/views/manage-users/edit-user-email.html b/app/templates/views/manage-users/edit-user-email.html index cc1074e3e..d1bdfeb91 100644 --- a/app/templates/views/manage-users/edit-user-email.html +++ b/app/templates/views/manage-users/edit-user-email.html @@ -1,24 +1,24 @@ -{% extends "org_template.html" %} +{% extends "withnav_template.html" %} {% from "components/textbox.html" import textbox %} {% from "components/page-footer.html" import page_footer %} {% from "components/form.html" import form_wrapper %} {% block per_page_title %} - Edit user email address + Change team member’s email address {% endblock %} {% block maincolumn_content %} -

Edit user email address

-

for {{ user.name }}

+

Change team member’s email address

+

This will change the email address for {{ user.name }}.

{% call form_wrapper() %} {{ textbox(form.email_address) }} {{ page_footer( 'Save', - back_link=url_for('.manage_users', service_id=service_id), - back_link_text="Back to team members" + back_link=url_for('.edit_user_permissions', service_id=service_id, user_id=user.id), + back_link_text="Back" ) }} {% endcall %}
diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 7356da657..7bc21baaf 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -39,7 +39,7 @@ from tests.conftest import service_one as create_sample_service 'Can’t Add and edit templates ' 'Can’t Manage settings, team and usage ' 'Can’t Manage API integration ' - 'Edit permissions Edit user email' + 'Edit team member' ) ), ( @@ -786,9 +786,10 @@ def test_edit_user_email_page( user_id=test_user.id ) - assert page.find('h1').text == "Edit user email address" - assert page.select('p[id=user_name]')[0].text == "for " + user.name + assert page.find('h1').text == "Change team member’s email address" + assert page.select('p[id=user_name]')[0].text == "This will change the email address for {}.".format(user.name) assert page.select('input[type=email]')[0].attrs["value"] == user.email_address + assert page.select('button[type=submit]')[0].text == "Save" def test_edit_user_email_redirects_to_confirmation( @@ -828,10 +829,13 @@ def test_confirm_edit_user_email_page( user_id=active_user_with_permissions.id )) - assert 'Confirm changes to user email' in response.get_data(as_text=True) - assert 'Email address for {} will be changed from "{}" to "{}".'.format( - active_user_with_permissions.name, active_user_with_permissions.email_address, new_email - ) in response.get_data(as_text=True) + assert 'Confirm change of email address' in response.get_data(as_text=True) + for text in [ + 'New email address:', + new_email, + 'We will send {} an email to tell them about the change.'.format(active_user_with_permissions.name) + ]: + assert text in response.get_data(as_text=True) assert 'Confirm' in response.get_data(as_text=True) assert response.status_code == 200 From 6c406ae5cd841ab4d984c63e71847ec950bc75c8 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 22 Feb 2019 16:13:46 +0000 Subject: [PATCH 5/6] Redirect from confirmation page if session empty --- app/main/views/manage_users.py | 9 ++++++++- tests/app/main/views/test_manage_users.py | 16 ++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 35b6431a2..6033ec7c3 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -164,7 +164,14 @@ def edit_user_email(service_id, user_id): @user_has_permissions('manage_service') def confirm_edit_user_email(service_id, user_id): user = user_api_client.get_user(user_id) - new_email = session['team_member_email_change'] + if 'team_member_email_change' in session: + new_email = session['team_member_email_change'] + else: + return redirect(url_for( + '.edit_user_email', + service_id=service_id, + user_id=user_id + )) if request.method == 'POST': try: user_api_client.update_user_attribute(user_id, email_address=new_email) diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 7bc21baaf..162f81b1f 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -840,6 +840,22 @@ def test_confirm_edit_user_email_page( assert response.status_code == 200 +def test_confirm_edit_user_email_page_redirects_if_session_empty( + logged_in_client, + active_user_with_permissions, + service_one, + mocker, + mock_get_user, +): + response = logged_in_client.get(url_for( + 'main.confirm_edit_user_email', + service_id=service_one['id'], + user_id=active_user_with_permissions.id + )) + assert response.status_code == 302 + assert 'Confirm change of email address' not in response.get_data(as_text=True) + + def test_confirm_edit_user_email_changes_user_email( logged_in_client, active_user_with_permissions, From 909e42fae25b8aa0c2fddf8e1ec726c74b0f8ca7 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 22 Feb 2019 16:20:54 +0000 Subject: [PATCH 6/6] Clear new email address from session after transaction --- app/main/views/manage_users.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 6033ec7c3..e2a8cf785 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -183,6 +183,8 @@ def confirm_edit_user_email(service_id, user_id): service_id=service_id)) else: abort(500, e) + finally: + session.pop("team_member_email_change", None) return redirect(url_for( '.manage_users',