Merge pull request #2807 from alphagov/edit_user_phone_number

Edit user phone number

app/main/views/manage_users.py

@@ -19,12 +19,13 @@ from app import (
 from app.main import main
 from app.main.forms import (
     ChangeEmailForm,
+    ChangeMobileNumberForm,
     InviteUserForm,
     PermissionsForm,
     SearchUsersForm,
 )
 from app.models.user import permissions
-from app.utils import user_has_permissions
+from app.utils import redact_mobile_number, user_has_permissions
 
 
 @main.route("/services/<service_id>/users")
@@ -78,7 +79,10 @@ def invite_user(service_id):
 def edit_user_permissions(service_id, user_id):
     service_has_email_auth = current_service.has_permission('email_auth')
     user = current_service.get_team_member(user_id)
-    user_has_no_mobile_number = user.mobile_number is None
+
+    mobile_number = None
+    if user.mobile_number:
+        mobile_number = redact_mobile_number(user.mobile_number, " ")
 
     form = PermissionsForm.from_user(user, service_id)
 
@@ -96,7 +100,7 @@ def edit_user_permissions(service_id, user_id):
         user=user,
         form=form,
         service_has_email_auth=service_has_email_auth,
-        user_has_no_mobile_number=user_has_no_mobile_number
+        mobile_number=mobile_number
     )
 
 
