Merge pull request #8414 from edx/jsa/fix-deactivate-enrollment

Jsa/fix deactivate enrollment

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,

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.

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`.

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):
         """