Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 10 commits into from
Jul 23, 2013
19 changes: 19 additions & 0 deletions cms/static/sass/views/_unit.scss
Original file line number Diff line number Diff line change
Expand Up @@ -747,6 +747,7 @@ body.unit {

// Unit Page Sidebar
.unit-settings {

.window-contents {
padding: $baseline/2 $baseline;
}
Expand Down Expand Up @@ -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%;
Expand Down
7 changes: 6 additions & 1 deletion cms/templates/unit.html
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,12 @@ <h4 class="header">${_("Unit Settings")}</h4>
<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 />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for escaping? What kinds of things can be in a name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

location.name here is always a GUID when it is created in Studio. If it's imported from an XML course, then it will be the 'url_name', which - I presume - is appropriate for URL purposes.

</p>
</div>
<ol>
<li>
<a href="#" class="section-item">${section.display_name_with_default}</a>
Expand Down
30 changes: 29 additions & 1 deletion common/djangoapps/static_replace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please give an example of a jump_to_id_base_url (absolute, relative?), and specify what's returned?


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])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this been tested with non-ASCII text?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, any suggestions on how to do so with the XML 'toy' course?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just add some non-ASCII chars inside and outside the link text in toyjumpto.html?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK added some chinese text to toyjumpto.html.


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
Expand All @@ -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')
Expand Down
22 changes: 22 additions & 0 deletions common/djangoapps/xmodule_modifiers.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,28 @@ def _get_html():
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
Expand Down
1 change: 1 addition & 0 deletions common/test/data/toy/course/2012_Fall.xml
Original file line number Diff line number Diff line change
Expand Up @@ -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"/>
Expand Down
1 change: 1 addition & 0 deletions common/test/data/toy/html/toyjumpto.html
Original file line number Diff line number Diff line change
@@ -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>
1 change: 1 addition & 0 deletions common/test/data/toy/html/toyjumpto.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<html filename="toyjumpto.html"/>
15 changes: 14 additions & 1 deletion lms/djangoapps/courseware/module_render.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
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
Expand Down Expand Up @@ -393,6 +393,19 @@ def publish(event):
# 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)
Expand Down
31 changes: 31 additions & 0 deletions lms/djangoapps/courseware/tests/test_module_render.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
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


Expand Down Expand Up @@ -50,6 +52,35 @@ def test_get_module(self):
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')
Expand Down
17 changes: 15 additions & 2 deletions lms/djangoapps/courseware/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,30 @@ def setUp(self):

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):
Expand Down
22 changes: 22 additions & 0 deletions lms/djangoapps/courseware/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
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

Expand Down Expand Up @@ -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):
"""
Expand Down
2 changes: 2 additions & 0 deletions lms/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,8 @@
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'),
Expand Down