diff --git a/cms/djangoapps/contentstore/views/tabs.py b/cms/djangoapps/contentstore/views/tabs.py
index f79d4d0721..38d1c6a228 100644
--- a/cms/djangoapps/contentstore/views/tabs.py
+++ b/cms/djangoapps/contentstore/views/tabs.py
@@ -14,7 +14,8 @@ from edxmako.shortcuts import render_to_response
from xmodule.modulestore.django import modulestore
from xmodule.modulestore import ModuleStoreEnum
from xmodule.tabs import CourseTabList, CourseTab, InvalidTabsException, StaticTab
-from opaque_keys.edx.keys import CourseKey, UsageKey
+from opaque_keys.edx.keys import UsageKey
+from util.course_key_utils import course_key_from_string_or_404
from ..utils import get_lms_link_for_item
@@ -39,7 +40,7 @@ def tabs_handler(request, course_key_string):
Creating a tab, deleting a tab, or changing its contents is not supported through this method.
Instead use the general xblock URL (see item.xblock_handler).
"""
- course_key = CourseKey.from_string(course_key_string)
+ course_key = course_key_from_string_or_404(course_key_string)
if not has_course_author_access(request.user, course_key):
raise PermissionDenied()
diff --git a/cms/djangoapps/contentstore/views/tests/test_tabs.py b/cms/djangoapps/contentstore/views/tests/test_tabs.py
index fde68931a3..d79630b845 100644
--- a/cms/djangoapps/contentstore/views/tests/test_tabs.py
+++ b/cms/djangoapps/contentstore/views/tests/test_tabs.py
@@ -10,6 +10,7 @@ from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
from xmodule.tabs import CourseTabList
from xmodule.modulestore.django import modulestore
+from django.http import Http404
class TabsPageTests(CourseTestCase):
@@ -191,6 +192,13 @@ class TabsPageTests(CourseTestCase):
self.assertIn('Delete this component', html)
self.assertIn('', html)
+ def test_invalid_course_id(self):
+ """ Asserts that Http404 is raised when the course id is not valid. """
+ invalid_tab_url = reverse_course_url('tabs_handler', "/some.invalid.key/TTT/CS01/2015_T0")
+ with self.assertRaises(Http404):
+ self.client.get(invalid_tab_url)
+
+
class PrimitiveTabEdit(ModuleStoreTestCase):
"""Tests for the primitive tab edit data manipulations"""
diff --git a/cms/djangoapps/contentstore/views/tests/test_user.py b/cms/djangoapps/contentstore/views/tests/test_user.py
index 725858e44d..8c26ff4515 100644
--- a/cms/djangoapps/contentstore/views/tests/test_user.py
+++ b/cms/djangoapps/contentstore/views/tests/test_user.py
@@ -9,6 +9,7 @@ from django.contrib.auth.models import User
from student.models import CourseEnrollment
from student.roles import CourseStaffRole, CourseInstructorRole
from student import auth
+from django.http import Http404
class UsersTestCase(CourseTestCase):
@@ -315,3 +316,12 @@ class UsersTestCase(CourseTestCase):
CourseEnrollment.is_enrolled(self.ext_user, self.course.id),
'User ext_user should have been enrolled in the course'
)
+
+ def test_invalid_course_id(self):
+ """ Asserts that Http404 is raised when the course id is not valid. """
+ wrong_url = reverse_course_url(
+ 'course_team_handler', "/some.invalid.key/TTT/CS01/2015_T0",
+ kwargs={'email': self.ext_user.email}
+ )
+ with self.assertRaises(Http404):
+ self.client.get(wrong_url)
diff --git a/cms/djangoapps/contentstore/views/user.py b/cms/djangoapps/contentstore/views/user.py
index e5da0a8194..79208a535d 100644
--- a/cms/djangoapps/contentstore/views/user.py
+++ b/cms/djangoapps/contentstore/views/user.py
@@ -8,7 +8,7 @@ from django.views.decorators.csrf import ensure_csrf_cookie
from edxmako.shortcuts import render_to_response
from xmodule.modulestore.django import modulestore
-from opaque_keys.edx.keys import CourseKey
+from util.course_key_utils import course_key_from_string_or_404
from opaque_keys.edx.locator import LibraryLocator
from util.json_request import JsonResponse, expect_json
from student.roles import CourseInstructorRole, CourseStaffRole, LibraryUserRole
@@ -49,7 +49,7 @@ def course_team_handler(request, course_key_string=None, email=None):
DELETE:
json: remove a particular course team member from the course team (email is required).
"""
- course_key = CourseKey.from_string(course_key_string) if course_key_string else None
+ course_key = course_key_from_string_or_404(course_key_string) if course_key_string else None
# No permissions check here - each helper method does its own check.
if 'application/json' in request.META.get('HTTP_ACCEPT', 'application/json'):
diff --git a/common/djangoapps/util/course_key_utils.py b/common/djangoapps/util/course_key_utils.py
index ab58a6558d..44328d7d78 100644
--- a/common/djangoapps/util/course_key_utils.py
+++ b/common/djangoapps/util/course_key_utils.py
@@ -6,14 +6,15 @@ from opaque_keys import InvalidKeyError
from opaque_keys.edx.keys import CourseKey
-def from_string_or_404(course_key_string):
+def course_key_from_string_or_404(course_key_string, message=None):
"""
Gets CourseKey from the string passed as parameter.
Parses course key from string(containing course key) or raises 404 if the string's format is invalid.
Arguments:
- course_key_string(str): It is string containing the course key
+ course_key_string(str): It contains the course key
+ message(str): It contains the exception message
Returns:
CourseKey: A key that uniquely identifies a course
@@ -25,6 +26,6 @@ def from_string_or_404(course_key_string):
try:
course_key = CourseKey.from_string(course_key_string)
except InvalidKeyError:
- raise Http404
+ raise Http404(message)
- return course_key
+ return course_key
\ No newline at end of file
diff --git a/common/djangoapps/util/tests/test_course_key_utils.py b/common/djangoapps/util/tests/test_course_key_utils.py
index efc88a09ee..709abac9a2 100644
--- a/common/djangoapps/util/tests/test_course_key_utils.py
+++ b/common/djangoapps/util/tests/test_course_key_utils.py
@@ -1,32 +1,46 @@
"""
Tests for util.course_key_utils
"""
-from nose.tools import assert_equals, assert_raises # pylint: disable=no-name-in-module
-from util.course_key_utils import from_string_or_404
+from util.course_key_utils import course_key_from_string_or_404
from opaque_keys.edx.keys import CourseKey
from django.http import Http404
+import ddt
+import unittest
-def test_from_string_or_404():
+@ddt.ddt
+class TestFromStringOr404(unittest.TestCase):
+ """
+ Base Test class for course_key_from_string_or_404 utility tests
+ """
+ @ddt.data(
+ ("/some.invalid.key/course-v1:TTT+CS01+2015_T0", "course-v1:TTT+CS01+2015_T0"), # split style course keys
+ ("/some.invalid.key/TTT/CS01/2015_T0", "TTT/CS01/2015_T0"), # mongo style course keys
+ )
+ def test_from_string_or_404(self, (invalid_course_key, valid_course_key)):
+ """
+ Tests course_key_from_string_or_404 for valid and invalid split style course keys and mongo style course keys.
+ """
+ self.assertRaises(
+ Http404,
+ course_key_from_string_or_404,
+ invalid_course_key,
+ )
+ self.assertEquals(
+ CourseKey.from_string(valid_course_key),
+ course_key_from_string_or_404(valid_course_key)
+ )
- #testing with split style course keys
- assert_raises(
- Http404,
- from_string_or_404,
- "/some.invalid.key/course-v1:TTT+CS01+2015_T0"
- )
- assert_equals(
- CourseKey.from_string("course-v1:TTT+CS01+2015_T0"),
- from_string_or_404("course-v1:TTT+CS01+2015_T0")
- )
-
- #testing with mongo style course keys
- assert_raises(
- Http404,
- from_string_or_404,
- "/some.invalid.key/TTT/CS01/2015_T0"
- )
- assert_equals(
- CourseKey.from_string("TTT/CS01/2015_T0"),
- from_string_or_404("TTT/CS01/2015_T0")
+ @ddt.data(
+ "/some.invalid.key/course-v1:TTT+CS01+2015_T0", # split style invalid course key
+ "/some.invalid.key/TTT/CS01/2015_T0" # mongo style invalid course key
)
+ def test_from_string_or_404_with_message(self, course_string):
+ """
+ Tests course_key_from_string_or_404 with exception message for split style and monog style invalid course keys.
+ :return:
+ """
+ try:
+ course_key_from_string_or_404(course_string, message="Invalid Keys")
+ except Http404 as exception:
+ self.assertEquals(str(exception), "Invalid Keys")
\ No newline at end of file
diff --git a/lms/djangoapps/django_comment_client/forum/tests.py b/lms/djangoapps/django_comment_client/forum/tests.py
index 66323a1968..06399ea2c5 100644
--- a/lms/djangoapps/django_comment_client/forum/tests.py
+++ b/lms/djangoapps/django_comment_client/forum/tests.py
@@ -1312,6 +1312,14 @@ class InlineDiscussionUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixi
self.assertEqual(response_data["discussion_data"][0]["title"], text)
self.assertEqual(response_data["discussion_data"][0]["body"], text)
+ def _test_invalid_course_id(self):
+ """ Asserts that Http404 is raised when the course id is not valid. """
+ request = RequestFactory().get("dummy_url")
+ request.user = self.student
+ with self.assertRaises(Http404):
+ views.inline_discussion(
+ request, self.course.id.to_deprecated_string(), self.course.discussion_topics['General']['id']
+ )
class ForumFormDiscussionUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixin):
@classmethod
diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py
index 1459d4c39c..2e6efcc023 100644
--- a/lms/djangoapps/django_comment_client/forum/views.py
+++ b/lms/djangoapps/django_comment_client/forum/views.py
@@ -40,7 +40,7 @@ from django_comment_client.utils import (
import django_comment_client.utils as utils
import lms.lib.comment_client as cc
-from opaque_keys.edx.keys import CourseKey
+from util.course_key_utils import course_key_from_string_or_404
THREADS_PER_PAGE = 20
INLINE_THREADS_PER_PAGE = 20
@@ -178,7 +178,7 @@ def use_bulk_ops(view_func):
"""
@wraps(view_func)
def wrapped_view(request, course_id, *args, **kwargs): # pylint: disable=missing-docstring
- course_key = CourseKey.from_string(course_id)
+ course_key = course_key_from_string_or_404(course_id)
with modulestore().bulk_operations(course_key):
return view_func(request, course_key, *args, **kwargs)
return wrapped_view