diff --git a/app/event_handlers.py b/app/event_handlers.py index 650d72d52..6a6124513 100644 --- a/app/event_handlers.py +++ b/app/event_handlers.py @@ -3,18 +3,43 @@ from flask import request from app import events_api_client -def on_user_logged_in(sender, user): - _send_event(sender, event_type='sucessful_login', user=user) +def on_user_logged_in(_sender, user): + _send_event(event_type='sucessful_login', user_id=user.id) -def _send_event(sender, **kwargs): +def create_email_change_event(user_id, updated_by_id, original_email_address, new_email_address): + _send_event( + event_type='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_mobile_number_change_event(user_id, updated_by_id, original_mobile_number, new_mobile_number): + _send_event( + event_type='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 _send_event(**kwargs): if not kwargs.get('event_type'): return event_data = _construct_event_data(request) + event_fields = ('user_id', + 'updated_by_id', + 'original_email_address', + 'new_email_address', + 'original_mobile_number', + 'new_mobile_number') - if kwargs.get('user'): - event_data['user_id'] = kwargs.get('user').id + for field in event_fields: + if kwargs.get(field): + event_data[field] = kwargs[field] events_api_client.create_event(kwargs['event_type'], event_data) diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 30cab629a..7dd69bcde 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -16,6 +16,10 @@ from app import ( service_api_client, user_api_client, ) +from app.event_handlers import ( + create_email_change_event, + create_mobile_number_change_event, +) from app.main import main from app.main.forms import ( ChangeEmailForm, @@ -196,6 +200,8 @@ def confirm_edit_user_email(service_id, user_id): user_api_client.update_user_attribute(str(user_id), email_address=new_email, updated_by=current_user.id) except HTTPError as e: abort(500, e) + else: + create_email_change_event(user.id, current_user.id, user.email_address, new_email) finally: session.pop('team_member_email_change', None) @@ -254,6 +260,8 @@ def confirm_edit_user_mobile_number(service_id, user_id): user_api_client.update_user_attribute(str(user_id), mobile_number=new_number, updated_by=current_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) finally: session.pop('team_member_mobile_change', None) diff --git a/app/models/user.py b/app/models/user.py index b74c8c2af..e54315ed5 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -240,7 +240,7 @@ class InvitedUser(object): self.created_at = created_at self.auth_type = auth_type self.permissions = translate_permissions_from_db_to_admin_roles(self.permissions) - self.folder_permissions = folder_permissions or [] + self.folder_permissions = folder_permissions def has_permissions(self, *permissions): if self.status == 'cancelled': diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 5450e647d..fa4007a01 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -1112,19 +1112,30 @@ def test_confirm_edit_user_email_page_404s_for_non_team_member( def test_confirm_edit_user_email_changes_user_email( client_request, active_user_with_permissions, - mock_get_users_by_service, + api_user_active, service_one, mocker, - mock_get_user, - mock_update_user_attribute + mock_update_user_attribute, ): + # We want active_user_with_permissions (the current user) to update the email address for api_user_active + # By default both users would have the same id, so we change the id of api_user_active + api_user_active.id = str(uuid.uuid4()) + mocker.patch('app.user_api_client.get_users_for_service', + return_value=[api_user_active, active_user_with_permissions]) + # get_user gets called twice - first to check if current user can see the page, then to see if the team member + # whose email address we're changing belongs to the service + mocker.patch('app.models.service.user_api_client.get_user', + side_effect=[active_user_with_permissions, api_user_active]) + mock_event_handler = mocker.patch('app.main.views.manage_users.create_email_change_event') + new_email = 'new_email@gov.uk' with client_request.session_transaction() as session: session['team_member_email_change'] = new_email + client_request.post( 'main.confirm_edit_user_email', service_id=service_one['id'], - user_id=active_user_with_permissions.id, + user_id=api_user_active.id, _expected_status=302, _expected_redirect=url_for( 'main.manage_users', @@ -1132,11 +1143,17 @@ def test_confirm_edit_user_email_changes_user_email( _external=True, ), ) + mock_update_user_attribute.assert_called_once_with( - active_user_with_permissions.id, + api_user_active.id, email_address=new_email, - updated_by=mocker.ANY + 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) def test_confirm_edit_user_email_doesnt_change_user_email_for_non_team_member( @@ -1313,19 +1330,31 @@ def test_confirm_edit_user_mobile_number_page_redirects_if_session_empty( def test_confirm_edit_user_mobile_number_changes_user_mobile_number( client_request, active_user_with_permissions, - mock_get_users_by_service, + api_user_active, service_one, mocker, - mock_get_user, mock_update_user_attribute ): + # We want active_user_with_permissions (the current user) to update the mobile number for api_user_active + # By default both users would have the same id, so we change the id of api_user_active + api_user_active.id = str(uuid.uuid4()) + + mocker.patch('app.user_api_client.get_users_for_service', + return_value=[api_user_active, active_user_with_permissions]) + # get_user gets called twice - first to check if current user can see the page, then to see if the team member + # whose mobile number we're changing belongs to the service + mocker.patch('app.models.service.user_api_client.get_user', + side_effect=[active_user_with_permissions, api_user_active]) + mock_event_handler = mocker.patch('app.main.views.manage_users.create_mobile_number_change_event') + new_number = '07554080636' with client_request.session_transaction() as session: session['team_member_mobile_change'] = new_number + client_request.post( 'main.confirm_edit_user_mobile_number', service_id=SERVICE_ONE_ID, - user_id=active_user_with_permissions.id, + user_id=api_user_active.id, _expected_status=302, _expected_redirect=url_for( 'main.manage_users', @@ -1334,10 +1363,15 @@ def test_confirm_edit_user_mobile_number_changes_user_mobile_number( ), ) mock_update_user_attribute.assert_called_once_with( - active_user_with_permissions.id, + api_user_active.id, mobile_number=new_number, - updated_by=mocker.ANY + 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) def test_confirm_edit_user_mobile_number_doesnt_change_user_mobile_for_non_team_member( diff --git a/tests/app/test_event_handlers.py b/tests/app/test_event_handlers.py index 7f1ae1fb7..22d398186 100644 --- a/tests/app/test_event_handlers.py +++ b/tests/app/test_event_handlers.py @@ -1,6 +1,11 @@ +import uuid from unittest.mock import ANY -from app.event_handlers import on_user_logged_in +from app.event_handlers import ( + create_email_change_event, + create_mobile_number_change_event, + on_user_logged_in, +) def test_on_user_logged_in_calls_events_api(app_, api_user_active, mock_events): @@ -11,3 +16,37 @@ def test_on_user_logged_in_calls_events_api(app_, api_user_active, mock_events): {'browser_fingerprint': {'browser': ANY, 'version': ANY, 'platform': ANY, 'user_agent_string': ''}, 'ip_address': ANY, 'user_id': str(api_user_active.id)}) + + +def test_create_email_change_event_calls_events_api(app_, mock_events): + user_id = str(uuid.uuid4()) + updated_by_id = str(uuid.uuid4()) + + with app_.test_request_context(): + create_email_change_event(user_id, updated_by_id, 'original@example.com', 'new@example.com') + + mock_events.assert_called_with('update_user_email', + {'browser_fingerprint': + {'browser': ANY, 'version': ANY, 'platform': ANY, 'user_agent_string': ''}, + 'ip_address': ANY, + 'user_id': user_id, + 'updated_by_id': updated_by_id, + 'original_email_address': 'original@example.com', + 'new_email_address': 'new@example.com'}) + + +def test_create_mobile_number_change_event_calls_events_api(app_, mock_events): + user_id = str(uuid.uuid4()) + updated_by_id = str(uuid.uuid4()) + + with app_.test_request_context(): + create_mobile_number_change_event(user_id, updated_by_id, '07700900000', '07700900999') + + mock_events.assert_called_with('update_user_mobile_number', + {'browser_fingerprint': + {'browser': ANY, 'version': ANY, 'platform': ANY, 'user_agent_string': ''}, + 'ip_address': ANY, + 'user_id': user_id, + 'updated_by_id': updated_by_id, + 'original_mobile_number': '07700900000', + 'new_mobile_number': '07700900999'})