fix: Change search order for users (#516)

* fix: Change search order for users

In some cases the context user and the data user are different, such as
when an instructor enrolls a learner through the instructor dashboard.
This change ensures that the data user is checked first, since that
should be the "actor" for our purposes. It also changes search order to
look for user id before username for performance reasons.

Author: bmtcril

CHANGELOG.rst

@@ -11,6 +11,10 @@ Change Log
 
 .. There should always be an "Unreleased" section for changes pending release.
 
+[9.3.6]
+
+* Fixes issues where the context user is not the same as the data user, such as enrolling uses via the Instructor Dashboard
+
 [9.3.5]
 
 * Adding django52 support.

event_routing_backends/__init__.py

@@ -2,4 +2,4 @@
 Various backends for receiving edX LMS events..
 """
 
-__version__ = '9.3.5'
+__version__ = "9.3.6"

event_routing_backends/models.py

@@ -1,7 +1,7 @@
-
 """
 Database models for event_routing_backends.
 """
+
 import logging
 import re
 
@@ -39,7 +39,7 @@ def get_value_from_dotted_path(dict_obj, dotted_key):
         ANY :                 Returns the value found in the dict or `None` if
                               no value exists for provided dotted path.
     """
-    nested_keys = dotted_key.split('.')
+    nested_keys = dotted_key.split(".")
     result = dict_obj
     try:
         for key in nested_keys:
@@ -64,12 +64,18 @@ def get_routers(self, backend_name):
         if not backend_name:
             return []
 
-        cache_key = get_cache_key(namespace="event_routing_backends", resource=backend_name)
+        cache_key = get_cache_key(
+            namespace="event_routing_backends", resource=backend_name
+        )
         cached_response = TieredCache.get_cached_response(cache_key)
         if cached_response.is_found:
             return cached_response.value
 
-        current = self.current_set().filter(backend_name=backend_name, enabled=True).order_by('-change_date')
+        current = (
+            self.current_set()
+            .filter(backend_name=backend_name, enabled=True)
+            .order_by("-change_date")
+        )
         TieredCache.set_all_tiers(cache_key, current, backend_cache_ttl())
         return current
 
@@ -114,13 +120,19 @@ class RouterConfiguration(ConfigurationModel):
 
     """
 
-    AUTH_BASIC = 'Basic'
-    AUTH_BEARER = 'Bearer'
-    AUTH_CHOICES = ((AUTH_BASIC, 'Basic'), (AUTH_BEARER, 'Bearer'),)
-    CALIPER_BACKEND = 'Caliper'
-    XAPI_BACKEND = 'xAPI'
-    BACKEND_CHOICES = ((CALIPER_BACKEND, 'Caliper'), (XAPI_BACKEND, 'xAPI'),)
-    KEY_FIELDS = ('route_url',)
+    AUTH_BASIC = "Basic"
+    AUTH_BEARER = "Bearer"
+    AUTH_CHOICES = (
+        (AUTH_BASIC, "Basic"),
+        (AUTH_BEARER, "Bearer"),
+    )
+    CALIPER_BACKEND = "Caliper"
+    XAPI_BACKEND = "xAPI"
+    BACKEND_CHOICES = (
+        (CALIPER_BACKEND, "Caliper"),
+        (XAPI_BACKEND, "xAPI"),
+    )
+    KEY_FIELDS = ("route_url",)
     backend_name = models.CharField(
         choices=BACKEND_CHOICES,
         max_length=50,
@@ -129,49 +141,40 @@ class RouterConfiguration(ConfigurationModel):
         db_index=True,
         default=XAPI_BACKEND,
         help_text=(
-            'Name of the tracking backend on which this router should be applied.'
-            '<br/>'
-            'Please note that this field is <b>case sensitive.</b>'
-        )
+            "Name of the tracking backend on which this router should be applied."
+            "<br/>"
+            "Please note that this field is <b>case sensitive.</b>"
+        ),
     )
 
     route_url = models.CharField(
         max_length=255,
-        verbose_name='Route url',
+        verbose_name="Route url",
         null=False,
         blank=False,
         help_text=(
-            'Route Url of the tracking backend on which this router should be applied.'
-            '<br/>'
-            'Please note that this field is <b>case sensitive.</b>'
-        )
+            "Route Url of the tracking backend on which this router should be applied."
+            "<br/>"
+            "Please note that this field is <b>case sensitive.</b>"
+        ),
     )
 
     auth_scheme = models.CharField(
         choices=AUTH_CHOICES,
-        verbose_name='Auth Scheme',
+        verbose_name="Auth Scheme",
         max_length=6,
         default=None,
         blank=True,
-        null=True
+        null=True,
     )
     auth_key = EncryptedCharField(
-        verbose_name='Auth Key',
-        max_length=256,
-        blank=True,
-        null=True
+        verbose_name="Auth Key", max_length=256, blank=True, null=True
     )
     username = EncryptedCharField(
-        verbose_name='Username',
-        max_length=256,
-        blank=True,
-        null=True
+        verbose_name="Username", max_length=256, blank=True, null=True
     )
     password = EncryptedCharField(
-        verbose_name='Password',
-        max_length=256,
-        blank=True,
-        null=True
+        verbose_name="Password", max_length=256, blank=True, null=True
     )
     configurations = EncryptedJSONField(blank=True, default=None)
     objects = RouterConfigurationManager()
@@ -181,17 +184,17 @@ class Meta:
         Addition of class names.
         """
 
-        verbose_name = 'Router Configuration'
-        verbose_name_plural = 'Router Configurations'
+        verbose_name = "Router Configuration"
+        verbose_name_plural = "Router Configurations"
 
     def __str__(self):
         """
         Return string representation for class instance.
         """
-        return '{id} - {backend} - {enabled}'.format(
+        return "{id} - {backend} - {enabled}".format(
             id=self.pk,
             backend=self.backend_name,
-            enabled='Enabled' if self.enabled else 'Disabled'
+            enabled="Enabled" if self.enabled else "Disabled",
         )
 
     @classmethod
@@ -255,7 +258,7 @@ def get_allowed_host(self, original_event):
             dict
         """
         if not self.configurations:
-            return {'host_configurations': {}}
+            return {"host_configurations": {}}
 
         is_allowed = self._match_event_for_host(original_event, self.configurations)
 
@@ -275,7 +278,7 @@ def _match_event_for_host(self, original_event, host_config):
         Returns:
             bool
         """
-        for key, value in host_config.get('match_params', {}).items():
+        for key, value in host_config.get("match_params", {}).items():
             original_event_value = get_value_from_dotted_path(original_event, key)
             if isinstance(value, list):
                 matched = False
@@ -302,7 +305,5 @@ def _is_match(self, regex_exp, value_str):
         try:
             return bool(re.compile(str(regex_exp))) and re.search(regex_exp, value_str)
         except TypeError as err:
-            logger.info(
-                        'Invalid regex %s with error: %s', regex_exp, err
-                    )
+            logger.info("Invalid regex %s with error: %s", regex_exp, err)
             return False

event_routing_backends/processors/mixins/base_transformer.py

@@ -1,6 +1,7 @@
 """
 Base Transformer Mixin to add or transform common data values.
 """
+
 import logging
 
 from django.conf import settings
@@ -43,6 +44,7 @@ def find_nested(source_dict, key):
         Returns:
             ANY
         """
+
         def _find_nested(event_dict):
             """
             Inner recursive method to find the key in dict.
@@ -84,7 +86,7 @@ def transformer_version(self):
         version of transformer package used to transform events
         """
 
-        if getattr(settings, 'RUNNING_WITH_TEST_SETTINGS', False):
+        if getattr(settings, "RUNNING_WITH_TEST_SETTINGS", False):
             return "{}@{}".format("event-routing-backends", "1.1.1")
         else:
             return "{}@{}".format("event-routing-backends", __version__)
@@ -103,13 +105,13 @@ def transform(self):
             if hasattr(self, key):
                 value = getattr(self, key)
                 transformed_event[key] = value
-            elif hasattr(self, f'get_{key}'):
-                value = getattr(self, f'get_{key}')()
+            elif hasattr(self, f"get_{key}"):
+                value = getattr(self, f"get_{key}")()
                 transformed_event[key] = value
             else:
                 raise ValueError(
                     'Cannot find value for "{}" in transformer {} for the edx event "{}"'.format(
-                        key, self.__class__.__name__, self.get_data('name', True)
+                        key, self.__class__.__name__, self.get_data("name", True)
                     )
                 )
 
@@ -125,12 +127,17 @@ def extract_username_or_userid(self):
         Returns:
             str
         """
-        username_or_id = self.get_data('username') or self.get_data('user_id')
-        if not username_or_id:
-            username_or_id = self.get_data('data.username') or self.get_data('data.user_id')
-            if not username_or_id:
-                username_or_id = self.get_data('context.username') or self.get_data('context.user_id')
-        return username_or_id
+        # Prefer the user data in "data", since in some cases the user performing an action
+        # (the one in "context") is not the user being acted upon. This happens when an instructor
+        # creates an enrollment via the Instructor Dashboard, for instance.
+        #
+        # Prefer user id to username, since usernames can change (via retirement) and usernames can
+        # require more database lookups (again, for finding retired users).
+        id_or_username = self.get_data("data.user_id") or self.get_data("data.username")
+        if not id_or_username:
+            id_or_username = self.get_data("user_id") or self.get_data("username")
+
+        return id_or_username
 
     def extract_sessionid(self):
         """
@@ -139,7 +146,7 @@ def extract_sessionid(self):
         Returns:
             str
         """
-        return self.get_data('session') or self.get_data('context.session') or self.get_data('data.session')
+        return self.get_data("session")
 
     def get_data(self, key, required=False):
         """
@@ -166,7 +173,7 @@ def get_data(self, key, required=False):
                     }
                 }
         """
-        if '.' in key:
+        if "." in key:
             result = get_value_from_dotted_path(self.event, key)
         else:
             result = BaseTransformerMixin.find_nested(self.event, key)
@@ -177,7 +184,9 @@ def get_data(self, key, required=False):
         if result is None:
             if required:
                 raise ValueError(
-                    'Could not get value for {} in event "{}"'.format(key, self.event.get('name', None))
+                    'Could not get value for {} in event "{}"'.format(
+                        key, self.event.get("name", None)
+                    )
                 )
 
         return result
@@ -210,10 +219,8 @@ def get_object_iri(self, object_type, object_id):
         if object_id is None or object_type is None:
             return None
 
-        return '{root_url}/{object_type}/{object_id}'.format(
-            root_url=settings.LMS_ROOT_URL,
-            object_type=object_type,
-            object_id=object_id
+        return "{root_url}/{object_type}/{object_id}".format(
+            root_url=settings.LMS_ROOT_URL, object_type=object_type, object_id=object_id
         )
 
     def get_object(self):

test_utils/__init__.py

@@ -8,6 +8,7 @@
 
 So this package is the place to put them.
 """
+
 import sys
 from unittest import mock
 
@@ -18,36 +19,41 @@ def _mock_third_party_modules():
     """
     # mock external_user_ids module
     external_id = mock.MagicMock()
-    external_id.external_user_id = '32e08e30-f8ae-4ce2-94a8-c2bfe38a70cb'
+    external_id.external_user_id = "32e08e30-f8ae-4ce2-94a8-c2bfe38a70cb"
     external_user_ids_module = mock.MagicMock()
-    external_user_ids_module.ExternalId.add_new_user_id.return_value = (external_id, True)
-    external_user_ids_module.ExternalIdType.XAPI = 'xapi'
-    sys.modules['openedx.core.djangoapps.external_user_ids.models'] = external_user_ids_module
+    external_user_ids_module.ExternalId.add_new_user_id.return_value = (
+        external_id,
+        True,
+    )
+    external_user_ids_module.ExternalIdType.XAPI = "xapi"
+    sys.modules["openedx.core.djangoapps.external_user_ids.models"] = (
+        external_user_ids_module
+    )
 
     # mock course
     mocked_course = {
-        'display_name': 'Demonstration Course',
+        "display_name": "Demonstration Course",
     }
 
     mocked_courses = mock.MagicMock()
     mocked_courses.get_course_overviews.return_value = [mocked_course]
-    sys.modules['openedx.core.djangoapps.content.course_overviews.api'] = mocked_courses
+    sys.modules["openedx.core.djangoapps.content.course_overviews.api"] = mocked_courses
 
     # mock opaque keys module
     mocked_keys = mock.MagicMock()
-    sys.modules['opaque_keys.edx.keys'] = mocked_keys
+    sys.modules["opaque_keys.edx.keys"] = mocked_keys
 
     # mock retired user
     mocked_user = mock.MagicMock()
-    mocked_user.username = 'edx_retired'
-    mocked_user.email = 'edx_retired@example.com'
+    mocked_user.username = "edx"
+    mocked_user.email = "edx@example.com"
     mocked_models = mock.MagicMock()
     mocked_models.get_potentially_retired_user_by_username.return_value = mocked_user
-    sys.modules['common.djangoapps.student.models'] = mocked_models
+    sys.modules["common.djangoapps.student.models"] = mocked_models
 
 
 def mocked_course_reverse(_, kwargs):
     """
     Return the reverse method to return course root URL.
     """
-    return '/courses/{}/'.format(kwargs.get('course_id'))
+    return "/courses/{}/".format(kwargs.get("course_id"))