Merge pull request #3659 from edx/opaque-keys-validate

Opaque keys validate

cms/djangoapps/contentstore/management/commands/edit_course_tabs.py

@@ -71,6 +71,7 @@ command again, adding --insert or --delete to edit the list.
             course_key = CourseKey.from_string(options['course'])
         except InvalidKeyError:
             course_key = SlashSeparatedCourseKey.from_deprecated_string(options['course'])
+            print u'Warning: you are using a deprecated format. Please use {} in the future'.format(course_key)
         course = get_course_by_id(course_key)
 
         print 'Warning: this command directly edits the list of course tabs in mongo.'

lms/djangoapps/courseware/courses.py

@@ -12,7 +12,7 @@ from xmodule.modulestore import XML_MODULESTORE_TYPE
 from xmodule.modulestore.keys import CourseKey
 from xmodule.modulestore.django import modulestore
 from xmodule.contentstore.content import StaticContent
-from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError
+from xmodule.modulestore.exceptions import ItemNotFoundError
 from static_replace import replace_static_urls
 from xmodule.modulestore import MONGO_MODULESTORE_TYPE
 
@@ -43,18 +43,15 @@ def get_course(course_id, depth=0):
     """
     Given a course id, return the corresponding course descriptor.
 
-    If course_id is not valid, raises a ValueError.  This is appropriate
+    If the course does not exist, raises a ValueError.  This is appropriate
     for internal use.
 
     depth: The number of levels of children for the modulestore to cache.
     None means infinite depth.  Default is to fetch no children.
     """
-    try:
+    course = modulestore().get_course(course_id, depth=depth)
-        return modulestore().get_course(course_id, depth=depth)
+    if course is None:
-    except (KeyError, ItemNotFoundError):
         raise ValueError(u"Course not found: {0}".format(course_id))
-    except InvalidLocationError:
-        raise ValueError(u"Invalid location: {0}".format(course_id))
     return course
 
 
@@ -63,20 +60,15 @@ def get_course_by_id(course_key, depth=0):
     """
     Given a course id, return the corresponding course descriptor.
 
-    If course_id is not valid, raises a 404.
+    If such a course does not exist, raises a 404.
 
     depth: The number of levels of children for the modulestore to cache. None means infinite depth
     """
-    try:
+    course = modulestore().get_course(course_key, depth=depth)
-        course = modulestore().get_course(course_key, depth=depth)
+    if course:
-        if course:
+        return course
-            return course
+    else:
-        else:
-            raise Http404("Course not found.")
-    except (KeyError, ItemNotFoundError):
         raise Http404("Course not found.")
-    except InvalidLocationError:
-        raise Http404("Invalid location")
 
 
 def get_course_with_access(user, action, course_key, depth=0):

lms/djangoapps/courseware/tests/test_courses.py

@@ -14,7 +14,7 @@ from xmodule.tests.xml import XModuleXmlImportTest
 
 from courseware.courses import (
     get_course_by_id, get_cms_course_link, course_image_url,
-    get_course_info_section, get_course_about_section, get_course
+    get_course_info_section, get_course_about_section, get_cms_block_link
 )
 from courseware.tests.helpers import get_request_for_user
 from courseware.tests.tests import TEST_DATA_MONGO_MODULESTORE, TEST_DATA_MIXED_MODULESTORE
@@ -27,32 +27,6 @@ CMS_BASE_TEST = 'testcms'
 class CoursesTest(ModuleStoreTestCase):
     """Test methods related to fetching courses."""
 
-    def test_get_course_by_id_invalid_chars(self):
-        """
-        Test that `get_course` throws a 404, rather than an exception,
-        when faced with unexpected characters (such as unicode characters,
-        and symbols such as = and ' ')
-        """
-        with self.assertRaises(Http404):
-            get_course_by_id(SlashSeparatedCourseKey('MITx', 'foobar', 'business and management'))
-        with self.assertRaises(Http404):
-            get_course_by_id(SlashSeparatedCourseKey('MITx', 'foobar' 'statistics=introduction'))
-        with self.assertRaises(Http404):
-            get_course_by_id(SlashSeparatedCourseKey('MITx', 'foobar', 'NiñøJoséMaríáßç'))
-
-    def test_get_course_invalid_chars(self):
-        """
-        Test that `get_course` throws a ValueError, rather than a 404,
-        when faced with unexpected characters (such as unicode characters,
-        and symbols such as = and ' ')
-        """
-        with self.assertRaises(ValueError):
-            get_course(SlashSeparatedCourseKey('MITx', 'foobar', 'business and management'))
-        with self.assertRaises(ValueError):
-            get_course(SlashSeparatedCourseKey('MITx', 'foobar', 'statistics=introduction'))
-        with self.assertRaises(ValueError):
-            get_course(SlashSeparatedCourseKey('MITx', 'foobar', 'NiñøJoséMaríáßç'))
-
     @override_settings(
         MODULESTORE=TEST_DATA_MONGO_MODULESTORE, CMS_BASE=CMS_BASE_TEST
     )
@@ -60,14 +34,13 @@ class CoursesTest(ModuleStoreTestCase):
         """
         Tests that get_cms_course_link_by_id and get_cms_block_link_by_id return the right thing
         """
-
-        cms_url = u"//{}/course/org.num.name/branch/draft/block/name".format(CMS_BASE_TEST)
         self.course = CourseFactory.create(
             org='org', number='num', display_name='name'
         )
 
         cms_url = u"//{}/course/slashes:org+num+name".format(CMS_BASE_TEST)
         self.assertEqual(cms_url, get_cms_course_link(self.course))
+        cms_url = u"//{}/course/location:org+num+name+course+name".format(CMS_BASE_TEST)
         self.assertEqual(cms_url, get_cms_block_link(self.course, 'course'))
 
     @mock.patch(