From a495bb5f9ef8b1106f32417aaf5e0296455592ba Mon Sep 17 00:00:00 2001 From: Martyn James Date: Tue, 3 Mar 2015 09:29:20 -0500 Subject: [PATCH] Addresses problem with 'up' and 'down' errors within SOL-289 --- .../lib/xmodule/xmodule/modulestore/search.py | 32 ++++++++ .../tests/test_mixed_modulestore.py | 14 +++- lms/djangoapps/courseware/tests/test_views.py | 79 +++++++++++++++++++ lms/djangoapps/courseware/views.py | 30 +++++-- .../courseware_search/lms_result_processor.py | 15 ++-- 5 files changed, 155 insertions(+), 15 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/search.py b/common/lib/xmodule/xmodule/modulestore/search.py index 8854aa1865..14cea26498 100644 --- a/common/lib/xmodule/xmodule/modulestore/search.py +++ b/common/lib/xmodule/xmodule/modulestore/search.py @@ -1,5 +1,10 @@ +''' useful functions for finding content and its position ''' +from logging import getLogger + from .exceptions import (ItemNotFoundError, NoPathToItem) +LOGGER = getLogger(__name__) + def path_to_location(modulestore, usage_key): ''' @@ -105,3 +110,30 @@ def path_to_location(modulestore, usage_key): position = "_".join(position_list) return (course_id, chapter, section, position) + + +def navigation_index(position): + """ + Get the navigation index from the position argument (where the position argument was recieved from a call to + path_to_location) + + Argument: + position - result of position returned from call to path_to_location. This is an underscore (_) separated string of + vertical 1-indexed positions. If the course is built in Studio then you'll never see verticals as children of + verticals, and so extremely often one will only see the first vertical as an integer position. This specific action + is to allow navigation / breadcrumbs to locate the topmost item because this is the location actually required by + the LMS code + + Returns: + 1-based integer of the position of the desired item within the vertical + """ + if position is None: + return None + + try: + navigation_position = int(position.split('_', 1)[0]) + except (ValueError, TypeError): + LOGGER.exception(u'Bad position %r passed to navigation_index, will assume first position', position) + navigation_position = 1 + + return navigation_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 f5ce48e85d..cbd6c6762e 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -38,7 +38,7 @@ from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.draft_and_published import UnsupportedRevisionError, DIRECT_ONLY_CATEGORIES from xmodule.modulestore.exceptions import ItemNotFoundError, DuplicateCourseError, ReferentialIntegrityError, NoPathToItem from xmodule.modulestore.mixed import MixedModuleStore -from xmodule.modulestore.search import path_to_location +from xmodule.modulestore.search import path_to_location, navigation_index from xmodule.modulestore.tests.factories import check_mongo_calls, check_exact_number_of_calls, \ mongo_uses_error_check from xmodule.modulestore.tests.utils import create_modulestore_instance, LocationMixin @@ -1153,6 +1153,18 @@ class TestMixedModuleStore(CourseComparisonTest): with self.assertRaises(ItemNotFoundError): path_to_location(self.store, location) + def test_navigation_index(self): + """ + Make sure that navigation_index correctly parses the various position values that we might get from calls to + path_to_location + """ + self.assertEqual(1, navigation_index("1")) + self.assertEqual(10, navigation_index("10")) + self.assertEqual(None, navigation_index(None)) + self.assertEqual(1, navigation_index("1_2")) + self.assertEqual(5, navigation_index("5_2")) + self.assertEqual(7, navigation_index("7_3_5_6_")) + @ddt.data('draft', 'split') def test_revert_to_published_root_draft(self, default_ms): """ diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 4437bd04ae..ab3ec55475 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -71,6 +71,85 @@ class TestJumpTo(ModuleStoreTestCase): response = self.client.get(jumpto_url) self.assertRedirects(response, expected, status_code=302, target_status_code=302) + def test_jumpto_from_section(self): + 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( + course_id=unicode(course.id), + chapter_id=chapter.url_name, + section_id=section.url_name, + ) + jumpto_url = '{0}/{1}/jump_to/{2}'.format( + '/courses', + unicode(course.id), + unicode(section.location), + ) + response = self.client.get(jumpto_url) + self.assertRedirects(response, expected, status_code=302, target_status_code=302) + + def test_jumpto_from_module(self): + course = CourseFactory.create() + chapter = ItemFactory.create(category='chapter', parent_location=course.location) + section = ItemFactory.create(category='sequential', parent_location=chapter.location) + vertical1 = ItemFactory.create(category='vertical', parent_location=section.location) + vertical2 = ItemFactory.create(category='vertical', parent_location=section.location) + 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( + course_id=unicode(course.id), + chapter_id=chapter.url_name, + section_id=section.url_name, + ) + jumpto_url = '{0}/{1}/jump_to/{2}'.format( + '/courses', + unicode(course.id), + unicode(module1.location), + ) + 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( + course_id=unicode(course.id), + chapter_id=chapter.url_name, + section_id=section.url_name, + ) + jumpto_url = '{0}/{1}/jump_to/{2}'.format( + '/courses', + unicode(course.id), + unicode(module2.location), + ) + response = self.client.get(jumpto_url) + self.assertRedirects(response, expected, status_code=302, target_status_code=302) + + def test_jumpto_from_nested_module(self): + course = CourseFactory.create() + chapter = ItemFactory.create(category='chapter', parent_location=course.location) + section = ItemFactory.create(category='sequential', parent_location=chapter.location) + vertical = ItemFactory.create(category='vertical', parent_location=section.location) + nested_section = ItemFactory.create(category='sequential', parent_location=vertical.location) + nested_vertical1 = ItemFactory.create(category='vertical', parent_location=nested_section.location) + # put a module into nested_vertical1 for completeness + ItemFactory.create(category='html', parent_location=nested_vertical1.location) + nested_vertical2 = ItemFactory.create(category='vertical', parent_location=nested_section.location) + module2 = ItemFactory.create(category='html', parent_location=nested_vertical2.location) + + # internal position of module2 will be 1_2 (2nd item withing 1st item) + + expected = 'courses/{course_id}/courseware/{chapter_id}/{section_id}/1'.format( + course_id=unicode(course.id), + chapter_id=chapter.url_name, + section_id=section.url_name, + ) + jumpto_url = '{0}/{1}/jump_to/{2}'.format( + '/courses', + unicode(course.id), + unicode(module2.location), + ) + response = self.client.get(jumpto_url) + self.assertRedirects(response, expected, status_code=302, target_status_code=302) + def test_jumpto_id_invalid_location(self): location = Location('edX', 'toy', 'NoSuchPlace', None, None, None) jumpto_url = '{0}/{1}/jump_to_id/{2}'.format('/courses', self.course_key.to_deprecated_string(), location.to_deprecated_string()) diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 961d924d36..8c674b0217 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -50,7 +50,7 @@ from util.cache import cache, cache_if_anonymous from xblock.fragment import Fragment from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError, NoPathToItem -from xmodule.modulestore.search import path_to_location +from xmodule.modulestore.search import path_to_location, navigation_index from xmodule.tabs import CourseTabList, StaffGradingTab, PeerGradingTab, OpenEndedGradingTab from xmodule.x_module import STUDENT_VIEW import shoppingcart @@ -61,7 +61,7 @@ from util.milestones_helpers import get_prerequisite_courses_display from microsite_configuration import microsite from opaque_keys.edx.locations import SlashSeparatedCourseKey -from opaque_keys.edx.keys import CourseKey +from opaque_keys.edx.keys import CourseKey, UsageKey from instructor.enrollment import uses_shib from util.db import commit_on_success_with_read_committed @@ -619,8 +619,8 @@ def jump_to(_request, course_id, location): has access, and what they should see. """ try: - course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) - usage_key = course_key.make_usage_key_from_deprecated_string(location) + course_key = CourseKey.from_string(course_id) + usage_key = UsageKey.from_string(location).replace(course_key=course_key) except InvalidKeyError: raise Http404(u"Invalid course_key or usage_key") try: @@ -634,13 +634,27 @@ def jump_to(_request, course_id, location): # args provided by the redirect. # Rely on index to do all error handling and access control. if chapter is None: - return redirect('courseware', course_id=course_key.to_deprecated_string()) + return redirect('courseware', course_id=unicode(course_key)) elif section is None: - return redirect('courseware_chapter', course_id=course_key.to_deprecated_string(), chapter=chapter) + return redirect('courseware_chapter', course_id=unicode(course_key), chapter=chapter) elif position is None: - return redirect('courseware_section', course_id=course_key.to_deprecated_string(), chapter=chapter, section=section) + return redirect( + 'courseware_section', + course_id=unicode(course_key), + chapter=chapter, + section=section + ) else: - return redirect('courseware_position', course_id=course_key.to_deprecated_string(), chapter=chapter, section=section, position=position) + # Here we use the navigation_index from the position returned from + # path_to_location - we can only navigate to the topmost vertical at the + # moment + return redirect( + 'courseware_position', + course_id=unicode(course_key), + chapter=chapter, + section=section, + position=navigation_index(position) + ) @ensure_csrf_cookie diff --git a/lms/lib/courseware_search/lms_result_processor.py b/lms/lib/courseware_search/lms_result_processor.py index 51ffb38214..0b8d7c2e88 100644 --- a/lms/lib/courseware_search/lms_result_processor.py +++ b/lms/lib/courseware_search/lms_result_processor.py @@ -9,7 +9,7 @@ from django.utils.translation import ugettext as _ from opaque_keys.edx.locations import SlashSeparatedCourseKey from search.result_processor import SearchResultProcessor from xmodule.modulestore.django import modulestore -from xmodule.modulestore.search import path_to_location +from xmodule.modulestore.search import path_to_location, navigation_index from courseware.access import has_access @@ -77,10 +77,10 @@ class LmsSearchResultProcessor(SearchResultProcessor): def get_position_name(section, position): """ helper to fetch name corresponding to the position therein """ - pos = int(position) - section_item = self.get_item(course_key.make_usage_key("sequential", section)) - if section_item.has_children and len(section_item.children) >= pos: - return get_display_name(section_item.children[pos - 1]) + if position: + section_item = self.get_item(course_key.make_usage_key("sequential", section)) + if section_item.has_children and len(section_item.children) >= position: + return get_display_name(section_item.children[position - 1]) return None location_description = [] @@ -89,7 +89,10 @@ class LmsSearchResultProcessor(SearchResultProcessor): if section: location_description.append(get_display_name(course_key.make_usage_key("sequential", section))) if position: - location_description.append(get_position_name(section, position)) + # We're only wanting to show the first vertical, so we use the + # navigation_index function to display the same location to which one + # would be sent if navigating + location_description.append(get_position_name(section, navigation_index(position))) return location_description