Auto enroll students even if there are multiple course modes

Add session middleware to an external auth test to fix a test failure with the auto-registration AB-test changes

common/djangoapps/course_modes/views.py

@@ -30,20 +30,52 @@ class ChooseModeView(View):
         from the selection page, parses the response, and then sends user
         to the next step in the flow
     """
+
     @method_decorator(login_required)
     def get(self, request, course_id, error=None):
-        """ Displays the course mode choice page """
+        """ Displays the course mode choice page
+
+        Args:
+            request (`Request`): The Django Request object.
+            course_id (unicode): The slash-separated course key.
+
+        Keyword Args:
+            error (unicode): If provided, display this error message
+                on the page.
 
+        Returns:
+            Response
+
+        """
         course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id)
 
         enrollment_mode, is_active = CourseEnrollment.enrollment_mode_for_user(request.user, course_key)
         upgrade = request.GET.get('upgrade', False)
         request.session['attempting_upgrade'] = upgrade
 
+        # TODO (ECOM-16): Remove once the AB-test of auto-registration completes
+        auto_register = request.session.get('auto_register', False)
+
         # Inactive users always need to re-register
-        # verified and professional users do not need to register or upgrade
-        # registered users who are not trying to upgrade do not need to re-register
-        if is_active and (upgrade is False or enrollment_mode == 'verified' or enrollment_mode == 'professional'):
+        # Verified and professional users do not need to register or upgrade
+        # Registered users who are not trying to upgrade do not need to re-register
+        if not auto_register:
+            go_to_dashboard = (
+                is_active and
+                (not upgrade or enrollment_mode in ['verified', 'professional'])
+            )
+
+        # If auto-registration is enabled, then students might already be registered,
+        # but we should still show them the "choose your track" page so they have
+        # the option to enter the verification/payment flow.
+        # TODO (ECOM-16): Based on the results of the AB-test, set the default behavior to
+        # either enable or disable auto-registration.
+        else:
+            go_to_dashboard = (
+                not upgrade and enrollment_mode in ['verified', 'professional']
+            )
+
+        if go_to_dashboard:
             return redirect(reverse('dashboard'))
 
         modes = CourseMode.modes_for_course_dict(course_key)
@@ -73,6 +105,7 @@ class ChooseModeView(View):
             "error": error,
             "upgrade": upgrade,
             "can_audit": "audit" in modes,
+            "autoreg": auto_register
         }
         if "verified" in modes:
             context["suggested_prices"] = [

common/djangoapps/external_auth/tests/test_shib.py

@@ -13,6 +13,7 @@ from django.test.client import RequestFactory, Client as DjangoTestClient
 from django.test.utils import override_settings
 from django.core.urlresolvers import reverse
 from django.contrib.auth.models import AnonymousUser, User
+from django.contrib.sessions.middleware import SessionMiddleware
 from django.utils.importlib import import_module
 
 from xmodule.modulestore.tests.factories import CourseFactory
@@ -513,6 +514,11 @@ class ShibSPTest(ModuleStoreTestCase):
         for course in [shib_course, open_enroll_course]:
             for student in [shib_student, other_ext_student, int_student]:
                 request = self.request_factory.post('/change_enrollment')
+
+                # Add a session to the request
+                SessionMiddleware().process_request(request)
+                request.session.save()
+
                 request.POST.update({'enrollment_action': 'enroll',
                                      'course_id': course.id.to_deprecated_string()})
                 request.user = student

common/djangoapps/student/tests/test_enrollment.py

+"""
+Tests for student enrollment.
+"""
+from datetime import datetime, timedelta
+import pytz
+import ddt
+import unittest
+
+from django.test.utils import override_settings
+from django.conf import settings
+from django.core.urlresolvers import reverse
+from xmodule.modulestore.tests.django_utils import (
+    ModuleStoreTestCase, mixed_store_config
+)
+from xmodule.modulestore.tests.factories import CourseFactory
+from student.tests.factories import UserFactory, CourseModeFactory
+from student.models import CourseEnrollment
+
+
+# Since we don't need any XML course fixtures, use a modulestore configuration
+# that disables the XML modulestore.
+MODULESTORE_CONFIG = mixed_store_config(settings.COMMON_TEST_DATA_ROOT, {}, include_xml=False)
+
+
+@ddt.ddt
+@override_settings(MODULESTORE=MODULESTORE_CONFIG)
+@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
+class EnrollmentTest(ModuleStoreTestCase):
+    """
+    Test student enrollment, especially with different course modes.
+    """
+    def setUp(self):
+        """ Create a course and user, then log in. """
+        super(EnrollmentTest, self).setUp()
+        self.course = CourseFactory.create()
+        self.user = UserFactory.create(username="Bob", email="bob@example.com", password="edx")
+        self.client.login(username=self.user.username, password="edx")
+
+        self.urls = [
+            reverse('course_modes_choose', kwargs={'course_id': unicode(self.course.id)})
+        ]
+
+    # TODO (ECOM-16): We need separate test cases for both conditions in the auto-registration
+    # AB-test.  Once we get the results of that test, we should
+    # remove the losing condition from this test.
+    @ddt.data(
+        # Default (no course modes in the database)
+        # Expect that we're redirected to the dashboard
+        # and automatically enrolled as "honor"
+        ([], '', 'honor', False),
+        ([], '', 'honor', True),
+
+        # Audit / Verified / Honor
+        # We should always go to the "choose your course" page,
+        # If auto-registration is enabled, we should also be registered
+        # as "honor" by default.
+        (['honor', 'verified', 'audit'], 'course_modes_choose', None, False),
+        (['honor', 'verified', 'audit'], 'course_modes_choose', 'honor', True),
+
+        # Professional ed
+        # Expect that we're sent to the "choose your track" page
+        # (which will, in turn, redirect us to a page where we can verify/pay)
+        # Even if auto registration is enabled, we should NOT be auto-registered,
+        # because that would be giving away an expensive course for free :)
+        (['professional'], 'course_modes_choose', None, False),
+        (['professional'], 'course_modes_choose', None, True),
+
+    )
+    @ddt.unpack
+    def test_enroll(self, course_modes, next_url, enrollment_mode, auto_reg):
+        # Create the course modes (if any) required for this test case
+        for mode_slug in course_modes:
+            CourseModeFactory.create(
+                course_id=self.course.id,
+                mode_slug=mode_slug,
+                mode_display_name=mode_slug,
+                expiration_datetime=datetime.now(pytz.UTC) + timedelta(days=1)
+            )
+
+        # Reverse the expected next URL, if one is provided
+        # (otherwise, use an empty string, which the JavaScript client
+        # interprets as a redirect to the dashboard)
+        full_url = (
+            reverse(next_url, kwargs={'course_id': unicode(self.course.id)})
+            if next_url else next_url
+        )
+
+        # Enroll in the course and verify the URL we get sent to
+        resp = self._change_enrollment('enroll', auto_reg=auto_reg)
+        self.assertEqual(resp.status_code, 200)
+        self.assertEqual(resp.content, full_url)
+
+        # TODO (ECOM-16): If auto-registration is enabled, check that we're
+        # storing the auto-reg flag in the user's session
+        if auto_reg:
+            self.assertIn('auto_register', self.client.session)
+            self.assertTrue(self.client.session['auto_register'])
+
+        # If we're not expecting to be enrolled, verify that this is the case
+        if enrollment_mode is None:
+            self.assertFalse(CourseEnrollment.is_enrolled(self.user, self.course.id))
+
+        # Otherwise, verify that we're enrolled with the expected course mode
+        else:
+            self.assertTrue(CourseEnrollment.is_enrolled(self.user, self.course.id))
+            course_mode, is_active = CourseEnrollment.enrollment_mode_for_user(self.user, self.course.id)
+            self.assertTrue(is_active)
+            self.assertEqual(course_mode, enrollment_mode)
+
+    def test_unenroll(self):
+        # Enroll the student in the course
+        CourseEnrollment.enroll(self.user, self.course.id, mode="honor")
+
+        # Attempt to unenroll the student
+        resp = self._change_enrollment('unenroll')
+        self.assertEqual(resp.status_code, 200)
+
+        # Expect that we're no longer enrolled
+        self.assertFalse(CourseEnrollment.is_enrolled(self.user, self.course.id))
+
+    def test_user_not_authenticated(self):
+        # Log out, so we're no longer authenticated
+        self.client.logout()
+
+        # Try to enroll, expecting a forbidden response
+        resp = self._change_enrollment('enroll')
+        self.assertEqual(resp.status_code, 403)
+
+    def test_missing_course_id_param(self):
+        resp = self.client.post(
+            reverse('change_enrollment'),
+            {'enrollment_action': 'enroll'}
+        )
+        self.assertEqual(resp.status_code, 400)
+
+    def test_unenroll_not_enrolled_in_course(self):
+        # Try unenroll without first enrolling in the course
+        resp = self._change_enrollment('unenroll')
+        self.assertEqual(resp.status_code, 400)
+
+    def test_invalid_enrollment_action(self):
+        resp = self._change_enrollment('not_an_action')
+        self.assertEqual(resp.status_code, 400)
+
+    def _change_enrollment(self, action, course_id=None, auto_reg=False):
+        """
+        Change the student's enrollment status in a course.
+
+        Args:
+            action (string): The action to perform (either "enroll" or "unenroll")
+
+        Keyword Args:
+            course_id (unicode): If provided, use this course ID.  Otherwise, use the
+                course ID created in the setup for this test.
+
+            auto_reg (boolean): Whether to use the auto-registration hook.
+                TODO (ECOM-16): remove this once we complete the AB test for auto-registration.
+
+        Returns:
+            Response
+
+        """
+        if course_id is None:
+            course_id = unicode(self.course.id)
+
+        url = (
+            reverse('change_enrollment')
+            if not auto_reg
+            else reverse('change_enrollment_autoreg')
+        )
+        params = {
+            'enrollment_action': action,
+            'course_id': course_id
+        }
+        return self.client.post(url, params)

common/djangoapps/student/views.py

@@ -582,7 +582,7 @@ def try_change_enrollment(request):
 
 
 @require_POST
-def change_enrollment(request):
+def change_enrollment(request, auto_register=False):
     """
     Modify the enrollment status for the logged-in user.
 
