PR Updates 2

common/djangoapps/entitlements/api/v1/tests/test_views.py

@@ -555,7 +555,7 @@ class EntitlementEnrollmentViewSetTest(ModuleStoreTestCase):
             revoke_url,
             content_type='application/json',
         )
-        assert response.status_code == 409
+        assert response.status_code == 500
 
         course_entitlement.refresh_from_db()
         assert CourseEnrollment.is_enrolled(self.user, self.course.id)

common/djangoapps/entitlements/api/v1/views.py

 import logging
 
-from django.db import transaction
+from django.db import IntegrityError, transaction
 from django.utils import timezone
 from django_filters.rest_framework import DjangoFilterBackend
 from edx_rest_framework_extensions.authentication import JwtAuthentication
@@ -23,6 +23,58 @@ from student.models import CourseEnrollmentException, AlreadyEnrolledError
 log = logging.getLogger(__name__)
 
 
+def _unenroll_entitlement(course_entitlement, course_run_key):
+    """
+    Internal method to handle the details of Unenrolling a User in a Course Run.
+    """
+    CourseEnrollment.unenroll(course_entitlement.user, course_run_key, skip_refund=True)
+    course_entitlement.set_enrollment(None)
+
+
+@transaction.atomic
+def _process_revoke_and_unenroll_entitlement(course_entitlement, is_refund=False):
+    """
+    Process the revoke of the Course Entitlement and refund if needed
+
+    Arguments:
+        course_entitlement: Course Entitlement Object
+
+        is_refund (bool): True if a refund should be processed
+
+    Exceptions:
+        IntegrityError if there is an issue that should reverse the database changes
+    """
+    if course_entitlement.expired_at is None:
+        course_entitlement.expired_at = timezone.now()
+        log.info(
+            'Set expired_at to [%s] for course entitlement [%s]',
+            course_entitlement.expired_at,
+            course_entitlement.uuid
+        )
+        course_entitlement.save()
+
+    if course_entitlement.enrollment_course_run is not None:
+        course_id = course_entitlement.enrollment_course_run.course_id
+        _unenroll_entitlement(course_entitlement, course_id)
+        log.info(
+            'Unenrolled user [%s] from course run [%s] as part of revocation of course entitlement [%s]',
+            course_entitlement.user.username,
+            course_id,
+            course_entitlement.uuid
+        )
+
+    if is_refund:
+        refund_successful = refund_entitlement(course_entitlement=course_entitlement)
+        if not refund_successful:
+            # This state is achieved in most cases by a failure in the ecommerce service to process the refund.
+            log.warn(
+                'Entitlement Refund failed for Course Entitlement [%s], alert User',
+                str(course_entitlement.uuid)
+            )
+            # Force Transaction reset with an Integrity error exception, this will revert all previous transactions
+            raise IntegrityError
+
+
 class EntitlementViewSet(viewsets.ModelViewSet):
     ENTITLEMENT_UUID4_REGEX = '[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}'
 
@@ -93,35 +145,7 @@ class EntitlementViewSet(viewsets.ModelViewSet):
             'Entitlement Revoke requested for Course Entitlement[%s]',
             str(instance.uuid)
         )
-        process_revoke_and_unenroll_entitlement(instance)
-
-
-@transaction.atomic
-def process_revoke_and_unenroll_entitlement(course_entitlement):
-    save_model = False
-
-    if course_entitlement.expired_at is None:
-        course_entitlement.expired_at = timezone.now()
-        log.info('Set expired_at to [%s] for course entitlement [%s]', course_entitlement.expired_at, course_entitlement.uuid)
-        save_model = True
-
-    if course_entitlement.enrollment_course_run is not None:
-        CourseEnrollment.unenroll(
-            user=course_entitlement.user,
-            course_id=course_entitlement.enrollment_course_run.course_id,
-            skip_refund=True
-        )
-        enrollment = course_entitlement.enrollment_course_run
-        course_entitlement.enrollment_course_run = None
-        save_model = True
-        log.info(
-            'Unenrolled user [%s] from course run [%s] as part of revocation of course entitlement [%s]',
-            course_entitlement.user.username,
-            enrollment.course_id,
-            course_entitlement.uuid
-        )
-    if save_model:
-        course_entitlement.save()
+        _process_revoke_and_unenroll_entitlement(instance)
 
 
 class EntitlementEnrollmentViewSet(viewsets.GenericViewSet):
@@ -183,13 +207,6 @@ class EntitlementEnrollmentViewSet(viewsets.GenericViewSet):
         entitlement.set_enrollment(enrollment)
         return None
 
