Response to CR

common/djangoapps/student/models.py

@@ -47,7 +47,6 @@ from xmodule.modulestore.exceptions import ItemNotFoundError
 from xmodule.modulestore.django import modulestore
 from opaque_keys.edx.keys import CourseKey
 from functools import total_ordering
-from courseware.access import has_access
 
 from certificates.models import GeneratedCertificate
 from course_modes.models import CourseMode
@@ -796,7 +795,7 @@ class CourseEnrollment(models.Model):
                 log.exception('Unable to emit event %s for user %s and course %s', event_name, self.user.username, self.course_id)
 
     @classmethod
-    def enroll(cls, user, course_key, mode="honor"):
+    def enroll(cls, user, course_key, mode="honor", check_access=False):
         """
         Enroll a user in a course. This saves immediately.
 
@@ -806,13 +805,19 @@ class CourseEnrollment(models.Model):
                attribute), this method will automatically save it before
                adding an enrollment for it.
 
-        `course_id` is our usual course_id string (e.g. "edX/Test101/2013_Fall)
+        `course_key` is our usual course_id string (e.g. "edX/Test101/2013_Fall)
 
         `mode` is a string specifying what kind of enrollment this is. The
                default is "honor", meaning honor certificate. Future options
                may include "audit", "verified_id", etc. Please don't use it
                until we have these mapped out.
 
+        `check_access`: if True, we check that an accessible course actually
+                exists for the given course_key before we enroll the student.
+                The default is set to False to avoid breaking legacy code or
+                code with non-standard flows (ex. beta tester invitations), but
+                for any standard enrollment flow you probably want this to be True.
+
         Exceptions that can be raised: NonExistentCourseError,
         EnrollmentClosedError, CourseFullError, AlreadyEnrolledError.  All these
         are subclasses of CourseEnrollmentException if you want to catch all of
@@ -823,6 +828,7 @@ class CourseEnrollment(models.Model):
 
         Also emits relevant events for analytics purposes.
         """
+        from courseware.access import has_access
 
         # All the server-side checks for whether a user is allowed to enroll.
         try:
@@ -835,25 +841,27 @@ class CourseEnrollment(models.Model):
                 )
             )
             raise NonExistentCourseError
-        if course is None:
-            raise NonExistentCourseError
 
