Merge pull request #4171 from edx/adam/fix-lms-2773

Prevents students from accidentally changing their enrollment on login (...

common/djangoapps/student/tests/test_login.py

@@ -11,6 +11,7 @@ from django.test.utils import override_settings
 from django.conf import settings
 from django.core.cache import cache
 from django.core.urlresolvers import reverse, NoReverseMatch
+from django.http import HttpResponseBadRequest, HttpResponse
 from student.tests.factories import UserFactory, RegistrationFactory, UserProfileFactory
 from student.views import _parse_course_id_from_string, _get_course_enrollment_domain
 
@@ -206,9 +207,65 @@ class LoginTest(TestCase):
         # client1 will be logged out
         self.assertEqual(response.status_code, 302)
 
-    def _login_response(self, email, password, patched_audit_log='student.views.AUDIT_LOG'):
+    def test_change_enrollment_400(self):
+        """
+        Tests that a 400 in change_enrollment doesn't lead to a 404
+        and in fact just logs in the user without incident
+        """
+        # add this post param to trigger a call to change_enrollment
+        extra_post_params = {"enrollment_action": "enroll"}
+        with patch('student.views.change_enrollment') as mock_change_enrollment:
+            mock_change_enrollment.return_value = HttpResponseBadRequest("I am a 400")
+            response, _ = self._login_response(
+                'test@edx.org',
+                'test_password',
+                extra_post_params=extra_post_params,
+            )
+        response_content = json.loads(response.content)
+        self.assertIsNone(response_content["redirect_url"])
+        self._assert_response(response, success=True)
+
+    def test_change_enrollment_200_no_redirect(self):
+        """
+        Tests "redirect_url" is None if change_enrollment returns a HttpResponse
+        with no content
+        """
+        # add this post param to trigger a call to change_enrollment
+        extra_post_params = {"enrollment_action": "enroll"}
+        with patch('student.views.change_enrollment') as mock_change_enrollment:
+            mock_change_enrollment.return_value = HttpResponse()
+            response, _ = self._login_response(
+                'test@edx.org',
+                'test_password',
+                extra_post_params=extra_post_params,
+            )
+        response_content = json.loads(response.content)
+        self.assertIsNone(response_content["redirect_url"])
+        self._assert_response(response, success=True)
+
+    def test_change_enrollment_200_redirect(self):
+        """
+        Tests that "redirect_url" is the content of the HttpResponse returned
+        by change_enrollment, if there is content
+        """
+        # add this post param to trigger a call to change_enrollment
+        extra_post_params = {"enrollment_action": "enroll"}
+        with patch('student.views.change_enrollment') as mock_change_enrollment:
+            mock_change_enrollment.return_value = HttpResponse("in/nature/there/is/nothing/melancholy")
+            response, _ = self._login_response(
+                'test@edx.org',
+                'test_password',
+                extra_post_params=extra_post_params,
+            )
+        response_content = json.loads(response.content)
+        self.assertEqual(response_content["redirect_url"], "in/nature/there/is/nothing/melancholy")
+        self._assert_response(response, success=True)
+
+    def _login_response(self, email, password, patched_audit_log='student.views.AUDIT_LOG', extra_post_params=None):
         ''' Post the login info '''
         post_params = {'email': email, 'password': password}
+        if extra_post_params is not None:
+            post_params.update(extra_post_params)
         with patch(patched_audit_log) as mock_audit_log:
             result = self.client.post(self.url, post_params)
         return result, mock_audit_log

common/djangoapps/student/tests/tests.py

@@ -431,6 +431,88 @@ class EnrollInCourseTest(TestCase):
 
 
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
+@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
+class ChangeEnrollmentViewTest(ModuleStoreTestCase):
+    """Tests the student.views.change_enrollment view"""
+
+    def setUp(self):
+        super(ChangeEnrollmentViewTest, self).setUp()
+        self.course = CourseFactory.create()
+        self.user = UserFactory.create(password='secret')
+        self.client.login(username=self.user.username, password='secret')
+        self.url = reverse('change_enrollment')
+
+    def enroll_through_view(self, course):
+        response = self.client.post(
+            reverse('change_enrollment'), {
+                'course_id': course.id.to_deprecated_string(),
+                'enrollment_action': 'enroll'
+            }
+        )
+        return response
+
+    def test_enroll_as_honor(self):
+        """Tests that a student can successfully enroll through this view"""
+        response = self.enroll_through_view(self.course)
+        self.assertEqual(response.status_code, 200)
+        enrollment_mode, is_active = CourseEnrollment.enrollment_mode_for_user(
+            self.user, self.course.id
+        )
+        self.assertTrue(is_active)
+        self.assertEqual(enrollment_mode, u'honor')
+
+    def test_cannot_enroll_if_already_enrolled(self):
+        """
+        Tests that a student will not be able to enroll through this view if
+        they are already enrolled in the course
+        """
+        CourseEnrollment.enroll(self.user, self.course.id)
+        self.assertTrue(CourseEnrollment.is_enrolled(self.user, self.course.id))
+        # now try to enroll that student
+        response = self.enroll_through_view(self.course)
+        self.assertEqual(response.status_code, 400)
+
+    def test_change_to_honor_if_verified(self):
+        """
+        Tests that a student that is a currently enrolled verified student cannot
+        accidentally change their enrollment to verified
+        """
+        CourseEnrollment.enroll(self.user, self.course.id, mode=u'verified')
+        self.assertTrue(CourseEnrollment.is_enrolled(self.user, self.course.id))
+        # now try to enroll the student in the honor mode:
+        response = self.enroll_through_view(self.course)
+        self.assertEqual(response.status_code, 400)
+        enrollment_mode, is_active = CourseEnrollment.enrollment_mode_for_user(
+            self.user, self.course.id
+        )
+        self.assertTrue(is_active)
+        self.assertEqual(enrollment_mode, u'verified')
+
+    def test_change_to_honor_if_verified_not_active(self):
+        """
+        Tests that one can renroll for a course if one has already unenrolled
+        """
+        # enroll student
+        CourseEnrollment.enroll(self.user, self.course.id, mode=u'verified')
+        # now unenroll student:
+        CourseEnrollment.unenroll(self.user, self.course.id)
+        # check that they are verified but inactive
+        enrollment_mode, is_active = CourseEnrollment.enrollment_mode_for_user(
+            self.user, self.course.id
+        )
+        self.assertFalse(is_active)
+        self.assertEqual(enrollment_mode, u'verified')
+        # now enroll them through the view:
+        response = self.enroll_through_view(self.course)
+        self.assertEqual(response.status_code, 200)
+        enrollment_mode, is_active = CourseEnrollment.enrollment_mode_for_user(
+            self.user, self.course.id
+        )
+        self.assertTrue(is_active)
+        self.assertEqual(enrollment_mode, u'honor')
+
+
+@override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
 class PaidRegistrationTest(ModuleStoreTestCase):
     """
     Tests for paid registration functionality (not verified student), involves shoppingcart

common/djangoapps/student/views.py

@@ -543,7 +543,7 @@ def dashboard(request):
 def try_change_enrollment(request):
     """
     This method calls change_enrollment if the necessary POST
-    parameters are present, but does not return anything. It
+    parameters are present, but does not return anything in most cases. It
     simply logs the result or exception. This is usually
     called after a registration or login, as secondary action.
     It should not interrupt a successful registration or login.
@@ -559,7 +559,10 @@ def try_change_enrollment(request):
                     enrollment_response.content
                 )
             )
-            if enrollment_response.content != '':
+            # Hack: since change_enrollment delivers its redirect_url in the content
+            # of its response, we check here that only the 200 codes with content
+            # will return redirect_urls.
+            if enrollment_response.status_code == 200 and enrollment_response.content != '':
                 return enrollment_response.content
         except Exception, e:
             log.exception("Exception automatically enrolling after login: {0}".format(str(e)))
@@ -613,6 +616,12 @@ def change_enrollment(request):
         if is_course_full:
             return HttpResponseBadRequest(_("Course is full"))
 
+        # check to see if user is currently enrolled in that course
+        if CourseEnrollment.is_enrolled(user, course_id):
+            return HttpResponseBadRequest(
+                _("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)