Merge pull request #452 from edx/feature/cdodge/add-jump-to-substituions

add a /jump_to_id/ shortcut for producing more durable links between cou...

cms/static/sass/views/_unit.scss

@@ -747,6 +747,7 @@ body.unit {
 
 // Unit Page Sidebar
 .unit-settings {
+
   .window-contents {
     padding: $baseline/2 $baseline;
   }
@@ -854,6 +855,24 @@ body.unit {
 }
 
 .unit-location {
+
+  // unit id
+  .wrapper-unit-id {
+
+    .unit-id {
+
+      .label {
+        @extend .t-title7;
+        margin-bottom: ($baseline/4);
+        color: $gray-d1;
+      }
+
+      .value {
+        margin-bottom: 0;
+      }
+    }
+  }
+
   .url {
     box-shadow: none;
     width: 100%;

cms/templates/unit.html

@@ -171,7 +171,12 @@
         <div class="window unit-location">
           <h4 class="header">${_("Unit Location")}</h4>
           <div class="window-contents">
-            <div><input type="text" class="url" value="/courseware/${section.url_name}/${subsection.url_name}" disabled /></div>
+            <div class="row wrapper-unit-id">
+              <p class="unit-id">
+                <span class="label">${_("Unit Identifier:")}</span>
+                <input type="text" class="url value" value="${unit.location.name}" disabled />
+              </p>
+            </div>
             <ol>
               <li>
                 <a href="${reverse('course_index', kwargs=dict(org=context_course.location.org, course=context_course.location.course, name=context_course.location.name))}" class="section-item">${section.display_name_with_default}</a>

common/djangoapps/static_replace/__init__.py

@@ -43,6 +43,35 @@ def try_staticfiles_lookup(path):
     return url
 
 
+def replace_jump_to_id_urls(text, course_id, jump_to_id_base_url):
+    """
+    This will replace a link to another piece of courseware to a 'jump_to'
+    URL that will redirect to the right place in the courseware
+
+    NOTE: This is similar to replace_course_urls in terms of functionality
+    but it is intended to be used when we only have a 'id' that the
+    course author provides. This is much more helpful when using
+    Studio authored courses since they don't need to know the path. This
+    is also durable with respect to item moves.
+
+    text: The content over which to perform the subtitutions
+    course_id: The course_id in which this rewrite happens
+    jump_to_id_base_url: 
+        A app-tier (e.g. LMS) absolute path to the base of the handler that will perform the
+        redirect. e.g. /courses/<org>/<course>/<run>/jump_to_id. NOTE the <id> will be appended to
+        the end of this URL at re-write time
+
+    output: <text> after the link rewriting rules are applied
+    """
+
+    def replace_jump_to_id_url(match):
+        quote = match.group('quote')
+        rest = match.group('rest')
+        return "".join([quote, jump_to_id_base_url + rest, quote])
+
+    return re.sub(_url_replace_regex('/jump_to_id/'), replace_jump_to_id_url, text)
+
+
 def replace_course_urls(text, course_id):
     """
     Replace /course/$stuff urls with /courses/$course_id/$stuff urls
@@ -53,7 +82,6 @@ def replace_course_urls(text, course_id):
     returns: text with the links replaced
     """
 
-
     def replace_course_url(match):
         quote = match.group('quote')
         rest = match.group('rest')

common/djangoapps/xmodule_modifiers.py

@@ -42,6 +42,28 @@ def wrap_xmodule(get_html, module, template, context=None):
     return _get_html
 
 
+def replace_jump_to_id_urls(get_html, course_id, jump_to_id_base_url):
+    """
+    This will replace a link between courseware in the format
+    /jump_to/<id> with a URL for a page that will correctly redirect
+    This is similar to replace_course_urls, but much more flexible and
+    durable for Studio authored courses. See more comments in static_replace.replace_jump_to_urls
+
+    course_id: The course_id in which this rewrite happens
+    jump_to_id_base_url:
+        A app-tier (e.g. LMS) absolute path to the base of the handler that will perform the
+        redirect. e.g. /courses/<org>/<course>/<run>/jump_to_id. NOTE the <id> will be appended to
+        the end of this URL at re-write time
+
+    output: a wrapped get_html() function pointer, which, when called, will apply the
+        rewrite rules
+    """
+    @wraps(get_html)
+    def _get_html():
+        return static_replace.replace_jump_to_id_urls(get_html(), course_id, jump_to_id_base_url)
+    return _get_html
+
+
 def replace_course_urls(get_html, course_id):
     """
     Updates the supplied module with a new get_html function that wraps

common/test/data/toy/course/2012_Fall.xml

@@ -3,6 +3,7 @@
   <chapter url_name="Overview">
     <videosequence url_name="Toy_Videos">
       <html url_name="secret:toylab"/>
+      <html url_name="toyjumpto"/>
       <video url_name="Video_Resources" youtube_id_1_0="1bK-WdDi6Qw" display_name="Video Resources"/>
     </videosequence>
     <video url_name="Welcome" youtube_id_1_0="p2Q6BrNhdh8" display_name="Welcome"/>

common/test/data/toy/html/toyjumpto.html

@@ -0,0 +1 @@
+<a href="/jump_to_id/vertical_test">This is a link to another page and some Chinese 四節比分和七年前</a> <p>Some more Chinese 四節比分和七年前</p>

common/test/data/toy/html/toyjumpto.xml

@@ -0,0 +1 @@
+<html filename="toyjumpto.html"/>

lms/djangoapps/courseware/module_render.py

@@ -27,7 +27,7 @@ from xmodule.modulestore import Location
 from xmodule.modulestore.django import modulestore
 from xmodule.modulestore.exceptions import ItemNotFoundError
 from xmodule.x_module import ModuleSystem
-from xmodule_modifiers import replace_course_urls, replace_static_urls, add_histogram, wrap_xmodule, save_module  # pylint: disable=F0401
+from xmodule_modifiers import replace_course_urls, replace_jump_to_id_urls, replace_static_urls, add_histogram, wrap_xmodule, save_module  # pylint: disable=F0401
 
 import static_replace
 from psychometrics.psychoanalyze import make_psychometrics_data_update_handler
@@ -393,6 +393,19 @@ def get_module_for_descriptor_internal(user, descriptor, model_data_cache, cours
     #   hierarchy of this course
     module.get_html = replace_course_urls(module.get_html, course_id)
 
+    # this will rewrite intra-courseware links
+    # that use the shorthand /jump_to_id/<id>. This is very helpful
+    # for studio authored courses (compared to the /course/... format) since it is
+    # is durable with respect to moves and the author doesn't need to
+    # know the hierarchy
+    # NOTE: module_id is empty string here. The 'module_id' will get assigned in the replacement
+    # function, we just need to specify something to get the reverse() to work
+    module.get_html = replace_jump_to_id_urls(
+        module.get_html,
+        course_id,
+        reverse('jump_to_id', kwargs={'course_id': course_id, 'module_id': ''})
+    )
+
     if settings.MITX_FEATURES.get('DISPLAY_HISTOGRAMS_TO_STAFF'):
         if has_access(user, module, 'staff', course_id):
             module.get_html = add_histogram(module.get_html, module, user)

lms/djangoapps/courseware/tests/test_module_render.py

@@ -17,6 +17,8 @@ from courseware.tests.tests import LoginEnrollmentTestCase
 from courseware.model_data import ModelDataCache
 from modulestore_config import TEST_DATA_XML_MODULESTORE
 
+from courseware.courses import get_course_with_access
+
 from .factories import UserFactory
 
 
@@ -50,6 +52,35 @@ class ModuleRenderTestCase(LoginEnrollmentTestCase):
         self.assertIsNone(render.get_module('dummyuser', None,
                                             'invalid location', None, None))
 
+    def test_module_render_with_jump_to_id(self):
+        """
+        This test validates that the /jump_to_id/<id> shorthand for intracourse linking works assertIn
+        expected. Note there's a HTML element in the 'toy' course with the url_name 'toyjumpto' which
+        defines this linkage
+        """
+        mock_request = MagicMock()
+        mock_request.user = self.mock_user
+
+        course = get_course_with_access(self.mock_user, self.course_id, 'load')
+
+        model_data_cache = ModelDataCache.cache_for_descriptor_descendents(
+            self.course_id, self.mock_user, course, depth=2)
+
+        module = render.get_module(
+            self.mock_user,
+            mock_request,
+            ['i4x', 'edX', 'toy', 'html', 'toyjumpto'],
+            model_data_cache,
+            self.course_id
+        )
+
+        # get the rendered HTML output which should have the rewritten link
+        html = module.get_html()
+
+        # See if the url got rewritten to the target link
+        # note if the URL mapping changes then this assertion will break
+        self.assertIn('/courses/'+self.course_id+'/jump_to_id/vertical_test', html)
+
     def test_modx_dispatch(self):
         self.assertRaises(Http404, render.modx_dispatch, 'dummy', 'dummy',
                           'invalid Location', 'dummy')

lms/djangoapps/courseware/tests/test_views.py

@@ -33,17 +33,30 @@ class TestJumpTo(TestCase):
 
     def test_jumpto_invalid_location(self):
         location = Location('i4x', 'edX', 'toy', 'NoSuchPlace', None)
-        jumpto_url = '%s/%s/jump_to/%s' % ('/courses', self.course_name, location)
+        jumpto_url = '{0}/{1}/jump_to/{2}'.format('/courses', self.course_name, location)
         response = self.client.get(jumpto_url)
         self.assertEqual(response.status_code, 404)
 
     def test_jumpto_from_chapter(self):
         location = Location('i4x', 'edX', 'toy', 'chapter', 'Overview')
-        jumpto_url = '%s/%s/jump_to/%s' % ('/courses', self.course_name, location)
+        jumpto_url = '{0}/{1}/jump_to/{2}'.format('/courses', self.course_name, location)
         expected = 'courses/edX/toy/2012_Fall/courseware/Overview/'
         response = self.client.get(jumpto_url)
         self.assertRedirects(response, expected, status_code=302, target_status_code=302)
 
+    def test_jumpto_id(self):
+        location = Location('i4x', 'edX', 'toy', 'chapter', 'Overview')
+        jumpto_url = '{0}/{1}/jump_to_id/{2}'.format('/courses', self.course_name, location.name)
+        expected = 'courses/edX/toy/2012_Fall/courseware/Overview/'
+        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('i4x', 'edX', 'toy', 'NoSuchPlace', None)
+        jumpto_url = '{0}/{1}/jump_to_id/{2}'.format('/courses', self.course_name, location.name)
+        response = self.client.get(jumpto_url)
+        self.assertEqual(response.status_code, 404)
+
 
 class ViewsTestCase(TestCase):
     def setUp(self):

lms/djangoapps/courseware/views.py

@@ -33,6 +33,7 @@ from xmodule.modulestore import Location
 from xmodule.modulestore.django import modulestore
 from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError, NoPathToItem
 from xmodule.modulestore.search import path_to_location
+from xmodule.course_module import CourseDescriptor
 
 import comment_client
 
@@ -446,6 +447,27 @@ def index(request, course_id, chapter=None, section=None,
     return result
 
 
+@ensure_csrf_cookie
+def jump_to_id(request, course_id, module_id):
+    """
+    This entry point allows for a shorter version of a jump to where just the id of the element is
+    passed in. This assumes that id is unique within the course_id namespace
+    """
+
+    course_location = CourseDescriptor.id_to_location(course_id)
+
+    items = modulestore().get_items(['i4x', course_location.org, course_location.course, None, module_id])
+
+    if len(items) == 0:
+        raise Http404("Could not find id = {0} in course_id = {1}. Referer = {2}".
+                      format(module_id, course_id, request.META.get("HTTP_REFERER", "")))
+    if len(items) > 1:
+        log.warning("Multiple items found with id = {0} in course_id = {1}. Referer = {2}. Using first found {3}...".
+                    format(module_id, course_id, request.META.get("HTTP_REFERER", ""), items[0].location.url()))
+
+    return jump_to(request, course_id, items[0].location.url())
+
+
 @ensure_csrf_cookie
 def jump_to(request, course_id, location):
     """

lms/urls.py

@@ -177,6 +177,8 @@ if settings.COURSEWARE_ENABLED:
     urlpatterns += (
         url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/jump_to/(?P<location>.*)$',
             'courseware.views.jump_to', name="jump_to"),
+        url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/jump_to_id/(?P<module_id>.*)$',
+            'courseware.views.jump_to_id', name="jump_to_id"),
         url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/modx/(?P<location>.*?)/(?P<dispatch>[^/]*)$',
             'courseware.module_render.modx_dispatch',
             name='modx_dispatch'),