-        if not has_access(user, 'enroll', course):
-            log.warning(
-                "User {0} failed to enroll in course {1} because enrollment is closed".format(
-                    user.username,
-                    course_key.to_deprecated_string()
+        if check_access:
+            if course is None:
+                raise NonExistentCourseError
+            if not has_access(user, 'enroll', course):
+                log.warning(
+                    "User {0} failed to enroll in course {1} because enrollment is closed".format(
+                        user.username,
+                        course_key.to_deprecated_string()
+                    )
                 )
-            )
-            raise EnrollmentClosedError
-        if CourseEnrollment.is_course_full(course):
-            log.warning(
-                "User {0} failed to enroll in full course {1}".format(
-                    user.username,
-                    course_key.to_deprecated_string()
+                raise EnrollmentClosedError
+
+            if CourseEnrollment.is_course_full(course):
+                log.warning(
+                    "User {0} failed to enroll in full course {1}".format(
+                        user.username,
+                        course_key.to_deprecated_string()
+                    )
                 )
-            )
-            raise CourseFullError
+                raise CourseFullError
         if CourseEnrollment.is_enrolled(user, course_key):
             log.warning(
                 "User {0} attempted to enroll in {1}, but they were already enrolled".format(
@@ -861,7 +869,8 @@ class CourseEnrollment(models.Model):
                     course_key.to_deprecated_string()
                 )
             )
-            raise AlreadyEnrolledError
+            if check_access:
+                raise AlreadyEnrolledError
 
         # User is allowed to enroll if they've reached this point.
         enrollment = cls.get_or_create_enrollment(user, course_key)

common/djangoapps/student/roles.py

@@ -7,6 +7,7 @@ from abc import ABCMeta, abstractmethod
 
 from django.contrib.auth.models import User
 
+from student.models import CourseAccessRole
 from xmodule_django.models import CourseKeyField
 
 
@@ -15,7 +16,6 @@ class RoleCache(object):
     A cache of the CourseAccessRoles held by a particular user
     """
     def __init__(self, user):
-        from student.models import CourseAccessRole
         self._roles = set(
             CourseAccessRole.objects.filter(user=user).all()
         )
@@ -140,7 +140,6 @@ class RoleBase(AccessRole):
         """
         Remove the supplied django users from this role.
         """
-        from student.models import CourseAccessRole
         entries = CourseAccessRole.objects.filter(
             user__in=users, role=self._role_name, org=self.org, course_id=self.course_key
         )
@@ -177,7 +176,6 @@ class CourseRole(RoleBase):
 
     @classmethod
     def course_group_already_exists(self, course_key):
-        from student.models import CourseAccessRole
         return CourseAccessRole.objects.filter(org=course_key.org, course_id=course_key).exists()
 
 
@@ -271,7 +269,6 @@ class UserBasedRole(object):
         """
         Grant this object's user the object's role for the supplied courses
         """
-        from student.models import CourseAccessRole
         if self.user.is_authenticated and self.user.is_active:
             for course_key in course_keys:
                 entry = CourseAccessRole(user=self.user, role=self.role, course_id=course_key, org=course_key.org)
@@ -285,7 +282,6 @@ class UserBasedRole(object):
         """
         Remove the supplied courses from this user's configured role.
         """
-        from student.models import CourseAccessRole
         entries = CourseAccessRole.objects.filter(user=self.user, role=self.role, course_id__in=course_keys)
         entries.delete()
         if hasattr(self.user, '_roles'):
@@ -300,5 +296,4 @@ class UserBasedRole(object):
         * course_id
         * role (will be self.role--thus uninteresting)
         """
-        from student.models import CourseAccessRole
         return CourseAccessRole.objects.filter(role=self.role, user=self.user)

common/djangoapps/student/tests/test_enrollment.py

@@ -13,9 +13,12 @@ from xmodule.modulestore.tests.django_utils import (
     ModuleStoreTestCase, mixed_store_config
 )
 from xmodule.modulestore.tests.factories import CourseFactory
+from social.strategies.django_strategy import DjangoStrategy
+from django.test.client import RequestFactory
 from student.tests.factories import UserFactory, CourseModeFactory
 from student.models import CourseEnrollment
-
+from student.views import register_user
+from third_party_auth.pipeline import change_enrollment as change_enrollment_third_party
 
 # Since we don't need any XML course fixtures, use a modulestore configuration
 # that disables the XML modulestore.
@@ -153,6 +156,25 @@ class EnrollmentTest(ModuleStoreTestCase):
         self.assertIn('auto_register', self.client.session)
         self.assertTrue(self.client.session['auto_register'])
 
+    def test_enroll_from_redirect_autoreg_third_party(self):
+        """
+        Test that, when a user visits the registration page *after* visiting a course,
+        if they go on to register and/or log in via third-party auth, they'll be registered
+        in that course.
+
+        The testing here is a bit hackish, since we just ping the registration page, then
+        directly call the step in the third party pipeline that registers the user if
+        `registration_course_id` is set in the session, but it should catch any major breaks.
+        """
+        self.client.logout()
+        self.client.get(reverse('register_user'), {'course_id': self.course.id})
+        self.client.login(username=self.USERNAME, password=self.PASSWORD)
+        self.dummy_request = RequestFactory().request()
+        self.dummy_request.session = self.client.session
+        strategy = DjangoStrategy(RequestFactory, request=self.dummy_request)
+        change_enrollment_third_party(is_register=True, strategy=strategy, user=self.user)
+        self.assertTrue(CourseEnrollment.is_enrolled(self.user, self.course.id))
+
     # TODO (ECOM-16): Remove once the auto-registration A/B test completes
     def test_enroll_auto_registration_excluded_course(self):
         # Create the course modes

common/djangoapps/student/views.py

@@ -626,7 +626,7 @@ def try_change_enrollment(request):
 
 
 @require_POST
-def change_enrollment(request, auto_register=False):
+def change_enrollment(request, auto_register=False, check_access=True):
     """
     Modify the enrollment status for the logged-in user.
 
@@ -661,9 +661,25 @@ def change_enrollment(request, auto_register=False):
         Response
 
     """
+    # Sets the auto_register flag, if that's desired
+    # TODO (ECOM-16): Remove this once the auto-registration A/B test completes
+    # If a user is in the experimental condition (auto-registration enabled),
+    # immediately set a session flag so they stay in the experimental condition.
+    # We keep them in the experimental condition even if later on the user
+    # tries to register using the control URL (e.g. because of a redirect from the login page,
+    # which is hard-coded to use the control URL).
+    if auto_register:
+        request.session['auto_register'] = True
+    if request.session.get('auto_register') and not auto_register:
+        auto_register = True
+
     # Get the user
     user = request.user
 
+    # Ensure the user is authenticated
+    if not user.is_authenticated():
+        return HttpResponseForbidden()
+
     # Ensure we received a course_id
     action = request.POST.get("enrollment_action")
     if 'course_id' not in request.POST:
@@ -681,22 +697,6 @@ def change_enrollment(request, auto_register=False):
         )
         return HttpResponseBadRequest(_("Invalid course id"))
 
-    # Ensure the user is authenticated
-    if not user.is_authenticated():
-        return HttpResponseForbidden()
-
-    # Sets the auto_register flag, if that's desired
-    # TODO (ECOM-16): Remove this once the auto-registration A/B test completes
-    # If a user is in the experimental condition (auto-registration enabled),
-    # immediately set a session flag so they stay in the experimental condition.
-    # We keep them in the experimental condition even if later on the user
-    # tries to register using the control URL (e.g. because of a redirect from the login page,
-    # which is hard-coded to use the control URL).
-    if auto_register:
-        request.session['auto_register'] = True
-    if request.session.get('auto_register') and not auto_register:
-        auto_register = True
-
     # Don't execute auto-register for the set of courses excluded from auto-registration
     # TODO (ECOM-16): Remove this once the auto-registration A/B test completes
     # We've agreed to exclude certain courses from the A/B test.  If we find ourselves
@@ -733,7 +733,7 @@ def change_enrollment(request, auto_register=False):
                 # for no such model to exist, even though we've set the enrollment type
                 # to "honor".
                 try:
-                    CourseEnrollment.enroll(user, course_id, mode=current_mode.slug)
+                    CourseEnrollment.enroll(user, course_id, check_access=check_access)
                 except Exception:
                     return HttpResponseBadRequest(_("Could not enroll"))
 
@@ -769,7 +769,7 @@ def change_enrollment(request, auto_register=False):
                 )
 
             try:
