Merge pull request #8946 from edx/lliu/view-course-access

Add access response information into view_course_access

lms/djangoapps/courseware/courses.py

@@ -21,6 +21,7 @@ from microsite_configuration import microsite
 from courseware.access import has_access
 from courseware.model_data import FieldDataCache
 from courseware.module_render import get_module
+from lms.djangoapps.courseware.courseware_access_exception import CoursewareAccessException
 from student.models import CourseEnrollment
 import branding
 
@@ -98,11 +99,12 @@ def get_course_with_access(user, action, course_key, depth=0, check_if_enrolled=
     """
     assert isinstance(course_key, CourseKey)
     course = get_course_by_id(course_key, depth=depth)
+    access_response = has_access(user, action, course, course_key)
 
-    if not has_access(user, action, course, course_key):
+    if not access_response:
         # Deliberately return a non-specific error message to avoid
         # leaking info about access control settings
-        raise Http404("Course not found.")
+        raise CoursewareAccessException(access_response)
 
     if check_if_enrolled:
         # Verify that the user is either enrolled in the course or a staff member.

lms/djangoapps/courseware/courseware_access_exception.py

+"""
+This file contains the exception used in courseware access
+"""
+from django.http import Http404
+
+
+class CoursewareAccessException(Http404):
+    """
+    Exception for courseware access errors
+    """
+
+    def __init__(self, access_response):
+        super(CoursewareAccessException, self).__init__()
+        self.access_response = access_response
+        self.message = "Course not found."
+
+    def to_json(self):
+        """
+        Creates a serializable JSON representation of an CoursewareAccessException.
+
+        Returns:
+            dict: JSON representation
+        """
+        return self.access_response.to_json()

lms/djangoapps/courseware/tests/test_courses.py

@@ -17,9 +17,12 @@ from courseware.courses import (
     get_course_by_id, get_cms_course_link, course_image_url,
     get_course_info_section, get_course_about_section, get_cms_block_link
 )
+
+from courseware.courses import get_course_with_access
 from courseware.module_render import get_module_for_descriptor
 from courseware.tests.helpers import get_request_for_user
 from courseware.model_data import FieldDataCache
+from lms.djangoapps.courseware.courseware_access_exception import CoursewareAccessException
 from student.tests.factories import UserFactory
 from xmodule.modulestore.django import _get_modulestore_branch_setting, modulestore
 from xmodule.modulestore import ModuleStoreEnum
@@ -53,6 +56,16 @@ class CoursesTest(ModuleStoreTestCase):
         cms_url = u"//{}/course/{}".format(CMS_BASE_TEST, unicode(self.course.location))
         self.assertEqual(cms_url, get_cms_block_link(self.course, 'course'))
 
+    def test_get_course_with_access(self):
+        user = UserFactory.create()
+        course = CourseFactory.create(visible_to_staff_only=True)
+
+        with self.assertRaises(CoursewareAccessException) as error:
+            get_course_with_access(user, 'load', course.id)
+        self.assertEqual(error.exception.message, "Course not found.")
+        self.assertEqual(error.exception.access_response.error_code, "not_visible_to_user")
+        self.assertFalse(error.exception.access_response.has_access)
+
 
 @attr('shard_1')
 class ModuleStoreBranchSettingTest(ModuleStoreTestCase):

lms/djangoapps/mobile_api/test_milestones.py

@@ -3,6 +3,7 @@ Milestone related tests for the mobile_api
 """
 from mock import patch
 
+from courseware.access_response import MilestoneError
 from courseware.tests.helpers import get_request_for_user
 from courseware.tests.test_entrance_exam import answer_entrance_exam_problem, add_entrance_exam_milestone
 from util.milestones_helpers import (
@@ -23,10 +24,6 @@ class MobileAPIMilestonesMixin(object):
     the mobile api will appropriately block content until the milestone is
     fulfilled.
     """
-    MILESTONE_MESSAGE = {
-        'developer_message':
-            'Cannot access content with unfulfilled pre-requisites or unpassed entrance exam.'
-    }
 
     ALLOW_ACCESS_TO_MILESTONE_COURSE = False  # pylint: disable=invalid-name
 
@@ -126,12 +123,12 @@ class MobileAPIMilestonesMixin(object):
 
         Since different endpoints will have different behaviours towards milestones,
         setting ALLOW_ACCESS_TO_MILESTONE_COURSE (default is False) to True, will
-        not return a 204. For example, when getting a list of courses a user is
+        not return a 404. For example, when getting a list of courses a user is
         enrolled in, although a user may have unfulfilled milestones, the course
         should still show up in the course enrollments list.
         """
         if self.ALLOW_ACCESS_TO_MILESTONE_COURSE:
             self.api_response()
         else:
-            response = self.api_response(expected_response_code=204)
-            self.assertEqual(response.data, self.MILESTONE_MESSAGE)
+            response = self.api_response(expected_response_code=404)
+            self.assertEqual(response.data, MilestoneError().to_json())

lms/djangoapps/mobile_api/testutils.py

@@ -20,6 +20,11 @@ from rest_framework.test import APITestCase
 
 from opaque_keys.edx.keys import CourseKey
 
+from courseware.access_response import (
+    MobileAvailabilityError,
+    StartDateError,
+    VisibilityError
+)
 from courseware.tests.factories import UserFactory
 from student import auth
 from student.models import CourseEnrollment
@@ -164,12 +169,7 @@ class MobileCourseAccessTestMixin(MobileAPIMilestonesMixin):
     @patch.dict('django.conf.settings.FEATURES', {'DISABLE_START_DATES': False})
     def test_unreleased_course(self):
         self.init_course_access()
-
-        response = self.api_response(expected_response_code=None)
-        if self.ALLOW_ACCESS_TO_UNRELEASED_COURSE:
-            self.verify_success(response)
-        else:
-            self.verify_failure(response)
+        self._verify_response(self.ALLOW_ACCESS_TO_UNRELEASED_COURSE, StartDateError(self.course.start))
 
     # A tuple of Role Types and Boolean values that indicate whether access should be given to that role.
     @ddt.data(
@@ -181,24 +181,40 @@ class MobileCourseAccessTestMixin(MobileAPIMilestonesMixin):
     @ddt.unpack
     def test_non_mobile_available(self, role, should_succeed):
         self.init_course_access()
-
         # set mobile_available to False for the test course
         self.course.mobile_available = False
         self.store.update_item(self.course, self.user.id)
+        self._verify_response(should_succeed, MobileAvailabilityError(), role)
 
+    def test_unenrolled_user(self):
+        self.login()
+        self.unenroll()
+        response = self.api_response(expected_response_code=None)
+        self.verify_failure(response)
+
+    @ddt.data(
+        (auth.CourseStaffRole, True),
+        (None, False)
+    )
+    @ddt.unpack
+    def test_visible_to_staff_only_course(self, role, should_succeed):
+        self.init_course_access()
+        self.course.visible_to_staff_only = True
+        self.store.update_item(self.course, self.user.id)
+        self._verify_response(should_succeed, VisibilityError(), role)
+
+    def _verify_response(self, should_succeed, error_type, role=None):
+        """
+        Calls API and verifies the response
+        """
         # set user's role in the course
         if role:
             role(self.course.id).add_users(self.user)
 
-        # call API and verify response
         response = self.api_response(expected_response_code=None)
+
         if should_succeed:
             self.verify_success(response)
         else:
             self.verify_failure(response)
-
-    def test_unenrolled_user(self):
-        self.login()
-        self.unenroll()
-        response = self.api_response(expected_response_code=None)
-        self.verify_failure(response)
+            self.assertEqual(response.data, error_type.to_json())

lms/djangoapps/mobile_api/users/tests.py

@@ -60,7 +60,7 @@ class TestUserInfoApi(MobileAPITestCase, MobileAuthTestMixin):
 
 
 @ddt.ddt
-class TestUserEnrollmentApi(MobileAPITestCase, MobileAuthUserTestMixin, MobileCourseAccessTestMixin):
+class TestUserEnrollmentApi(MobileAPITestCase, MobileAuthUserTestMixin):
     """
     Tests for /api/mobile/v0.5/users/<user_name>/course_enrollments/
     """

openedx/core/lib/api/view_utils.py

@@ -14,6 +14,7 @@ from rest_framework.mixins import RetrieveModelMixin, UpdateModelMixin
 from rest_framework.generics import GenericAPIView
 
 from lms.djangoapps.courseware.courses import get_course_with_access
+from lms.djangoapps.courseware.courseware_access_exception import CoursewareAccessException
 from opaque_keys.edx.keys import CourseKey
 from xmodule.modulestore.django import modulestore
 
@@ -104,16 +105,8 @@ def view_course_access(depth=0, access_action='load', check_for_milestones=False
                         depth=depth,
                         check_if_enrolled=True,
                     )
-                except Http404:
-                    # any_unfulfilled_milestones called a second time since has_access returns a bool
-                    if check_for_milestones and any_unfulfilled_milestones(course_id, request.user.id):
-                        message = {
-                            "developer_message": "Cannot access content with unfulfilled "
-                                                 "pre-requisites or unpassed entrance exam."
-                        }
-                        return response.Response(data=message, status=status.HTTP_204_NO_CONTENT)
-                    else:
-                        raise
+                except CoursewareAccessException as error:
+                    return response.Response(data=error.to_json(), status=status.HTTP_404_NOT_FOUND)
                 return func(self, request, course=course, *args, **kwargs)
         return _wrapper
     return _decorator