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}.. """