@@ -598,6 +598,24 @@ def change_enrollment(request):
     happens. This function should only be called from an AJAX request or
     as a post-login/registration helper, so the error messages in the responses
     should never actually be user-visible.
+    The original version of the change enrollment handler,
+    which does NOT perform auto-registration.
+
+    TODO (ECOM-16): We created a second variation of this handler that performs
+    auto-registration for an AB-test.  Depending on the results of that test,
+    we should make the winning implementation the default.
+
+    Args:
+        request (`Request`): The Django request object
+
+    Keyword Args:
+        auto_register (boolean): If True, auto-register the user
+            for a default course mode when they first enroll
+            before sending them to the "choose your track" page
+
+    Returns:
+        Response
+
     """
     user = request.user
 
@@ -635,24 +653,79 @@ def change_enrollment(request):
                 _("Student is already enrolled")
             )
 
-        # If this course is available in multiple modes, redirect them to a page
-        # where they can choose which mode they want.
-        available_modes = CourseMode.modes_for_course(course_id)
-        if len(available_modes) > 1:
-            return HttpResponse(
-                reverse("course_modes_choose", kwargs={'course_id': unicode(course_id)})
-            )
+        # We use this flag to determine which condition of an AB-test
+        # for auto-registration we're currently in.
+        # (We have two URLs that both point to this view, but vary the
+        # value of `auto_register`)
+        # In the auto-registration case, we automatically register the student
+        # as "honor" before allowing them to choose a track.
+        # TODO (ECOM-16): Once the auto-registration AB-test is complete, delete
+        # one of these two conditions and remove the `auto_register` flag.
+        if auto_register:
+            # TODO (ECOM-16): This stores a flag in the session so downstream
+            # views will recognize that the user is in the "auto-registration"
+            # experimental condition.  We can remove this once the AB test completes.
+            request.session['auto_register'] = True
+
+            available_modes = CourseMode.modes_for_course_dict(course_id)
+
+            # Handle professional ed as a special case.
+            # If professional ed is included in the list of available modes,
+            # then do NOT automatically enroll the student (we want them to pay first!)
+            # By convention, professional ed should be the *only* available course mode,
+            # if it's included at all -- anything else is a misconfiguration.  But if someone
+            # messes up and adds an additional course mode, we err on the side of NOT
+            # accidentally giving away free courses.
+            if "professional" not in available_modes:
+                # Enroll the user using the default mode (honor)
+                # We're assuming that users of the course enrollment table
+                # will NOT try to look up the course enrollment model
+                # by its slug.  If they do, it's possible (based on the state of the database)
+                # for no such model to exist, even though we've set the enrollment type
+                # to "honor".
+                CourseEnrollment.enroll(user, course.id)
+
+            # If we have more than one course mode or professional ed is enabled,
+            # then send the user to the choose your track page.
+            # (In the case of professional ed, this will redirect to a page that
+            # funnels users directly into the verification / payment flow)
+            if len(available_modes) > 1 or "professional" in available_modes:
+                return HttpResponse(
+                    reverse("course_modes_choose", kwargs={'course_id': unicode(course_id)})
+                )
 
-        current_mode = available_modes[0]
-        # only automatically enroll people if the only mode is 'honor'
-        if current_mode.slug != 'honor':
-            return HttpResponse(
-                reverse("course_modes_choose", kwargs={'course_id': unicode(course_id)})
-            )
+            # Otherwise, there is only one mode available (the default)
+            return HttpResponse()
 
-        CourseEnrollment.enroll(user, course.id, mode=current_mode.slug)
+        # If auto-registration is disabled, do NOT register the student
+        # before sending them to the "choose your track" page.
+        # This is the control for the auto-registration AB-test.
+        else:
+            # TODO (ECOM-16): If the user is NOT in the experimental condition,
+            # make sure their session reflects this.  We can remove this
+            # once the AB test completes.
+            if 'auto_register' in request.session:
+                del request.session['auto_register']
+
+            # If this course is available in multiple modes, redirect them to a page
+            # where they can choose which mode they want.
+            available_modes = CourseMode.modes_for_course(course_id)
+            if len(available_modes) > 1:
+                return HttpResponse(
+                    reverse("course_modes_choose", kwargs={'course_id': unicode(course_id)})
+                )
+
+            current_mode = available_modes[0]
+            # only automatically enroll people if the only mode is 'honor'
+            if current_mode.slug != 'honor':
+                return HttpResponse(
+                    reverse("course_modes_choose", kwargs={'course_id': unicode(course_id)})
+                )
+
+            CourseEnrollment.enroll(user, course.id, mode=current_mode.slug)
+
+            return HttpResponse()
 
-        return HttpResponse()
 
     elif action == "add_to_cart":
         # Pass the request handling to shoppingcart.views

common/lib/xmodule/xmodule/modulestore/tests/django_utils.py

@@ -15,36 +15,45 @@ from xmodule.tabs import CoursewareTab, CourseInfoTab, StaticTab, DiscussionTab,
 from xmodule.modulestore.tests.sample_courses import default_block_info_tree, TOY_BLOCK_INFO_TREE
 
 
-def mixed_store_config(data_dir, mappings):
+def mixed_store_config(data_dir, mappings, include_xml=True):
     """
     Return a `MixedModuleStore` configuration, which provides
     access to both Mongo- and XML-backed courses.
 
