From 22ac1bfcae9b95f809bfd24f4aa583be008f0dea Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 12 Jul 2021 15:29:53 +0100 Subject: [PATCH 1/4] DRY-up and enforce kwargs for most events For most events this makes the purpose of each argument clearer at the point the event is called. It's still worth having a function for each event type, as this abstracts knowledge of the event label. Using a schema approach will make adding new events easier. In the next commit we'll DRY-up the duplication in the tests as well. --- app/event_handlers.py | 100 ++++++------------ app/main/views/find_users.py | 2 +- app/main/views/manage_users.py | 20 +++- app/main/views/service_settings.py | 4 +- tests/app/main/views/test_manage_users.py | 16 +-- tests/app/main/views/test_service_settings.py | 4 +- tests/app/test_event_handlers.py | 50 ++++++--- 7 files changed, 100 insertions(+), 96 deletions(-) diff --git a/app/event_handlers.py b/app/event_handlers.py index 5c5eacc6e..fcf6a8180 100644 --- a/app/event_handlers.py +++ b/app/event_handlers.py @@ -2,94 +2,62 @@ from flask import request from app import events_api_client +EVENT_SCHEMAS = { + "sucessful_login": {"user_id"}, + "update_user_email": {"user_id", "updated_by_id", "original_email_address", "new_email_address"}, + "update_user_mobile_number": {"user_id", "updated_by_id", "original_mobile_number", "new_mobile_number"}, + "remove_user_from_service": {"user_id", "removed_by_id", "service_id"}, + "add_user_to_service": {"user_id", "invited_by_id", "service_id"}, + "archive_user": {"user_id", "archived_by_id"}, + "change_broadcast_account_type": {"service_id", "changed_by_id", "service_mode", "broadcast_channel", "provider_restriction"}, # noqa: E501 (length) + "archive_service": {"service_id", "archived_by_id"}, + "suspend_service": {"service_id", "suspended_by_id"}, +} + def on_user_logged_in(_sender, user): _send_event('sucessful_login', user_id=user.id) -def create_email_change_event(user_id, updated_by_id, original_email_address, new_email_address): - _send_event( - 'update_user_email', - user_id=user_id, - updated_by_id=updated_by_id, - original_email_address=original_email_address, - new_email_address=new_email_address) +def create_email_change_event(**kwargs): + _send_event('update_user_email', **kwargs) -def create_mobile_number_change_event(user_id, updated_by_id, original_mobile_number, new_mobile_number): - _send_event( - 'update_user_mobile_number', - user_id=user_id, - updated_by_id=updated_by_id, - original_mobile_number=original_mobile_number, - new_mobile_number=new_mobile_number) +def create_mobile_number_change_event(**kwargs): + _send_event('update_user_mobile_number', **kwargs) -def create_remove_user_from_service_event(user_id, removed_by_id, service_id): - _send_event( - 'remove_user_from_service', - user_id=user_id, - removed_by_id=removed_by_id, - service_id=service_id - ) +def create_remove_user_from_service_event(**kwargs): + _send_event('remove_user_from_service', **kwargs) -def create_add_user_to_service_event(user_id, invited_by_id, service_id): - _send_event( - 'add_user_to_service', - user_id=user_id, - invited_by_id=invited_by_id, - service_id=service_id - ) +def create_add_user_to_service_event(**kwargs): + _send_event('add_user_to_service', **kwargs) -def create_archive_user_event(user_id, archived_by_id): - _send_event( - 'archive_user', - user_id=user_id, - archived_by_id=archived_by_id) +def create_archive_user_event(**kwargs): + _send_event('archive_user', **kwargs) -def create_broadcast_account_type_change_event( - service_id, - changed_by_id, - service_mode, - broadcast_channel, - provider_restriction, -): - _send_event( - 'change_broadcast_account_type', - service_id=service_id, - changed_by_id=changed_by_id, - service_mode=service_mode, - broadcast_channel=broadcast_channel, - provider_restriction=provider_restriction - ) +def create_broadcast_account_type_change_event(**kwargs): + _send_event('change_broadcast_account_type', **kwargs) -def create_suspend_service_event( - service_id, - suspended_by_id, -): - _send_event( - 'suspend_service', - service_id=service_id, - suspended_by_id=suspended_by_id, - ) +def create_suspend_service_event(**kwargs): + _send_event('suspend_service', **kwargs) -def create_archive_service_event( - service_id, - archived_by_id, -): - _send_event( - 'archive_service', - service_id=service_id, - archived_by_id=archived_by_id, - ) +def create_archive_service_event(**kwargs): + _send_event('archive_service', **kwargs) def _send_event(event_type, **kwargs): + expected_keys = EVENT_SCHEMAS[event_type] + actual_keys = set(kwargs.keys()) + + if expected_keys != actual_keys: + raise ValueError(f'Expected {expected_keys}, but got {actual_keys}') + event_data = _construct_event_data(request) event_data.update(kwargs) diff --git a/app/main/views/find_users.py b/app/main/views/find_users.py index 00c8bdac2..3fb817d0b 100644 --- a/app/main/views/find_users.py +++ b/app/main/views/find_users.py @@ -44,7 +44,7 @@ def archive_user(user_id): flash('User can’t be removed from a service - ' 'check all services have another team member with manage_settings') return redirect(url_for('main.user_information', user_id=user_id)) - create_archive_user_event(str(user_id), current_user.id) + create_archive_user_event(user_id=str(user_id), archived_by_id=current_user.id) return redirect(url_for('.user_information', user_id=user_id)) else: diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 57e00ac3f..cd0daed86 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -173,7 +173,11 @@ def remove_user_from_service(service_id, user_id): else: abort(500, e) else: - create_remove_user_from_service_event(user_id=user_id, removed_by_id=current_user.id, service_id=service_id) + create_remove_user_from_service_event( + user_id=user_id, + removed_by_id=current_user.id, + service_id=service_id + ) return redirect(url_for( '.manage_users', @@ -228,7 +232,12 @@ def confirm_edit_user_email(service_id, user_id): except HTTPError as e: abort(500, e) else: - create_email_change_event(user.id, current_user.id, user.email_address, new_email) + create_email_change_event( + user_id=user.id, + updated_by_id=current_user.id, + original_email_address=user.email_address, + new_email_address=new_email + ) finally: session.pop(session_key, None) @@ -286,7 +295,12 @@ def confirm_edit_user_mobile_number(service_id, user_id): except HTTPError as e: abort(500, e) else: - create_mobile_number_change_event(user.id, current_user.id, user.mobile_number, new_number) + create_mobile_number_change_event( + user_id=user.id, + updated_by_id=current_user.id, + original_mobile_number=user.mobile_number, + new_mobile_number=new_number + ) finally: session.pop('team_member_mobile_change', None) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 8dc06152d..2ad63815a 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -429,7 +429,7 @@ def archive_service(service_id): cached_service_user_ids = [user.id for user in current_service.active_users] service_api_client.archive_service(service_id, cached_service_user_ids) - create_archive_service_event(service_id, archived_by_id=current_user.id) + create_archive_service_event(service_id=service_id, archived_by_id=current_user.id) flash( '‘{}’ was deleted'.format(current_service.name), @@ -449,7 +449,7 @@ def archive_service(service_id): def suspend_service(service_id): if request.method == 'POST': service_api_client.suspend_service(service_id) - create_suspend_service_event(service_id, suspended_by_id=current_user.id) + create_suspend_service_event(service_id=service_id, suspended_by_id=current_user.id) return redirect(url_for('.service_settings', service_id=service_id)) else: flash("This will suspend the service and revoke all api keys. Are you sure you want to suspend this service?", diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 11a74420c..8b20013bd 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -1827,10 +1827,10 @@ def test_confirm_edit_user_email_changes_user_email( updated_by=active_user_with_permissions['id'] ) mock_event_handler.assert_called_once_with( - api_user_active['id'], - active_user_with_permissions['id'], - api_user_active['email_address'], - new_email) + user_id=api_user_active['id'], + updated_by_id=active_user_with_permissions['id'], + original_email_address=api_user_active['email_address'], + new_email_address=new_email) def test_confirm_edit_user_email_doesnt_change_user_email_for_non_team_member( @@ -2040,10 +2040,10 @@ def test_confirm_edit_user_mobile_number_changes_user_mobile_number( updated_by=active_user_with_permissions['id'] ) mock_event_handler.assert_called_once_with( - api_user_active['id'], - active_user_with_permissions['id'], - api_user_active['mobile_number'], - new_number) + user_id=api_user_active['id'], + updated_by_id=active_user_with_permissions['id'], + original_mobile_number=api_user_active['mobile_number'], + new_mobile_number=new_number) def test_confirm_edit_user_mobile_number_doesnt_change_user_mobile_for_non_team_member( diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 4078c4e43..067eb1b16 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -4138,7 +4138,7 @@ def test_archive_service_after_confirm( ) mock_api.assert_called_once_with('/service/{}/archive'.format(SERVICE_ONE_ID), data=None) - mock_event.assert_called_once_with(SERVICE_ONE_ID, archived_by_id=user['id']) + mock_event.assert_called_once_with(service_id=SERVICE_ONE_ID, archived_by_id=user['id']) assert normalize_spaces(page.select_one('h1').text) == 'Choose service' assert normalize_spaces(page.select_one('.banner-default-with-tick').text) == ( @@ -4230,7 +4230,7 @@ def test_suspend_service_after_confirm( ) 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']) + mock_event.assert_called_once_with(service_id=SERVICE_ONE_ID, suspended_by_id=user['id']) @pytest.mark.parametrize('user', ( diff --git a/tests/app/test_event_handlers.py b/tests/app/test_event_handlers.py index 0bd4a12d3..bda91b9ac 100644 --- a/tests/app/test_event_handlers.py +++ b/tests/app/test_event_handlers.py @@ -35,7 +35,12 @@ def test_create_email_change_event_calls_events_api(client, mock_events): user_id = str(uuid.uuid4()) updated_by_id = str(uuid.uuid4()) - create_email_change_event(user_id, updated_by_id, 'original@example.com', 'new@example.com') + create_email_change_event( + user_id=user_id, + updated_by_id=updated_by_id, + original_email_address='original@example.com', + new_email_address='new@example.com' + ) mock_events.assert_called_with('update_user_email', event_dict( user_id=user_id, @@ -50,7 +55,11 @@ def test_create_add_user_to_service_event_calls_events_api(client, mock_events): invited_by_id = str(uuid.uuid4()) service_id = str(uuid.uuid4()) - create_add_user_to_service_event(user_id, invited_by_id, service_id) + create_add_user_to_service_event( + user_id=user_id, + invited_by_id=invited_by_id, + service_id=service_id + ) mock_events.assert_called_with('add_user_to_service', event_dict( user_id=user_id, @@ -64,7 +73,11 @@ def test_create_remove_user_from_service_event_calls_events_api(client, mock_eve removed_by_id = str(uuid.uuid4()) service_id = str(uuid.uuid4()) - create_remove_user_from_service_event(user_id, removed_by_id, service_id) + create_remove_user_from_service_event( + user_id=user_id, + removed_by_id=removed_by_id, + service_id=service_id + ) mock_events.assert_called_with('remove_user_from_service', event_dict( user_id=user_id, @@ -77,7 +90,12 @@ def test_create_mobile_number_change_event_calls_events_api(client, mock_events) user_id = str(uuid.uuid4()) updated_by_id = str(uuid.uuid4()) - create_mobile_number_change_event(user_id, updated_by_id, '07700900000', '07700900999') + create_mobile_number_change_event( + user_id=user_id, + updated_by_id=updated_by_id, + original_mobile_number='07700900000', + new_mobile_number='07700900999' + ) mock_events.assert_called_with('update_user_mobile_number', event_dict( user_id=user_id, @@ -91,7 +109,10 @@ def test_create_archive_user_event_calls_events_api(client, mock_events): user_id = str(uuid.uuid4()) archived_by_id = str(uuid.uuid4()) - create_archive_user_event(user_id, archived_by_id) + create_archive_user_event( + user_id=user_id, + archived_by_id=archived_by_id + ) mock_events.assert_called_with('archive_user', event_dict( user_id=user_id, @@ -104,11 +125,12 @@ def test_create_broadcast_account_type_change_event(client, mock_events): changed_by_id = str(uuid.uuid4()) create_broadcast_account_type_change_event( - service_id, - changed_by_id, - 'training', - 'severe', - None) + service_id=service_id, + changed_by_id=changed_by_id, + service_mode='training', + broadcast_channel='severe', + provider_restriction=None + ) mock_events.assert_called_with('change_broadcast_account_type', event_dict( service_id=service_id, @@ -124,8 +146,8 @@ def test_suspend_service(client, mock_events): suspended_by_id = str(uuid.uuid4()) create_suspend_service_event( - service_id, - suspended_by_id, + service_id=service_id, + suspended_by_id=suspended_by_id, ) mock_events.assert_called_with('suspend_service', event_dict( @@ -139,8 +161,8 @@ def test_archive_service(client, mock_events): archived_by_id = str(uuid.uuid4()) create_archive_service_event( - service_id, - archived_by_id, + service_id=service_id, + archived_by_id=archived_by_id, ) mock_events.assert_called_with('archive_service', event_dict( From d37c2abb9ea72b47d22214d878066b6488b8cad8 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 12 Jul 2021 15:37:22 +0100 Subject: [PATCH 2/4] DRY-up arg assertions in event handler tests This will make it easier to add another handler in the next commit. --- tests/app/test_event_handlers.py | 165 +++++++++++-------------------- 1 file changed, 57 insertions(+), 108 deletions(-) diff --git a/tests/app/test_event_handlers.py b/tests/app/test_event_handlers.py index bda91b9ac..4f51f2982 100644 --- a/tests/app/test_event_handlers.py +++ b/tests/app/test_event_handlers.py @@ -32,140 +32,89 @@ def test_on_user_logged_in_calls_events_api(client, api_user_active, mock_events def test_create_email_change_event_calls_events_api(client, mock_events): - user_id = str(uuid.uuid4()) - updated_by_id = str(uuid.uuid4()) + kwargs = { + "user_id": str(uuid.uuid4()), + "updated_by_id": str(uuid.uuid4()), + "original_email_address": 'original@example.com', + "new_email_address": 'new@example.com' + } - create_email_change_event( - user_id=user_id, - updated_by_id=updated_by_id, - original_email_address='original@example.com', - new_email_address='new@example.com' - ) - - mock_events.assert_called_with('update_user_email', event_dict( - user_id=user_id, - updated_by_id=updated_by_id, - original_email_address='original@example.com', - new_email_address='new@example.com' - )) + create_email_change_event(**kwargs) + mock_events.assert_called_with('update_user_email', event_dict(**kwargs)) def test_create_add_user_to_service_event_calls_events_api(client, mock_events): - user_id = str(uuid.uuid4()) - invited_by_id = str(uuid.uuid4()) - service_id = str(uuid.uuid4()) + kwargs = { + "user_id": str(uuid.uuid4()), + "invited_by_id": str(uuid.uuid4()), + "service_id": str(uuid.uuid4()) + } - create_add_user_to_service_event( - user_id=user_id, - invited_by_id=invited_by_id, - service_id=service_id - ) - - mock_events.assert_called_with('add_user_to_service', event_dict( - user_id=user_id, - invited_by_id=invited_by_id, - service_id=service_id, - )) + create_add_user_to_service_event(**kwargs) + mock_events.assert_called_with('add_user_to_service', event_dict(**kwargs)) def test_create_remove_user_from_service_event_calls_events_api(client, mock_events): - user_id = str(uuid.uuid4()) - removed_by_id = str(uuid.uuid4()) - service_id = str(uuid.uuid4()) + kwargs = { + "user_id": str(uuid.uuid4()), + "removed_by_id": str(uuid.uuid4()), + "service_id": str(uuid.uuid4()) + } - create_remove_user_from_service_event( - user_id=user_id, - removed_by_id=removed_by_id, - service_id=service_id - ) - - mock_events.assert_called_with('remove_user_from_service', event_dict( - user_id=user_id, - removed_by_id=removed_by_id, - service_id=service_id, - )) + create_remove_user_from_service_event(**kwargs) + mock_events.assert_called_with('remove_user_from_service', event_dict(**kwargs)) def test_create_mobile_number_change_event_calls_events_api(client, mock_events): - user_id = str(uuid.uuid4()) - updated_by_id = str(uuid.uuid4()) + kwargs = { + "user_id": str(uuid.uuid4()), + "updated_by_id": str(uuid.uuid4()), + "original_mobile_number": '07700900000', + "new_mobile_number": '07700900999' + } - create_mobile_number_change_event( - user_id=user_id, - updated_by_id=updated_by_id, - original_mobile_number='07700900000', - new_mobile_number='07700900999' - ) - - mock_events.assert_called_with('update_user_mobile_number', event_dict( - user_id=user_id, - updated_by_id=updated_by_id, - original_mobile_number='07700900000', - new_mobile_number='07700900999' - )) + create_mobile_number_change_event(**kwargs) + mock_events.assert_called_with('update_user_mobile_number', event_dict(**kwargs)) def test_create_archive_user_event_calls_events_api(client, mock_events): - user_id = str(uuid.uuid4()) - archived_by_id = str(uuid.uuid4()) + kwargs = { + "user_id": str(uuid.uuid4()), + "archived_by_id": str(uuid.uuid4()) + } - create_archive_user_event( - user_id=user_id, - archived_by_id=archived_by_id - ) - - mock_events.assert_called_with('archive_user', event_dict( - user_id=user_id, - archived_by_id=archived_by_id - )) + create_archive_user_event(**kwargs) + mock_events.assert_called_with('archive_user', event_dict(**kwargs)) def test_create_broadcast_account_type_change_event(client, mock_events): - service_id = str(uuid.uuid4()) - changed_by_id = str(uuid.uuid4()) + kwargs = { + "service_id": str(uuid.uuid4()), + "changed_by_id": str(uuid.uuid4()), + "service_mode": 'training', + "broadcast_channel": 'severe', + "provider_restriction": None + } - create_broadcast_account_type_change_event( - service_id=service_id, - changed_by_id=changed_by_id, - service_mode='training', - broadcast_channel='severe', - provider_restriction=None - ) - - mock_events.assert_called_with('change_broadcast_account_type', event_dict( - service_id=service_id, - changed_by_id=changed_by_id, - service_mode='training', - broadcast_channel='severe', - provider_restriction=None - )) + create_broadcast_account_type_change_event(**kwargs) + mock_events.assert_called_with('change_broadcast_account_type', event_dict(**kwargs)) def test_suspend_service(client, mock_events): - service_id = str(uuid.uuid4()) - suspended_by_id = str(uuid.uuid4()) + kwargs = { + "service_id": str(uuid.uuid4()), + "suspended_by_id": str(uuid.uuid4()) + } - create_suspend_service_event( - service_id=service_id, - suspended_by_id=suspended_by_id, - ) - - mock_events.assert_called_with('suspend_service', event_dict( - service_id=service_id, - suspended_by_id=suspended_by_id, - )) + create_suspend_service_event(**kwargs) + mock_events.assert_called_with('suspend_service', event_dict(**kwargs)) def test_archive_service(client, mock_events): - service_id = str(uuid.uuid4()) - archived_by_id = str(uuid.uuid4()) + kwargs = { + "service_id": str(uuid.uuid4()), + "archived_by_id": str(uuid.uuid4()) + } - create_archive_service_event( - service_id=service_id, - archived_by_id=archived_by_id, - ) - - mock_events.assert_called_with('archive_service', event_dict( - service_id=service_id, - archived_by_id=archived_by_id, - )) + create_archive_service_event(**kwargs) + mock_events.assert_called_with('archive_service', event_dict(**kwargs)) From 1fb529c4480d1ff0e75f4a87f49710ce4a9d01a3 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 12 Jul 2021 15:54:37 +0100 Subject: [PATCH 3/4] Improve coverage of resume service tests This makes the tests consistent with those for suspend / archive, logging in with different users to make it clearer who can/not do this action in the backend. Note that we think this functionality may be too permissive [1], so we may restrict it in a future PR. [1]: https://github.com/alphagov/notifications-admin/pull/3959#issuecomment-878291295 --- tests/app/main/views/test_service_settings.py | 48 ++++++++++++------- 1 file changed, 31 insertions(+), 17 deletions(-) diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 067eb1b16..24f970c9f 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -4275,27 +4275,42 @@ def test_cant_suspend_inactive_service( assert 'Suspend 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_resume_service_after_confirm( - platform_admin_client, - service_one, - single_reply_to_email_address, - single_letter_contact_block, - mock_get_organisation, mocker, - mock_get_inbound_number_for_service, + user, + service_one, + client_request, ): service_one['active'] = False - mocked_fn = mocker.patch('app.service_api_client.post', return_value=service_one) + mock_api = mocker.patch('app.service_api_client.post') - response = platform_admin_client.post(url_for('main.resume_service', service_id=service_one['id'])) + client_request.login(user) + client_request.post( + 'main.resume_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) - assert mocked_fn.call_args == call('/service/{}/resume'.format(service_one['id']), data=None) + assert mock_api.called_once_with('/service/{}/resume'.format(SERVICE_ONE_ID), data=None) +@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_resume_service_prompts_user( - platform_admin_client, + client_request, + user, service_one, single_reply_to_email_address, single_letter_contact_block, @@ -4304,15 +4319,14 @@ def test_resume_service_prompts_user( mock_get_service_settings_page_common, ): service_one['active'] = False - 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.resume_service', service_id=service_one['id'])) + client_request.login(user) + page = client_request.get('main.resume_service', service_id=service_one['id']) - assert response.status_code == 200 - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert 'This will resume the service. New api key are required for this service to use the API.' in \ page.find('div', class_='banner-dangerous').text - assert mocked_fn.called is False + assert mock_api.called is False def test_cant_resume_active_service( From 1cde6ac68693c52e73ad97731ee534ac4a04d0f1 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 12 Jul 2021 16:00:16 +0100 Subject: [PATCH 4/4] Audit when a service is resumed This could also be an issue if the service can send broadcasts, so it's worth auditing who performed this action. --- app/event_handlers.py | 5 +++++ app/main/views/service_settings.py | 2 ++ tests/app/main/views/test_service_settings.py | 2 ++ tests/app/test_event_handlers.py | 11 +++++++++++ 4 files changed, 20 insertions(+) diff --git a/app/event_handlers.py b/app/event_handlers.py index fcf6a8180..8c141cd59 100644 --- a/app/event_handlers.py +++ b/app/event_handlers.py @@ -12,6 +12,7 @@ EVENT_SCHEMAS = { "change_broadcast_account_type": {"service_id", "changed_by_id", "service_mode", "broadcast_channel", "provider_restriction"}, # noqa: E501 (length) "archive_service": {"service_id", "archived_by_id"}, "suspend_service": {"service_id", "suspended_by_id"}, + "resume_service": {"service_id", "resumed_by_id"}, } @@ -51,6 +52,10 @@ def create_archive_service_event(**kwargs): _send_event('archive_service', **kwargs) +def create_resume_service_event(**kwargs): + _send_event('resume_service', **kwargs) + + def _send_event(event_type, **kwargs): expected_keys = EVENT_SCHEMAS[event_type] actual_keys = set(kwargs.keys()) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 2ad63815a..0203783a4 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -31,6 +31,7 @@ from app import ( from app.event_handlers import ( create_archive_service_event, create_broadcast_account_type_change_event, + create_resume_service_event, create_suspend_service_event, ) from app.extensions import zendesk_client @@ -462,6 +463,7 @@ def suspend_service(service_id): def resume_service(service_id): if request.method == 'POST': service_api_client.resume_service(service_id) + create_resume_service_event(service_id=service_id, resumed_by_id=current_user.id) return redirect(url_for('.service_settings', service_id=service_id)) else: flash("This will resume the service. New api key are required for this service to use the API.", 'resume') diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 24f970c9f..5b15f2b2b 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -4288,6 +4288,7 @@ def test_resume_service_after_confirm( ): service_one['active'] = False mock_api = mocker.patch('app.service_api_client.post') + mock_event = mocker.patch('app.main.views.service_settings.create_resume_service_event') client_request.login(user) client_request.post( @@ -4301,6 +4302,7 @@ def test_resume_service_after_confirm( ) assert mock_api.called_once_with('/service/{}/resume'.format(SERVICE_ONE_ID), data=None) + assert mock_event.called_once_with(service_id=SERVICE_ONE_ID, resumed_by_id=user['id']) @pytest.mark.parametrize('user', ( diff --git a/tests/app/test_event_handlers.py b/tests/app/test_event_handlers.py index 4f51f2982..29d511992 100644 --- a/tests/app/test_event_handlers.py +++ b/tests/app/test_event_handlers.py @@ -9,6 +9,7 @@ from app.event_handlers import ( create_email_change_event, create_mobile_number_change_event, create_remove_user_from_service_event, + create_resume_service_event, create_suspend_service_event, on_user_logged_in, ) @@ -118,3 +119,13 @@ def test_archive_service(client, mock_events): create_archive_service_event(**kwargs) mock_events.assert_called_with('archive_service', event_dict(**kwargs)) + + +def test_resume_service(client, mock_events): + kwargs = { + "service_id": str(uuid.uuid4()), + "resumed_by_id": str(uuid.uuid4()) + } + + create_resume_service_event(**kwargs) + mock_events.assert_called_with('resume_service', event_dict(**kwargs))