From cd1fe0640c8ece975ac43d8ea7d1fdca23cdd7fb Mon Sep 17 00:00:00 2001
From: Ben Thorner <ben.thorner@digital.cabinet-office.gov.uk>
Date: Thu, 8 Jul 2021 15:56:58 +0100
Subject: [PATCH] Improve coverage of suspend service tests

Previously these only tested with a Platform Admin user, but service
admins can suspend a service too. I've rewritten the tests to match
the 'archive_service' ones, which use the client_request fixture to
make changing the user easier.

Note that the return value of the service API client wasn't used for
anything, so it's safe to remove it from the mock.
---
 tests/app/main/views/test_service_settings.py | 47 ++++++++++++-------
 1 file changed, 31 insertions(+), 16 deletions(-)

diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py
index 3a9004d46..4078c4e43 100644
--- a/tests/app/main/views/test_service_settings.py
+++ b/tests/app/main/views/test_service_settings.py
@@ -4205,26 +4205,42 @@ def test_cant_archive_inactive_service(
     assert 'Delete service' not in {a.text for a in page.find_all('a', class_='button')}
 
 
+@pytest.mark.parametrize('user', (
+    create_platform_admin_user(),
+    create_active_user_with_permissions(),
+    pytest.param(create_active_user_no_settings_permission(), marks=pytest.mark.xfail),
+))
 def test_suspend_service_after_confirm(
-    platform_admin_client,
-    api_user_active,
-    service_one,
+    client_request,
+    user,
     mocker,
-    mock_get_inbound_number_for_service,
 ):
-    mock_api = mocker.patch('app.service_api_client.post', return_value=service_one)
+    mock_api = mocker.patch('app.service_api_client.post')
     mock_event = mocker.patch('app.main.views.service_settings.create_suspend_service_event')
 
-    response = platform_admin_client.post(url_for('main.suspend_service', service_id=service_one['id']))
+    client_request.login(user)
+    client_request.post(
+        'main.suspend_service',
+        service_id=SERVICE_ONE_ID,
+        _expected_redirect=url_for(
+            'main.service_settings',
+            service_id=SERVICE_ONE_ID,
+            _external=True
+        ),
+    )
 
-    assert response.status_code == 302
-    assert response.location == url_for('main.service_settings', service_id=service_one['id'], _external=True)
-    mock_api.assert_called_once_with('/service/{}/suspend'.format(service_one['id']), data=None)
-    mock_event.assert_called_once_with(service_one['id'], suspended_by_id=api_user_active['id'])
+    mock_api.assert_called_once_with('/service/{}/suspend'.format(SERVICE_ONE_ID), data=None)
+    mock_event.assert_called_once_with(SERVICE_ONE_ID, suspended_by_id=user['id'])
 
 
+@pytest.mark.parametrize('user', (
+    create_platform_admin_user(),
+    create_active_user_with_permissions(),
+    pytest.param(create_active_user_no_settings_permission(), marks=pytest.mark.xfail),
+))
 def test_suspend_service_prompts_user(
-    platform_admin_client,
+    client_request,
+    user,
     service_one,
     mocker,
     single_reply_to_email_address,
@@ -4232,15 +4248,14 @@ def test_suspend_service_prompts_user(
     single_sms_sender,
     mock_get_service_settings_page_common,
 ):
-    mocked_fn = mocker.patch('app.service_api_client.post')
+    mock_api = mocker.patch('app.service_api_client.post')
 
-    response = platform_admin_client.get(url_for('main.suspend_service', service_id=service_one['id']))
+    client_request.login(user)
+    page = client_request.get('main.suspend_service', service_id=service_one['id'])
 
-    assert response.status_code == 200
-    page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
     assert 'This will suspend the service and revoke all api keys. Are you sure you want to suspend this service?' in \
            page.find('div', class_='banner-dangerous').text
-    assert mocked_fn.called is False
+    assert mock_api.called is False
 
 
 def test_cant_suspend_inactive_service(