From 87556687abdcf6d11899f2888b6c9b07c828ecea Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 30 Jan 2017 16:32:44 +0000 Subject: [PATCH 1/3] This is the first PR in the story to create a suspend service feature. - Renaming /service//deactivate to /service//archive to match language on the UI. - Will need to update admin before deleting the deactive service method - Created dao and endpoint methods to suspend and resume a service. - I confirm the use of suspend and resume with a couple people on the team, seems to be the right choice. The idea is that if you archive a service there is no coming back from that. To suspend a service is marking it as inactive and revoking the api keys. To resume a service is to mark the service as active, the service will need to create new API keys. It makes sense that if a service is under threat that the API keys should be renewed. The next PR will update the code to check that the service is active before sending the request or allowing any actions by the service. --- app/dao/services_dao.py | 26 +++- app/service/rest.py | 66 +++++++++- tests/app/dao/test_services_dao.py | 27 +++- tests/app/service/test_archived_service.py | 123 ++++++++++++++++++ tests/app/service/test_deactivate.py | 4 +- .../service/test_suspend_resume_service.py | 72 ++++++++++ 6 files changed, 309 insertions(+), 9 deletions(-) create mode 100644 tests/app/service/test_archived_service.py create mode 100644 tests/app/service/test_suspend_resume_service.py diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index aaefec145..eb53c769c 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -73,7 +73,7 @@ def dao_fetch_all_services_by_user(user_id, only_active=False): @version_class(Service) @version_class(Template, TemplateHistory) @version_class(ApiKey) -def dao_deactive_service(service_id): +def dao_archive_service(service_id): # have to eager load templates and api keys so that we don't flush when we loop through them # to ensure that db.session still contains the models when it comes to creating history objects service = Service.query.options( @@ -291,3 +291,27 @@ def fetch_stats_by_date_range_for_all_services(start_date, end_date, include_fro query = query.filter(NotificationHistory.key_type != KEY_TYPE_TEST) return query.all() + + +@transactional +@version_class(Service) +@version_class(ApiKey) +def dao_suspend_service(service_id): + # have to eager load api keys so that we don't flush when we loop through them + # to ensure that db.session still contains the models when it comes to creating history objects + service = Service.query.options( + joinedload('api_keys'), + ).filter(Service.id == service_id).one() + + service.active = False + + for api_key in service.api_keys: + if not api_key.expiry_date: + api_key.expiry_date = datetime.utcnow() + + +@transactional +@version_class(Service) +def dao_resume_service(service_id): + service = Service.query.get(service_id) + service.active = True diff --git a/app/service/rest.py b/app/service/rest.py index 905408a3b..138b74f39 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -27,9 +27,10 @@ from app.dao.services_dao import ( dao_fetch_todays_stats_for_service, dao_fetch_weekly_historical_stats_for_service, dao_fetch_todays_stats_for_all_services, - dao_deactive_service, - fetch_stats_by_date_range_for_all_services -) + dao_archive_service, + fetch_stats_by_date_range_for_all_services, + dao_suspend_service, + dao_resume_service) from app.dao.service_whitelist_dao import ( dao_fetch_service_whitelist, dao_add_and_commit_whitelisted_contacts, @@ -335,6 +336,7 @@ def update_whitelist(service_id): return '', 204 +# Renaming this endpoint to archive @service_blueprint.route('//deactivate', methods=['POST']) def deactivate_service(service_id): service = dao_fetch_service_by_id(service_id) @@ -343,7 +345,63 @@ def deactivate_service(service_id): # assume already inactive, don't change service name return '', 204 - dao_deactive_service(service.id) + dao_archive_service(service.id) + + return '', 204 + + +@service_blueprint.route('//archive', methods=['POST']) +def archive_service(service_id): + """ + When a service is archived the service is made inactive, templates are archived and api keys are revoked. + There is no coming back from this operation. + :param service_id: + :return: + """ + service = dao_fetch_service_by_id(service_id) + + if not service.active: + # assume already inactive, don't change service name + return '', 204 + + dao_archive_service(service.id) + + return '', 204 + + +@service_blueprint.route('//suspend', methods=['POST']) +def suspend_service(service_id): + """ + Suspending a service will mark the service as inactive and revoke API keys. + :param service_id: + :return: + """ + service = dao_fetch_service_by_id(service_id) + + if not service.active: + # assume already inactive, don't change service name + return '', 204 + + dao_suspend_service(service.id) + + return '', 204 + + +@service_blueprint.route('//resume', methods=['POST']) +def resume_service(service_id): + """ + Resuming a service that has been suspended will mark the service as active. + The service will need to re-create API keys + :param service_id: + :return: + """ + service = dao_fetch_service_by_id(service_id) + + if service.active: + # assume already inactive, don't change service name + return '', 204 + + dao_resume_service(service.id) return '', 204 diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index a3f5d1de7..5fb2c01d9 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -22,8 +22,9 @@ from app.dao.services_dao import ( dao_fetch_weekly_historical_stats_for_service, fetch_todays_total_message_count, dao_fetch_todays_stats_for_all_services, - fetch_stats_by_date_range_for_all_services -) + fetch_stats_by_date_range_for_all_services, + dao_suspend_service, + dao_resume_service) from app.dao.users_dao import save_model_user from app.models import ( NotificationStatistics, @@ -656,3 +657,25 @@ def test_fetch_stats_by_date_range_for_all_services(notify_db, notify_db_session assert len(results) == 1 assert results[0] == ('sms', 'created', result_one.service_id, 2) + + +def test_dao_suspend_service_marks_service_as_inactive_and_expires_api_keys(sample_service, sample_api_key): + dao_suspend_service(sample_service.id) + service = Service.query.get(sample_service.id) + assert not service.active + assert service.name == sample_service.name + + api_key = ApiKey.query.get(sample_api_key.id) + assert api_key.expiry_date.date() == datetime.utcnow().date() + + +def test_dao_resume_service_marks_service_as_active_and_api_keys_are_still_revoked(sample_service, sample_api_key): + dao_suspend_service(sample_service.id) + service = Service.query.get(sample_service.id) + assert not service.active + + dao_resume_service(service.id) + assert Service.query.get(service.id).active + + api_key = ApiKey.query.get(sample_api_key.id) + assert api_key.expiry_date.date() == datetime.utcnow().date() diff --git a/tests/app/service/test_archived_service.py b/tests/app/service/test_archived_service.py new file mode 100644 index 000000000..b06e7ba01 --- /dev/null +++ b/tests/app/service/test_archived_service.py @@ -0,0 +1,123 @@ +import uuid +from datetime import datetime + +import pytest +from freezegun import freeze_time + +from app import db +from app.models import Service +from app.dao.services_dao import dao_archive_service +from app.dao.api_key_dao import expire_api_key +from app.dao.templates_dao import dao_update_template + +from tests import create_authorization_header, unwrap_function +from tests.app.conftest import ( + sample_template as create_template, + sample_api_key as create_api_key +) + + +def test_archive_only_allows_post(client): + auth_header = create_authorization_header() + response = client.get('/service/{}/archive'.format(uuid.uuid4()), headers=[auth_header]) + assert response.status_code == 405 + + +def test_archive_service_errors_with_bad_service_id(client): + auth_header = create_authorization_header() + response = client.post('/service/{}/archive'.format(uuid.uuid4()), headers=[auth_header]) + assert response.status_code == 404 + + +def test_deactivating_inactive_service_does_nothing(client, sample_service): + auth_header = create_authorization_header() + sample_service.active = False + response = client.post('/service/{}/archive'.format(sample_service.id), headers=[auth_header]) + assert response.status_code == 204 + assert sample_service.name == 'Sample service' + + +@pytest.fixture +def archived_service(client, notify_db, notify_db_session, sample_service): + create_template(notify_db, notify_db_session, template_name='a') + create_template(notify_db, notify_db_session, template_name='b') + create_api_key(notify_db, notify_db_session) + create_api_key(notify_db, notify_db_session) + + auth_header = create_authorization_header() + response = client.post('/service/{}/archive'.format(sample_service.id), headers=[auth_header]) + assert response.status_code == 204 + assert response.data == b'' + return sample_service + + +def test_deactivating_service_changes_name_and_email(archived_service): + assert archived_service.name == '_archived_Sample service' + assert archived_service.email_from == '_archived_sample.service' + + +def test_deactivating_service_revokes_api_keys(archived_service): + assert len(archived_service.api_keys) == 2 + for key in archived_service.api_keys: + assert key.expiry_date is not None + assert key.version == 2 + + +def test_deactivating_service_archives_templates(archived_service): + assert len(archived_service.templates) == 2 + for template in archived_service.templates: + assert template.archived is True + assert template.version == 2 + + +def test_deactivating_service_creates_history(archived_service): + ServiceHistory = Service.get_history_model() + history = ServiceHistory.query.filter_by( + id=archived_service.id + ).order_by( + ServiceHistory.version.desc() + ).first() + + assert history.version == 2 + assert history.active is False + + +@pytest.fixture +def archived_service_with_deleted_stuff(client, notify_db, notify_db_session, sample_service): + with freeze_time('2001-01-01'): + template = create_template(notify_db, notify_db_session, template_name='a') + api_key = create_api_key(notify_db, notify_db_session) + + expire_api_key(sample_service.id, api_key.id) + + template.archived = True + dao_update_template(template) + + with freeze_time('2002-02-02'): + auth_header = create_authorization_header() + response = client.post('/service/{}/archive'.format(sample_service.id), headers=[auth_header]) + + assert response.status_code == 204 + assert response.data == b'' + return sample_service + + +def test_deactivating_service_doesnt_affect_existing_archived_templates(archived_service_with_deleted_stuff): + assert archived_service_with_deleted_stuff.templates[0].archived is True + assert archived_service_with_deleted_stuff.templates[0].updated_at == datetime(2001, 1, 1, 0, 0, 0) + assert archived_service_with_deleted_stuff.templates[0].version == 2 + + +def test_deactivating_service_doesnt_affect_existing_revoked_api_keys(archived_service_with_deleted_stuff): + assert archived_service_with_deleted_stuff.api_keys[0].expiry_date == datetime(2001, 1, 1, 0, 0, 0) + assert archived_service_with_deleted_stuff.api_keys[0].version == 2 + + +def test_deactivating_service_rolls_back_everything_on_error(sample_service, sample_api_key, sample_template): + unwrapped_deactive_service = unwrap_function(dao_archive_service) + + unwrapped_deactive_service(sample_service.id) + + assert sample_service in db.session.dirty + assert sample_api_key in db.session.dirty + assert sample_template in db.session.dirty diff --git a/tests/app/service/test_deactivate.py b/tests/app/service/test_deactivate.py index 2ed2c01a6..7ef20d8e8 100644 --- a/tests/app/service/test_deactivate.py +++ b/tests/app/service/test_deactivate.py @@ -6,7 +6,7 @@ from freezegun import freeze_time from app import db from app.models import Service -from app.dao.services_dao import dao_deactive_service +from app.dao.services_dao import dao_archive_service from app.dao.api_key_dao import expire_api_key from app.dao.templates_dao import dao_update_template @@ -114,7 +114,7 @@ def test_deactivating_service_doesnt_affect_existing_revoked_api_keys(deactivate def test_deactivating_service_rolls_back_everything_on_error(sample_service, sample_api_key, sample_template): - unwrapped_deactive_service = unwrap_function(dao_deactive_service) + unwrapped_deactive_service = unwrap_function(dao_archive_service) unwrapped_deactive_service(sample_service.id) diff --git a/tests/app/service/test_suspend_resume_service.py b/tests/app/service/test_suspend_resume_service.py new file mode 100644 index 000000000..12f1b9239 --- /dev/null +++ b/tests/app/service/test_suspend_resume_service.py @@ -0,0 +1,72 @@ +from datetime import datetime + +import pytest + +import uuid + +from app.models import Service +from tests import create_authorization_header + + +@pytest.mark.parametrize("endpoint", ["suspend", "resume"]) +def test_only_allows_post(client, endpoint): + auth_header = create_authorization_header() + response = client.get("/service/{}/{}".format(uuid.uuid4(), endpoint), + headers=[auth_header]) + assert response.status_code == 405 + + +@pytest.mark.parametrize("endpoint", ["suspend", "resume"]) +def test_returns_404_when_service_does_not_exist(client, endpoint): + auth_header = create_authorization_header() + response = client.post("/service/{}/{}".format(uuid.uuid4(), endpoint), + headers=[auth_header]) + assert response.status_code == 404 + + +@pytest.mark.parametrize("action, active", [("suspend", False), ("resume", True)]) +def test_has_not_effect_when_service_is_already_that_state(client, sample_service, action, active, mocker): + mocked = mocker.patch("app.service.rest.dao_{}_service".format(action)) + sample_service.active = active + auth_header = create_authorization_header() + response = client.post("/service/{}/{}".format(sample_service.id, action), + headers=[auth_header]) + assert response.status_code == 204 + mocked.assert_not_called() + assert mocked.called == 0 + + +def test_suspending_service_revokes_api_keys(client, sample_service, sample_api_key): + auth_header = create_authorization_header() + response = client.post("/service/{}/archive".format(sample_service.id), + headers=[auth_header]) + assert response.status_code == 204 + assert sample_api_key.expiry_date.date() == datetime.utcnow().date() + + +def test_resume_service_leaves_api_keys_revokes(client, sample_service, sample_api_key): + sample_api_key.expiry_date = datetime.utcnow() + sample_service.active = False + auth_header = create_authorization_header() + response = client.post("/service/{}/archive".format(sample_service.id), + headers=[auth_header]) + assert response.status_code == 204 + assert sample_api_key.expiry_date.date() == datetime.utcnow().date() + + +@pytest.mark.parametrize("action, original_state", [("suspend", True), ("resume", False)]) +def test_service_history_is_created(client, sample_service, action, original_state): + sample_service.active = original_state + auth_header = create_authorization_header() + response = client.post("/service/{}/{}".format(sample_service.id, action), + headers=[auth_header]) + ServiceHistory = Service.get_history_model() + history = ServiceHistory.query.filter_by( + id=sample_service.id + ).order_by( + ServiceHistory.version.desc() + ).first() + + assert response.status_code == 204 + assert history.version == 2 + assert history.active != original_state From 907a1e91ac003647d57ac5ea76e9a5b03101c19d Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 31 Jan 2017 11:33:36 +0000 Subject: [PATCH 2/3] Removed the need for 2 return statements. Update tests to fix copy/paste errors, use freezetime to ensure there is never a random failing test. --- app/service/rest.py | 25 +++++----------- tests/app/dao/test_services_dao.py | 6 ++-- .../service/test_suspend_resume_service.py | 30 ++++++++++++------- 3 files changed, 31 insertions(+), 30 deletions(-) diff --git a/app/service/rest.py b/app/service/rest.py index 138b74f39..416bace74 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -205,7 +205,7 @@ def get_service_provider_aggregate_statistics(service_id): # tables. This is so product owner can pass stories as done @service_blueprint.route('//history', methods=['GET']) def get_service_history(service_id): - from app.models import (Service, ApiKey, Template, TemplateHistory, Event) + from app.models import (Service, ApiKey, TemplateHistory, Event) from app.schemas import ( service_history_schema, api_key_history_schema, @@ -330,7 +330,7 @@ def update_whitelist(service_id): current_app.logger.exception(e) dao_rollback() msg = '{} is not a valid email address or phone number'.format(str(e)) - return jsonify(result='error', message=msg), 400 + raise InvalidRequest(msg, 400) else: dao_add_and_commit_whitelisted_contacts(whitelist_objs) return '', 204 @@ -360,11 +360,8 @@ def archive_service(service_id): """ service = dao_fetch_service_by_id(service_id) - if not service.active: - # assume already inactive, don't change service name - return '', 204 - - dao_archive_service(service.id) + if service.active: + dao_archive_service(service.id) return '', 204 @@ -378,11 +375,8 @@ def suspend_service(service_id): """ service = dao_fetch_service_by_id(service_id) - if not service.active: - # assume already inactive, don't change service name - return '', 204 - - dao_suspend_service(service.id) + if service.active: + dao_suspend_service(service.id) return '', 204 @@ -397,11 +391,8 @@ def resume_service(service_id): """ service = dao_fetch_service_by_id(service_id) - if service.active: - # assume already inactive, don't change service name - return '', 204 - - dao_resume_service(service.id) + if not service.active: + dao_resume_service(service.id) return '', 204 diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 5fb2c01d9..52624e0d1 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -659,6 +659,7 @@ def test_fetch_stats_by_date_range_for_all_services(notify_db, notify_db_session assert results[0] == ('sms', 'created', result_one.service_id, 2) +@freeze_time('2001-01-01T23:59:00') def test_dao_suspend_service_marks_service_as_inactive_and_expires_api_keys(sample_service, sample_api_key): dao_suspend_service(sample_service.id) service = Service.query.get(sample_service.id) @@ -666,9 +667,10 @@ def test_dao_suspend_service_marks_service_as_inactive_and_expires_api_keys(samp assert service.name == sample_service.name api_key = ApiKey.query.get(sample_api_key.id) - assert api_key.expiry_date.date() == datetime.utcnow().date() + assert api_key.expiry_date == datetime(2001, 1, 1, 23, 59, 00) +@freeze_time('2001-01-01T23:59:00') def test_dao_resume_service_marks_service_as_active_and_api_keys_are_still_revoked(sample_service, sample_api_key): dao_suspend_service(sample_service.id) service = Service.query.get(sample_service.id) @@ -678,4 +680,4 @@ def test_dao_resume_service_marks_service_as_active_and_api_keys_are_still_revok assert Service.query.get(service.id).active api_key = ApiKey.query.get(sample_api_key.id) - assert api_key.expiry_date.date() == datetime.utcnow().date() + assert api_key.expiry_date == datetime(2001, 1, 1, 23, 59, 00) diff --git a/tests/app/service/test_suspend_resume_service.py b/tests/app/service/test_suspend_resume_service.py index 12f1b9239..8f33a22d2 100644 --- a/tests/app/service/test_suspend_resume_service.py +++ b/tests/app/service/test_suspend_resume_service.py @@ -4,7 +4,9 @@ import pytest import uuid -from app.models import Service +from freezegun import freeze_time + +from app.models import Service, ApiKey from tests import create_authorization_header @@ -33,25 +35,31 @@ def test_has_not_effect_when_service_is_already_that_state(client, sample_servic headers=[auth_header]) assert response.status_code == 204 mocked.assert_not_called() - assert mocked.called == 0 + assert sample_service.active == active +@freeze_time('2001-01-01T23:59:00') def test_suspending_service_revokes_api_keys(client, sample_service, sample_api_key): auth_header = create_authorization_header() - response = client.post("/service/{}/archive".format(sample_service.id), + response = client.post("/service/{}/suspend".format(sample_service.id), headers=[auth_header]) assert response.status_code == 204 - assert sample_api_key.expiry_date.date() == datetime.utcnow().date() + assert sample_api_key.expiry_date == datetime(2001, 1, 1, 23, 59, 00) def test_resume_service_leaves_api_keys_revokes(client, sample_service, sample_api_key): - sample_api_key.expiry_date = datetime.utcnow() - sample_service.active = False - auth_header = create_authorization_header() - response = client.post("/service/{}/archive".format(sample_service.id), - headers=[auth_header]) - assert response.status_code == 204 - assert sample_api_key.expiry_date.date() == datetime.utcnow().date() + with freeze_time('2001-10-22T11:59:00'): + sample_api_key.expiry_date = datetime.utcnow() + sample_service.active = False + auth_header = create_authorization_header() + client.post("/service/{}/resume".format(sample_service.id), + headers=[auth_header]) + with freeze_time('2001-10-22T13:59:00'): + auth_header = create_authorization_header() + response = client.post("/service/{}/resume".format(sample_service.id), + headers=[auth_header]) + assert response.status_code == 204 + assert sample_api_key.expiry_date == datetime(2001, 10, 22, 11, 59, 00) @pytest.mark.parametrize("action, original_state", [("suspend", True), ("resume", False)]) From 539ebdce7008023a609e2848208c3ab009b322b7 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 31 Jan 2017 11:45:35 +0000 Subject: [PATCH 3/3] This is what I meant to test --- tests/app/service/test_suspend_resume_service.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/app/service/test_suspend_resume_service.py b/tests/app/service/test_suspend_resume_service.py index 8f33a22d2..7f38d1be6 100644 --- a/tests/app/service/test_suspend_resume_service.py +++ b/tests/app/service/test_suspend_resume_service.py @@ -49,10 +49,8 @@ def test_suspending_service_revokes_api_keys(client, sample_service, sample_api_ def test_resume_service_leaves_api_keys_revokes(client, sample_service, sample_api_key): with freeze_time('2001-10-22T11:59:00'): - sample_api_key.expiry_date = datetime.utcnow() - sample_service.active = False auth_header = create_authorization_header() - client.post("/service/{}/resume".format(sample_service.id), + client.post("/service/{}/suspend".format(sample_service.id), headers=[auth_header]) with freeze_time('2001-10-22T13:59:00'): auth_header = create_authorization_header()