-    def _unenroll_entitlement(self, entitlement, course_run_key, user):
-        """
-        Internal method to handle the details of Unenrolling a User in a Course Run.
-        """
-        CourseEnrollment.unenroll(user, course_run_key, skip_refund=True)
-        entitlement.set_enrollment(None)
-
     def create(self, request, uuid):
         """
         On POST this method will be called and will handle enrolling a user in the
@@ -247,10 +264,9 @@ class EntitlementEnrollmentViewSet(viewsets.GenericViewSet):
             if response:
                 return response
         elif entitlement.enrollment_course_run.course_id != course_run_id:
-            self._unenroll_entitlement(
-                entitlement=entitlement,
+            _unenroll_entitlement(
+                course_entitlement=entitlement,
                 course_run_key=entitlement.enrollment_course_run.course_id,
-                user=request.user
             )
             response = self._enroll_entitlement(
                 entitlement=entitlement,
@@ -291,28 +307,23 @@ class EntitlementEnrollmentViewSet(viewsets.GenericViewSet):
                 'Entitlement Refund requested for Course Entitlement[%s]',
                 str(entitlement.uuid)
             )
-            refund_successful = refund_entitlement(course_entitlement=entitlement)
-
-            if refund_successful:
-                process_revoke_and_unenroll_entitlement(course_entitlement=entitlement)
-            else:
-                # This state is achieved in most cases by a failure in the ecommerce service to process the refund.
-                log.warn(
-                    'Entitlement Refund failed for Course Entitlement [%s], alert User',
-                    str(entitlement.uuid)
-                )
+
+            try:
+                _process_revoke_and_unenroll_entitlement(course_entitlement=entitlement, is_refund=True)
+            except IntegrityError:
+                # This state is reached when there was a failure in revoke and refund process resulting
+                # in a reversion of DB changes
                 return Response(
-                    status=status.HTTP_409_CONFLICT,
+                    status=status.HTTP_500_INTERNAL_SERVER_ERROR,
                     data={
-                        'message': 'Entitlement refund failed due to refund process failure or conflict'
+                        'message': 'Entitlement revoke and refund failed due to refund internal process failure'
                     })
 
         elif not is_refund:
             if entitlement.enrollment_course_run is not None:
-                self._unenroll_entitlement(
-                    entitlement=entitlement,
+                _unenroll_entitlement(
+                    course_entitlement=entitlement,
                     course_run_key=entitlement.enrollment_course_run.course_id,
-                    user=request.user
                 )
         else:
             log.info(

lms/djangoapps/commerce/utils.py

@@ -181,7 +181,7 @@ def refund_entitlement(course_entitlement):
         )
     else:
         log.info('No refund opened for user [%s], course entitlement [%s]', enrollee.id, entitlement_uuid)
-        return True
+        return False
 
 
 def refund_seat(course_enrollment):
@@ -227,7 +227,8 @@ def refund_seat(course_enrollment):
 
 def _process_refund(refund_ids, api_client, course_product):
     """
-    Helper method to process a refund for a given course_product
+    Helper method to process a refund for a given course_product. This method assumes that the User has already
+    been unenrolled.
 
     Returns:
         bool: True if the refund process was successful, False if there are any Errors that are not handled
@@ -239,9 +240,9 @@ def _process_refund(refund_ids, api_client, course_product):
 
         for refund_id in refund_ids:
             try:
-                # NOTE: Approve payment only because the user has already been unenrolled. Additionally, this
-                # ensures we don't tie up an additional web worker when the E-Commerce Service tries to unenroll
-                # the learner
+                # NOTE: The following assumes that the user has already been unenrolled.
+                # We are then able to approve payment. Additionally, this ensures we don't tie up an
+                # additional web worker when the E-Commerce Service tries to unenroll the learner.
                 api_client.refunds(refund_id).process.put({'action': 'approve_payment_only'})
                 log.info('Refund [%d] successfully approved.', refund_id)
             except:  # pylint: disable=bare-except
@@ -275,7 +276,7 @@ def _process_refund(refund_ids, api_client, course_product):
             return False
         else:
             try:
-                _send_refund_notification(course_product, refunds_requiring_approval)
+                return _send_refund_notification(course_product, refunds_requiring_approval)
             except:  # pylint: disable=bare-except
                 # Unable to send notification to Support, do not break as this method is used by Signals
                 log.warning('Could not send support notification for refund.', exc_info=True)
@@ -284,7 +285,13 @@ def _process_refund(refund_ids, api_client, course_product):
 
 
 def _send_refund_notification(course_product, refund_ids):
-    """ Notify the support team of the refund request. """
+    """
+    Notify the support team of the refund request.
+
+    Returns:
+        bool: True if we are able to send the notification.  In this case that means we were able to create
+              a ZenDesk ticket
+    """
 
     tags = ['auto_refund']
 
@@ -292,11 +299,13 @@ def _send_refund_notification(course_product, refund_ids):
         # this is not presently supported with the external service.
         raise NotImplementedError("Unable to send refund processing emails to support teams.")
 
+    # Build the information for the ZenDesk ticket
     student = course_product.user
     subject = _("[Refund] User-Requested Refund")
     body = _generate_refund_notification_body(student, refund_ids)
     requester_name = student.profile.name or student.username
-    create_zendesk_ticket(requester_name, student.email, subject, body, tags)
+
+    return create_zendesk_ticket(requester_name, student.email, subject, body, tags)
 
 
 def _generate_refund_notification_body(student, refund_ids):  # pylint: disable=invalid-name
@@ -317,10 +326,15 @@ def _generate_refund_notification_body(student, refund_ids):  # pylint: disable=
 
 
 def create_zendesk_ticket(requester_name, requester_email, subject, body, tags=None):
-    """ Create a Zendesk ticket via API. """
+    """
+    Create a Zendesk ticket via API.
+
+    Returns:
+        bool: False if we are unable to create the ticket for any reason
+    """
     if not (settings.ZENDESK_URL and settings.ZENDESK_USER and settings.ZENDESK_API_KEY):
-        log.debug('Zendesk is not configured. Cannot create a ticket.')
-        return
+        log.error('Zendesk is not configured. Cannot create a ticket.')
+        return False
 
     # Copy the tags to avoid modifying the original list.
     tags = list(tags or [])
@@ -356,8 +370,9 @@ def create_zendesk_ticket(requester_name, requester_email, subject, body, tags=N
         # Check for HTTP codes other than 201 (Created)
         if response.status_code != 201:
             log.error('Failed to create ticket. Status: [%d], Body: [%s]', response.status_code, response.content)
+            return False
         else:
             log.debug('Successfully created ticket.')
     except Exception:  # pylint: disable=broad-except
         log.exception('Failed to create ticket.')
-        return
+        return False