From 6b1a7dc259e9604430f0e3c4b5a5075e39bcfb85 Mon Sep 17 00:00:00 2001 From: Adam Palay Date: Thu, 19 Jun 2014 12:57:13 -0400 Subject: [PATCH 1/3] Prevents students from accidentally changing their enrollment on login (LMS-2773) skip tests if not run in lms --- common/djangoapps/student/tests/tests.py | 82 ++++++++++++++++++++++++ common/djangoapps/student/views.py | 6 ++ 2 files changed, 88 insertions(+) diff --git a/common/djangoapps/student/tests/tests.py b/common/djangoapps/student/tests/tests.py index c5845ac8bc..cce4d9e66f 100644 --- a/common/djangoapps/student/tests/tests.py +++ b/common/djangoapps/student/tests/tests.py @@ -430,6 +430,88 @@ class EnrollInCourseTest(TestCase): self.assert_enrollment_event_was_emitted(user, course_id) +@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): """ diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 9b9dea4a48..941069c316 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -613,6 +613,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) From 92f53236cf213f8bfb930283341f710f9b45486f Mon Sep 17 00:00:00 2001 From: Adam Palay Date: Mon, 30 Jun 2014 16:19:13 -0400 Subject: [PATCH 2/3] fix 400s from change_enrollment causing 404s (LMS-6625) --- common/djangoapps/student/tests/test_login.py | 24 ++++++++++++++++++- common/djangoapps/student/views.py | 2 -- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/common/djangoapps/student/tests/test_login.py b/common/djangoapps/student/tests/test_login.py index 7d3201ca8b..b3c7e7b655 100644 --- a/common/djangoapps/student/tests/test_login.py +++ b/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 from student.tests.factories import UserFactory, RegistrationFactory, UserProfileFactory from student.views import _parse_course_id_from_string, _get_course_enrollment_domain @@ -206,9 +207,30 @@ 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 400 + and in fact just redirects the user to the dashboard + without incident. + """ + # add these post params 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 _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 diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 941069c316..7f411ddf5e 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -559,8 +559,6 @@ def try_change_enrollment(request): enrollment_response.content ) ) - if enrollment_response.content != '': - return enrollment_response.content except Exception, e: log.exception("Exception automatically enrolling after login: {0}".format(str(e))) From 536b9dbe31733ffdb95dec2e213c39defd236936 Mon Sep 17 00:00:00 2001 From: Adam Palay Date: Mon, 30 Jun 2014 17:05:29 -0400 Subject: [PATCH 3/3] in some cases we do want that redirect url --- common/djangoapps/student/tests/test_login.py | 45 ++++++++++++++++--- common/djangoapps/student/views.py | 7 ++- 2 files changed, 46 insertions(+), 6 deletions(-) diff --git a/common/djangoapps/student/tests/test_login.py b/common/djangoapps/student/tests/test_login.py index b3c7e7b655..b836ad31d6 100644 --- a/common/djangoapps/student/tests/test_login.py +++ b/common/djangoapps/student/tests/test_login.py @@ -11,7 +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 +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 @@ -209,11 +209,10 @@ class LoginTest(TestCase): def test_change_enrollment_400(self): """ - Tests that a 400 in change_enrollment doesn't lead to a 400 - and in fact just redirects the user to the dashboard - without incident. + Tests that a 400 in change_enrollment doesn't lead to a 404 + and in fact just logs in the user without incident """ - # add these post params to trigger a call to change_enrollment + # 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") @@ -226,6 +225,42 @@ class LoginTest(TestCase): 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} diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 7f411ddf5e..ea79450b26 100644 --- a/common/djangoapps/student/views.py +++ b/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,6 +559,11 @@ def try_change_enrollment(request): 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)))