From e1d1c5c3f5d21d6d8a733041b35d6df45de7a07b Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Wed, 23 Feb 2022 18:31:00 +0000 Subject: [PATCH 1/4] Let users on email auth delete their mobile numbers Sometimes users ask us to delete their mobile numbers for them. If those users are on email auth, they should be able to delete their number themselves. This will save them writing a support ticket and save us going into the database. --- app/main/views/user_profile.py | 23 ++++++++- app/templates/views/user-profile/change.html | 15 +++++- tests/app/main/views/test_user_profile.py | 53 ++++++++++++++++++++ 3 files changed, 88 insertions(+), 3 deletions(-) diff --git a/app/main/views/user_profile.py b/app/main/views/user_profile.py index c02256964..478819188 100644 --- a/app/main/views/user_profile.py +++ b/app/main/views/user_profile.py @@ -118,22 +118,43 @@ def user_profile_email_confirm(token): @main.route("/user-profile/mobile-number", methods=['GET', 'POST']) +@main.route( + "/user-profile/mobile-number/delete", + methods=['GET'], + endpoint="user_profile_confirm_delete_mobile_number" +) @user_is_logged_in def user_profile_mobile_number(): + user = User.from_id(current_user.id) form = ChangeMobileNumberForm(mobile_number=current_user.mobile_number) if form.validate_on_submit(): session[NEW_MOBILE] = form.mobile_number.data return redirect(url_for('.user_profile_mobile_number_authenticate')) + if (request.endpoint == "main.user_profile_confirm_delete_mobile_number"): + flash("Are you sure you want to delete your mobile number from Notify?", 'delete') + return render_template( 'views/user-profile/change.html', thing='mobile number', - form_field=form.mobile_number + form_field=form.mobile_number, + user_auth=user.auth_type ) +@main.route("/user-profile/mobile-number/delete", methods=['POST']) +@user_is_logged_in +def user_profile_mobile_number_delete(): + if current_user.auth_type == 'sms_auth': + abort(403) + + current_user.update(mobile_number=None) + + return redirect(url_for('.user_profile')) + + @main.route("/user-profile/mobile-number/authenticate", methods=['GET', 'POST']) @user_is_logged_in def user_profile_mobile_number_authenticate(): diff --git a/app/templates/views/user-profile/change.html b/app/templates/views/user-profile/change.html index ae8a54a13..c7ffa8b51 100644 --- a/app/templates/views/user-profile/change.html +++ b/app/templates/views/user-profile/change.html @@ -20,9 +20,20 @@
{% call form_wrapper() %} {{ form_field(error_message_with_html=True) }} - {{ page_footer('Save') }} + {% if current_user.auth_type == 'email_auth' %} + {{ page_footer( + 'Save', + delete_link=url_for( + 'main.user_profile_mobile_number_delete', + user_id=current_user.id + ), + delete_link_text='Delete your number' + ) + }} + {% else %} + {{ page_footer('Save')}} + {% endif %} {% endcall %}
- {% endblock %} diff --git a/tests/app/main/views/test_user_profile.py b/tests/app/main/views/test_user_profile.py index 21866e775..d3688143a 100644 --- a/tests/app/main/views/test_user_profile.py +++ b/tests/app/main/views/test_user_profile.py @@ -183,6 +183,59 @@ def test_should_show_mobile_number_page( ): page = client_request.get(('main.user_profile_mobile_number')) assert 'Change your mobile number' in page.text + assert 'Delete your number' not in page.text + + +def test_change_your_mobile_number_page_shows_delete_link_if_user_on_email_auth( + client_request, + api_user_active_email_auth, + mocker +): + mocker.patch('app.user_api_client.get_user', return_value=api_user_active_email_auth) + page = client_request.get(('main.user_profile_mobile_number')) + assert 'Change your mobile number' in page.text + assert 'Delete your number' in page.text + + +def test_confirm_delete_mobile_number( + client_request, + api_user_active_email_auth, + mocker +): + mocker.patch('app.user_api_client.get_user', return_value=api_user_active_email_auth) + + page = client_request.get( + '.user_profile_confirm_delete_mobile_number', + _test_page_title=False, + ) + + assert normalize_spaces(page.select_one('.banner-dangerous').text) == ( + 'Are you sure you want to delete your mobile number from Notify? ' + 'Yes, delete' + ) + assert 'action' not in page.select_one('.banner-dangerous form') + assert page.select_one('.banner-dangerous form')['method'] == 'post' + + +def test_delete_mobile_number( + client_request, + api_user_active_email_auth, + mocker +): + mocker.patch('app.user_api_client.get_user', return_value=api_user_active_email_auth) + mock_delete = mocker.patch('app.user_api_client.update_user_attribute') + + client_request.post( + '.user_profile_mobile_number_delete', + _expected_redirect=url_for( + '.user_profile', + _external=True, + ) + ) + mock_delete.assert_called_once_with( + api_user_active_email_auth["id"], + mobile_number=None + ) @pytest.mark.parametrize('phone_number_to_register_with', [ From eb0851e1e3642b3ccc0a00607252e0afc0edd6da Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 18 Mar 2022 13:08:09 +0000 Subject: [PATCH 2/4] Please the navigation thing --- app/navigation.py | 2 ++ tests/app/test_navigation.py | 2 ++ 2 files changed, 4 insertions(+) diff --git a/app/navigation.py b/app/navigation.py index a928000b4..4f0b56ef2 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -67,12 +67,14 @@ class HeaderNavigation(Navigation): }, 'user-profile': { 'user_profile', + 'user_profile_confirm_delete_mobile_number', 'user_profile_email', 'user_profile_email_authenticate', 'user_profile_email_confirm', 'user_profile_mobile_number', 'user_profile_mobile_number_authenticate', 'user_profile_mobile_number_confirm', + 'user_profile_mobile_number_delete', 'user_profile_name', 'user_profile_password', 'user_profile_disable_platform_admin_view', diff --git a/tests/app/test_navigation.py b/tests/app/test_navigation.py index 2cc4058fa..fb4e678f4 100644 --- a/tests/app/test_navigation.py +++ b/tests/app/test_navigation.py @@ -314,6 +314,7 @@ EXCLUDED_ENDPOINTS = tuple(map(Navigation.get_endpoint_with_blueprint, { 'usage', 'user_information', 'user_profile', + 'user_profile_confirm_delete_mobile_number', 'user_profile_confirm_delete_security_key', 'user_profile_delete_security_key', 'user_profile_disable_platform_admin_view', @@ -324,6 +325,7 @@ EXCLUDED_ENDPOINTS = tuple(map(Navigation.get_endpoint_with_blueprint, { 'user_profile_mobile_number', 'user_profile_mobile_number_authenticate', 'user_profile_mobile_number_confirm', + 'user_profile_mobile_number_delete', 'user_profile_name', 'user_profile_password', 'user_profile_security_keys', From 99682db7bfcb1c57775cce8189536aea32ef31a0 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 22 Mar 2022 17:55:55 +0000 Subject: [PATCH 3/4] Change conditional for consistency --- app/main/views/user_profile.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/main/views/user_profile.py b/app/main/views/user_profile.py index 478819188..ad744a313 100644 --- a/app/main/views/user_profile.py +++ b/app/main/views/user_profile.py @@ -147,7 +147,7 @@ def user_profile_mobile_number(): @main.route("/user-profile/mobile-number/delete", methods=['POST']) @user_is_logged_in def user_profile_mobile_number_delete(): - if current_user.auth_type == 'sms_auth': + if current_user.auth_type != 'email_auth': abort(403) current_user.update(mobile_number=None) From 190381c57800c133e0cbfb021203feb24a0e63b6 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Wed, 23 Mar 2022 12:40:43 +0000 Subject: [PATCH 4/4] Make sure delete mobile number only shown when needed Users who have no mobile number set, users who are not on email auth and users who are not on "Change mobile number" page should not see the delete link. --- app/templates/views/user-profile/change.html | 2 +- tests/app/main/views/test_user_profile.py | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/app/templates/views/user-profile/change.html b/app/templates/views/user-profile/change.html index c7ffa8b51..fc8858572 100644 --- a/app/templates/views/user-profile/change.html +++ b/app/templates/views/user-profile/change.html @@ -20,7 +20,7 @@
{% call form_wrapper() %} {{ form_field(error_message_with_html=True) }} - {% if current_user.auth_type == 'email_auth' %} + {% if current_user.auth_type == 'email_auth' and (current_user.mobile_number and thing == "mobile number") %} {{ page_footer( 'Save', delete_link=url_for( diff --git a/tests/app/main/views/test_user_profile.py b/tests/app/main/views/test_user_profile.py index d3688143a..c01af3027 100644 --- a/tests/app/main/views/test_user_profile.py +++ b/tests/app/main/views/test_user_profile.py @@ -12,6 +12,8 @@ from app.models.webauthn_credential import ( ) from tests.conftest import ( create_api_user_active, + create_user, + fake_uuid, normalize_spaces, url_for_endpoint_with_token, ) @@ -91,6 +93,8 @@ def test_should_show_email_page( 'main.user_profile_email' ) assert page.select_one('h1').text.strip() == 'Change your email address' + # template is shared with "Change your mobile number" but we don't want to show Delete mobile number link + assert 'Delete your number' not in page.text def test_should_redirect_after_email_change( @@ -197,6 +201,22 @@ def test_change_your_mobile_number_page_shows_delete_link_if_user_on_email_auth( assert 'Delete your number' in page.text +def test_change_your_mobile_number_page_doesnt_show_delete_link_if_user_has_no_mobile_number( + client_request, + api_user_active_email_auth, + mocker +): + user = create_user( + id=fake_uuid, + auth_type='email_auth', + mobile_number=None + ) + mocker.patch('app.user_api_client.get_user', return_value=user) + page = client_request.get(('main.user_profile_mobile_number')) + assert 'Change your mobile number' in page.text + assert 'Delete your number' not in page.text + + def test_confirm_delete_mobile_number( client_request, api_user_active_email_auth,