From 69a2d9882e213ff5d439076262ae6fb5e34b59e4 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Tue, 6 May 2014 11:31:15 -0400 Subject: [PATCH] Validate the args to SSCK --- .../management/commands/edit_course_tabs.py | 1 + lms/djangoapps/courseware/courses.py | 26 ++++++---------- .../courseware/tests/test_courses.py | 31 ++----------------- 3 files changed, 12 insertions(+), 46 deletions(-) diff --git a/cms/djangoapps/contentstore/management/commands/edit_course_tabs.py b/cms/djangoapps/contentstore/management/commands/edit_course_tabs.py index d8b011a1c5..5224bb1310 100644 --- a/cms/djangoapps/contentstore/management/commands/edit_course_tabs.py +++ b/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.' diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index 6642aca5ba..ff746f29f7 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/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: - return modulestore().get_course(course_id, depth=depth) - except (KeyError, ItemNotFoundError): + course = modulestore().get_course(course_id, depth=depth) + if course is None: 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) - if course: - return course - else: - raise Http404("Course not found.") - except (KeyError, ItemNotFoundError): + course = modulestore().get_course(course_key, depth=depth) + if course: + return course + else: raise Http404("Course not found.") - except InvalidLocationError: - raise Http404("Invalid location") def get_course_with_access(user, action, course_key, depth=0): diff --git a/lms/djangoapps/courseware/tests/test_courses.py b/lms/djangoapps/courseware/tests/test_courses.py index 340221b03c..b871e0a314 100644 --- a/lms/djangoapps/courseware/tests/test_courses.py +++ b/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(