From a6ae44a0db674545baba0f1ee033e17f208532c4 Mon Sep 17 00:00:00 2001 From: Jonathan Piacenti Date: Wed, 1 Jul 2015 17:57:44 -0500 Subject: [PATCH 1/4] Add ability to activate a child block via jump_to_id. --- .../lib/xmodule/xmodule/modulestore/search.py | 3 +- .../tests/test_mixed_modulestore.py | 17 +++--- lms/djangoapps/courseware/tests/test_views.py | 58 +++++++++++++++++-- lms/djangoapps/courseware/url_helpers.py | 9 ++- lms/djangoapps/courseware/views.py | 3 +- lms/djangoapps/open_ended_grading/utils.py | 2 +- 6 files changed, 76 insertions(+), 16 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/search.py b/common/lib/xmodule/xmodule/modulestore/search.py index 14cea26498..c57059c7b8 100644 --- a/common/lib/xmodule/xmodule/modulestore/search.py +++ b/common/lib/xmodule/xmodule/modulestore/search.py @@ -86,6 +86,7 @@ def path_to_location(modulestore, usage_key): # pull out the location names chapter = path[1].name if n > 1 else None section = path[2].name if n > 2 else None + vertical = path[3].name if n > 3 else None # Figure out the position position = None @@ -109,7 +110,7 @@ def path_to_location(modulestore, usage_key): position_list.append(str(child_locs.index(path[path_index + 1]) + 1)) position = "_".join(position_list) - return (course_id, chapter, section, position) + return (course_id, chapter, section, vertical, position, path[-1]) def navigation_index(position): diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index 74aa23066b..30d6404a93 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -1221,15 +1221,16 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): should_work = ( (self.problem_x1a_2, - (course_key, u"Chapter_x", u"Sequential_x1", '1')), + (course_key, u"Chapter_x", u"Sequential_x1", u'Vertical_x1a', '1', self.problem_x1a_2)), (self.chapter_x, - (course_key, "Chapter_x", None, None)), + (course_key, "Chapter_x", None, None, None, self.chapter_x)), ) for location, expected in should_work: # each iteration has different find count, pop this iter's find count with check_mongo_calls(num_finds.pop(0), num_sends): - self.assertEqual(path_to_location(self.store, location), expected) + path = path_to_location(self.store, location) + self.assertEqual(path, expected) not_found = ( course_key.make_usage_key('video', 'WelcomeX'), @@ -1259,11 +1260,13 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): # only needs course_locations set self.initdb('draft') course_key = self.course_locations[self.XML_COURSEID1].course_key + video_key = course_key.make_usage_key('video', 'Welcome') + chapter_key = course_key.make_usage_key('chapter', 'Overview') should_work = ( - (course_key.make_usage_key('video', 'Welcome'), - (course_key, "Overview", "Welcome", None)), - (course_key.make_usage_key('chapter', 'Overview'), - (course_key, "Overview", None, None)), + (video_key, + (course_key, "Overview", "Welcome", None, None, video_key)), + (chapter_key, + (course_key, "Overview", None, None, None, chapter_key)), ) for location, expected in should_work: diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 8dbfcb42d0..abb0ee2465 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -83,10 +83,11 @@ class TestJumpTo(ModuleStoreTestCase): course = CourseFactory.create() chapter = ItemFactory.create(category='chapter', parent_location=course.location) section = ItemFactory.create(category='sequential', parent_location=chapter.location) - expected = 'courses/{course_id}/courseware/{chapter_id}/{section_id}/'.format( + expected = 'courses/{course_id}/courseware/{chapter_id}/{section_id}/?activate_block_id={section_key}'.format( course_id=unicode(course.id), chapter_id=chapter.url_name, section_id=section.url_name, + section_key=unicode(section.location) ) jumpto_url = '{0}/{1}/jump_to/{2}'.format( '/courses', @@ -105,10 +106,11 @@ class TestJumpTo(ModuleStoreTestCase): module1 = ItemFactory.create(category='html', parent_location=vertical1.location) module2 = ItemFactory.create(category='html', parent_location=vertical2.location) - expected = 'courses/{course_id}/courseware/{chapter_id}/{section_id}/1'.format( + expected = 'courses/{course_id}/courseware/{chapter_id}/{section_id}/1?activate_block_id={module_key}'.format( course_id=unicode(course.id), chapter_id=chapter.url_name, section_id=section.url_name, + module_key=unicode(module1.location) ) jumpto_url = '{0}/{1}/jump_to/{2}'.format( '/courses', @@ -118,10 +120,11 @@ class TestJumpTo(ModuleStoreTestCase): response = self.client.get(jumpto_url) self.assertRedirects(response, expected, status_code=302, target_status_code=302) - expected = 'courses/{course_id}/courseware/{chapter_id}/{section_id}/2'.format( + expected = 'courses/{course_id}/courseware/{chapter_id}/{section_id}/2?activate_block_id={module_key}'.format( course_id=unicode(course.id), chapter_id=chapter.url_name, section_id=section.url_name, + module_key=unicode(module2.location), ) jumpto_url = '{0}/{1}/jump_to/{2}'.format( '/courses', @@ -145,10 +148,11 @@ class TestJumpTo(ModuleStoreTestCase): # internal position of module2 will be 1_2 (2nd item withing 1st item) - expected = 'courses/{course_id}/courseware/{chapter_id}/{section_id}/1'.format( + expected = 'courses/{course_id}/courseware/{chapter_id}/{section_id}/1?activate_block_id={module_key}'.format( course_id=unicode(course.id), chapter_id=chapter.url_name, section_id=section.url_name, + module_key=unicode(module2.location) ) jumpto_url = '{0}/{1}/jump_to/{2}'.format( '/courses', @@ -1084,6 +1088,23 @@ class GenerateUserCertTests(ModuleStoreTestCase): ), resp.content) +class ActivateIDCheckerBlock(XBlock): + """ + XBlock for checking for an activate_block_id entry in the render context. + """ + # We don't need actual children to test this. + has_children = False + + def student_view(self, context): + """ + A student view that displays the activate_block_id context variable. + """ + result = Fragment() + if 'activate_block_id' in context: + result.add_content(u"Activate Block ID: {block_id}

