From 091d6aebe9d4e30b3ced2fdf52e820e4c22cd1d9 Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Wed, 4 Mar 2015 11:34:44 -0500 Subject: [PATCH 1/4] Mobile API: Modularize VideoOutline code --- .../mobile_api/video_outlines/serializers.py | 192 +++++++----------- 1 file changed, 79 insertions(+), 113 deletions(-) diff --git a/lms/djangoapps/mobile_api/video_outlines/serializers.py b/lms/djangoapps/mobile_api/video_outlines/serializers.py index 76ed9b972e..58b882aee8 100644 --- a/lms/djangoapps/mobile_api/video_outlines/serializers.py +++ b/lms/djangoapps/mobile_api/video_outlines/serializers.py @@ -30,108 +30,17 @@ class BlockOutline(object): self.local_cache['course_videos'] = {} def __iter__(self): + def parent_or_requested_block_type(usage_key): + """ + Returns whether the usage_key's block_type is one of self.block_types or a parent type. + """ + return ( + usage_key.block_type in self.block_types or + usage_key.block_type in BLOCK_TYPES_WITH_CHILDREN + ) + child_to_parent = {} stack = [self.start_block] - - # path should be optional - def path(block): - """path for block""" - block_path = [] - while block in child_to_parent: - block = child_to_parent[block] - if block is not self.start_block: - block_path.append({ - # to be consistent with other edx-platform clients, return the defaulted display name - 'name': block.display_name_with_default, - 'category': block.category, - 'id': unicode(block.location) - }) - return reversed(block_path) - - def find_urls(block): - """ - Find the section and unit urls for a block. - - Returns: - unit_url, section_url: - unit_url (str): The url of a unit - section_url (str): The url of a section - - """ - block_path = [] - while block in child_to_parent: - block = child_to_parent[block] - block_path.append(block) - - block_list = list(reversed(block_path)) - block_count = len(block_list) - - chapter_id = block_list[1].location.block_id if block_count > 1 else None - section = block_list[2] if block_count > 2 else None - position = None - - #position is found traversing the section block - if block_count > 3: - position = 1 - for block in section.children: - if block.name == block_list[3].url_name: - break - position += 1 - - if chapter_id is None: - no_chapter_url = reverse( - "courseware", - kwargs=dict( - course_id=unicode(self.course_id), - ), - request=self.request - ) - return no_chapter_url, no_chapter_url - elif section is None: - no_section_url = reverse( - "courseware_chapter", - kwargs=dict( - course_id=unicode(self.course_id), - chapter=chapter_id - ), - request=self.request - ) - return no_section_url, no_section_url - elif position is None: - no_position_url = reverse( - "courseware_section", - kwargs=dict( - course_id=unicode(self.course_id), - chapter=chapter_id, - section=section.url_name - ), - request=self.request - ) - return no_position_url, no_position_url - else: - section_url = reverse( - "courseware_section", - kwargs=dict( - course_id=unicode(self.course_id), - chapter=chapter_id, - section=section.url_name - ), - request=self.request - ) - unit_url = reverse( - "courseware_position", - kwargs=dict( - course_id=unicode(self.course_id), - chapter=chapter_id, - section=section.url_name, - position=position - ), - request=self.request - ) - return unit_url, section_url - - user = self.request.user - while stack: curr_block = stack.pop() @@ -145,12 +54,12 @@ class BlockOutline(object): continue if curr_block.location.block_type in self.block_types: - if not has_access(user, 'load', curr_block, course_key=self.course_id): + if not has_access(self.request.user, 'load', curr_block, course_key=self.course_id): continue summary_fn = self.block_types[curr_block.category] - block_path = list(path(curr_block)) - unit_url, section_url = find_urls(curr_block) + block_path = list(path(curr_block, child_to_parent, self.start_block)) + unit_url, section_url = find_urls(self.course_id, curr_block, child_to_parent, self.request) yield { "path": block_path, @@ -160,21 +69,78 @@ class BlockOutline(object): "summary": summary_fn(self.course_id, curr_block, self.request, self.local_cache) } - def parent_or_requested_block_type(usage_key): - """ - Returns whether the usage_key's block_type is one of self.block_types or a parent type. - """ - return ( - usage_key.block_type in self.block_types or - usage_key.block_type in BLOCK_TYPES_WITH_CHILDREN - ) - if curr_block.has_children: - for block in reversed(curr_block.get_children(usage_key_filter=parent_or_requested_block_type)): + children = curr_block.get_children(usage_key_filter=parent_or_requested_block_type) + for block in reversed(children): stack.append(block) child_to_parent[block] = curr_block +def path(block, child_to_parent, start_block): + """path for block""" + block_path = [] + while block in child_to_parent: + block = child_to_parent[block] + if block is not start_block: + block_path.append({ + # to be consistent with other edx-platform clients, return the defaulted display name + 'name': block.display_name_with_default, + 'category': block.category, + 'id': unicode(block.location) + }) + return reversed(block_path) + + +def find_urls(course_id, block, child_to_parent, request): + """ + Find the section and unit urls for a block. + + Returns: + unit_url, section_url: + unit_url (str): The url of a unit + section_url (str): The url of a section + + """ + block_path = [] + while block in child_to_parent: + block = child_to_parent[block] + block_path.append(block) + + block_list = list(reversed(block_path)) + block_count = len(block_list) + + chapter_id = block_list[1].location.block_id if block_count > 1 else None + section = block_list[2] if block_count > 2 else None + position = None + + if block_count > 3: + position = 1 + for block in section.children: + if block.name == block_list[3].url_name: + break + position += 1 + + kwargs = {'course_id': unicode(course_id)} + if chapter_id is None: + no_chapter_url = reverse("courseware", kwargs=kwargs, request=request) + return no_chapter_url, no_chapter_url + + kwargs['chapter'] = chapter_id + if section is None: + no_section_url = reverse("courseware_chapter", kwargs=kwargs, request=request) + return no_section_url, no_section_url + + kwargs['section'] = section.url_name + if position is None: + no_position_url = reverse("courseware_section", kwargs=kwargs, request=request) + return no_position_url, no_position_url + + section_url = reverse("courseware_section", kwargs=kwargs, request=request) + kwargs['position'] = position + unit_url = reverse("courseware_position", kwargs=kwargs, request=request) + return unit_url, section_url + + def video_summary(course, course_id, video_descriptor, request, local_cache): """ returns summary dict for the given video module From eca523e01e345ae46342817fb55e13d44f0e1868 Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Wed, 4 Mar 2015 21:36:18 -0500 Subject: [PATCH 2/4] Mobile API MA-7: Support A/B test and cohorted content --- common/djangoapps/util/module_utils.py | 29 +-- common/lib/xmodule/xmodule/x_module.py | 4 +- .../mobile_api/video_outlines/serializers.py | 22 +- .../mobile_api/video_outlines/tests.py | 226 +++++++++++++++--- 4 files changed, 227 insertions(+), 54 deletions(-) diff --git a/common/djangoapps/util/module_utils.py b/common/djangoapps/util/module_utils.py index 017f879b1d..15a5e0f003 100644 --- a/common/djangoapps/util/module_utils.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index a251d48db6..9c2d59dd2b 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/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) diff --git a/lms/djangoapps/mobile_api/video_outlines/serializers.py b/lms/djangoapps/mobile_api/video_outlines/serializers.py index 58b882aee8..aaa32c24a7 100644 --- a/lms/djangoapps/mobile_api/video_outlines/serializers.py +++ b/lms/djangoapps/mobile_api/video_outlines/serializers.py @@ -5,6 +5,9 @@ from rest_framework.reverse import reverse from xmodule.modulestore.mongo.base import BLOCK_TYPES_WITH_CHILDREN from courseware.access import has_access +from courseware.model_data import FieldDataCache +from courseware.module_render import get_module_for_descriptor +from util.module_utils import get_dynamic_descriptor_children from edxval.api import ( get_video_info_for_course_and_profile, ValInternalError @@ -39,6 +42,17 @@ class BlockOutline(object): usage_key.block_type in BLOCK_TYPES_WITH_CHILDREN ) + def create_module(descriptor): + """ + Factory method for creating and binding a module for the given descriptor. + """ + field_data_cache = FieldDataCache.cache_for_descriptor_descendents( + self.course_id, self.request.user, descriptor + ) + return get_module_for_descriptor( + self.request.user, self.request, descriptor, field_data_cache, self.course_id + ) + child_to_parent = {} stack = [self.start_block] while stack: @@ -49,7 +63,7 @@ class BlockOutline(object): # the hierarchy. The reason being is that these blocks may not have human-readable names # to display on the mobile clients. # Eventually, we'll need to figure out how we want these blocks to be displayed on the - # mobile clients. As, they are still accessible in the browser, just not navigatable + # mobile clients. As they are still accessible in the browser, just not navigatable # from the table-of-contents. continue @@ -70,7 +84,11 @@ class BlockOutline(object): } if curr_block.has_children: - children = curr_block.get_children(usage_key_filter=parent_or_requested_block_type) + children = get_dynamic_descriptor_children( + curr_block, + create_module, + usage_key_filter=parent_or_requested_block_type + ) for block in reversed(children): stack.append(block) child_to_parent[block] = curr_block diff --git a/lms/djangoapps/mobile_api/video_outlines/tests.py b/lms/djangoapps/mobile_api/video_outlines/tests.py index 65729aa20b..fd05d7d6c9 100644 --- a/lms/djangoapps/mobile_api/video_outlines/tests.py +++ b/lms/djangoapps/mobile_api/video_outlines/tests.py @@ -3,6 +3,8 @@ Tests for video outline API """ # pylint: disable=no-member +import ddt +import itertools from uuid import uuid4 from collections import namedtuple @@ -10,6 +12,10 @@ from edxval import api from xmodule.modulestore.tests.factories import ItemFactory from xmodule.video_module import transcripts_utils from xmodule.modulestore.django import modulestore +from xmodule.partitions.partitions import Group, UserPartition + +from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory +from openedx.core.djangoapps.course_groups.models import CourseUserGroupPartitionGroup from ..testutils import MobileAPITestCase, MobileAuthTestMixin, MobileEnrolledCourseAccessTestMixin @@ -49,20 +55,8 @@ class TestVideoAPITestCase(MobileAPITestCase): metadata={'graded': True, 'format': 'Homework'}, display_name=None, ) - self.split_unit = ItemFactory.create( - parent=self.sub_section, - category="vertical", - display_name=u"split test vertical\u03a9", - ) - - self.split_test = ItemFactory.create( - parent=self.split_unit, - category="split_test", - display_name=u"split test unit" - ) self.edx_video_id = 'testing-123' - self.video_url = 'http://val.edx.org/val/video.mp4' self.html5_video_url = 'http://video.edx.org/html5/video.mp4' @@ -101,6 +95,11 @@ class TestVideoAPITestCase(MobileAPITestCase): } ]}) + +class TestVideoAPIMixin(object): + """ + Mixin class that provides helpers for testing video related mobile APIs + """ def _create_video_with_subs(self, custom_subid=None): """ Creates and returns a video with stored subtitles. @@ -128,32 +127,77 @@ class TestVideoAPITestCase(MobileAPITestCase): sub=subid ) - -class TestNonStandardCourseStructure(MobileAPITestCase): - """ - Tests /api/mobile/v0.5/video_outlines/courses/{course_id} with no course set - """ - REVERSE_INFO = {'name': 'video-summary-list', 'params': ['course_id']} - - def _verify_paths(self, course_outline, path_list): + def _verify_paths(self, course_outline, path_list, outline_index=0): """ Takes a path_list and compares it against the course_outline Attributes: - path_list (list): A list of the expected strings course_outline (list): A list of dictionaries that includes a 'path' and 'named_path' field which we will be comparing path_list to + path_list (list): A list of the expected strings + outline_index (int): Index into the course_outline list for which the + path is being tested. """ - path = course_outline[0]['path'] + path = course_outline[outline_index]['path'] self.assertEqual(len(path), len(path_list)) for i in range(0, len(path_list)): self.assertEqual(path_list[i], path[i]['name']) #named_path will be deprecated eventually - named_path = course_outline[0]['named_path'] + named_path = course_outline[outline_index]['named_path'] self.assertEqual(len(named_path), len(path_list)) for i in range(0, len(path_list)): self.assertEqual(path_list[i], named_path[i]) + def _setup_course_partitions(self, scheme_id='random', is_cohorted=False): + """Helper method to configure the user partitions in the course.""" + self.partition_id = 0 # pylint: disable=attribute-defined-outside-init + self.course.user_partitions = [ + UserPartition( + self.partition_id, 'first_partition', 'First Partition', + [Group(0, 'alpha'), Group(1, 'beta')], + scheme=None, scheme_id=scheme_id + ), + ] + self.course.cohort_config = {'cohorted': is_cohorted} + self.store.update_item(self.course, self.user.id) + + def _setup_group_access(self, xblock, partition_id, group_ids): + """Helper method to configure the partition and group mapping for the given xblock.""" + xblock.group_access = {partition_id: group_ids} + self.store.update_item(xblock, self.user.id) + + def _setup_split_module(self, sub_block_category): + """Helper method to configure a split_test unit with children of type sub_block_category.""" + self._setup_course_partitions() + self.split_test = ItemFactory.create( # pylint: disable=attribute-defined-outside-init + parent=self.unit, + category="split_test", + display_name=u"split test unit", + user_partition_id=0, + ) + sub_block_a = ItemFactory.create( + parent=self.split_test, + category=sub_block_category, + display_name=u"split test block a", + ) + sub_block_b = ItemFactory.create( + parent=self.split_test, + category=sub_block_category, + display_name=u"split test block b", + ) + self.split_test.group_id_to_child = { + str(index): url for index, url in enumerate([sub_block_a.location, sub_block_b.location]) + } + self.store.update_item(self.split_test, self.user.id) + return sub_block_a, sub_block_b + + +class TestNonStandardCourseStructure(MobileAPITestCase, TestVideoAPIMixin): + """ + Tests /api/mobile/v0.5/video_outlines/courses/{course_id} with no course set + """ + REVERSE_INFO = {'name': 'video-summary-list', 'params': ['course_id']} + def setUp(self): super(TestNonStandardCourseStructure, self).setUp() self.chapter_under_course = ItemFactory.create( @@ -357,7 +401,10 @@ class TestNonStandardCourseStructure(MobileAPITestCase): ) -class TestVideoSummaryList(TestVideoAPITestCase, MobileAuthTestMixin, MobileEnrolledCourseAccessTestMixin): +@ddt.ddt +class TestVideoSummaryList( + TestVideoAPITestCase, MobileAuthTestMixin, MobileEnrolledCourseAccessTestMixin, TestVideoAPIMixin # pylint: disable=bad-continuation +): """ Tests for /api/mobile/v0.5/video_outlines/courses/{course_id}.. """ @@ -443,25 +490,130 @@ class TestVideoSummaryList(TestVideoAPITestCase, MobileAuthTestMixin, MobileEnro self.assertTrue(unit_url) self.assertEqual(section_url, unit_url) - def test_with_split_test(self): + @ddt.data( + *itertools.product([True, False], ["video", "problem"]) + ) + @ddt.unpack + def test_with_split_block(self, is_user_staff, sub_block_category): + """Test with split_module->sub_block_category and for both staff and non-staff users.""" self.login_and_enroll() + self.user.is_staff = is_user_staff + self.user.save() + self._setup_split_module(sub_block_category) + + video_outline = self.api_response().data + num_video_blocks = 1 if sub_block_category == "video" else 0 + self.assertEqual(len(video_outline), num_video_blocks) + for block_index in range(num_video_blocks): + self._verify_paths( + video_outline, + [ + self.section.display_name, + self.sub_section.display_name, + self.unit.display_name, + self.split_test.display_name + ], + block_index + ) + self.assertIn(u"split test block", video_outline[block_index]["summary"]["name"]) + + def test_with_split_vertical(self): + """Test with split_module->vertical->video structure.""" + self.login_and_enroll() + split_vertical_a, split_vertical_b = self._setup_split_module("vertical") ItemFactory.create( - parent=self.split_test, + parent=split_vertical_a, category="video", - display_name=u"split test video a", + display_name=u"video in vertical a", ) ItemFactory.create( - parent=self.split_test, + parent=split_vertical_b, category="video", - display_name=u"split test video b", + display_name=u"video in vertical b", ) - course_outline = self.api_response().data - self.assertEqual(len(course_outline), 2) - self.assertEqual(len(course_outline[0]["path"]), 4) - self.assertEqual(len(course_outline[1]["path"]), 4) - self.assertEqual(course_outline[0]["summary"]["name"], u"split test video a") - self.assertEqual(course_outline[1]["summary"]["name"], u"split test video b") + + video_outline = self.api_response().data + + # user should see only one of the videos (a or b). + self.assertEqual(len(video_outline), 1) + self.assertIn(u"video in vertical", video_outline[0]["summary"]["name"]) + a_or_b = video_outline[0]["summary"]["name"][-1:] + self._verify_paths( + video_outline, + [ + self.section.display_name, + self.sub_section.display_name, + self.unit.display_name, + self.split_test.display_name, + u"split test block " + a_or_b + ], + ) + + def _create_cohorted_video(self, group_id): + """Creates a cohorted video block, giving access to only the given group_id.""" + video_block = ItemFactory.create( + parent=self.unit, + category="video", + display_name=u"video for group " + unicode(group_id), + ) + self._setup_group_access(video_block, self.partition_id, [group_id]) + + def _create_cohorted_vertical_with_video(self, group_id): + """Creates a cohorted vertical with a child video block, giving access to only the given group_id.""" + vertical_block = ItemFactory.create( + parent=self.sub_section, + category="vertical", + display_name=u"vertical for group " + unicode(group_id), + ) + self._setup_group_access(vertical_block, self.partition_id, [group_id]) + ItemFactory.create( + parent=vertical_block, + category="video", + display_name=u"video for group " + unicode(group_id), + ) + + @ddt.data("_create_cohorted_video", "_create_cohorted_vertical_with_video") + def test_with_cohorted_content(self, content_creator_method_name): + self.login_and_enroll() + self._setup_course_partitions(scheme_id='cohort', is_cohorted=True) + + cohorts = [] + for group_id in [0, 1]: + getattr(self, content_creator_method_name)(group_id) + + cohorts.append(CohortFactory(course_id=self.course.id, name=u"Cohort " + unicode(group_id))) + link = CourseUserGroupPartitionGroup( + course_user_group=cohorts[group_id], + partition_id=self.partition_id, + group_id=group_id, + ) + link.save() + + for cohort_index in range(len(cohorts)): + # add user to this cohort + cohorts[cohort_index].users.add(self.user) + + # should only see video for this cohort + video_outline = self.api_response().data + self.assertEqual(len(video_outline), 1) + self.assertEquals( + u"video for group " + unicode(cohort_index), + video_outline[0]["summary"]["name"] + ) + + # remove user from this cohort + cohorts[cohort_index].users.remove(self.user) + + # un-cohorted user should see no videos + video_outline = self.api_response().data + self.assertEqual(len(video_outline), 0) + + # staff user sees all videos + self.user.is_staff = True + self.user.save() + video_outline = self.api_response().data + self.assertEqual(len(video_outline), 2) def test_with_hidden_blocks(self): self.login_and_enroll() @@ -558,7 +710,9 @@ class TestVideoSummaryList(TestVideoAPITestCase, MobileAuthTestMixin, MobileEnro ) -class TestTranscriptsDetail(TestVideoAPITestCase, MobileAuthTestMixin, MobileEnrolledCourseAccessTestMixin): +class TestTranscriptsDetail( + TestVideoAPITestCase, MobileAuthTestMixin, MobileEnrolledCourseAccessTestMixin, TestVideoAPIMixin # pylint: disable=bad-continuation +): """ Tests for /api/mobile/v0.5/video_outlines/transcripts/{course_id}.. """ From d7386ecc87568ef06316d4362a015de09f19ee50 Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Thu, 12 Mar 2015 21:33:41 -0400 Subject: [PATCH 3/4] Use depth=0 for call to cache_for_descriptor_descendents. --- lms/djangoapps/mobile_api/video_outlines/serializers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/mobile_api/video_outlines/serializers.py b/lms/djangoapps/mobile_api/video_outlines/serializers.py index aaa32c24a7..a9499bffeb 100644 --- a/lms/djangoapps/mobile_api/video_outlines/serializers.py +++ b/lms/djangoapps/mobile_api/video_outlines/serializers.py @@ -47,7 +47,7 @@ class BlockOutline(object): Factory method for creating and binding a module for the given descriptor. """ field_data_cache = FieldDataCache.cache_for_descriptor_descendents( - self.course_id, self.request.user, descriptor + self.course_id, self.request.user, descriptor, depth=0, ) return get_module_for_descriptor( self.request.user, self.request, descriptor, field_data_cache, self.course_id From ff77505f420265b586857f2702a04e9f3663316a Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 18 Mar 2015 06:06:04 -0400 Subject: [PATCH 4/4] Add caching to partitions_service. --- .../xmodule/partitions/partitions_service.py | 17 ++++- .../partitions/tests/test_partitions.py | 74 +++++++++++++++++-- lms/djangoapps/lms_xblock/runtime.py | 3 + 3 files changed, 87 insertions(+), 7 deletions(-) diff --git a/common/lib/xmodule/xmodule/partitions/partitions_service.py b/common/lib/xmodule/xmodule/partitions/partitions_service.py index 5eafa11259..e5e5c1a0aa 100644 --- a/common/lib/xmodule/xmodule/partitions/partitions_service.py +++ b/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): """ diff --git a/common/lib/xmodule/xmodule/partitions/tests/test_partitions.py b/common/lib/xmodule/xmodule/partitions/tests/test_partitions.py index a23871fae6..474b9eac69 100644 --- a/common/lib/xmodule/xmodule/partitions/tests/test_partitions.py +++ b/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. diff --git a/lms/djangoapps/lms_xblock/runtime.py b/lms/djangoapps/lms_xblock/runtime.py index 65b0f3eb87..64fd643e62 100644 --- a/lms/djangoapps/lms_xblock/runtime.py +++ b/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()