diff --git a/cms/djangoapps/contentstore/tests/test_course_index.py b/cms/djangoapps/contentstore/tests/test_course_index.py index ec0c0b1f8e..42ca510c1f 100644 --- a/cms/djangoapps/contentstore/tests/test_course_index.py +++ b/cms/djangoapps/contentstore/tests/test_course_index.py @@ -61,7 +61,7 @@ class TestCourseIndex(CourseTestCase): Test the error conditions for the access """ locator = loc_mapper().translate_location(self.course.location.course_id, self.course.location, False, True) - outline_url = reverse('contentstore.views.course_handler', kwargs={'course_url': unicode(locator)}) + outline_url = locator.url_reverse('course/', '') # register a non-staff member and try to delete the course branch non_staff_client, _ = self.createNonStaffAuthedUserClient() response = non_staff_client.delete(outline_url, {}, HTTP_ACCEPT='application/json') diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index cf6ef7f626..8fd1c1a5dc 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -60,7 +60,7 @@ __all__ = ['create_new_course', 'course_info', 'course_handler', @login_required -def course_handler(request, course_url): +def course_handler(request, tag=None, course_id=None, branch=None, version_guid=None, block=None): """ The restful handler for course specific requests. It provides the course tree with the necessary information for identifying and labeling the parts. The root @@ -84,7 +84,10 @@ def course_handler(request, course_url): if 'application/json' in request.META.get('HTTP_ACCEPT', 'application/json'): if request.method == 'GET': raise NotImplementedError('coming soon') - elif not has_access(request.user, BlockUsageLocator(course_url)): + elif not has_access( + request.user, + BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block) + ): raise PermissionDenied() elif request.method == 'POST': raise NotImplementedError() @@ -95,7 +98,7 @@ def course_handler(request, course_url): else: return HttpResponseBadRequest() elif request.method == 'GET': # assume html - return course_index(request, course_url) + return course_index(request, course_id, branch, version_guid, block) else: return HttpResponseNotFound() @@ -108,18 +111,18 @@ def old_course_index_shim(request, org, course, name): """ old_location = Location(['i4x', org, course, 'course', name]) locator = loc_mapper().translate_location(old_location.course_id, old_location, False, True) - return course_index(request, locator) + return course_index(request, locator.course_id, locator.branch, locator.version_guid, locator.usage_id) @login_required @ensure_csrf_cookie -def course_index(request, course_url): +def course_index(request, course_id, branch, version_guid, block): """ Display an editable course overview. org, course, name: Attributes of the Location for the item to edit """ - location = BlockUsageLocator(course_url) + location = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block) # TODO: when converting to split backend, if location does not have a usage_id, # we'll need to get the course's root block_id if not has_access(request.user, location): diff --git a/cms/djangoapps/contentstore/views/user.py b/cms/djangoapps/contentstore/views/user.py index d307e1295e..1d6804b564 100644 --- a/cms/djangoapps/contentstore/views/user.py +++ b/cms/djangoapps/contentstore/views/user.py @@ -47,12 +47,13 @@ def index(request): def format_course_for_view(course): # published = false b/c studio manipulates draft versions not b/c the course isn't pub'd - course_url = loc_mapper().translate_location( + course_loc = loc_mapper().translate_location( course.location.course_id, course.location, published=False, add_entry_if_missing=True ) return ( course.display_name, - reverse("contentstore.views.course_handler", kwargs={'course_url': course_url}), + # note, couldn't get django reverse to work; so, wrote workaround + course_loc.url_reverse('course/', ''), get_lms_link_for_item( course.location ), diff --git a/cms/templates/widgets/header.html b/cms/templates/widgets/header.html index 3be0b3723a..5ae7d0d659 100644 --- a/cms/templates/widgets/header.html +++ b/cms/templates/widgets/header.html @@ -15,10 +15,7 @@ % if context_course: <% ctx_loc = context_course.location - index_url = reverse( - 'contentstore.views.course_handler', - kwargs={'course_url': loc_mapper().translate_location(ctx_loc.course_id, ctx_loc, False, True)} - ) + index_url = loc_mapper().translate_location(ctx_loc.course_id, ctx_loc, False, True).url_reverse('course/', '') %>

