From e89f89bd11d0d54f29b3a5adea7fe00400f46055 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 2 Nov 2016 10:29:44 +0000 Subject: [PATCH 1/3] remove unused delete service functionality --- app/main/views/service_settings.py | 33 ------- app/notify_client/service_api_client.py | 8 -- tests/app/main/views/test_service_settings.py | 92 ------------------- tests/conftest.py | 9 -- 4 files changed, 142 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index b65f449d0..fc7f2a675 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -184,39 +184,6 @@ 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']) -@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) - - @main.route("/services//service-settings/set-reply-to-email", methods=['GET', 'POST']) @login_required @user_has_permissions('manage_settings', admin_override=True) diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index 17ff797be..b3df8d18f 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -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) diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 60ff51134..5505283e6 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -389,99 +389,11 @@ 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' ]) def test_route_permissions(mocker, app_, api_user_active, service_one, route): with app_.test_request_context(): @@ -504,8 +416,6 @@ 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' ]) def test_route_invalid_permissions(mocker, app_, api_user_active, service_one, route): with app_.test_request_context(): @@ -525,8 +435,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(): diff --git a/tests/conftest.py b/tests/conftest.py index 7dd5c9359..56c712635 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): From a9821448218c068216d6a61d73caa163a696bab4 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 2 Nov 2016 16:53:40 +0000 Subject: [PATCH 2/3] add deactivate service button on the service settings page only visible for platform admins looking at active services. no way to undo. no confirm button. --- app/main/views/service_settings.py | 8 ++ app/notify_client/service_api_client.py | 7 +- app/templates/views/service-settings.html | 7 ++ tests/app/main/views/test_api_keys.py | 65 +++++++------- tests/app/main/views/test_dashboard.py | 24 ++---- tests/app/main/views/test_send.py | 85 +++++++++---------- tests/app/main/views/test_service_settings.py | 54 ++++++++---- 7 files changed, 140 insertions(+), 110 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index fc7f2a675..7dce7adba 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -184,6 +184,14 @@ def service_switch_can_send_letters(service_id): return redirect(url_for('.service_settings', service_id=service_id)) +@main.route("/services//service-settings/deactivate", methods=['GET', 'POST']) +@login_required +@user_has_permissions('manage_settings', admin_override=True) +def deactivate_service(service_id): + service_api_client.deactivate_service(service_id) + return redirect(url_for('.service_settings', service_id=service_id)) + + @main.route("/services//service-settings/set-reply-to-email", methods=['GET', 'POST']) @login_required @user_has_permissions('manage_settings', admin_override=True) diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index b3df8d18f..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): @@ -97,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 3fda72be4..041fe301f 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -98,6 +98,13 @@ {{ 'Stop sending letters' if current_service.can_send_letters else 'Allow to send letters' }} + {% if current_service.active %} +
  • + + Deactivate service + +
    • + {% endif %} {% endif %} diff --git a/tests/app/main/views/test_api_keys.py b/tests/app/main/views/test_api_keys.py index 0de62599f..12462d0de 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 @@ -244,48 +245,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 09a243e8c..1d8b79c24 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 5505283e6..e8e30f01d 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -416,6 +416,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.deactivate_service', ]) def test_route_invalid_permissions(mocker, app_, api_user_active, service_one, route): with app_.test_request_context(): @@ -448,22 +449,23 @@ 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', + 'main.deactivate_service', +]) +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( @@ -748,3 +750,25 @@ 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(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.get(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_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')} From 7a48e25dbb3d027a453b090a2dd6e45c8e9ae6d0 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 8 Nov 2016 14:33:53 +0000 Subject: [PATCH 3/3] flash up prompt when archiving a service same way as we do when deleting templates (also rename button from deactivate -> archive) --- app/main/views/service_settings.py | 8 ++++++-- app/templates/views/service-settings.html | 2 +- tests/app/main/views/test_service_settings.py | 18 +++++++++++++++--- 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 7dce7adba..99c2c70c9 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -188,8 +188,12 @@ def service_switch_can_send_letters(service_id): @login_required @user_has_permissions('manage_settings', admin_override=True) def deactivate_service(service_id): - service_api_client.deactivate_service(service_id) - return redirect(url_for('.service_settings', service_id=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/templates/views/service-settings.html b/app/templates/views/service-settings.html index 041fe301f..08fc368a9 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -101,7 +101,7 @@ {% if current_service.active %}
  • - Deactivate service + Archive service
    • {% endif %} diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index e8e30f01d..f269d2316 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -394,6 +394,7 @@ def test_log_error_on_request_to_go_live( 'main.service_name_change', 'main.service_name_change_confirm', 'main.service_request_to_go_live', + 'main.deactivate_service' ]) def test_route_permissions(mocker, app_, api_user_active, service_one, route): with app_.test_request_context(): @@ -453,7 +454,6 @@ def test_route_for_platform_admin(mocker, app_, platform_admin_user, service_one 'main.service_switch_live', 'main.service_switch_research_mode', 'main.service_switch_can_send_letters', - 'main.deactivate_service', ]) def test_route_for_platform_admin_update_service(mocker, app_, platform_admin_user, service_one, route): mocker.patch('app.service_api_client.deactivate_service') @@ -752,17 +752,29 @@ def test_switch_service_disable_letters(client, platform_admin_user, mocker): assert mocked_fn.call_args == call(service['id'], {"can_send_letters": False}) -def test_deactivate_service(client, platform_admin_user, service_one, mocker): +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.get(url_for('main.deactivate_service', service_id=service_one['id'])) + 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