Remove re-enter password step from rename service

The original idea behind was to always ask users to re-enter their
password any time:
- we want them to be sure that they want to do what they’re about to do
- we want to be sure it’s really the user trying to do the thing (and
  not someone malicious)

In reality we:
- removed this from the initial place it was added (a descendent of the
  ‘suspend service’ feature)
- only ever added it to the ‘rename service’ feature

So in reality it’s not a pattern we have persisted with. Arguably there
are several things you can now do in the admin app without re-entering
your password which are much more high consequence than changing the
service name.

Also, with browser autofill there’s a lot less chance that forcing
someone to re-enter a password really gives much defence against an
unatteneded laptop, for example.

I also wonder whether we might get people to give better service names
if we make the process of renaming the service less intimidating.

So this commit removes the need to re-enter your password when renaming
a service.

Note that re-naming an organisation still has the same check, but I
haven’t removed that too for the sake of keeping scope of the PR small.

app/main/views/service_settings.py

@@ -9,7 +9,6 @@ from flask import (
     redirect,
     render_template,
     request,
-    session,
     url_for,
 )
 from flask_login import current_user
@@ -28,7 +27,6 @@ from app import (
     notification_api_client,
     organisations_client,
     service_api_client,
-    user_api_client,
 )
 from app.event_handlers import (
     create_archive_service_event,
@@ -42,7 +40,6 @@ from app.main import main
 from app.main.forms import (
     BillingDetailsForm,
     BrandingOptions,
-    ConfirmPasswordForm,
     EditNotesForm,
     EstimateUsageForm,
     FreeSMSAllowance,
@@ -95,24 +92,28 @@ def service_settings(service_id):
 @main.route("/services/<uuid:service_id>/service-settings/name", methods=['GET', 'POST'])
 @user_has_permissions('manage_service')
 def service_name_change(service_id):
-    form = RenameServiceForm()
-
-    if request.method == 'GET':
-        form.name.data = current_service.name
+    form = RenameServiceForm(name=current_service.name)
 
     if form.validate_on_submit():
 
         if form.name.data == current_service.name:
             return redirect(url_for('.service_settings', service_id=service_id))
 
-        unique_name = service_api_client.is_service_name_unique(service_id, form.name.data, email_safe(form.name.data))
-
-        if not unique_name:
-            form.name.errors.append("This service name is already in use")
-            return render_template('views/service-settings/name.html', form=form)
-
-        session['service_name_change'] = form.name.data
-        return redirect(url_for('.service_name_change_confirm', service_id=service_id))
+        try:
+            current_service.update(
+                name=form.name.data,
+                email_from=email_safe(form.name.data),
+            )
+        except HTTPError as http_error:
+            if http_error.status_code == 400 and any(
+                name_error_message.startswith('Duplicate service name')
+                for name_error_message in http_error.message['name']
+            ):
+                form.name.errors.append('This service name is already in use')
+            else:
+                raise http_error
+        else:
+            return redirect(url_for('.service_settings', service_id=service_id))
 
     if current_service.organisation_type == 'local':
         return render_template(
@@ -126,42 +127,6 @@ def service_name_change(service_id):
     )
 
 
-@main.route("/services/<uuid:service_id>/service-settings/name/confirm", methods=['GET', 'POST'])
-@user_has_permissions('manage_service')
-def service_name_change_confirm(service_id):
-    if 'service_name_change' not in session:
-        flash("The change you made was not saved. Please try again.", 'error')
-        return redirect(url_for('main.service_name_change', service_id=service_id))
-
-    # Validate password for form
-    def _check_password(pwd):
-        return user_api_client.verify_password(current_user.id, pwd)
-
-    form = ConfirmPasswordForm(_check_password)
-
-    if form.validate_on_submit():
-        try:
-            current_service.update(
-                name=session['service_name_change'],
-                email_from=email_safe(session['service_name_change'])
-            )
-        except HTTPError as e:
-            error_msg = "Duplicate service name '{}'".format(session['service_name_change'])
-            if e.status_code == 400 and error_msg in e.message['name']:
-                # Redirect the user back to the change service name screen
-                flash('This service name is already in use', 'error')
-                return redirect(url_for('main.service_name_change', service_id=service_id))
-            else:
-                raise e
-        else:
-            session.pop('service_name_change')
-            return redirect(url_for('.service_settings', service_id=service_id))
-    return render_template(
-        'views/service-settings/confirm.html',
-        heading='Change your service name',
-        form=form)
-
-
 @main.route("/services/<uuid:service_id>/service-settings/request-to-go-live/estimate-usage", methods=['GET', 'POST'])
 @user_has_permissions('manage_service')
 def estimate_usage(service_id):

app/navigation.py

@@ -239,7 +239,6 @@ class MainNavigation(Navigation):
             'service_letter_contact_details',
             'service_make_blank_default_letter_contact',
             'service_name_change',
-            'service_name_change_confirm',
             'service_preview_email_branding',
             'service_preview_letter_branding',
             'service_set_auth_type',

tests/app/main/views/test_service_settings.py

@@ -15,7 +15,6 @@ from notifications_utils.clients.zendesk.zendesk_client import (
 )
 
 import app
-from app.formatters import email_safe
 from tests import (
     find_element_by_tag_and_partial_text,
     invite_json,
@@ -542,26 +541,6 @@ def test_should_show_service_name_with_no_prefixing(
     ).text == 'Your service name should tell users what the message is about as well as who it’s from.'
 
 
-def test_should_redirect_after_change_service_name(
-    client_request,
-    mock_update_service,
-    mock_service_name_is_unique,
-):
-    client_request.post(
-        'main.service_name_change',
-        service_id=SERVICE_ONE_ID,
-        _data={'name': "new name"},
-        _expected_status=302,
-        _expected_redirect=url_for(
-            'main.service_name_change_confirm',
-            service_id=SERVICE_ONE_ID,
-            _external=True,
-        )
-    )
-
-    assert mock_service_name_is_unique.called is True
-
-
 def test_should_not_hit_api_if_service_name_hasnt_changed(
     client_request,
     mock_update_service,
@@ -578,7 +557,6 @@ def test_should_not_hit_api_if_service_name_hasnt_changed(
             _external=True,
         ),
     )
-    assert not mock_service_name_is_unique.called
     assert not mock_update_service.called
 
 
@@ -590,7 +568,6 @@ def test_should_not_hit_api_if_service_name_hasnt_changed(
 def test_service_name_change_fails_if_new_name_fails_validation(
     client_request,
     mock_update_service,
-    mock_service_name_is_unique,
     name,
     error_message,
 ):
@@ -600,7 +577,6 @@ def test_service_name_change_fails_if_new_name_fails_validation(
         _data={'name': name},
         _expected_status=200,
     )
-    assert not mock_service_name_is_unique.called
     assert not mock_update_service.called
     assert error_message in page.find("span", {"class": "govuk-error-message"}).text
 
@@ -796,9 +772,9 @@ def test_switch_service_to_count_as_live(
     )
 
 
-def test_should_not_allow_duplicate_names(
+def test_should_not_allow_duplicate_service_names(
     client_request,
-    mock_service_name_is_not_unique,
+    mock_update_service_raise_httperror_duplicate_name,
     service_one,
 ):
     page = client_request.post(
@@ -809,39 +785,18 @@ def test_should_not_allow_duplicate_names(
     )
 
     assert 'This service name is already in use' in page.text
-    app.service_api_client.is_service_name_unique.assert_called_once_with(
-        SERVICE_ONE_ID,
-        'SErvICE TWO',
-        'service.two',
-    )
-
-
-def test_should_show_service_name_confirmation(
-    client_request,
-):
-    service_new_name = 'New Name'
-    with client_request.session_transaction() as session:
-        session['service_name_change'] = service_new_name
-    page = client_request.get(
-        'main.service_name_change_confirm',
-        service_id=SERVICE_ONE_ID,
-    )
-    assert 'Change your service name' in page.text
-    app.service_api_client.get_service.assert_called_with(SERVICE_ONE_ID)
 
 
 def test_should_redirect_after_service_name_confirmation(
     client_request,
     mock_update_service,
-    mock_verify_password,
-    mock_get_inbound_number_for_service,
 ):
-    service_new_name = 'New Name'
-    with client_request.session_transaction() as session:
-        session['service_name_change'] = service_new_name
     client_request.post(
-        'main.service_name_change_confirm',
+        'main.service_name_change',
         service_id=SERVICE_ONE_ID,
+        _data={
+            'name': 'New Name'
+        },
         _expected_status=302,
         _expected_redirect=url_for(
             'main.service_settings',
@@ -852,47 +807,9 @@ def test_should_redirect_after_service_name_confirmation(
 
     mock_update_service.assert_called_once_with(
         SERVICE_ONE_ID,
-        name=service_new_name,
-        email_from=email_safe(service_new_name)
+        name='New Name',
+        email_from='new.name',
     )
-    assert mock_verify_password.called is True
-
-
-def test_should_raise_duplicate_name_handled(
-    client_request,
-    mock_update_service_raise_httperror_duplicate_name,
-    mock_verify_password,
-):
-    with client_request.session_transaction() as session:
-        session['service_name_change'] = 'New Name'
-
-    client_request.post(
-        'main.service_name_change_confirm',
-        service_id=SERVICE_ONE_ID,
-        _expected_status=302,
-        _expected_redirect=url_for(
-            'main.service_name_change',
-            service_id=SERVICE_ONE_ID,
-            _external=True,
-        ),
-    )
-
-    assert mock_update_service_raise_httperror_duplicate_name.called
-    assert mock_verify_password.called
-
-
-def test_service_name_change_confirm_handles_expired_session(
-    client_request, mock_verify_password, mock_update_service
-):
-    page = client_request.post(
-        'main.service_name_change_confirm',
-        service_id=SERVICE_ONE_ID,
-        _follow_redirects=True
-    )
-    assert mock_verify_password.called is False
-    assert mock_update_service.called is False
-
-    assert page.find('div', 'banner-dangerous').text.strip() == "The change you made was not saved. Please try again."
 
 
 @pytest.mark.parametrize('volumes, consent_to_research, expected_estimated_volumes_item', [
@@ -2092,7 +2009,6 @@ def test_ready_to_go_live(
 @pytest.mark.parametrize('route', [
     'main.service_settings',
     'main.service_name_change',
-    'main.service_name_change_confirm',
     'main.request_to_go_live',
     'main.submit_request_to_go_live',
     'main.archive_service'
@@ -2127,7 +2043,6 @@ def test_route_permissions(
 @pytest.mark.parametrize('route', [
     'main.service_settings',
     'main.service_name_change',
-    'main.service_name_change_confirm',
     'main.request_to_go_live',
     'main.submit_request_to_go_live',
     'main.service_switch_live',
@@ -2157,7 +2072,6 @@ def test_route_invalid_permissions(
 @pytest.mark.parametrize('route', [
     'main.service_settings',
     'main.service_name_change',
-    'main.service_name_change_confirm',
     'main.request_to_go_live',
     'main.submit_request_to_go_live',
 ])

tests/app/test_navigation.py

@@ -244,7 +244,6 @@ EXCLUDED_ENDPOINTS = tuple(map(Navigation.get_endpoint_with_blueprint, {
     'service_letter_contact_details',
     'service_make_blank_default_letter_contact',
     'service_name_change',
-    'service_name_change_confirm',
     'service_preview_email_branding',
     'service_preview_letter_branding',
     'service_set_auth_type',