-    `data_dir` is the directory from which to load XML-backed courses.
-    `mappings` is a dictionary mapping course IDs to modulestores, for example:
+    Args:
+        data_dir (string): the directory from which to load XML-backed courses.
+        mappings (string): a dictionary mapping course IDs to modulestores, for example:
 
-        {
-            'MITx/2.01x/2013_Spring': 'xml',
-            'edx/999/2013_Spring': 'default'
-        }
+            {
+                'MITx/2.01x/2013_Spring': 'xml',
+                'edx/999/2013_Spring': 'default'
+            }
+
+        where 'xml' and 'default' are the two options provided by this configuration,
+        mapping (respectively) to XML-backed and Mongo-backed modulestores..
+
+    Keyword Args:
+
+        include_xml (boolean): If True, include an XML modulestore in the configuration.
+            Note that this will require importing multiple XML courses from disk,
+            so unless your tests really needs XML course fixtures or is explicitly
+            testing mixed modulestore, set this to False.
 
-    where 'xml' and 'default' are the two options provided by this configuration,
-    mapping (respectively) to XML-backed and Mongo-backed modulestores..
     """
-    draft_mongo_config = draft_mongo_store_config(data_dir)
-    xml_config = xml_store_config(data_dir)
-    split_mongo = split_mongo_store_config(data_dir)
+    stores = [
+        draft_mongo_store_config(data_dir)['default'],
+        split_mongo_store_config(data_dir)['default']
+    ]
+
+    if include_xml:
+        stores.append(xml_store_config(data_dir)['default'])
 
     store = {
         'default': {
             'ENGINE': 'xmodule.modulestore.mixed.MixedModuleStore',
             'OPTIONS': {
                 'mappings': mappings,
-                'stores': [
-                    draft_mongo_config['default'],
-                    split_mongo['default'],
-                    xml_config['default'],
-                ]
+                'stores': stores,
             }
         }
     }

lms/djangoapps/verify_student/views.py

@@ -111,6 +111,9 @@ class VerifyView(View):
             "upgrade": upgrade == u'True',
             "can_audit": CourseMode.mode_for_course(course_id, 'audit') is not None,
             "modes_dict": CourseMode.modes_for_course_dict(course_id),
+
+            # TODO (ECOM-16): Remove once the AB test completes
+            "autoreg": request.session.get('auto_register', False),
         }
 
         return render_to_response('verify_student/photo_verification.html', context)
@@ -160,6 +163,9 @@ class VerifiedView(View):
             "upgrade": upgrade == u'True',
             "can_audit": "audit" in modes_dict,
             "modes_dict": modes_dict,
+
+            # TODO (ECOM-16): Remove once the AB test completes
+            "autoreg": request.session.get('auto_register', False),
         }
         return render_to_response('verify_student/verified.html', context)
 
@@ -326,6 +332,9 @@ def show_requirements(request, course_id):
         "is_not_active": not request.user.is_active,
         "upgrade": upgrade == u'True',
         "modes_dict": modes_dict,
+
+        # TODO (ECOM-16): Remove once the AB test completes
+        "autoreg": request.session.get('auto_register', False),
     }
     return render_to_response("verify_student/show_requirements.html", context)
 

lms/urls.py

@@ -224,6 +224,17 @@ if settings.COURSEWARE_ENABLED:
             'student.views.change_enrollment', name="change_enrollment"),
         url(r'^change_email_settings$', 'student.views.change_email_settings', name="change_email_settings"),
 
+        # Used for an AB-test of auto-registration
+        # TODO (ECOM-16): Based on the AB-test, update the default behavior and change
+        # this URL to point to the original view.  Eventually, this URL
+        # should be removed, but not the AB test completes.
+        url(
+            r'^change_enrollment_autoreg$',
+            'student.views.change_enrollment',
+            {'auto_register': True},
+            name="change_enrollment_autoreg",
+        ),
+
         #About the course
         url(r'^courses/{}/about$'.format(settings.COURSE_ID_PATTERN),
             'courseware.views.course_about', name="about_course"),