${_("Current Course:")} diff --git a/cms/urls.py b/cms/urls.py index 893dc32884..1cf92b8d81 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -1,8 +1,10 @@ +import re from django.conf import settings from django.conf.urls import patterns, include, url # TODO: This should be removed once the CMS is running via wsgi on all production servers import cms.startup as startup +from xmodule.modulestore import parsers startup.run() # There is a course creators admin table. @@ -136,7 +138,8 @@ urlpatterns += patterns( ), url(r'^course$', 'index'), - url(r'^course/(?P.*)$', 'course_handler'), + # (?ix) == ignore case and verbose (multiline regex) + url(r'(?ix)^course/{}$'.format(parsers.URL_RE_SOURCE), 'course_handler'), ) js_info_dict = { diff --git a/common/lib/xmodule/xmodule/modulestore/locator.py b/common/lib/xmodule/xmodule/modulestore/locator.py index 9194b9a400..5b8367172d 100644 --- a/common/lib/xmodule/xmodule/modulestore/locator.py +++ b/common/lib/xmodule/xmodule/modulestore/locator.py @@ -266,8 +266,8 @@ class CourseLocator(Locator): def url_reverse(self, prefix, postfix): """ Do what reverse is supposed to do but seems unable to do. Generate a url using prefix unicode(self) postfix - :param prefix: the beginning of the url (should begin and end with / if non-empty) - :param postfix: the part to append to the url (should begin w/ / if non-empty) + :param prefix: the beginning of the url (will be forced to begin and end with / if non-empty) + :param postfix: the part to append to the url (will be forced to begin w/ / if non-empty) """ if prefix: if not prefix.endswith('/'): @@ -278,14 +278,10 @@ class CourseLocator(Locator): prefix = '/' if postfix and not postfix.startswith('/'): postfix = '/' + postfix + elif postfix is None: + postfix = '' return prefix + unicode(self) + postfix - def reverse_kwargs(self): - """ - Get the kwargs list to supply to reverse (basically, a dict of the set properties) - """ - return {key: value for key, value in self.__dict__.iteritems() if value is not None} - def init_from_url(self, url): """ url must be a string beginning with 'edx://' and containing @@ -452,16 +448,6 @@ class BlockUsageLocator(CourseLocator): branch=self.branch, usage_id=self.usage_id) - def reverse_kwargs(self): - """ - Get the kwargs list to supply to reverse (basically, a dict of the set properties) - """ - result = super(BlockUsageLocator, self).reverse_kwargs() - if self.usage_id: - del result['usage_id'] - result['block'] = self.usage_id - return result - def set_usage_id(self, new): """ Initialize usage_id to new value. diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py b/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py index f60af2b210..c6c21c1540 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py @@ -285,6 +285,28 @@ class LocatorTest(TestCase): Locator.to_locator_or_location("hello.world.not.a.url") self.assertIsNone(Locator.parse_url("unknown://foo.bar/baz")) + def test_url_reverse(self): + """ + Test the url_reverse method + """ + locator = CourseLocator(course_id="a.fancy_course-id", branch="branch_1.2-3") + self.assertEqual( + '/expression/{}/format'.format(unicode(locator)), + locator.url_reverse('expression', 'format') + ) + self.assertEqual( + '/expression/{}/format'.format(unicode(locator)), + locator.url_reverse('/expression', '/format') + ) + self.assertEqual( + '/expression/{}'.format(unicode(locator)), + locator.url_reverse('expression/', None) + ) + self.assertEqual( + '/expression/{}'.format(unicode(locator)), + locator.url_reverse('/expression/', '') + ) + def test_description_locator_url(self): object_id = '{:024x}'.format(random.randrange(16 ** 24)) definition_locator = DefinitionLocator(object_id)