From 3e5f7bacdd641c50433509c521696e1dfa24c9b4 Mon Sep 17 00:00:00 2001 From: christopher lee Date: Tue, 23 Dec 2014 16:00:14 -0500 Subject: [PATCH] MA-187 ValueError fix Fix for MA-187 where a value error was thrown when a course has a video directly under a section. This PR handles the situations where a chapter or section can be none, and then returns the appropriate url if available. --- common/test/data/split_test_module/course.xml | 2 +- .../mobile_api/video_outlines/serializers.py | 101 ++++++++--- .../mobile_api/video_outlines/tests.py | 162 ++++++++++++++++-- 3 files changed, 228 insertions(+), 37 deletions(-) diff --git a/common/test/data/split_test_module/course.xml b/common/test/data/split_test_module/course.xml index 0880f8ccd4..206eac969e 100644 --- a/common/test/data/split_test_module/course.xml +++ b/common/test/data/split_test_module/course.xml @@ -16,4 +16,4 @@ - \ No newline at end of file + diff --git a/lms/djangoapps/mobile_api/video_outlines/serializers.py b/lms/djangoapps/mobile_api/video_outlines/serializers.py index 30a75a46be..031512dba3 100644 --- a/lms/djangoapps/mobile_api/video_outlines/serializers.py +++ b/lms/djangoapps/mobile_api/video_outlines/serializers.py @@ -48,37 +48,86 @@ class BlockOutline(object): return reversed(block_path) def find_urls(block): - """section and unit urls for 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) - course, chapter, section, unit = list(reversed(block_path))[:4] - position = 1 - unit_name = unit.url_name - for block in section.children: - if block.name == unit_name: - break - position += 1 + block_list = list(reversed(block_path)) + block_count = len(block_list) - kwargs = dict( - course_id=course.id.to_deprecated_string(), - chapter=chapter.url_name, - section=section.url_name - ) - section_url = reverse( - "courseware_section", - kwargs=kwargs, - request=self.request, - ) - kwargs['position'] = position - unit_url = reverse( - "courseware_position", - kwargs=kwargs, - request=self.request, - ) - return unit_url, section_url, block_path + 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 @@ -100,7 +149,7 @@ class BlockOutline(object): summary_fn = self.categories_to_outliner[curr_block.category] block_path = list(path(curr_block)) - unit_url, section_url, _ = find_urls(curr_block) + unit_url, section_url = find_urls(curr_block) yield { "path": block_path, diff --git a/lms/djangoapps/mobile_api/video_outlines/tests.py b/lms/djangoapps/mobile_api/video_outlines/tests.py index afdd1e9ce2..be1556a0b8 100644 --- a/lms/djangoapps/mobile_api/video_outlines/tests.py +++ b/lms/djangoapps/mobile_api/video_outlines/tests.py @@ -1,6 +1,7 @@ """ Tests for video outline API """ +# pylint: disable=no-member from uuid import uuid4 from collections import namedtuple @@ -47,6 +48,17 @@ class TestVideoAPITestCase(MobileAPITestCase): metadata={'graded': True, 'format': 'Homework'}, display_name=None, ) + self.split_unit = ItemFactory.create( + parent_location=self.sub_section.location, + category="vertical", + display_name=u"split test vertical\u03a9", + ) + + self.split_test = ItemFactory.create( + parent_location=self.split_unit.location, + category="split_test", + display_name=u"split test unit" + ) self.edx_video_id = 'testing-123' @@ -93,16 +105,18 @@ class TestVideoAPITestCase(MobileAPITestCase): Creates and returns a video with stored subtitles. """ subid = uuid4().hex - transcripts_utils.save_subs_to_store({ - 'start': [100, 200, 240, 390, 1000], - 'end': [200, 240, 380, 1000, 1500], - 'text': [ - 'subs #1', - 'subs #2', - 'subs #3', - 'subs #4', - 'subs #5' - ]}, + transcripts_utils.save_subs_to_store( + { + 'start': [100, 200, 240, 390, 1000], + 'end': [200, 240, 380, 1000, 1500], + 'text': [ + 'subs #1', + 'subs #2', + 'subs #3', + 'subs #4', + 'subs #5' + ] + }, subid, self.course) return ItemFactory.create( @@ -114,6 +128,86 @@ class TestVideoAPITestCase(MobileAPITestCase): ) +class TestEmptyCourseVideoSummaryList(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 test_chapter_is_none(self): + """ + Tests when there is no chapter under course, and video under course + """ + self.login_and_enroll() + ItemFactory.create( + parent_location=self.course.location, + category="video", + display_name=u"test factory video omega \u03a9", + ) + course_outline = self.api_response().data + self.assertEqual(len(course_outline), 1) + section_url = course_outline[0]["section_url"] + unit_url = course_outline[0]["unit_url"] + self.assertRegexpMatches(section_url, r'courseware$') + self.assertTrue(section_url) + self.assertTrue(unit_url) + self.assertEqual(section_url, unit_url) + + def test_section_is_none(self): + """ + Tests when there is no section under chapter, and video under chapter + """ + self.login_and_enroll() + self.chapter = ItemFactory.create( # pylint:disable=W0201 + parent_location=self.course.location, + category="chapter", + display_name=u"test factory chapter omega \u03a9", + ) + ItemFactory.create( + parent_location=self.chapter.location, + category="video", + display_name=u"test factory video omega \u03a9", + ) + course_outline = self.api_response().data + self.assertEqual(len(course_outline), 1) + section_url = course_outline[0]["section_url"] + unit_url = course_outline[0]["unit_url"] + self.assertRegexpMatches( + section_url, + r'courseware/test_factory_chapter_omega_%CE%A9/$' + ) + self.assertTrue(section_url) + self.assertTrue(unit_url) + self.assertEqual(section_url, unit_url) + + def test_section_under_course(self): + """ + Tests when chapter is none, and video under section under course + """ + self.login_and_enroll() + self.section = ItemFactory.create( # pylint:disable=W0201 + parent_location=self.course.location, + category="sequential", + display_name=u"test factory section omega \u03a9", + ) + ItemFactory.create( + parent_location=self.section.location, + category="video", + display_name=u"test factory video omega \u03a9", + ) + course_outline = self.api_response().data + self.assertEqual(len(course_outline), 1) + section_url = course_outline[0]["section_url"] + unit_url = course_outline[0]["unit_url"] + self.assertRegexpMatches( + section_url, + r'courseware/test_factory_section_omega_%CE%A9/$' + ) + self.assertTrue(section_url) + self.assertTrue(unit_url) + self.assertEqual(section_url, unit_url) + + class TestVideoSummaryList(TestVideoAPITestCase, MobileAuthTestMixin, MobileEnrolledCourseAccessTestMixin): """ Tests for /api/mobile/v0.5/video_outlines/courses/{course_id}.. @@ -172,6 +266,54 @@ class TestVideoSummaryList(TestVideoAPITestCase, MobileAuthTestMixin, MobileEnro self.assertEqual(len(course_outline), 1) self.assertEqual(course_outline[0]['path'][2]['name'], self.nameless_unit.location.block_id) + def test_with_video_in_sub_section(self): + """ + Tests a non standard xml format where a video is underneath a sequential + + We are expecting to return the same unit and section url since there is + no unit vertical. + """ + self.login_and_enroll() + ItemFactory.create( + parent_location=self.sub_section.location, + category="video", + edx_video_id=self.edx_video_id, + display_name=u"video in the sub section" + ) + course_outline = self.api_response().data + self.assertEqual(len(course_outline), 1) + self.assertEqual(len(course_outline[0]['path']), 2) + section_url = course_outline[0]["section_url"] + unit_url = course_outline[0]["unit_url"] + self.assertIn( + u'courseware/test_factory_section_omega_%CE%A9/test_subsection_omega_%CE%A9', + section_url + + ) + self.assertTrue(section_url) + self.assertTrue(unit_url) + self.assertEqual(section_url, unit_url) + + def test_with_split_test(self): + self.login_and_enroll() + + ItemFactory.create( + parent_location=self.split_test.location, + category="video", + display_name=u"split test video a", + ) + ItemFactory.create( + parent_location=self.split_test.location, + category="video", + display_name=u"split test video 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") + def test_with_hidden_blocks(self): self.login_and_enroll() hidden_subsection = ItemFactory.create(