Updated strategy per review feedback.

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

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)

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()

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)
         ]
 
 

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.