diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 4f369b4c4b..8d0ce1a7ca 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -166,11 +166,7 @@ def _has_access_course_desc(user, action, course): # check start date can_load() and # check mobile_available flag - ( - course.mobile_available or - auth.has_access(user, CourseBetaTesterRole(course.id)) or - _has_staff_access_to_descriptor(user, course, course.id) - ) + is_mobile_available_for_user(user, course) ) def can_enroll(): @@ -649,6 +645,20 @@ def _has_staff_access_to_descriptor(user, descriptor, course_key): return _has_staff_access_to_location(user, descriptor.location, course_key) +def is_mobile_available_for_user(user, course): + """ + Returns whether the given course is mobile_available for the given user. + Checks: + mobile_available flag on the course + Beta User and staff access overrides the mobile_available flag + """ + return ( + course.mobile_available or + auth.has_access(user, CourseBetaTesterRole(course.id)) or + _has_staff_access_to_descriptor(user, course, course.id) + ) + + def get_user_role(user, course_key): """ Return corresponding string if user has staff, instructor or student diff --git a/lms/djangoapps/mobile_api/tests.py b/lms/djangoapps/mobile_api/tests.py index 9c02de1b63..cc66d222e2 100644 --- a/lms/djangoapps/mobile_api/tests.py +++ b/lms/djangoapps/mobile_api/tests.py @@ -3,49 +3,9 @@ Tests for mobile API utilities. """ import ddt -from mock import patch from django.test import TestCase -from xmodule.modulestore.tests.factories import CourseFactory - -from .utils import mobile_access_when_enrolled, mobile_course_access, mobile_view -from .testutils import MobileAPITestCase, ROLE_CASES - - -@ddt.ddt -class TestMobileAccessWhenEnrolled(MobileAPITestCase): - """ - Tests for mobile_access_when_enrolled utility function. - """ - @ddt.data(*ROLE_CASES) - @ddt.unpack - def test_mobile_role_access(self, role, should_have_access): - """ - Verifies that our mobile access function properly handles using roles to grant access - """ - non_mobile_course = CourseFactory.create(mobile_available=False) - if role: - role(non_mobile_course.id).add_users(self.user) - self.assertEqual(should_have_access, mobile_access_when_enrolled(non_mobile_course, self.user)) - - def test_mobile_explicit_access(self): - """ - Verifies that our mobile access function listens to the mobile_available flag as it should - """ - self.assertTrue(mobile_access_when_enrolled(self.course, self.user)) - - def test_missing_course(self): - """ - Verifies that we handle the case where a course doesn't exist - """ - self.assertFalse(mobile_access_when_enrolled(None, self.user)) - - @patch.dict('django.conf.settings.FEATURES', {'DISABLE_START_DATES': False}) - def test_unreleased_course(self): - """ - Verifies that we handle the case where a course hasn't started - """ - self.assertFalse(mobile_access_when_enrolled(self.course, self.user)) +from .utils import mobile_course_access, mobile_view @ddt.ddt diff --git a/lms/djangoapps/mobile_api/testutils.py b/lms/djangoapps/mobile_api/testutils.py index 3c63308451..61e6f58faa 100644 --- a/lms/djangoapps/mobile_api/testutils.py +++ b/lms/djangoapps/mobile_api/testutils.py @@ -26,15 +26,6 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory -# A tuple of Role Types and Boolean values that indicate whether access should be given to that role. -ROLE_CASES = ( - (auth.CourseBetaTesterRole, True), - (auth.CourseStaffRole, True), - (auth.CourseInstructorRole, True), - (None, False) -) - - class MobileAPITestCase(ModuleStoreTestCase, APITestCase): """ Base class for testing Mobile APIs. @@ -140,6 +131,8 @@ class MobileCourseAccessTestMixin(object): Subclasses are expected to inherit from MobileAPITestCase. Subclasses can override verify_success, verify_failure, and init_course_access methods. """ + ALLOW_ACCESS_TO_UNRELEASED_COURSE = False # pylint: disable=invalid-name + def verify_success(self, response): """Base implementation of verifying a successful response.""" self.assertEqual(response.status_code, 200) @@ -170,9 +163,18 @@ class MobileCourseAccessTestMixin(object): self.init_course_access() response = self.api_response(expected_response_code=None) - self.verify_failure(response) # allow subclasses to override verification + if self.ALLOW_ACCESS_TO_UNRELEASED_COURSE: + self.verify_success(response) + else: + self.verify_failure(response) - @ddt.data(*ROLE_CASES) + # A tuple of Role Types and Boolean values that indicate whether access should be given to that role. + @ddt.data( + (auth.CourseBetaTesterRole, True), + (auth.CourseStaffRole, True), + (auth.CourseInstructorRole, True), + (None, False) + ) @ddt.unpack def test_non_mobile_available(self, role, should_succeed): self.init_course_access() diff --git a/lms/djangoapps/mobile_api/users/tests.py b/lms/djangoapps/mobile_api/users/tests.py index 2b28a20d4e..5f751442d0 100644 --- a/lms/djangoapps/mobile_api/users/tests.py +++ b/lms/djangoapps/mobile_api/users/tests.py @@ -5,7 +5,7 @@ Tests for users API import datetime from django.utils import timezone -from xmodule.modulestore.tests.factories import ItemFactory +from xmodule.modulestore.tests.factories import ItemFactory, CourseFactory from xmodule.modulestore.django import modulestore from student.models import CourseEnrollment @@ -48,6 +48,7 @@ class TestUserEnrollmentApi(MobileAPITestCase, MobileAuthUserTestMixin, MobileEn Tests for /api/mobile/v0.5/users//course_enrollments/ """ REVERSE_INFO = {'name': 'courseenrollment-detail', 'params': ['username']} + ALLOW_ACCESS_TO_UNRELEASED_COURSE = True def verify_success(self, response): super(TestUserEnrollmentApi, self).verify_success(response) @@ -65,6 +66,23 @@ class TestUserEnrollmentApi(MobileAPITestCase, MobileAuthUserTestMixin, MobileEn courses = response.data self.assertEqual(len(courses), 0) + def test_sort_order(self): + self.login() + + num_courses = 3 + courses = [] + for course_num in range(num_courses): + courses.append(CourseFactory.create(mobile_available=True)) + self.enroll(courses[course_num].id) + + # verify courses are returned in the order of enrollment, with most recently enrolled first. + response = self.api_response() + for course_num in range(num_courses): + self.assertEqual( + response.data[course_num]['course']['id'], # pylint: disable=no-member + unicode(courses[num_courses - course_num - 1].id) + ) + class CourseStatusAPITestCase(MobileAPITestCase): """ diff --git a/lms/djangoapps/mobile_api/users/views.py b/lms/djangoapps/mobile_api/users/views.py index 9ac03b9b5e..a21a06a3ac 100644 --- a/lms/djangoapps/mobile_api/users/views.py +++ b/lms/djangoapps/mobile_api/users/views.py @@ -2,9 +2,6 @@ Views for user API """ -from courseware.model_data import FieldDataCache -from courseware.module_render import get_module_for_descriptor - from django.shortcuts import redirect from django.utils import dateparse @@ -12,11 +9,13 @@ from rest_framework import generics, views from rest_framework.decorators import api_view from rest_framework.response import Response -from courseware.views import get_current_child, save_positions_recursively_up - from opaque_keys.edx.keys import UsageKey from opaque_keys import InvalidKeyError +from courseware.access import is_mobile_available_for_user +from courseware.model_data import FieldDataCache +from courseware.module_render import get_module_for_descriptor +from courseware.views import get_current_child, save_positions_recursively_up from student.models import CourseEnrollment, User from xblock.fields import Scope @@ -25,8 +24,8 @@ from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError from .serializers import CourseEnrollmentSerializer, UserSerializer -from mobile_api import errors -from mobile_api.utils import mobile_access_when_enrolled, mobile_view, mobile_course_access +from .. import errors +from ..utils import mobile_view, mobile_course_access @mobile_view(is_user=True) @@ -46,6 +45,7 @@ class UserDetail(generics.RetrieveAPIView): GET /api/mobile/v0.5/users/{username} + **Response Values** * id: The ID of the user. @@ -241,10 +241,13 @@ class UserCourseEnrollmentsList(generics.ListAPIView): lookup_field = 'username' def get_queryset(self): - enrollments = self.queryset.filter(user__username=self.kwargs['username'], is_active=True).order_by('created') + enrollments = self.queryset.filter( + user__username=self.kwargs['username'], + is_active=True + ).order_by('created').reverse() return [ enrollment for enrollment in enrollments - if mobile_access_when_enrolled(enrollment.course, self.request.user) + if enrollment.course and is_mobile_available_for_user(self.request.user, enrollment.course) ] diff --git a/lms/djangoapps/mobile_api/utils.py b/lms/djangoapps/mobile_api/utils.py index 8e8aa56fb4..5905fc886d 100644 --- a/lms/djangoapps/mobile_api/utils.py +++ b/lms/djangoapps/mobile_api/utils.py @@ -4,7 +4,6 @@ Common utility methods and decorators for Mobile APIs. import functools -from django.http import Http404 from opaque_keys.edx.keys import CourseKey from courseware.courses import get_course_with_access @@ -37,22 +36,6 @@ def mobile_course_access(depth=0, verify_enrolled=True): return _decorator -def mobile_access_when_enrolled(course, user): - """ - Determines whether a user has access to a course in a mobile context. - Checks the mobile_available flag and the start_date. - Note: Does not check if the user is actually enrolled in the course. - """ - # The course doesn't always really exist -- we can have bad data in the enrollments - # pointing to non-existent (or removed) courses, in which case `course` is None. - if not course: - return False - try: - return get_course_with_access(user, 'load_mobile_no_enrollment_check', course.id) is not None - except Http404: - return False - - def mobile_view(is_user=False): """ Function and class decorator that abstracts the authentication and permission checks for mobile api views.