diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index d80aa747c..97b708291 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -186,37 +186,16 @@ def service_switch_can_send_letters(service_id): return redirect(url_for('.service_settings', service_id=service_id)) -@main.route("/services//service-settings/delete", methods=['GET', 'POST']) +@main.route("/services//service-settings/deactivate", methods=['GET', 'POST']) @login_required @user_has_permissions('manage_settings', admin_override=True) -def service_delete(service_id): - if request.method == 'GET': - return render_template( - 'views/service-settings/delete.html' - ) - elif request.method == 'POST': - return redirect(url_for('.service_delete_confirm', service_id=service_id)) - - -@main.route("/services//service-settings/delete/confirm", methods=['GET', 'POST']) -@login_required -@user_has_permissions('manage_settings', admin_override=True) -def service_delete_confirm(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(): - service_api_client.delete_service(service_id) - return redirect(url_for('.choose_service')) - - return render_template( - 'views/service-settings/confirm.html', - heading='Delete this service from Notify', - destructive=True, - form=form) +def deactivate_service(service_id): + if request.method == 'POST': + service_api_client.deactivate_service(service_id) + return redirect(url_for('.service_settings', service_id=service_id)) + else: + flash('There\'s no way to reverse this! Are you sure you want to archive this service?', 'delete') + return service_settings(service_id) @main.route("/services//service-settings/set-reply-to-email", methods=['GET', 'POST']) diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index 17ff797be..9766d876c 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -1,11 +1,11 @@ from __future__ import unicode_literals from flask import url_for -from notifications_python_client.notifications import NotificationsAPIClient +from notifications_python_client.base import BaseAPIClient from app.utils import BrowsableItem from app.notify_client import _attach_current_user -class ServiceAPIClient(NotificationsAPIClient): +class ServiceAPIClient(BaseAPIClient): # Fudge assert in the super __init__ so # we can set those variables later. def __init__(self): @@ -31,14 +31,6 @@ class ServiceAPIClient(NotificationsAPIClient): data = _attach_current_user(data) return self.post("/service", data)['data']['id'] - def delete_service(self, service_id): - """ - Delete a service. - """ - endpoint = "/service/{0}".format(service_id) - data = _attach_current_user({}) - return self.delete(endpoint, data) - def get_service(self, service_id): return self._get_service(service_id, detailed=False, today_only=False) @@ -105,6 +97,9 @@ class ServiceAPIClient(NotificationsAPIClient): def update_service_with_properties(self, service_id, properties): return self.update_service(service_id, **properties) + def deactivate_service(self, service_id): + return self.post('/service/{}/deactivate'.format(service_id), data=None) + def remove_user_from_service(self, service_id, user_id): """ Remove a user from a service diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index cfe3b9009..76337e5df 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -109,6 +109,13 @@ {{ 'Stop sending letters' if current_service.can_send_letters else 'Allow to send letters' }} + {% if current_service.active %} +
  • + + Archive service + +
    • + {% endif %} {% endif %} diff --git a/tests/app/main/views/test_api_keys.py b/tests/app/main/views/test_api_keys.py index fe45ad2d7..33f3057d9 100644 --- a/tests/app/main/views/test_api_keys.py +++ b/tests/app/main/views/test_api_keys.py @@ -1,6 +1,7 @@ import uuid - from collections import OrderedDict + +import pytest from flask import url_for from bs4 import BeautifulSoup @@ -240,48 +241,50 @@ def test_should_redirect_after_revoking_api_key(app_, mock_get_api_keys.assert_called_once_with(service_id=fake_uuid, key_id=fake_uuid) +@pytest.mark.parametrize('route', [ + 'main.api_keys', + 'main.create_api_key', + 'main.revoke_api_key' +]) def test_route_permissions(mocker, app_, api_user_active, service_one, - mock_get_api_keys): - routes = [ - 'main.api_keys', - 'main.create_api_key', - 'main.revoke_api_key'] + mock_get_api_keys, + route): with app_.test_request_context(): - for route in routes: - validate_route_permission( - mocker, - app_, - "GET", - 200, - url_for(route, service_id=service_one['id'], key_id=123), - ['manage_api_keys'], - api_user_active, - service_one) + validate_route_permission( + mocker, + app_, + "GET", + 200, + url_for(route, service_id=service_one['id'], key_id=123), + ['manage_api_keys'], + api_user_active, + service_one) +@pytest.mark.parametrize('route', [ + 'main.api_keys', + 'main.create_api_key', + 'main.revoke_api_key' +]) def test_route_invalid_permissions(mocker, app_, api_user_active, service_one, - mock_get_api_keys): - routes = [ - 'main.api_keys', - 'main.create_api_key', - 'main.revoke_api_key'] + mock_get_api_keys, + route): with app_.test_request_context(): - for route in routes: - validate_route_permission( - mocker, - app_, - "GET", - 403, - url_for(route, service_id=service_one['id'], key_id=123), - ['view_activity'], - api_user_active, - service_one) + validate_route_permission( + mocker, + app_, + "GET", + 403, + url_for(route, service_id=service_one['id'], key_id=123), + ['view_activity'], + api_user_active, + service_one) def test_should_show_whitelist_page( diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index f0e1cacfc..3f9672a53 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -440,22 +440,16 @@ def test_route_for_service_permissions(mocker, mock_get_template_statistics, mock_get_detailed_service, mock_get_usage): - routes = [ - 'main.service_dashboard'] with app_.test_request_context(): - # Just test that the user is part of the service - for route in routes: - validate_route_permission( - mocker, - app_, - "GET", - 200, - url_for( - route, - service_id=service_one['id']), - ['view_activity'], - api_user_active, - service_one) + validate_route_permission( + mocker, + app_, + "GET", + 200, + url_for('main.service_dashboard', service_id=service_one['id']), + ['view_activity'], + api_user_active, + service_one) def test_aggregate_template_stats(): diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index fbcd64280..5c7736972 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -1,12 +1,14 @@ -import pytest -import re -from itertools import repeat from io import BytesIO from os import path from glob import glob -from bs4 import BeautifulSoup +import re +from itertools import repeat from functools import partial + +import pytest +from bs4 import BeautifulSoup from flask import url_for + from tests import validate_route_permission template_types = ['email', 'sms'] @@ -447,6 +449,12 @@ def test_check_messages_should_revalidate_file_when_uploading_file( assert 'There is a problem with your data' in response.get_data(as_text=True) +@pytest.mark.parametrize('route, response_code', [ + ('main.choose_template', 200), + ('main.send_messages', 200), + ('main.get_example_csv', 200), + ('main.send_test', 302) +]) def test_route_permissions(mocker, app_, api_user_active, @@ -457,35 +465,17 @@ def test_route_permissions(mocker, mock_get_notifications, mock_create_job, mock_s3_upload, - fake_uuid): - routes = [ - 'main.choose_template', - 'main.send_messages', - 'main.get_example_csv'] - with app_.test_request_context(): - for route in routes: - validate_route_permission( - mocker, - app_, - "GET", - 200, - url_for( - route, - service_id=service_one['id'], - template_type='sms', - template_id=fake_uuid), - ['send_texts', 'send_emails', 'send_letters'], - api_user_active, - service_one) - + fake_uuid, + route, + response_code): with app_.test_request_context(): validate_route_permission( mocker, app_, "GET", - 302, + response_code, url_for( - 'main.send_test', + route, service_id=service_one['id'], template_type='sms', template_id=fake_uuid), @@ -494,6 +484,12 @@ def test_route_permissions(mocker, service_one) +@pytest.mark.parametrize('route', [ + 'main.choose_template', + 'main.send_messages', + 'main.get_example_csv', + 'main.send_test' +]) def test_route_invalid_permissions(mocker, app_, api_user_active, @@ -503,27 +499,22 @@ def test_route_invalid_permissions(mocker, mock_get_jobs, mock_get_notifications, mock_create_job, - fake_uuid): - routes = [ - 'main.choose_template', - 'main.send_messages', - 'main.get_example_csv', - 'main.send_test'] + fake_uuid, + route): with app_.test_request_context(): - for route in routes: - validate_route_permission( - mocker, - app_, - "GET", - 403, - url_for( - route, - service_id=service_one['id'], - template_type='sms', - template_id=fake_uuid), - ['blah'], - api_user_active, - service_one) + validate_route_permission( + mocker, + app_, + "GET", + 403, + url_for( + route, + service_id=service_one['id'], + template_type='sms', + template_id=fake_uuid), + ['blah'], + api_user_active, + service_one) def test_route_choose_template_manage_service_permissions(mocker, diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 309dddcee..ae0484b28 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -391,99 +391,12 @@ def test_log_error_on_request_to_go_live( ) -def test_should_show_delete_page(app_, - api_user_active, - mock_login, - mock_get_service, - mock_get_user, - mock_get_user_by_email, - mock_has_permissions, - fake_uuid): - with app_.test_request_context(): - with app_.test_client() as client: - client.login(api_user_active) - service_id = fake_uuid - response = client.get(url_for( - 'main.service_delete', service_id=service_id)) - - assert response.status_code == 200 - assert 'Delete this service from GOV.UK Notify' in response.get_data(as_text=True) - assert mock_get_service.called - - -def test_should_show_redirect_after_deleting_service(app_, - api_user_active, - mock_get_service, - mock_get_user, - mock_get_user_by_email, - mock_login, - mock_has_permissions, - fake_uuid): - with app_.test_request_context(): - with app_.test_client() as client: - client.login(api_user_active) - service_id = fake_uuid - response = client.post(url_for( - 'main.service_delete', service_id=service_id)) - - assert response.status_code == 302 - delete_url = url_for( - 'main.service_delete_confirm', service_id=service_id, _external=True) - assert delete_url == response.location - - -def test_should_show_delete_confirmation(app_, - api_user_active, - mock_get_service, - mock_get_user, - mock_get_user_by_email, - mock_login, - mock_has_permissions, - fake_uuid): - with app_.test_request_context(): - with app_.test_client() as client: - client.login(api_user_active) - service_id = fake_uuid - response = client.get(url_for( - 'main.service_delete_confirm', service_id=service_id)) - - assert response.status_code == 200 - assert 'Delete this service from Notify' in response.get_data(as_text=True) - assert mock_get_service.called - - -def test_should_redirect_delete_confirmation(app_, - api_user_active, - mock_get_service, - mock_delete_service, - mock_get_user, - mock_get_user_by_email, - mock_login, - mock_verify_password, - mock_has_permissions, - fake_uuid): - with app_.test_request_context(): - with app_.test_client() as client: - client.login(api_user_active) - service_id = fake_uuid - response = client.post(url_for( - 'main.service_delete_confirm', service_id=service_id)) - - assert response.status_code == 302 - choose_url = url_for( - 'main.choose_service', _external=True) - assert choose_url == response.location - assert mock_get_service.called - assert mock_delete_service.called - - @pytest.mark.parametrize('route', [ 'main.service_settings', 'main.service_name_change', 'main.service_name_change_confirm', 'main.service_request_to_go_live', - 'main.service_delete', - 'main.service_delete_confirm' + 'main.deactivate_service' ]) def test_route_permissions(mocker, app_, api_user_active, service_one, route): with app_.test_request_context(): @@ -506,8 +419,7 @@ def test_route_permissions(mocker, app_, api_user_active, service_one, route): 'main.service_switch_live', 'main.service_switch_research_mode', 'main.service_switch_can_send_letters', - 'main.service_delete', - 'main.service_delete_confirm' + 'main.deactivate_service', ]) def test_route_invalid_permissions(mocker, app_, api_user_active, service_one, route): with app_.test_request_context(): @@ -527,8 +439,6 @@ def test_route_invalid_permissions(mocker, app_, api_user_active, service_one, r 'main.service_name_change', 'main.service_name_change_confirm', 'main.service_request_to_go_live', - 'main.service_delete', - 'main.service_delete_confirm' ]) def test_route_for_platform_admin(mocker, app_, platform_admin_user, service_one, route): with app_.test_request_context(): @@ -542,22 +452,22 @@ def test_route_for_platform_admin(mocker, app_, platform_admin_user, service_one service_one) -def test_route_for_platform_admin_update_service(mocker, app_, platform_admin_user, service_one): - routes = [ - 'main.service_switch_live', - 'main.service_switch_research_mode', - 'main.service_switch_can_send_letters' - ] +@pytest.mark.parametrize('route', [ + 'main.service_switch_live', + 'main.service_switch_research_mode', + 'main.service_switch_can_send_letters', +]) +def test_route_for_platform_admin_update_service(mocker, app_, platform_admin_user, service_one, route): + mocker.patch('app.service_api_client.deactivate_service') with app_.test_request_context(): - for route in routes: - validate_route_permission(mocker, - app_, - "GET", - 302, - url_for(route, service_id=service_one['id']), - [], - platform_admin_user, - service_one) + validate_route_permission(mocker, + app_, + "GET", + 302, + url_for(route, service_id=service_one['id']), + [], + platform_admin_user, + service_one) def test_set_reply_to_email_address( @@ -842,3 +752,37 @@ def test_switch_service_disable_letters(client, platform_admin_user, mocker): assert response.status_code == 302 assert response.location == url_for('main.service_settings', service_id=service['id'], _external=True) assert mocked_fn.call_args == call(service['id'], {"can_send_letters": False}) + + +def test_deactivate_service_after_confirm(client, platform_admin_user, service_one, mocker): + mocked_fn = mocker.patch('app.service_api_client.post', return_value=service_one) + + client.login(platform_admin_user, mocker, service_one) + response = client.post(url_for('main.deactivate_service', service_id=service_one['id'])) + + assert response.status_code == 302 + assert response.location == url_for('main.service_settings', service_id=service_one['id'], _external=True) + assert mocked_fn.call_args == call('/service/{}/deactivate'.format(service_one['id']), data=None) + + +def test_deactivate_service_prompts_user(client, platform_admin_user, service_one, mocker): + mocked_fn = mocker.patch('app.service_api_client.post') + + client.login(platform_admin_user, mocker, service_one) + response = client.get(url_for('main.deactivate_service', service_id=service_one['id'])) + + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert 'Are you sure you want to archive this service?' in page.find('div', class_='banner-dangerous').text + assert mocked_fn.called is False + + +def test_cant_deactivate_inactive_service(client, platform_admin_user, service_one, mocker): + service_one['active'] = False + + client.login(platform_admin_user, mocker, service_one) + response = client.get(url_for('main.service_settings', service_id=service_one['id'])) + + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert 'Deactivate service' not in {a.text for a in page.find_all('a', class_='button')} diff --git a/tests/conftest.py b/tests/conftest.py index 0b115a1b3..d526b06e6 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -230,15 +230,6 @@ def mock_get_services_with_one_service(mocker, fake_uuid, user=None): 'app.service_api_client.get_services', side_effect=_create) -@pytest.fixture(scope='function') -def mock_delete_service(mocker, mock_get_service): - def _delete(service_id): - return mock_get_service.side_effect(service_id) - - return mocker.patch( - 'app.service_api_client.delete_service', side_effect=_delete) - - @pytest.fixture(scope='function') def mock_get_service_template(mocker): def _get(service_id, template_id, version=None):