".format(block_id=context['activate_block_id'])) + return result + + class ViewCheckerBlock(XBlock): """ XBlock for testing user state in views. @@ -1117,7 +1138,7 @@ class TestIndexView(ModuleStoreTestCase): @XBlock.register_temp_plugin(ViewCheckerBlock, 'view_checker') @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) - def test_student_state(self, default_store): + def test_student_state(self, default_store, ): """ Verify that saved student state is loaded for xblocks rendered in the index view. """ @@ -1157,6 +1178,33 @@ class TestIndexView(ModuleStoreTestCase): response = views.index(request, unicode(course.id), chapter=chapter.url_name, section=section.url_name) self.assertEquals(response.content.count("ViewCheckerPassed"), 3) + @XBlock.register_temp_plugin(ActivateIDCheckerBlock, 'id_checker') + def test_activate_block_id(self): + user = UserFactory() + + course = CourseFactory.create() + chapter = ItemFactory.create(parent=course, category='chapter') + section = ItemFactory.create(parent=chapter, category='sequential', display_name="Sequence") + vertical = ItemFactory.create(parent=section, category='vertical', display_name="Vertical") + ItemFactory.create(parent=vertical, category='id_checker', display_name="ID Checker") + + CourseEnrollmentFactory(user=user, course_id=course.id) + + request = RequestFactory().get( + reverse( + 'courseware_section', + kwargs={ + 'course_id': unicode(course.id), + 'chapter': chapter.url_name, + 'section': section.url_name, + } + ) + '?activate_block_id=test_block_id' + ) + request.user = user + mako_middleware_process_request(request) + + response = views.index(request, unicode(course.id), chapter=chapter.url_name, section=section.url_name) + self.assertIn("Activate Block ID: test_block_id", response.content) class TestRenderXBlock(RenderXBlockTestMixin, ModuleStoreTestCase): """ diff --git a/lms/djangoapps/courseware/url_helpers.py b/lms/djangoapps/courseware/url_helpers.py index 22f4619396..17723da088 100644 --- a/lms/djangoapps/courseware/url_helpers.py +++ b/lms/djangoapps/courseware/url_helpers.py @@ -20,7 +20,10 @@ def get_redirect_url(course_key, usage_key): Redirect url string """ - (course_key, chapter, section, position) = path_to_location(modulestore(), usage_key) + ( + course_key, chapter, section, vertical_unused, + position, final_target_id + ) = path_to_location(modulestore(), usage_key) # choose the appropriate view (and provide the necessary args) based on the # args provided by the redirect. @@ -43,4 +46,8 @@ def get_redirect_url(course_key, usage_key): 'courseware_position', args=(unicode(course_key), chapter, section, navigation_index(position)) ) + + if final_target_id: + redirect_url += "?activate_block_id={final_target_id}".format(final_target_id=final_target_id) + return redirect_url diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 2b8a4f1285..4c467a5097 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -550,7 +550,8 @@ def _index_bulk_op(request, course_key, chapter, section, position): # Save where we are in the chapter. save_child_position(chapter_module, section) - context['fragment'] = section_module.render(STUDENT_VIEW) + section_render_context = {'activate_block_id': request.GET.get('activate_block_id')} + context['fragment'] = section_module.render(STUDENT_VIEW, section_render_context) context['section_title'] = section_descriptor.display_name_with_default else: # section is none, so display a message diff --git a/lms/djangoapps/open_ended_grading/utils.py b/lms/djangoapps/open_ended_grading/utils.py index 7728ab8d7f..b51ccc75ac 100644 --- a/lms/djangoapps/open_ended_grading/utils.py +++ b/lms/djangoapps/open_ended_grading/utils.py @@ -43,7 +43,7 @@ def generate_problem_url(problem_url_parts, base_course_url): # This is placed between the course id and the rest of the url. if i == 1: problem_url += "courseware/" - problem_url += part + "/" + problem_url += unicode(part) + "/" return problem_url From 405b266fa7491c4677de9c241df8cc308626d086 Mon Sep 17 00:00:00 2001 From: Jonathan Piacenti Date: Fri, 3 Jul 2015 20:52:45 +0000 Subject: [PATCH 2/4] Addressed notes. --- common/lib/xmodule/xmodule/modulestore/search.py | 4 ++-- lms/djangoapps/courseware/tests/test_views.py | 3 ++- lms/djangoapps/courseware/url_helpers.py | 4 ++-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/search.py b/common/lib/xmodule/xmodule/modulestore/search.py index c57059c7b8..0ee4e37d41 100644 --- a/common/lib/xmodule/xmodule/modulestore/search.py +++ b/common/lib/xmodule/xmodule/modulestore/search.py @@ -44,8 +44,8 @@ def path_to_location(modulestore, usage_key): If no path exists, return None. - If a path exists, return it as a list with target location first, and - the starting location last. + If a path exists, return it as a tuple with root location first, and + the target location last. ''' # Standard DFS diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index abb0ee2465..693061f031 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -1138,7 +1138,7 @@ class TestIndexView(ModuleStoreTestCase): @XBlock.register_temp_plugin(ViewCheckerBlock, 'view_checker') @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) - def test_student_state(self, default_store, ): + def test_student_state(self, default_store): """ Verify that saved student state is loaded for xblocks rendered in the index view. """ @@ -1206,6 +1206,7 @@ class TestIndexView(ModuleStoreTestCase): response = views.index(request, unicode(course.id), chapter=chapter.url_name, section=section.url_name) self.assertIn("Activate Block ID: test_block_id", response.content) + class TestRenderXBlock(RenderXBlockTestMixin, ModuleStoreTestCase): """ Tests for the courseware.render_xblock endpoint. diff --git a/lms/djangoapps/courseware/url_helpers.py b/lms/djangoapps/courseware/url_helpers.py index 17723da088..62ba41e42e 100644 --- a/lms/djangoapps/courseware/url_helpers.py +++ b/lms/djangoapps/courseware/url_helpers.py @@ -1,6 +1,7 @@ """ Module to define url helpers functions """ +from urllib import urlencode from xmodule.modulestore.search import path_to_location, navigation_index from xmodule.modulestore.django import modulestore from django.core.urlresolvers import reverse @@ -47,7 +48,6 @@ def get_redirect_url(course_key, usage_key): args=(unicode(course_key), chapter, section, navigation_index(position)) ) - if final_target_id: - redirect_url += "?activate_block_id={final_target_id}".format(final_target_id=final_target_id) + redirect_url += "?{}".format(urlencode({'activate_block_id': unicode(final_target_id)})) return redirect_url From eab0cbdd85c5cd901f2590cc460b652844d923cf Mon Sep 17 00:00:00 2001 From: Jonathan Piacenti Date: Fri, 3 Jul 2015 20:57:02 +0000 Subject: [PATCH 3/4] Better fix for open ended grading. --- lms/djangoapps/open_ended_grading/utils.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/open_ended_grading/utils.py b/lms/djangoapps/open_ended_grading/utils.py index b51ccc75ac..fac12a2cda 100644 --- a/lms/djangoapps/open_ended_grading/utils.py +++ b/lms/djangoapps/open_ended_grading/utils.py @@ -1,4 +1,5 @@ import logging +from urllib import urlencode from xmodule.modulestore import search from xmodule.modulestore.django import modulestore @@ -33,6 +34,8 @@ def generate_problem_url(problem_url_parts, base_course_url): @param base_course_url: Base url of a given course @return: A path to the problem """ + activate_block_id = problem_url_parts[-1] + problem_url_parts = problem_url_parts[0:-1] problem_url = base_course_url + "/" for i, part in enumerate(problem_url_parts): if part is not None: @@ -43,7 +46,8 @@ def generate_problem_url(problem_url_parts, base_course_url): # This is placed between the course id and the rest of the url. if i == 1: problem_url += "courseware/" - problem_url += unicode(part) + "/" + problem_url += part + "/" + problem_url += '?{}'.format(urlencode({'activate_block_id': unicode(activate_block_id)})) return problem_url From c8339072dd3a5bfe23633389c0e36817329976b2 Mon Sep 17 00:00:00 2001 From: Jonathan Piacenti Date: Fri, 3 Jul 2015 22:48:35 +0000 Subject: [PATCH 4/4] Fixup for tests. --- lms/djangoapps/courseware/tests/test_views.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 693061f031..4ece972ea6 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -3,6 +3,7 @@ Tests courseware views.py """ import cgi +from urllib import urlencode import ddt import json import unittest @@ -83,11 +84,11 @@ class TestJumpTo(ModuleStoreTestCase): course = CourseFactory.create() chapter = ItemFactory.create(category='chapter', parent_location=course.location) section = ItemFactory.create(category='sequential', parent_location=chapter.location) - expected = 'courses/{course_id}/courseware/{chapter_id}/{section_id}/?activate_block_id={section_key}'.format( + expected = 'courses/{course_id}/courseware/{chapter_id}/{section_id}/?{activate_block_id}'.format( course_id=unicode(course.id), chapter_id=chapter.url_name, section_id=section.url_name, - section_key=unicode(section.location) + activate_block_id=urlencode({'activate_block_id': unicode(section.location)}) ) jumpto_url = '{0}/{1}/jump_to/{2}'.format( '/courses', @@ -106,11 +107,11 @@ class TestJumpTo(ModuleStoreTestCase): module1 = ItemFactory.create(category='html', parent_location=vertical1.location) module2 = ItemFactory.create(category='html', parent_location=vertical2.location) - expected = 'courses/{course_id}/courseware/{chapter_id}/{section_id}/1?activate_block_id={module_key}'.format( + expected = 'courses/{course_id}/courseware/{chapter_id}/{section_id}/1?{activate_block_id}'.format( course_id=unicode(course.id), chapter_id=chapter.url_name, section_id=section.url_name, - module_key=unicode(module1.location) + activate_block_id=urlencode({'activate_block_id': unicode(module1.location)}) ) jumpto_url = '{0}/{1}/jump_to/{2}'.format( '/courses', @@ -120,11 +121,11 @@ class TestJumpTo(ModuleStoreTestCase): response = self.client.get(jumpto_url) self.assertRedirects(response, expected, status_code=302, target_status_code=302) - expected = 'courses/{course_id}/courseware/{chapter_id}/{section_id}/2?activate_block_id={module_key}'.format( + expected = 'courses/{course_id}/courseware/{chapter_id}/{section_id}/2?{activate_block_id}'.format( course_id=unicode(course.id), chapter_id=chapter.url_name, section_id=section.url_name, - module_key=unicode(module2.location), + activate_block_id=urlencode({'activate_block_id': unicode(module2.location)}) ) jumpto_url = '{0}/{1}/jump_to/{2}'.format( '/courses', @@ -148,11 +149,11 @@ class TestJumpTo(ModuleStoreTestCase): # internal position of module2 will be 1_2 (2nd item withing 1st item) - expected = 'courses/{course_id}/courseware/{chapter_id}/{section_id}/1?activate_block_id={module_key}'.format( + expected = 'courses/{course_id}/courseware/{chapter_id}/{section_id}/1?{activate_block_id}'.format( course_id=unicode(course.id), chapter_id=chapter.url_name, section_id=section.url_name, - module_key=unicode(module2.location) + activate_block_id=urlencode({'activate_block_id': unicode(module2.location)}) ) jumpto_url = '{0}/{1}/jump_to/{2}'.format( '/courses',