From 3af9943de8a0208722a03ff49d16cdcf34487fbe Mon Sep 17 00:00:00 2001 From: Will Daly Date: Thu, 28 Aug 2014 07:03:51 -0400 Subject: [PATCH 1/2] Bugfix for ECOM-199: Visitors removed from auto-enroll pool if they need to authenticate while attempting to enroll Sets a session variable as soon as a user enters the experimental condition to ensure that they stay in that condition, even if they later access the control URL (e.g. from a redirect on the login page) --- .../student/tests/test_enrollment.py | 52 ++++++++++++++++++- common/djangoapps/student/tests/tests.py | 14 ++++- common/djangoapps/student/views.py | 22 ++++---- 3 files changed, 73 insertions(+), 15 deletions(-) diff --git a/common/djangoapps/student/tests/test_enrollment.py b/common/djangoapps/student/tests/test_enrollment.py index 83fb38a58b..e453843083 100644 --- a/common/djangoapps/student/tests/test_enrollment.py +++ b/common/djangoapps/student/tests/test_enrollment.py @@ -29,12 +29,16 @@ class EnrollmentTest(ModuleStoreTestCase): """ Test student enrollment, especially with different course modes. """ + USERNAME = "Bob" + EMAIL = "bob@example.com" + PASSWORD = "edx" + 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.user = UserFactory.create(username=self.USERNAME, email=self.EMAIL, password=self.PASSWORD) + self.client.login(username=self.USERNAME, password=self.PASSWORD) self.urls = [ reverse('course_modes_choose', kwargs={'course_id': unicode(self.course.id)}) @@ -107,6 +111,50 @@ class EnrollmentTest(ModuleStoreTestCase): self.assertTrue(is_active) self.assertEqual(course_mode, enrollment_mode) + # TODO (ECOM-16): Remove once the auto-registration A/B test completes. + def test_enroll_from_redirect_autoreg(self): + # Bugfix (ECOM-199): Visitors removed from auto-enroll pool + # if they need to authenticate while attempting to enroll. + # If a user is (a) in the experimental condition of the A/B test (autoreg enabled) + # and (b) is not logged in when registering for a course, then + # the user will be redirected to the "control" URL (change_enrollment) + # instead of the experimental URL (change_enrollment_autoreg) + # We work around this by setting a flag in the session, such that + # if a user is *ever* in the experimental condition, they remain + # in the experimental condition. + for mode_slug in ['honor', 'audit', 'verified']: + 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) + ) + + # Log out, so we're no longer authenticated + self.client.logout() + + # Visit the experimental condition URL + resp = self._change_enrollment('enroll', auto_reg=True) + + # Expect that we're denied (since we're not authenticated) + # and instead are redirected to the login page + self.assertEqual(resp.status_code, 403) + + # Log the user in + self.client.login(username=self.USERNAME, password=self.PASSWORD) + + # Go to the control URL + # This simulates what the JavaScript client does after getting a 403 response + # from the register button. + resp = self._change_enrollment('enroll') + + # Expect that we're auto-enrolled (even though we used the control URL the second time) + self.assertTrue(CourseEnrollment.is_enrolled(self.user, self.course.id)) + + # Expect that the auto register flag is still set in the user's session + self.assertIn('auto_register', self.client.session) + self.assertTrue(self.client.session['auto_register']) + def test_unenroll(self): # Enroll the student in the course CourseEnrollment.enroll(self.user, self.course.id, mode="honor") diff --git a/common/djangoapps/student/tests/tests.py b/common/djangoapps/student/tests/tests.py index 50e30fcc87..8b2639565a 100644 --- a/common/djangoapps/student/tests/tests.py +++ b/common/djangoapps/student/tests/tests.py @@ -15,6 +15,7 @@ from django.test.utils import override_settings from django.test.client import RequestFactory, Client from django.contrib.auth.models import User, AnonymousUser from django.core.urlresolvers import reverse +from django.contrib.sessions.middleware import SessionMiddleware from xmodule.modulestore.tests.factories import CourseFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase @@ -634,8 +635,17 @@ class PaidRegistrationTest(ModuleStoreTestCase): @unittest.skipUnless(settings.FEATURES.get('ENABLE_SHOPPING_CART'), "Shopping Cart not enabled in settings") def test_change_enrollment_add_to_cart(self): - request = self.req_factory.post(reverse('change_enrollment'), {'course_id': self.course.id.to_deprecated_string(), - 'enrollment_action': 'add_to_cart'}) + request = self.req_factory.post( + reverse('change_enrollment'), { + 'course_id': self.course.id.to_deprecated_string(), + 'enrollment_action': 'add_to_cart' + } + ) + + # Add a session to the request + SessionMiddleware().process_request(request) + request.session.save() + request.user = self.user response = change_enrollment(request) self.assertEqual(response.status_code, 200) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 083fca156d..2c8da158f1 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -619,6 +619,17 @@ def change_enrollment(request, auto_register=False): Response """ + # 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 + user = request.user action = request.POST.get("enrollment_action") @@ -664,11 +675,6 @@ def change_enrollment(request, auto_register=False): # 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. @@ -703,12 +709,6 @@ def change_enrollment(request, auto_register=False): # 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) From 9d4bf89058e943152ddde18dc23449bd84ba0b5a Mon Sep 17 00:00:00 2001 From: Will Daly Date: Thu, 28 Aug 2014 15:18:27 -0400 Subject: [PATCH 2/2] Exclude courses from the auto-registration A/B test --- .../student/tests/test_enrollment.py | 29 +++++++++++++++++-- common/djangoapps/student/views.py | 22 +++++++++----- lms/envs/aws.py | 3 ++ lms/envs/common.py | 18 ++++++++++++ 4 files changed, 62 insertions(+), 10 deletions(-) diff --git a/common/djangoapps/student/tests/test_enrollment.py b/common/djangoapps/student/tests/test_enrollment.py index e453843083..657b4b5b6a 100644 --- a/common/djangoapps/student/tests/test_enrollment.py +++ b/common/djangoapps/student/tests/test_enrollment.py @@ -78,7 +78,6 @@ class EnrollmentTest(ModuleStoreTestCase): 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 @@ -126,8 +125,7 @@ class EnrollmentTest(ModuleStoreTestCase): 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) + mode_display_name=mode_slug ) # Log out, so we're no longer authenticated @@ -155,6 +153,31 @@ class EnrollmentTest(ModuleStoreTestCase): self.assertIn('auto_register', self.client.session) self.assertTrue(self.client.session['auto_register']) + # TODO (ECOM-16): Remove once the auto-registration A/B test completes + def test_enroll_auto_registration_excluded_course(self): + # Create the course modes + for mode_slug in ['honor', 'audit', 'verified']: + CourseModeFactory.create( + course_id=self.course.id, + mode_slug=mode_slug, + mode_display_name=mode_slug, + ) + + # Visit the experimental condition URL (when the course is NOT excluded) + # This should place us into the experimental condition flow + self._change_enrollment('enroll', auto_reg=True) + + # Unenroll from the course (we were registered because auto enroll was enabled) + self._change_enrollment('unenroll') + + # Register for the course again, with the course excluded + # At this point, we should NOT be in the experimental condition flow + excluded_course_ids = [self.course.id.to_deprecated_string()] + with self.settings(AUTO_REGISTRATION_AB_TEST_EXCLUDE_COURSES=excluded_course_ids): + self._change_enrollment('enroll') + self.assertFalse(CourseEnrollment.is_enrolled(self.user, self.course.id)) + self.assertNotIn('auto_register', self.client.session) + def test_unenroll(self): # Enroll the student in the course CourseEnrollment.enroll(self.user, self.course.id, mode="honor") diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 2c8da158f1..087866a6cf 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -619,6 +619,14 @@ def change_enrollment(request, auto_register=False): Response """ + user = request.user + + action = request.POST.get("enrollment_action") + if 'course_id' not in request.POST: + return HttpResponseBadRequest(_("Course id not specified")) + + course_id = SlashSeparatedCourseKey.from_deprecated_string(request.POST.get("course_id")) + # 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. @@ -630,13 +638,13 @@ def change_enrollment(request, auto_register=False): if request.session.get('auto_register') and not auto_register: auto_register = True - user = request.user - - action = request.POST.get("enrollment_action") - if 'course_id' not in request.POST: - return HttpResponseBadRequest(_("Course id not specified")) - - course_id = SlashSeparatedCourseKey.from_deprecated_string(request.POST.get("course_id")) + # 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 + # registering for one of these courses, immediately switch to the control. + if unicode(course_id) in getattr(settings, 'AUTO_REGISTRATION_AB_TEST_EXCLUDE_COURSES', []): + auto_register = False + if 'auto_register' in request.session: + del request.session['auto_register'] if not user.is_authenticated(): return HttpResponseForbidden() diff --git a/lms/envs/aws.py b/lms/envs/aws.py index d26fad116c..9deb623aa6 100644 --- a/lms/envs/aws.py +++ b/lms/envs/aws.py @@ -445,3 +445,6 @@ OPTIMIZELY_PROJECT_ID = AUTH_TOKENS.get('OPTIMIZELY_PROJECT_ID', OPTIMIZELY_PROJ #### Course Registration Code length #### REGISTRATION_CODE_LENGTH = ENV_TOKENS.get('REGISTRATION_CODE_LENGTH', 8) + +# TODO (ECOM-16): Remove once the A/B test of auto-registration completes +AUTO_REGISTRATION_AB_TEST_EXCLUDE_COURSES = set(ENV_TOKENS.get('AUTO_REGISTRATION_AB_TEST_EXCLUDE_COURSES', AUTO_REGISTRATION_AB_TEST_EXCLUDE_COURSES)) diff --git a/lms/envs/common.py b/lms/envs/common.py index 0ca6bd2ffb..cc98c27b81 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -1729,3 +1729,21 @@ OPENID_DOMAIN_PREFIX = 'openid:' ANALYTICS_DATA_URL = "" ANALYTICS_DATA_TOKEN = "" ANALYTICS_DASHBOARD_URL = "" + +# TODO (ECOM-16): Remove once the A/B test of auto-registration completes +AUTO_REGISTRATION_AB_TEST_EXCLUDE_COURSES = set([ + "HarvardX/SW12.2x/1T2014", + "HarvardX/SW12.3x/1T2014", + "HarvardX/SW12.4x/1T2014", + "HarvardX/SW12.5x/2T2014", + "HarvardX/SW12.6x/2T2014", + "HarvardX/HUM2.1x/3T2014", + "HarvardX/SW12x/2013_SOND", + "LinuxFoundationX/LFS101x/2T2014", + "HarvardX/CS50x/2014_T1", + "HarvardX/AmPoX.1/2014_T3", + "HarvardX/SW12.7x/3T2014", + "HarvardX/SW12.10x/1T2015", + "HarvardX/SW12.9x/3T2014", + "HarvardX/SW12.8x/3T2014", +])