Addresses problem with 'up' and 'down' errors within SOL-289

common/lib/xmodule/xmodule/modulestore/search.py

+''' 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

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):
         """

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())

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

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