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

@@ -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):
+def create_email_change_event(**kwargs):
-    _send_event(
+    _send_event('update_user_email', **kwargs)
-        '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):
+def create_mobile_number_change_event(**kwargs):
-    _send_event(
+    _send_event('update_user_mobile_number', **kwargs)
-        '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_remove_user_from_service_event(user_id, removed_by_id, service_id):
+def create_remove_user_from_service_event(**kwargs):
-    _send_event(
+    _send_event('remove_user_from_service', **kwargs)
-        'remove_user_from_service',
-        user_id=user_id,
-        removed_by_id=removed_by_id,
-        service_id=service_id
-    )
 
 
-def create_add_user_to_service_event(user_id, invited_by_id, service_id):
+def create_add_user_to_service_event(**kwargs):
-    _send_event(
+    _send_event('add_user_to_service', **kwargs)
-        'add_user_to_service',
-        user_id=user_id,
-        invited_by_id=invited_by_id,
-        service_id=service_id
-    )
 
 
-def create_archive_user_event(user_id, archived_by_id):
+def create_archive_user_event(**kwargs):
-    _send_event(
+    _send_event('archive_user', **kwargs)
-        'archive_user',
-        user_id=user_id,
-        archived_by_id=archived_by_id)
 
 
-def create_broadcast_account_type_change_event(
+def create_broadcast_account_type_change_event(**kwargs):
-    service_id,
+    _send_event('change_broadcast_account_type', **kwargs)
-    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_suspend_service_event(
+def create_suspend_service_event(**kwargs):
-    service_id,
+    _send_event('suspend_service', **kwargs)
-    suspended_by_id,
-):
-    _send_event(
-        'suspend_service',
-        service_id=service_id,
-        suspended_by_id=suspended_by_id,
-    )
 
 
-def create_archive_service_event(
+def create_archive_service_event(**kwargs):
-    service_id,
+    _send_event('archive_service', **kwargs)
-    archived_by_id,
-):
-    _send_event(
-        'archive_service',
-        service_id=service_id,
-        archived_by_id=archived_by_id,
-    )
 
 
 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)
 

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:

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)
 

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?",

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'],
+        user_id=api_user_active['id'],
-        active_user_with_permissions['id'],
+        updated_by_id=active_user_with_permissions['id'],
-        api_user_active['email_address'],
+        original_email_address=api_user_active['email_address'],
-        new_email)
+        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'],
+        user_id=api_user_active['id'],
-        active_user_with_permissions['id'],
+        updated_by_id=active_user_with_permissions['id'],
-        api_user_active['mobile_number'],
+        original_mobile_number=api_user_active['mobile_number'],
-        new_number)
+        new_mobile_number=new_number)
 
 
 def test_confirm_edit_user_mobile_number_doesnt_change_user_mobile_for_non_team_member(

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', (

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,
+        service_id=service_id,
-        changed_by_id,
+        changed_by_id=changed_by_id,
-        'training',
+        service_mode='training',
-        'severe',
+        broadcast_channel='severe',
-        None)
+        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,
+        service_id=service_id,
-        suspended_by_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,
+        service_id=service_id,
-        archived_by_id,
+        archived_by_id=archived_by_id,
     )
 
     mock_events.assert_called_with('archive_service', event_dict(