Skip to content

Commit

Permalink
feat!: Do not send events for unknown users (#338)
Browse files Browse the repository at this point in the history
In the past we would create a new UUID for each unknown user. Now we skip them entirely to avoid confusing the downstream database with a new id user per event.
  • Loading branch information
bmtcril authored Sep 11, 2023
1 parent bbe9f25 commit 41c960b
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 27 deletions.
2 changes: 1 addition & 1 deletion event_routing_backends/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
Various backends for receiving edX LMS events..
"""

__version__ = '5.6.0'
__version__ = '6.0.0'
44 changes: 22 additions & 22 deletions event_routing_backends/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,34 +51,34 @@ def get_anonymous_user_id(username_or_id, external_type):
Arguments:
username_or_id (str): username for the learner
external_type (str): external type id e.g caliper or xapi
external_type (str): external type id e.g. caliper or xapi
Returns:
str
"""
user = get_user(username_or_id)
if not user:
logger.info('User with username "%s" does not exist. '
'Cannot generate anonymous ID', username_or_id)

anonymous_id = str(uuid.uuid4())
else:
# Older versions of edx-platform do not have the XAPI or
# Caliper ExternalIdTypes, so we fall back to LTI here.
# Eventually this will be a problem when those instances
# upgrade and their actor id's all change, unless we
# eventually add a setting to force LTI here instead of the
# usual type.
try:
type_name = getattr(ExternalIdType, external_type)
except AttributeError: # pragma: no cover
type_name = ExternalIdType.LTI

external_id, _ = ExternalId.add_new_user_id(user, type_name)
if not external_id:
raise ValueError("External ID type: %s does not exist" % type_name)

anonymous_id = str(external_id.external_user_id)
logger.warning('User with username "%s" does not exist. '
'Cannot generate anonymous ID', username_or_id)

raise ValueError(f"User with username {username_or_id} does not exist.")

# Older versions of edx-platform do not have the XAPI or
# Caliper ExternalIdTypes, so we fall back to LTI here.
# Eventually this will be a problem when those instances
# upgrade and their actor id's all change, unless we
# eventually add a setting to force LTI here instead of the
# usual type.
try:
type_name = getattr(ExternalIdType, external_type)
except AttributeError: # pragma: no cover
type_name = ExternalIdType.LTI

external_id, _ = ExternalId.add_new_user_id(user, type_name)
if not external_id:
raise ValueError("External ID type: %s does not exist" % type_name)

anonymous_id = str(external_id.external_user_id)

return anonymous_id

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from unittest.mock import patch

import ddt
import pytest
from django.contrib.auth import get_user_model
from django.test.utils import override_settings

Expand Down Expand Up @@ -108,6 +109,9 @@ def test_event_transformer(self, event_filename, mocked_uuid):
with open(expected_event_file_path, encoding='utf-8') as expected:
expected_event = json.loads(expected.read())

actual_transformed_event = self.registry.get_transformer(original_event).transform()

self.compare_events(actual_transformed_event, expected_event)
if "anonymous" in event_filename:
with pytest.raises(ValueError):
self.registry.get_transformer(original_event).transform()
else:
actual_transformed_event = self.registry.get_transformer(original_event).transform()
self.compare_events(actual_transformed_event, expected_event)
5 changes: 4 additions & 1 deletion event_routing_backends/tests/test_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,13 @@ def test_get_user_email(self):
@patch('event_routing_backends.helpers.ExternalId')
def test_get_anonymous_user_id_with_error(self, mocked_external_id):
mocked_external_id.add_new_user_id.return_value = (None, False)
# Test that a failure to add an external id raises an error
with self.assertRaises(ValueError):
get_anonymous_user_id('edx', 'XAPI')

self.assertIsNotNone(get_anonymous_user_id('12345678', 'XAPI'))
# Test that an unknown user raises this error
with self.assertRaises(ValueError):
get_anonymous_user_id('12345678', 'XAPI')

def test_get_uuid5(self):
actor = '''{
Expand Down

0 comments on commit 41c960b

Please sign in to comment.