From 445d0dabb6a40d2f0aa4afbe6bfceddfd771e201 Mon Sep 17 00:00:00 2001 From: jsa Date: Thu, 4 Jun 2015 17:23:47 -0400 Subject: [PATCH 1/2] Correct deactivation logic in enrollment api and test. XCOM-396 --- .../djangoapps/enrollment/tests/test_views.py | 57 ++++++++++++++++++- common/djangoapps/enrollment/views.py | 18 +++++- 2 files changed, 72 insertions(+), 3 deletions(-) diff --git a/common/djangoapps/enrollment/tests/test_views.py b/common/djangoapps/enrollment/tests/test_views.py index c7fcd125aa..d119a8e21d 100644 --- a/common/djangoapps/enrollment/tests/test_views.py +++ b/common/djangoapps/enrollment/tests/test_views.py @@ -2,6 +2,7 @@ Tests for user enrollment. """ import json +import itertools import unittest import datetime @@ -91,11 +92,11 @@ class EnrollmentTestMixin(object): return response - def assert_enrollment_activation(self, expected_activation, expected_mode=CourseMode.VERIFIED): + def assert_enrollment_activation(self, expected_activation, expected_mode): """Change an enrollment's activation and verify its activation and mode are as expected.""" self.assert_enrollment_status( as_server=True, - mode=None, + mode=expected_mode, is_active=expected_activation, expected_status=status.HTTP_200_OK ) @@ -603,6 +604,58 @@ class EnrollmentTest(EnrollmentTestMixin, ModuleStoreTestCase, APITestCase): self.assertTrue(is_active) self.assertEqual(course_mode, CourseMode.HONOR) + @ddt.data(*itertools.product( + (CourseMode.HONOR, CourseMode.VERIFIED), + (CourseMode.HONOR, CourseMode.VERIFIED), + (True, False), + (True, False), + )) + @ddt.unpack + def test_change_mode_from_server(self, old_mode, new_mode, old_is_active, new_is_active): + """ + Server-to-server calls should be allowed to change the mode of any + enrollment, as long as the enrollment is not being deactivated during + the same call (this is assumed to be an error on the client's side). + """ + for mode in [CourseMode.HONOR, CourseMode.VERIFIED]: + CourseModeFactory.create( + course_id=self.course.id, + mode_slug=mode, + mode_display_name=mode, + ) + + # Set up the initial enrollment + self.assert_enrollment_status(as_server=True, mode=old_mode, is_active=old_is_active) + course_mode, is_active = CourseEnrollment.enrollment_mode_for_user(self.user, self.course.id) + self.assertEqual(is_active, old_is_active) + self.assertEqual(course_mode, old_mode) + + expected_status = status.HTTP_400_BAD_REQUEST if ( + old_mode != new_mode and + old_is_active != new_is_active and + not new_is_active + ) else status.HTTP_200_OK + + # simulate the server-server api call under test + response = self.assert_enrollment_status( + as_server=True, + mode=new_mode, + is_active=new_is_active, + expected_status=expected_status, + ) + + course_mode, is_active = CourseEnrollment.enrollment_mode_for_user(self.user, self.course.id) + if expected_status == status.HTTP_400_BAD_REQUEST: + # nothing should have changed + self.assertEqual(is_active, old_is_active) + self.assertEqual(course_mode, old_mode) + # error message should contain specific text. Otto checks for this text in the message. + self.assertRegexpMatches(json.loads(response.content)['message'], 'Enrollment mode mismatch') + else: + # call should have succeeded + self.assertEqual(is_active, new_is_active) + self.assertEqual(course_mode, new_mode) + def test_change_mode_invalid_user(self): """ Attempts to change an enrollment for a non-existent user should result in an HTTP 404 for non-server users, diff --git a/common/djangoapps/enrollment/views.py b/common/djangoapps/enrollment/views.py index a5c610c5d5..fca223264f 100644 --- a/common/djangoapps/enrollment/views.py +++ b/common/djangoapps/enrollment/views.py @@ -3,6 +3,8 @@ The Enrollment API Views should be simple, lean HTTP endpoints for API access. T consist primarily of authentication, request validation, and serialization. """ +import logging + from ipware.ip import get_ip from django.core.exceptions import ObjectDoesNotExist from django.utils.decorators import method_decorator @@ -31,6 +33,9 @@ from enrollment.errors import ( from student.models import User +log = logging.getLogger(__name__) + + class EnrollmentCrossDomainSessionAuth(SessionAuthenticationAllowInactiveUser, SessionAuthenticationCrossDomainCsrf): """Session authentication that allows inactive users and cross-domain requests. """ pass @@ -421,7 +426,18 @@ class EnrollmentListView(APIView, ApiKeyPermissionMixIn): ) enrollment = api.get_enrollment(username, unicode(course_id)) - if has_api_key_permissions and enrollment and enrollment['mode'] != mode: + mode_changed = enrollment and mode is not None and enrollment['mode'] != mode + active_changed = enrollment and is_active is not None and enrollment['is_active'] != is_active + if has_api_key_permissions and (mode_changed or active_changed): + if mode_changed and active_changed and not is_active: + # if the requester wanted to deactivate but specified the wrong mode, fail + # the request (on the assumption that the requester had outdated information + # about the currently active enrollment). + msg = u"Enrollment mode mismatch: active mode={}, requested mode={}. Won't deactivate.".format( + enrollment["mode"], mode + ) + log.warning(msg) + return Response(status=status.HTTP_400_BAD_REQUEST, data={"message": msg}) response = api.update_enrollment(username, unicode(course_id), mode=mode, is_active=is_active) else: # Will reactivate inactive enrollments. From f4b2c197db8a2b01a5803bfaff0c434c3f0fc2b9 Mon Sep 17 00:00:00 2001 From: jsa Date: Fri, 5 Jun 2015 15:45:17 -0400 Subject: [PATCH 2/2] =?UTF-8?q?don=E2=80=99t=20trigger=20a=20refund=20requ?= =?UTF-8?q?est=20in=20otto,=20if=20otto=20requested=20unenrollment.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit XCOM-396 --- lms/djangoapps/commerce/signals.py | 8 ++++++++ lms/djangoapps/commerce/tests/test_signals.py | 7 +++++++ 2 files changed, 15 insertions(+) diff --git a/lms/djangoapps/commerce/signals.py b/lms/djangoapps/commerce/signals.py index 0d54ab4e7d..b135325dca 100644 --- a/lms/djangoapps/commerce/signals.py +++ b/lms/djangoapps/commerce/signals.py @@ -5,6 +5,7 @@ import logging from urlparse import urljoin from django.conf import settings +from django.contrib.auth.models import AnonymousUser from django.core.mail import EmailMultiAlternatives from django.dispatch import receiver from django.utils.translation import ugettext as _ @@ -32,6 +33,13 @@ def handle_unenroll_done(sender, course_enrollment=None, skip_refund=False, **kw if course_enrollment and course_enrollment.refundable(): try: request_user = get_request_user() or course_enrollment.user + if isinstance(request_user, AnonymousUser): + # Assume the request was initiated via server-to-server + # api call (presumably Otto). In this case we cannot + # construct a client to call Otto back anyway, because + # the client does not work anonymously, and furthermore, + # there's certainly no need to inform Otto about this request. + return refund_seat(course_enrollment, request_user) except: # pylint: disable=bare-except # don't assume the signal was fired with `send_robust`. diff --git a/lms/djangoapps/commerce/tests/test_signals.py b/lms/djangoapps/commerce/tests/test_signals.py index 85eb7cb591..014d572a57 100644 --- a/lms/djangoapps/commerce/tests/test_signals.py +++ b/lms/djangoapps/commerce/tests/test_signals.py @@ -1,6 +1,7 @@ """ Tests for signal handling in commerce djangoapp. """ +from django.contrib.auth.models import AnonymousUser from django.test import TestCase from django.test.utils import override_settings @@ -109,6 +110,12 @@ class TestRefundSignal(TestCase): self.assertTrue(mock_refund_seat.called) self.assertEqual(mock_refund_seat.call_args[0], (self.course_enrollment, self.requester)) + # HTTP user is another server (AnonymousUser): do not try to initiate a refund at all. + mock_get_request_user.return_value = AnonymousUser() + mock_refund_seat.reset_mock() + self.send_signal() + self.assertFalse(mock_refund_seat.called) + @mock.patch('commerce.signals.log.warning') def test_not_authorized_warning(self, mock_log_warning): """