-                CourseEnrollment.enroll(user, course_id, mode=current_mode.slug)
+                CourseEnrollment.enroll(user, course_id, mode=current_mode.slug, check_access=check_access)
             except Exception:
                 return HttpResponseBadRequest(_("Could not enroll"))
 

common/djangoapps/third_party_auth/pipeline.py

@@ -69,9 +69,11 @@ from social.apps.django_app.default import models
 from social.exceptions import AuthException
 from social.pipeline import partial
 
-from student.models import CourseEnrollment
+from student.models import CourseEnrollment, CourseEnrollmentException
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
 
+from logging import getLogger
+
 from . import provider
 
 
@@ -87,6 +89,8 @@ _AUTH_ENTRY_CHOICES = frozenset([
 _DEFAULT_RANDOM_PASSWORD_LENGTH = 12
 _PASSWORD_CHARSET = string.letters + string.digits
 
+logger = getLogger(__name__)
+
 
 class AuthEntryError(AuthException):
     """Raised when auth_entry is missing or invalid on URLs.
@@ -404,7 +408,7 @@ def login_analytics(*args, **kwargs):
             }
         )
 
-@partial.partial
+#@partial.partial
 def change_enrollment(*args, **kwargs):
     """
     If the user accessed the third party auth flow after trying to register for
@@ -418,5 +422,7 @@ def change_enrollment(*args, **kwargs):
                     kwargs['strategy'].session_get('registration_course_id')
                 )
             )
-        except:
+        except CourseEnrollmentException:
             pass
+        except Exception, e:
+            logger.exception(e)

lms/djangoapps/courseware/access.py

@@ -21,6 +21,7 @@ from student.roles import (
     GlobalStaff, CourseStaffRole, CourseInstructorRole,
     OrgStaffRole, OrgInstructorRole, CourseBetaTesterRole
 )
+from student.models import CourseEnrollment, CourseEnrollmentAllowed
 from opaque_keys.edx.keys import CourseKey, UsageKey
 DEBUG_ACCESS = False
 
@@ -123,7 +124,6 @@ def _has_access_course_desc(user, action, course):
         """
         Can this user access the forums in this course?
         """
-        from student.models import CourseEnrollment
         return (
             can_load() and
             (
@@ -148,7 +148,6 @@ def _has_access_course_desc(user, action, course):
         """
 
         # if using registration method to restrict (say shibboleth)
-        from student.models import CourseEnrollmentAllowed
         if settings.FEATURES.get('RESTRICT_ENROLL_BY_REG_METHOD') and course.enrollment_domain:
             if user is not None and user.is_authenticated() and \
                 ExternalAuthMap.objects.filter(user=user, external_domain=course.enrollment_domain):

lms/djangoapps/courseware/tests/helpers.py

@@ -116,6 +116,7 @@ class LoginEnrollmentTestCase(TestCase):
         resp = self.client.post(reverse('change_enrollment'), {
             'enrollment_action': 'enroll',
             'course_id': course.id.to_deprecated_string(),
+            'check_access': True,
         })
         result = resp.status_code == 200
         if verify: