Merge pull request #7298 from edx/mobile/ab-test-support

MA-7: A/B test and cohorted content support

common/djangoapps/util/module_utils.py

@@ -9,22 +9,23 @@ def yield_dynamic_descriptor_descendents(descriptor, module_creator):  # pylint:
     has dynamic children, the module will be created using module_creator
     and the children (as descriptors) of that module will be returned.
     """
-    def get_dynamic_descriptor_children(descriptor):
-        """
-        Internal recursive helper for traversing the child hierarchy
-        """
-        module_children = []
-        if descriptor.has_dynamic_children():
-            module = module_creator(descriptor)
-            if module is not None:
-                module_children = module.get_child_descriptors()
-        else:
-            module_children = descriptor.get_children()
-        return module_children
-
     stack = [descriptor]
 
     while len(stack) > 0:
         next_descriptor = stack.pop()
-        stack.extend(get_dynamic_descriptor_children(next_descriptor))
+        stack.extend(get_dynamic_descriptor_children(next_descriptor, module_creator))
         yield next_descriptor
+
+
+def get_dynamic_descriptor_children(descriptor, module_creator, usage_key_filter=None):
+    """
+    Returns the children of the given descriptor, while supporting descriptors with dynamic children.
+    """
+    module_children = []
+    if descriptor.has_dynamic_children():
+        module = module_creator(descriptor)
+        if module is not None:
+            module_children = module.get_child_descriptors()
+    else:
+        module_children = descriptor.get_children(usage_key_filter)
+    return module_children

common/lib/xmodule/xmodule/partitions/partitions_service.py

@@ -21,10 +21,11 @@ class PartitionService(object):
         """
         raise NotImplementedError('Subclasses must implement course_partition')
 
-    def __init__(self, user, course_id, track_function=None):
+    def __init__(self, user, course_id, track_function=None, cache=None):
         self._user = user
         self._course_id = course_id
         self._track_function = track_function
+        self._cache = cache
 
     def get_user_group_id_for_partition(self, user_partition_id):
         """
@@ -47,6 +48,13 @@ class PartitionService(object):
         Raises:
             ValueError if the user_partition_id isn't found.
         """
+        cache_key = "PartitionService.ugidfp.{}.{}.{}".format(
+            self._user.id, self._course_id, user_partition_id
+        )
+
+        if self._cache and (cache_key in self._cache):
+            return self._cache[cache_key]
+
         user_partition = self._get_user_partition(user_partition_id)
         if user_partition is None:
             raise ValueError(
@@ -55,7 +63,12 @@ class PartitionService(object):
             )
 
         group = self.get_group(user_partition)
