From 3b483d14bc1023142ff67899dc41b5de5cebb52c Mon Sep 17 00:00:00 2001 From: tlindaliu Date: Wed, 15 Jul 2015 17:30:25 -0400 Subject: [PATCH] MA-851: Add access response information into view_course_access --- lms/djangoapps/courseware/courses.py | 6 ++- .../courseware/courseware_access_exception.py | 24 +++++++++ .../courseware/tests/test_courses.py | 13 +++++ lms/djangoapps/mobile_api/test_milestones.py | 11 ++-- lms/djangoapps/mobile_api/testutils.py | 52 ++++++++++++------- lms/djangoapps/mobile_api/users/tests.py | 2 +- openedx/core/lib/api/view_utils.py | 13 ++--- 7 files changed, 83 insertions(+), 38 deletions(-) create mode 100644 lms/djangoapps/courseware/courseware_access_exception.py diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index 70a9887db8..deb095b264 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/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. diff --git a/lms/djangoapps/courseware/courseware_access_exception.py b/lms/djangoapps/courseware/courseware_access_exception.py new file mode 100644 index 0000000000..d3a47d0c12 --- /dev/null +++ b/lms/djangoapps/courseware/courseware_access_exception.py @@ -0,0 +1,24 @@ +""" +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() diff --git a/lms/djangoapps/courseware/tests/test_courses.py b/lms/djangoapps/courseware/tests/test_courses.py index 0cae5f928a..23806a07a6 100644 --- a/lms/djangoapps/courseware/tests/test_courses.py +++ b/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): diff --git a/lms/djangoapps/mobile_api/test_milestones.py b/lms/djangoapps/mobile_api/test_milestones.py index b4429579f6..ccdcdb9e23 100644 --- a/lms/djangoapps/mobile_api/test_milestones.py +++ b/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()) diff --git a/lms/djangoapps/mobile_api/testutils.py b/lms/djangoapps/mobile_api/testutils.py index 8ea037a359..6756cb4a74 100644 --- a/lms/djangoapps/mobile_api/testutils.py +++ b/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) - - # 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) + 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) + + response = self.api_response(expected_response_code=None) + + if should_succeed: + self.verify_success(response) + else: + self.verify_failure(response) + self.assertEqual(response.data, error_type.to_json()) diff --git a/lms/djangoapps/mobile_api/users/tests.py b/lms/djangoapps/mobile_api/users/tests.py index d5c1adabf7..90f72a2f3f 100644 --- a/lms/djangoapps/mobile_api/users/tests.py +++ b/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//course_enrollments/ """ diff --git a/openedx/core/lib/api/view_utils.py b/openedx/core/lib/api/view_utils.py index 517c9c9508..79cd31492a 100644 --- a/openedx/core/lib/api/view_utils.py +++ b/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