@@ -178,15 +182,9 @@ def confirm_edit_user_email(service_id, user_id):
         try:
             user_api_client.update_user_attribute(str(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)
+            abort(500, e)
         finally:
-            session.pop("team_member_email_change", None)
+            session.pop('team_member_email_change', None)
 
         return redirect(url_for(
             '.manage_users',
@@ -200,6 +198,65 @@ def confirm_edit_user_email(service_id, user_id):
     )
 
 
+@main.route("/services/<service_id>/users/<uuid:user_id>/edit-mobile-number", methods=['GET', 'POST'])
+@login_required
+@user_has_permissions('manage_service')
+def edit_user_mobile_number(service_id, user_id):
+    user = current_service.get_team_member(user_id)
+    user_mobile_number = redact_mobile_number(user.mobile_number)
+
+    form = ChangeMobileNumberForm(mobile_number=user_mobile_number)
+    if form.mobile_number.data == user_mobile_number and request.method == 'POST':
+        return redirect(url_for(
+            '.manage_users',
+            service_id=service_id
+        ))
+    if form.validate_on_submit():
+        session['team_member_mobile_change'] = form.mobile_number.data
+
+        return redirect(url_for('.confirm_edit_user_mobile_number', user_id=user.id, service_id=service_id))
+    return render_template(
+        'views/manage-users/edit-user-mobile.html',
+        user=user,
+        form=form,
+        service_id=service_id
+    )
+
+
+@main.route("/services/<service_id>/users/<uuid:user_id>/edit-mobile-number/confirm", methods=['GET', 'POST'])
+@login_required
+@user_has_permissions('manage_service')
+def confirm_edit_user_mobile_number(service_id, user_id):
+    user = current_service.get_team_member(user_id)
+    if 'team_member_mobile_change' in session:
+        new_number = session['team_member_mobile_change']
+    else:
+        return redirect(url_for(
+            '.edit_user_mobile_number',
+            service_id=service_id,
+            user_id=user_id
+        ))
+    if request.method == 'POST':
+        try:
+            user_api_client.update_user_attribute(str(user_id), mobile_number=new_number)
+        except HTTPError as e:
+            abort(500, e)
+        finally:
+            session.pop('team_member_mobile_change', None)
+
+        return redirect(url_for(
+            '.manage_users',
+            service_id=service_id
+        ))
+
+    return render_template(
+        'views/manage-users/confirm-edit-user-mobile-number.html',
+        user=user,
+        service_id=service_id,
+        new_mobile_number=new_number
+    )
+
+
 @main.route("/services/<service_id>/cancel-invited-user/<uuid:invited_user_id>", methods=['GET'])
 @user_has_permissions('manage_service')
 def cancel_invited_user(service_id, invited_user_id):

app/navigation.py

@@ -136,6 +136,7 @@ class HeaderNavigation(Navigation):
         'choose_template_to_copy',
         'confirm_edit_organisation_name',
         'confirm_edit_user_email',
+        'confirm_edit_user_mobile_number',
         'confirm_redact_template',
         'conversation',
         'conversation_reply',
@@ -159,6 +160,7 @@ class HeaderNavigation(Navigation):
         'edit_template_postage',
         'edit_user_org_permissions',
         'edit_user_email',
+        'edit_user_mobile_number',
         'edit_user_permissions',
         'email_not_received',
         'email_template',
@@ -327,7 +329,9 @@ class MainNavigation(Navigation):
         },
         'team-members': {
             'confirm_edit_user_email',
+            'confirm_edit_user_mobile_number',
             'edit_user_email',
+            'edit_user_mobile_number',
             'edit_user_permissions',
             'invite_user',
             'manage_users',
@@ -584,6 +588,7 @@ class CaseworkNavigation(Navigation):
         'clear_cache',
         'confirm_edit_organisation_name',
         'confirm_edit_user_email',
+        'confirm_edit_user_mobile_number',
         'confirm_redact_template',
         'conversation',
         'conversation_reply',
@@ -609,6 +614,7 @@ class CaseworkNavigation(Navigation):
         'edit_service_template',
         'edit_template_postage',
         'edit_user_email',
+        'edit_user_mobile_number',
         'edit_user_org_permissions',
         'edit_user_permissions',
         'email_branding',
@@ -823,6 +829,7 @@ class OrgNavigation(Navigation):
         'choose_template_to_copy',
         'clear_cache',
         'confirm_edit_user_email',
+        'confirm_edit_user_mobile_number',
         'confirm_redact_template',
         'conversation',
         'conversation_reply',
@@ -847,6 +854,7 @@ class OrgNavigation(Navigation):
         'edit_service_template',
         'edit_template_postage',
         'edit_user_email',
+        'edit_user_mobile_number',
         'edit_user_permissions',
         'email_branding',
         'email_not_received',

app/templates/views/edit-user-permissions.html

@@ -16,7 +16,11 @@
   <p>
     {{ user.email_address }}&emsp;<a href="{{ url_for('.edit_user_email', service_id=current_service.id, user_id=user.id)}}">Change</a>
   </p>
-
+  {% if mobile_number %}
+    <p id="user_mobile_number">
+      {{ mobile_number }}&emsp;<a href="{{ url_for('.edit_user_mobile_number', service_id=current_service.id, user_id=user.id)}}">Change</a>
+    </p>
+  {% endif %}
   <div class="grid-row">
     {% call form_wrapper(class="column-three-quarters") %}
 

app/templates/views/manage-users/confirm-edit-user-mobile-number.html

@@ -0,0 +1,30 @@
+{% extends "withnav_template.html" %}
+{% from "components/page-footer.html" import page_footer %}
+{% from "components/form.html" import form_wrapper %}
+
+{% block per_page_title %}
+  Confirm change of mobile number
+{% endblock %}
+
+{% block maincolumn_content %}
+
+<h1 class="heading-large">Confirm change of mobile number</h1>
+
+<div class="grid-row">
+  <div class="column-whole">
+  {% call form_wrapper() %}
+    <p>New mobile number:</p>
+    <div class="panel panel-border-wide bottom-gutter">
+      <p>{{ new_mobile_number }}</p>
+    </div>
+    <p>We will send {{ user.name }} an email to tell them about the change.</p>
+    {{ page_footer(
+      'Confirm',
+      destructive=destructive,
+      back_link=url_for('.edit_user_mobile_number', service_id=service_id, user_id=user.id)
+    ) }}
+  {% endcall %}
+  </div>
+</div>
+
+{% endblock %}

app/templates/views/manage-users/edit-user-mobile.html

@@ -0,0 +1,27 @@
+{% 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 %}
+  Change team member’s mobile number
+{% endblock %}
+
+{% block maincolumn_content %}
+
+  <h1 class="heading-large">Change team member’s mobile number</h1>
+  <p id="user_name">This will change the mobile number for {{ user.name }}.</p>
+  <div class="grid-row">
+    <div class="column-three-quarters">
+      {% call form_wrapper(class="extra-tracking") %}
+        {{ textbox(form.mobile_number) }}
+        {{ page_footer(
+          'Save',
+          back_link=url_for('.edit_user_permissions', service_id=service_id, user_id=user.id),
+          back_link_text="Back"
+        ) }}
+      {% endcall %}
+    </div>
+  </div>
+
+{% endblock %}

app/templates/views/manage-users/permissions.html

@@ -15,7 +15,7 @@
 </p>
 
 {% if service_has_email_auth %}
-  {% if user_has_no_mobile_number %}
+  {% if not mobile_number %}
     {{ radios(
       form.login_authentication,
       disable=['sms_auth'],

app/utils.py

@@ -650,9 +650,9 @@ def guess_name_from_email_address(email_address):
 
 def should_skip_template_page(template_type):
     return (
-        current_user.has_permissions('send_messages') and
-        not current_user.has_permissions('manage_templates', 'manage_api_keys') and
-        template_type != 'letter'
+        current_user.has_permissions('send_messages')
+        and not current_user.has_permissions('manage_templates', 'manage_api_keys')
+        and template_type != 'letter'
     )
 
 
@@ -671,3 +671,12 @@ def printing_today_or_tomorrow():
         return 'today'
     else:
         return 'tomorrow'
+
+
+def redact_mobile_number(mobile_number, spacing=""):
+    indices = [-4, -5, -6, -7]
+    redact_character = spacing + "•" + spacing
+    mobile_number_list = list(mobile_number.replace(" ", ""))
+    for i in indices:
+        mobile_number_list[i] = redact_character
+    return "".join(mobile_number_list)

tests/app/main/views/test_manage_users.py

@@ -1005,3 +1005,176 @@ def test_confirm_edit_user_email_doesnt_change_user_email_for_non_team_member(
         user_id=USER_ONE_ID,
         _expected_status=404,
     )
+
+
+def test_edit_user_permissions_page_displays_redacted_mobile_number_and_change_link(
+    client_request,
+    active_user_with_permissions,
+    mock_get_users_by_service,
+    service_one,
+    mocker
+):
+    user = active_user_with_permissions
+    mocker.patch('app.user_api_client.get_user', return_value=user)
+
+    page = client_request.get(
+        'main.edit_user_permissions',
+        service_id=service_one['id'],
+        user_id=user.id
+    )
+
+    assert user.name in page.find('h1').text
+    mobile_number_paragraph = page.select('p[id=user_mobile_number]')[0]
+    assert '0770 •  •  •  • 762' in mobile_number_paragraph.text
+    change_link = mobile_number_paragraph.findChild()
+    assert change_link.attrs['href'] == '/services/{}/users/{}/edit-mobile-number'.format(
+        service_one['id'], user.id
+    )
+
+
+def test_edit_user_mobile_number_page(
+    client_request,
+    active_user_with_permissions,
+    mock_get_users_by_service,
+    service_one,
+    mocker
+):
+    user = active_user_with_permissions
+    mocker.patch('app.user_api_client.get_user', return_value=user)
+
+    page = client_request.get(
+        'main.edit_user_mobile_number',
+        service_id=service_one['id'],
+        user_id=user.id
+    )
+
+    assert page.find('h1').text == "Change team member’s mobile number"
+    assert page.select('p[id=user_name]')[0].text == "This will change the mobile number for {}.".format(user.name)
+    assert page.select('input[name=mobile_number]')[0].attrs["value"] == "0770••••762"
+    assert page.select('button[type=submit]')[0].text == "Save"
+
+
+def test_edit_user_mobile_number_redirects_to_confirmation(
+    logged_in_client,
+    active_user_with_permissions,
+    mock_get_users_by_service,
+    service_one,
+    mocker,
+    mock_get_user,
+):
+
+    data = {'mobile_number': '07554080636'}
+    response = logged_in_client.post(
+        url_for(
+            'main.edit_user_mobile_number',
+            service_id=service_one['id'],
+            user_id=active_user_with_permissions.id), data=data)
+    assert response.status_code == 302
+    assert response.location == url_for(
+        'main.confirm_edit_user_mobile_number',
+        service_id=service_one['id'],
+        user_id=active_user_with_permissions.id,
+        _external=True
+    )
+
+
+def test_edit_user_mobile_number_redirects_to_manage_users_if_number_not_changed(
+    logged_in_client,
+    active_user_with_permissions,
+    mock_get_users_by_service,
+    service_one,
+    mocker,
+    mock_get_user,
+):
+
+    data = {'mobile_number': '0770••••762'}
+    response = logged_in_client.post(
+        url_for(
+            'main.edit_user_mobile_number',
+            service_id=service_one['id'],
+            user_id=active_user_with_permissions.id), data=data)
+    assert response.status_code == 302
+    assert response.location == url_for(
+        'main.manage_users', service_id=service_one['id'], _external=True)
+
+
+def test_confirm_edit_user_mobile_number_page(
+    logged_in_client,
+    active_user_with_permissions,
+    mock_get_users_by_service,
+    service_one,
+    mocker,
+    mock_get_user,
+):
+    new_number = '07554080636'
+    with logged_in_client.session_transaction() as session:
+        session['team_member_mobile_change'] = new_number
+    response = logged_in_client.get(url_for(
+        'main.confirm_edit_user_mobile_number',
+        service_id=service_one['id'],
+        user_id=active_user_with_permissions.id
+    ))
+
+    assert response.status_code == 200
+    assert 'Confirm change of mobile number' in response.get_data(as_text=True)
+    for text in [
+        'New mobile number:',
+        new_number,
+        '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)
+
+
+def test_confirm_edit_user_mobile_number_page_redirects_if_session_empty(
+    logged_in_client,
+    active_user_with_permissions,
+    mock_get_users_by_service,
+    service_one,
+    mocker,
+    mock_get_user,
+):
+    response = logged_in_client.get(url_for(
+        'main.confirm_edit_user_mobile_number',
+        service_id=service_one['id'],
+        user_id=active_user_with_permissions.id
+    ))
+    assert response.status_code == 302
+    assert 'Confirm change of mobile number' not in response.get_data(as_text=True)
+
+
+def test_confirm_edit_user_mobile_number_changes_user_mobile_number(
+    logged_in_client,
+    active_user_with_permissions,
+    mock_get_users_by_service,
+    service_one,
+    mocker,
+    mock_get_user,
+    mock_update_user_attribute
+):
+    new_number = '07554080636'
+    with logged_in_client.session_transaction() as session:
+        session['team_member_mobile_change'] = new_number
+    response = logged_in_client.post(
+        url_for(
+            'main.confirm_edit_user_mobile_number',
+            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, mobile_number=new_number)
+
+
+def test_confirm_edit_user_mobile_number_doesnt_change_user_mobile_for_non_team_member(
+    client_request,
+    mock_get_users_by_service,
+):
+    with client_request.session_transaction() as session:
+        session['team_member_mobile_change'] = '07554080636'
+    client_request.post(
+        'main.confirm_edit_user_mobile_number',
+        service_id=SERVICE_ONE_ID,
+        user_id=USER_ONE_ID,
+        _expected_status=404,
+    )