-        return group.id if group else None
+        group_id = group.id if group else None
+
+        if self._cache is not None:
+            self._cache[cache_key] = group_id
+
+        return group_id
 
     def _get_user_partition(self, user_partition_id):
         """

common/lib/xmodule/xmodule/partitions/tests/test_partitions.py

@@ -305,12 +305,23 @@ class TestPartitionService(PartitionTestCase):
 
     def setUp(self):
         super(TestPartitionService, self).setUp()
-        course = Mock(id=SlashSeparatedCourseKey('org_0', 'course_0', 'run_0'))
-        self.partition_service = StaticPartitionService(
+        self.course = Mock(id=SlashSeparatedCourseKey('org_0', 'course_0', 'run_0'))
+        self.partition_service = self._create_service("ma")
+
+    def _create_service(self, username, cache=None):
+        """Convenience method to generate a StaticPartitionService for a user."""
+        # Derive a "user_id" from the username, just so we don't have to add an
+        # extra param to this method. Just has to be unique per user.
+        user_id = abs(hash(username))
+
+        return StaticPartitionService(
             [self.user_partition],
-            user=Mock(username='ma', email='ma@edx.org', is_staff=False, is_active=True),
-            course_id=course.id,
-            track_function=Mock()
+            user=Mock(
+                username=username, email='{}@edx.org'.format(username), is_staff=False, is_active=True, id=user_id
+            ),
+            course_id=self.course.id,
+            track_function=Mock(),
+            cache=cache
         )
 
     def test_get_user_group_id_for_partition(self):
@@ -328,6 +339,59 @@ class TestPartitionService(PartitionTestCase):
         group2_id = self.partition_service.get_user_group_id_for_partition(user_partition_id)
         self.assertEqual(group2_id, groups[1].id)    # pylint: disable=no-member
 
+    def test_caching(self):
+        username = "psvc_cache_user"
+        user_partition_id = self.user_partition.id    # pylint: disable=no-member
+        shared_cache = {}
+
+        # Two StaticPartitionService objects that share the same cache:
+        ps_shared_cache_1 = self._create_service(username, shared_cache)
+        ps_shared_cache_2 = self._create_service(username, shared_cache)
+
+        # A StaticPartitionService with its own local cache
+        ps_diff_cache = self._create_service(username, {})
+
+        # A StaticPartitionService that never uses caching.
+        ps_uncached = self._create_service(username)
+
+        # Set the group we expect users to be placed into
+        first_group = self.user_partition.groups[0]
+        self.user_partition.scheme.current_group = first_group    # pylint: disable=no-member
+
+        # Make sure our partition services all return the right thing, but skip
+        # ps_shared_cache_2 so we can see if its cache got updated anyway.
+        for part_svc in [ps_shared_cache_1, ps_diff_cache, ps_uncached]:
+            self.assertEqual(
+                first_group.id,
+                part_svc.get_user_group_id_for_partition(user_partition_id)
+            )
+
+        # Now select a new target group
+        second_group = self.user_partition.groups[1]
+        self.user_partition.scheme.current_group = second_group
+
+        # Both of the shared cache entries should return the old value, even
+        # ps_shared_cache_2, which was never asked for the value the first time
+        # Likewise, our separately cached piece should return the original answer
+        for part_svc in [ps_shared_cache_1, ps_shared_cache_2, ps_diff_cache]:
+            self.assertEqual(
+                first_group.id,
+                part_svc.get_user_group_id_for_partition(user_partition_id)
+            )
+
+        # Our uncached service should be accurate.
+        self.assertEqual(
+            second_group.id,
+            ps_uncached.get_user_group_id_for_partition(user_partition_id)
+        )
+
+        # And a newly created service should see the right thing
+        ps_new_cache = self._create_service(username, {})
+        self.assertEqual(
+            second_group.id,
+            ps_new_cache.get_user_group_id_for_partition(user_partition_id)
+        )
+
     def test_get_group(self):
         """
         Test that a partition group is assigned to a user.

common/lib/xmodule/xmodule/x_module.py

@@ -408,7 +408,7 @@ class XModuleMixin(XBlockMixin):
         else:
             return [self.display_name_with_default]
 
-    def get_children(self, usage_key_filter=lambda location: True):
+    def get_children(self, usage_key_filter=None):
         """Returns a list of XBlock instances for the children of
         this module"""
 
@@ -419,7 +419,7 @@ class XModuleMixin(XBlockMixin):
             self._child_instances = []  # pylint: disable=attribute-defined-outside-init
             for child_loc in self.children:
                 # Skip if it doesn't satisfy the filter function
-                if not usage_key_filter(child_loc):
+                if usage_key_filter and not usage_key_filter(child_loc):
                     continue
                 try:
                     child = self.runtime.get_block(child_loc)

lms/djangoapps/lms_xblock/runtime.py

@@ -7,6 +7,7 @@ import xblock.reference.plugins
 
 from django.core.urlresolvers import reverse
 from django.conf import settings
+from request_cache.middleware import RequestCache
 from lms.djangoapps.lms_xblock.models import XBlockAsidesConfig
 from openedx.core.djangoapps.user_api.course_tag import api as user_course_tag_api
 from xmodule.modulestore.django import modulestore
@@ -195,12 +196,14 @@ class LmsModuleSystem(LmsHandlerUrls, ModuleSystem):  # pylint: disable=abstract
     ModuleSystem specialized to the LMS
     """
     def __init__(self, **kwargs):
+        request_cache_dict = RequestCache.get_request_cache().data
         services = kwargs.setdefault('services', {})
         services['user_tags'] = UserTagsService(self)
         services['partitions'] = LmsPartitionService(
             user=kwargs.get('user'),
             course_id=kwargs.get('course_id'),
             track_function=kwargs.get('track_function', None),
+            cache=request_cache_dict
         )
         services['library_tools'] = LibraryToolsService(modulestore())
         services['fs'] = xblock.reference.plugins.FSService()