From 920dfb5270e13f24b95ba31a08466ba6ed129cfe Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Tue, 13 Jan 2015 19:55:32 -0500 Subject: [PATCH 1/3] MA-216 Mobile API: display unreleased courses in enrollment list. --- lms/djangoapps/mobile_api/tests.py | 53 ++++++++++++++++++++---- lms/djangoapps/mobile_api/testutils.py | 9 +++- lms/djangoapps/mobile_api/users/tests.py | 1 + lms/djangoapps/mobile_api/users/views.py | 5 ++- lms/djangoapps/mobile_api/utils.py | 40 +++++++++++++++--- 5 files changed, 92 insertions(+), 16 deletions(-) diff --git a/lms/djangoapps/mobile_api/tests.py b/lms/djangoapps/mobile_api/tests.py index 9c02de1b63..57fc58f9de 100644 --- a/lms/djangoapps/mobile_api/tests.py +++ b/lms/djangoapps/mobile_api/tests.py @@ -8,12 +8,12 @@ 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 .utils import mobile_course_listing_access, mobile_course_access, mobile_view, dict_value from .testutils import MobileAPITestCase, ROLE_CASES @ddt.ddt -class TestMobileAccessWhenEnrolled(MobileAPITestCase): +class TestMobileCourseListingAccess(MobileAPITestCase): """ Tests for mobile_access_when_enrolled utility function. """ @@ -26,26 +26,26 @@ class TestMobileAccessWhenEnrolled(MobileAPITestCase): 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)) + self.assertEqual(should_have_access, mobile_course_listing_access(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)) + self.assertTrue(mobile_course_listing_access(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)) + self.assertFalse(mobile_course_listing_access(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 + Verifies that we allow the case where a course hasn't started """ - self.assertFalse(mobile_access_when_enrolled(self.course, self.user)) + self.assertTrue(mobile_course_listing_access(self.course, self.user)) @ddt.ddt @@ -65,3 +65,42 @@ class TestMobileAPIDecorators(TestCase): self.assertIn("Test docstring of decorated function.", decorated_func.__doc__) self.assertEquals(decorated_func.__name__, "decorated_func") self.assertTrue(decorated_func.__module__.endswith("tests")) + + +@ddt.ddt +class TestDictContextManager(TestCase): + """ + Tests for dict contextmanager. + """ + def setUp(self): + super(TestDictContextManager, self).setUp() + self.test_dict = {} + self.test_key = 'test key' + + def call_context_manager(self, raise_exception): + """Helper method that calls the context manager.""" + new_value = "new value" + try: + with dict_value(self.test_dict, self.test_key, new_value): + # verify test_key is assigned to new_value within the context. + self.assertEquals(self.test_dict[self.test_key], new_value) + if raise_exception: + raise StandardError + except StandardError: + pass + + @ddt.data(True, False) + def test_no_previous_value(self, raise_exception): + self.call_context_manager(raise_exception) + + # verify test_key no longer exists in the dict. + self.assertNotIn(self.test_key, self.test_dict) + + @ddt.data(True, False) + def test_has_previous_value(self, raise_exception): + old_value = "old value" + self.test_dict[self.test_key] = old_value + self.call_context_manager(raise_exception) + + # verify test_key's value is reverted back to old_value. + self.assertEquals(self.test_dict[self.test_key], old_value) diff --git a/lms/djangoapps/mobile_api/testutils.py b/lms/djangoapps/mobile_api/testutils.py index 3c63308451..0c004132d4 100644 --- a/lms/djangoapps/mobile_api/testutils.py +++ b/lms/djangoapps/mobile_api/testutils.py @@ -10,7 +10,7 @@ Test utilities for mobile API tests: MobileCourseAccessTestMixin - tests for APIs with mobile_course_access and verify_enrolled=False. MobileEnrolledCourseAccessTestMixin - tests for APIs with mobile_course_access and verify_enrolled=True. """ -# pylint: disable=no-member +# pylint: disable=no-member, invalid-name import ddt from mock import patch from rest_framework.test import APITestCase @@ -140,6 +140,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 + def verify_success(self, response): """Base implementation of verifying a successful response.""" self.assertEqual(response.status_code, 200) @@ -170,7 +172,10 @@ 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) @ddt.unpack diff --git a/lms/djangoapps/mobile_api/users/tests.py b/lms/djangoapps/mobile_api/users/tests.py index 2b28a20d4e..58ca235e80 100644 --- a/lms/djangoapps/mobile_api/users/tests.py +++ b/lms/djangoapps/mobile_api/users/tests.py @@ -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) diff --git a/lms/djangoapps/mobile_api/users/views.py b/lms/djangoapps/mobile_api/users/views.py index 9ac03b9b5e..f0fd43809c 100644 --- a/lms/djangoapps/mobile_api/users/views.py +++ b/lms/djangoapps/mobile_api/users/views.py @@ -26,7 +26,7 @@ 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 mobile_api.utils import mobile_course_listing_access, mobile_view, mobile_course_access @mobile_view(is_user=True) @@ -46,6 +46,7 @@ class UserDetail(generics.RetrieveAPIView): GET /api/mobile/v0.5/users/{username} + **Response Values** * id: The ID of the user. @@ -244,7 +245,7 @@ class UserCourseEnrollmentsList(generics.ListAPIView): enrollments = self.queryset.filter(user__username=self.kwargs['username'], is_active=True).order_by('created') return [ enrollment for enrollment in enrollments - if mobile_access_when_enrolled(enrollment.course, self.request.user) + if mobile_course_listing_access(enrollment.course, self.request.user) ] diff --git a/lms/djangoapps/mobile_api/utils.py b/lms/djangoapps/mobile_api/utils.py index 8e8aa56fb4..0e419fb482 100644 --- a/lms/djangoapps/mobile_api/utils.py +++ b/lms/djangoapps/mobile_api/utils.py @@ -4,7 +4,9 @@ Common utility methods and decorators for Mobile APIs. import functools +from contextlib import contextmanager from django.http import Http404 +from django.conf import settings from opaque_keys.edx.keys import CourseKey from courseware.courses import get_course_with_access @@ -12,6 +14,30 @@ from rest_framework import permissions from rest_framework.authentication import OAuth2Authentication, SessionAuthentication +# TODO This contextmanager should be moved to a common utility library. +@contextmanager +def dict_value(dictionary, key, value): + """ + A context manager that assigns 'value' to the 'key' in the 'dictionary' when entering the context, + and then resets the key upon exiting the context. + """ + + # cache previous values + has_previous_value = key in dictionary + previous_value = dictionary[key] if has_previous_value else None + + try: + # temporarily set to new value + dictionary[key] = value + yield + finally: + # reset to previous values + if has_previous_value: + dictionary[key] = previous_value + else: + dictionary.pop(key, None) + + def mobile_course_access(depth=0, verify_enrolled=True): """ Method decorator for a mobile API endpoint that verifies the user has access to the course in a mobile context. @@ -37,18 +63,22 @@ def mobile_course_access(depth=0, verify_enrolled=True): return _decorator -def mobile_access_when_enrolled(course, user): +def mobile_course_listing_access(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. + Determines whether a user has access to a course' listing in a mobile context. + Checks the mobile_available flag. + Checks roles including Beta Tester and staff roles. + Note: + Does not check if the user is actually enrolled in the course. + Does not check the start_date. """ # 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 + with dict_value(settings.FEATURES, 'DISABLE_START_DATES', True): + return get_course_with_access(user, 'load_mobile_no_enrollment_check', course.id) is not None except Http404: return False From a07f6072817678dd31e0a2190613dd1ebe39b263 Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Thu, 15 Jan 2015 17:15:46 -0500 Subject: [PATCH 2/3] Updated strategy per review feedback. --- lms/djangoapps/courseware/access.py | 20 ++++-- lms/djangoapps/mobile_api/tests.py | 81 +----------------------- lms/djangoapps/mobile_api/testutils.py | 21 +++--- lms/djangoapps/mobile_api/users/views.py | 15 ++--- lms/djangoapps/mobile_api/utils.py | 47 -------------- 5 files changed, 32 insertions(+), 152 deletions(-) diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index fe4159fefe..28b248891a 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -163,11 +163,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(): @@ -559,6 +555,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 57fc58f9de..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_course_listing_access, mobile_course_access, mobile_view, dict_value -from .testutils import MobileAPITestCase, ROLE_CASES - - -@ddt.ddt -class TestMobileCourseListingAccess(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_course_listing_access(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_course_listing_access(self.course, self.user)) - - def test_missing_course(self): - """ - Verifies that we handle the case where a course doesn't exist - """ - self.assertFalse(mobile_course_listing_access(None, self.user)) - - @patch.dict('django.conf.settings.FEATURES', {'DISABLE_START_DATES': False}) - def test_unreleased_course(self): - """ - Verifies that we allow the case where a course hasn't started - """ - self.assertTrue(mobile_course_listing_access(self.course, self.user)) +from .utils import mobile_course_access, mobile_view @ddt.ddt @@ -65,42 +25,3 @@ class TestMobileAPIDecorators(TestCase): self.assertIn("Test docstring of decorated function.", decorated_func.__doc__) self.assertEquals(decorated_func.__name__, "decorated_func") self.assertTrue(decorated_func.__module__.endswith("tests")) - - -@ddt.ddt -class TestDictContextManager(TestCase): - """ - Tests for dict contextmanager. - """ - def setUp(self): - super(TestDictContextManager, self).setUp() - self.test_dict = {} - self.test_key = 'test key' - - def call_context_manager(self, raise_exception): - """Helper method that calls the context manager.""" - new_value = "new value" - try: - with dict_value(self.test_dict, self.test_key, new_value): - # verify test_key is assigned to new_value within the context. - self.assertEquals(self.test_dict[self.test_key], new_value) - if raise_exception: - raise StandardError - except StandardError: - pass - - @ddt.data(True, False) - def test_no_previous_value(self, raise_exception): - self.call_context_manager(raise_exception) - - # verify test_key no longer exists in the dict. - self.assertNotIn(self.test_key, self.test_dict) - - @ddt.data(True, False) - def test_has_previous_value(self, raise_exception): - old_value = "old value" - self.test_dict[self.test_key] = old_value - self.call_context_manager(raise_exception) - - # verify test_key's value is reverted back to old_value. - self.assertEquals(self.test_dict[self.test_key], old_value) diff --git a/lms/djangoapps/mobile_api/testutils.py b/lms/djangoapps/mobile_api/testutils.py index 0c004132d4..61e6f58faa 100644 --- a/lms/djangoapps/mobile_api/testutils.py +++ b/lms/djangoapps/mobile_api/testutils.py @@ -10,7 +10,7 @@ Test utilities for mobile API tests: MobileCourseAccessTestMixin - tests for APIs with mobile_course_access and verify_enrolled=False. MobileEnrolledCourseAccessTestMixin - tests for APIs with mobile_course_access and verify_enrolled=True. """ -# pylint: disable=no-member, invalid-name +# pylint: disable=no-member import ddt from mock import patch from rest_framework.test import APITestCase @@ -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,7 +131,7 @@ 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 + ALLOW_ACCESS_TO_UNRELEASED_COURSE = False # pylint: disable=invalid-name def verify_success(self, response): """Base implementation of verifying a successful response.""" @@ -177,7 +168,13 @@ class MobileCourseAccessTestMixin(object): 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/views.py b/lms/djangoapps/mobile_api/users/views.py index f0fd43809c..0b0434f7b5 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_course_listing_access, mobile_view, mobile_course_access +from .. import errors +from ..utils import mobile_view, mobile_course_access @mobile_view(is_user=True) @@ -245,7 +244,7 @@ class UserCourseEnrollmentsList(generics.ListAPIView): enrollments = self.queryset.filter(user__username=self.kwargs['username'], is_active=True).order_by('created') return [ enrollment for enrollment in enrollments - if mobile_course_listing_access(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 0e419fb482..5905fc886d 100644 --- a/lms/djangoapps/mobile_api/utils.py +++ b/lms/djangoapps/mobile_api/utils.py @@ -4,9 +4,6 @@ Common utility methods and decorators for Mobile APIs. import functools -from contextlib import contextmanager -from django.http import Http404 -from django.conf import settings from opaque_keys.edx.keys import CourseKey from courseware.courses import get_course_with_access @@ -14,30 +11,6 @@ from rest_framework import permissions from rest_framework.authentication import OAuth2Authentication, SessionAuthentication -# TODO This contextmanager should be moved to a common utility library. -@contextmanager -def dict_value(dictionary, key, value): - """ - A context manager that assigns 'value' to the 'key' in the 'dictionary' when entering the context, - and then resets the key upon exiting the context. - """ - - # cache previous values - has_previous_value = key in dictionary - previous_value = dictionary[key] if has_previous_value else None - - try: - # temporarily set to new value - dictionary[key] = value - yield - finally: - # reset to previous values - if has_previous_value: - dictionary[key] = previous_value - else: - dictionary.pop(key, None) - - def mobile_course_access(depth=0, verify_enrolled=True): """ Method decorator for a mobile API endpoint that verifies the user has access to the course in a mobile context. @@ -63,26 +36,6 @@ def mobile_course_access(depth=0, verify_enrolled=True): return _decorator -def mobile_course_listing_access(course, user): - """ - Determines whether a user has access to a course' listing in a mobile context. - Checks the mobile_available flag. - Checks roles including Beta Tester and staff roles. - Note: - Does not check if the user is actually enrolled in the course. - Does not check the start_date. - """ - # 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: - with dict_value(settings.FEATURES, 'DISABLE_START_DATES', True): - 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. From dea6b764de2b17267eab93dae69bc5cfec11c86f Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Thu, 15 Jan 2015 20:02:39 -0500 Subject: [PATCH 3/3] Mobile enrollments API: sort order by recent enrollment date. --- lms/djangoapps/mobile_api/users/tests.py | 19 ++++++++++++++++++- lms/djangoapps/mobile_api/users/views.py | 5 ++++- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/mobile_api/users/tests.py b/lms/djangoapps/mobile_api/users/tests.py index 58ca235e80..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 @@ -66,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 0b0434f7b5..a21a06a3ac 100644 --- a/lms/djangoapps/mobile_api/users/views.py +++ b/lms/djangoapps/mobile_api/users/views.py @@ -241,7 +241,10 @@ 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 enrollment.course and is_mobile_available_for_user(self.request.user, enrollment.course)