Addressing review comments

lms/djangoapps/lti_provider/migrations/0001_create_lti_consumer_model.py

@@ -34,4 +34,4 @@ class Migration(SchemaMigration):
         }
     }
 
-    complete_apps = ['lti_provider']
\ No newline at end of file
+    complete_apps = ['lti_provider']

lms/djangoapps/lti_provider/tests/test_views.py

@@ -74,7 +74,7 @@ class LtiLaunchTest(TestCase):
         Verifies that the LTI launch succeeds when passed a valid request.
         """
         request = build_launch_request()
-        views.lti_launch(request, str(COURSE_PARAMS['course_key']), str(COURSE_PARAMS['usage_key']))
+        views.lti_launch(request, unicode(COURSE_KEY), unicode(USAGE_KEY))
         render.assert_called_with(request, ALL_PARAMS)
 
     def launch_with_missing_parameter(self, missing_param):
@@ -114,7 +114,7 @@ class LtiLaunchTest(TestCase):
         properly stored in the session
         """
         request = build_launch_request()
-        views.lti_launch(request, str(COURSE_PARAMS['course_key']), str(COURSE_PARAMS['usage_key']))
+        views.lti_launch(request, unicode(COURSE_KEY), unicode(USAGE_KEY))
         session = request.session[views.LTI_SESSION_KEY]
         self.assertEqual(session['course_key'], COURSE_KEY, 'Course key not set in the session')
         self.assertEqual(session['usage_key'], USAGE_KEY, 'Usage key not set in the session')
@@ -128,9 +128,7 @@ class LtiLaunchTest(TestCase):
         URL
         """
         request = build_launch_request(False)
-        response = views.lti_launch(
-            request, str(COURSE_PARAMS['course_key']), str(COURSE_PARAMS['usage_key'])
-        )
+        response = views.lti_launch(request, unicode(COURSE_KEY), unicode(USAGE_KEY))
         self.assertEqual(response.status_code, 302)
         self.assertEqual(response['Location'], '/accounts/login?next=/lti_provider/lti_run')
 
@@ -143,6 +141,7 @@ class LtiLaunchTest(TestCase):
         request = build_launch_request()
         response = views.lti_launch(request, None, None)
         self.assertEqual(response.status_code, 403)
+        self.assertEqual(response.status_code, 403)
 
 
 class LtiRunTest(TestCase):
@@ -253,7 +252,7 @@ class RenderCoursewareTest(TestCase):
         """
         request = build_run_request()
         views.render_courseware(request, ALL_PARAMS.copy())
-        self.module_mock.assert_called_with(request, str(ALL_PARAMS['course_key']), str(ALL_PARAMS['usage_key']))
+        self.module_mock.assert_called_with(request, unicode(COURSE_KEY), unicode(USAGE_KEY))
 
     def test_render(self):
         """

lms/djangoapps/lti_provider/views.py

@@ -66,9 +66,12 @@ def lti_launch(request, course_id, usage_id):
     # Store the course, and usage ID in the session to prevent privilege
     # escalation if a staff member in one course tries to access material in
     # another.
-    course_key, usage_key = parse_course_and_usage_keys(course_id, usage_id)
-    if not course_key:
-        raise Http404('Invalid course or usage key')
+    try:
+        course_key, usage_key = parse_course_and_usage_keys(course_id, usage_id)
+    except InvalidKeyError:
+        raise Http404(
+            'Invalid course key {} or usage key {}'.format(course_id, usage_id)
+        )
     params['course_key'] = course_key
     params['usage_key'] = usage_key
     request.session[LTI_SESSION_KEY] = params
@@ -164,7 +167,11 @@ def render_courseware(request, lti_params):
     user = request.user
     course = get_course_with_access(user, 'load', course_key)
     staff = has_access(request.user, 'staff', course)
-    instance, _ = get_module_by_usage_id(request, str(course_key), str(usage_key))
+    instance, _dummy = get_module_by_usage_id(
+        request,
+        unicode(course_key),
+        unicode(usage_key)
+    )
 
     fragment = instance.render('student_view', context=request.GET)
 
@@ -188,15 +195,7 @@ def parse_course_and_usage_keys(course_id, usage_id):
     Convert course and usage ID strings into key objects. Return a tuple of
     (course_key, usage_key), or (None, None) if the translation fails.
     """
-    try:
-        course_key = CourseKey.from_string(course_id)
-    except InvalidKeyError:
-        return None, None
-    if not course_key:
-        return None, None
-    try:
-        usage_id = unquote_slashes(usage_id)
-        usage_key = UsageKey.from_string(usage_id).map_into_course(course_key)
-    except InvalidKeyError:
-        return None, None
+    course_key = CourseKey.from_string(course_id)
+    usage_id = unquote_slashes(usage_id)
+    usage_key = UsageKey.from_string(usage_id).map_into_course(course_key)
     return course_key, usage_key