feat: unfulfillment logic consolidation

Encapsulates logic around canceling platform fulfillment/enrollment, external fulfillment (e.g. GEAG),
and transaction reversal into a new transaction.api module. Then refactors and/or modifies
three main integration points to make use of this new module as follows:
1. The reversal-writing management command now calls the common business logic functions
to cancel external fulfillments before reversing a transaction. This is just a refactor, no new behavior here.
2. Modified the listen_for_transaction_reversal signal handler to cancel both external (GEAG)
and platform fulfillments. New behavior here is to cancel the external fulfillment.
For background, this signal handler is what enables the "Unenroll & Refund" Transaction Django Admin action to perform the "unenroll" portion.
3. Modified the "Unenroll only" Transaction Django Admin view to cancel both
external and platform fulfillments. Prior to this change, only the platform fulfillment was canceled.
ENT-8633

.github/workflows/ci.yml

@@ -16,7 +16,7 @@ jobs:
       matrix:
         os: [ubuntu-20.04]
         python-version: ['3.8']
-        toxenv: ["django42", "py38", "quality", "pii_check"]
+        toxenv: ["py38", "quality", "pii_check"]
 
     steps:
       - uses: actions/checkout@v3
@@ -41,4 +41,4 @@ jobs:
         uses: codecov/codecov-action@v3
         with:
           flags: unittests
-          fail_ci_if_error: true
+          fail_ci_if_error: false

enterprise_subsidy/apps/fulfillment/api.py

@@ -204,3 +204,12 @@ def fulfill(self, transaction):
             return self._save_fulfillment_reference(transaction, external_reference_id)
         except HTTPError as exc:
             raise FulfillmentException(response_payload.get('errors') or geag_response.text) from exc
+
+    def cancel_fulfillment(self, external_transaction_reference):
+        """
+        Cancels the provided external reference's (related to some ``Transaction`` record)
+        related enterprise allocation.
+        """
+        self.get_smarter_client().cancel_enterprise_allocation(
+            external_transaction_reference.external_reference_id,
+        )

enterprise_subsidy/apps/transaction/api.py

@@ -0,0 +1,94 @@
+"""
+Core business logic around transactions.
+"""
+import logging
+
+import requests
+from django.utils import timezone
+from openedx_ledger.api import reverse_full_transaction
+from openedx_ledger.models import TransactionStateChoices
+
+from enterprise_subsidy.apps.api_client.enterprise import EnterpriseApiClient
+from enterprise_subsidy.apps.fulfillment.api import GEAGFulfillmentHandler
+from enterprise_subsidy.apps.transaction.utils import generate_transaction_reversal_idempotency_key
+
+from .exceptions import TransactionFulfillmentCancelationException
+
+logger = logging.getLogger(__name__)
+
+
+def cancel_transaction_fulfillment(transaction):
+    """
+    Cancels the edx-platform fulfillment (typically the verified enrollment gets moved
+    to the ``audit`` mode).
+    """
+    if transaction.state != TransactionStateChoices.COMMITTED:
+        logger.info(
+            "[fulfillment cancelation] %s is not committed, will not cancel fulfillment",
+            transaction.uuid,
+        )
+        raise TransactionFulfillmentCancelationException(
+            "Transaction is not committed"
+        )
+    if not transaction.fulfillment_identifier:
+        logger.info(
+            "[fulfillment cancelation] %s has no fulfillment uuid, will not cancel fulfillment",
+            transaction.uuid,
+        )
+        raise TransactionFulfillmentCancelationException(
+            "Transaction has no associated platform fulfillment identifier"
+        )
+
+    try:
+        EnterpriseApiClient().cancel_fulfillment(transaction.fulfillment_identifier)
+    except requests.exceptions.HTTPError as exc:
+        error_msg = (
+            "Error canceling platform fulfillment "
+            f"{transaction.fulfillment_identifier}: {exc}"
+        )
+        logger.exception("[fulfillment cancelation] %s", error_msg)
+        raise TransactionFulfillmentCancelationException(error_msg) from exc
+
+
+def cancel_transaction_external_fulfillment(transaction):
+    """
+    Cancels all related external GEAG allocations for the given transaction.
+
+    raises:
+      FulfillmentException if the related external references for the transaction
+        are not for a GEAG fulfillment provider.
+    """
+    if transaction.state != TransactionStateChoices.COMMITTED:
+        logger.info(
+            "[fulfillment cancelation] %s is not committed, will not cancel fulfillment",
+            transaction.uuid,
+        )
+        raise TransactionFulfillmentCancelationException(
+            "Transaction is not committed"
+        )
+
+    for external_reference in transaction.external_reference.all():
+        provider_slug = external_reference.external_fulfillment_provider.slug
+        geag_handler = GEAGFulfillmentHandler()
+        if provider_slug == geag_handler.EXTERNAL_FULFILLMENT_PROVIDER_SLUG:
+            geag_handler.cancel_fulfillment(external_reference)
+        else:
+            logger.warning(
+                '[fulfillment cancelation] dont know how to cancel transaction %s with provider %s',
+                transaction.uuid,
+                provider_slug,
+            )
+
+
+def reverse_transaction(transaction, unenroll_time=None):
+    """
+    Creates a reversal for the provided transaction.
+    """
+    idempotency_key = generate_transaction_reversal_idempotency_key(
+        transaction.fulfillment_identifier,
+        unenroll_time or timezone.now(),
+    )
+    return reverse_full_transaction(
+        transaction=transaction,
+        idempotency_key=idempotency_key,
+    )

enterprise_subsidy/apps/transaction/apps.py

@@ -2,13 +2,22 @@
 Initialization app for enterprise_subsidy.apps.transaction
 """
 from django.apps import AppConfig
-from openedx_ledger.signals.signals import TRANSACTION_REVERSED
-
-from enterprise_subsidy.apps.transaction.signals.handlers import listen_for_transaction_reversal
 
 
 class TransactionsConfig(AppConfig):
+    """
+    App configuration for the ``transaction`` module.
+    """
     name = 'enterprise_subsidy.apps.transaction'
 
     def ready(self):
+        """
+        Wait to import any non-trivial dependencies until this app is ready.
+        The local imports below help avoid a Django "AppConfig ready deadlock".
+        """
+        # pylint: disable=import-outside-toplevel
+        from openedx_ledger.signals.signals import TRANSACTION_REVERSED
+
+        from enterprise_subsidy.apps.transaction.signals.handlers import listen_for_transaction_reversal
+
         TRANSACTION_REVERSED.connect(listen_for_transaction_reversal)

enterprise_subsidy/apps/transaction/exceptions.py

@@ -0,0 +1,15 @@
+"""
+Common exceptions related to Transactions.
+"""
+
+
+class TransactionException(Exception):
+    """
+    Base exception class around transactions.
+    """
+
+
+class TransactionFulfillmentCancelationException(TransactionException):
+    """
+    Raised when a Transaction cannot be unfulfilled (un-enrolled).
+    """

enterprise_subsidy/apps/transaction/management/commands/write_reversals_from_enterprise_unenrollments.py

@@ -9,14 +9,11 @@
 from django.contrib import auth
 from django.core.management.base import BaseCommand
 from getsmarter_api_clients.geag import GetSmarterEnterpriseApiClient
-from openedx_ledger.api import reverse_full_transaction
 from openedx_ledger.models import Transaction, TransactionStateChoices
 
 from enterprise_subsidy.apps.api_client.enterprise import EnterpriseApiClient
 from enterprise_subsidy.apps.content_metadata.api import ContentMetadataApi
-from enterprise_subsidy.apps.fulfillment.api import GEAGFulfillmentHandler
-from enterprise_subsidy.apps.fulfillment.exceptions import FulfillmentException
-from enterprise_subsidy.apps.transaction.utils import generate_transaction_reversal_idempotency_key
+from enterprise_subsidy.apps.transaction.api import cancel_transaction_external_fulfillment, reverse_transaction
 
 logger = logging.getLogger(__name__)
 User = auth.get_user_model()
@@ -202,25 +199,8 @@ def handle_reversing_enterprise_course_unenrollment(self, unenrollment):
         )
 
         if not self.dry_run:
-            idempotency_key = generate_transaction_reversal_idempotency_key(
-                fulfillment_uuid,
-                enrollment_unenrolled_at
-            )
-
-            for external_reference in related_transaction.external_reference.all():
-                provider_slug = external_reference.external_fulfillment_provider.slug
-                if provider_slug == GEAGFulfillmentHandler.EXTERNAL_FULFILLMENT_PROVIDER_SLUG:
-                    # this will raise if there is a problem before we reverse the ledger transaction
-                    # allowing us to try again later
-                    self.geag_client.cancel_enterprise_allocation(external_reference.external_reference_id)
-                else:
-                    raise FulfillmentException(f'dont know how to cancel {provider_slug}')
-
-            # Do we need to write any additional metadata to the Reversal?
-            reverse_full_transaction(
-                transaction=related_transaction,
-                idempotency_key=idempotency_key,
-            )
+            cancel_transaction_external_fulfillment(related_transaction)
+            reverse_transaction(related_transaction, unenroll_time=enrollment_unenrolled_at)
             return 1
         else:
             logger.info(

enterprise_subsidy/apps/transaction/signals/handlers.py

@@ -3,11 +3,11 @@
 """
 import logging
 
-import requests
 from django.dispatch import receiver
 from openedx_ledger.signals.signals import TRANSACTION_REVERSED
 
-from enterprise_subsidy.apps.api_client.enterprise import EnterpriseApiClient
+from ..api import cancel_transaction_external_fulfillment, cancel_transaction_fulfillment
+from ..exceptions import TransactionFulfillmentCancelationException
 
 logger = logging.getLogger(__name__)
 
@@ -27,8 +27,9 @@ def listen_for_transaction_reversal(sender, **kwargs):
         logger.info(msg)
         raise ValueError(msg)
     try:
-        EnterpriseApiClient().cancel_fulfillment(transaction.fulfillment_identifier)
-    except requests.exceptions.HTTPError as exc:
+        cancel_transaction_external_fulfillment(transaction)
+        cancel_transaction_fulfillment(transaction)
+    except TransactionFulfillmentCancelationException as exc:
         error_msg = f"Error canceling platform fulfillment {transaction.fulfillment_identifier}: {exc}"
         logger.exception(error_msg)
         raise exc

enterprise_subsidy/apps/transaction/tests/test_management.py

@@ -19,7 +19,6 @@
 from pytest import mark
 
 from enterprise_subsidy.apps.fulfillment.api import GEAGFulfillmentHandler
-from enterprise_subsidy.apps.fulfillment.exceptions import FulfillmentException
 from enterprise_subsidy.apps.subsidy.tests.factories import SubsidyFactory
 from test_utils.utils import MockResponse
 
@@ -149,7 +148,7 @@ def test_write_reversals_from_enterprise_unenrollment_with_existing_reversal(sel
         'ContentMetadataApi'
     )
     @mock.patch(
-        'enterprise_subsidy.apps.transaction.signals.handlers.EnterpriseApiClient'
+        'enterprise_subsidy.apps.transaction.api.EnterpriseApiClient'
     )
     def test_write_reversals_from_enterprise_unenrollments_with_microsecond_datetime_strings(
         self,
@@ -249,7 +248,7 @@ def test_write_reversals_from_enterprise_unenrollments_with_microsecond_datetime
         'ContentMetadataApi'
     )
     @mock.patch(
-        'enterprise_subsidy.apps.transaction.signals.handlers.EnterpriseApiClient'
+        'enterprise_subsidy.apps.transaction.api.EnterpriseApiClient'
     )
     def test_write_reversals_from_enterprise_unenrollment_does_not_rerequest_metadata(
         self,
@@ -412,7 +411,7 @@ def test_write_reversals_from_enterprise_unenrollment_with_uncommitted_transacti
         'ContentMetadataApi'
     )
     @mock.patch(
-        'enterprise_subsidy.apps.transaction.signals.handlers.EnterpriseApiClient'
+        'enterprise_subsidy.apps.transaction.api.EnterpriseApiClient'
     )
     @ddt.data(
         ('2023-05-25T19:27:29Z', '2023-06-1T19:27:29Z'),
@@ -520,7 +519,7 @@ def test_write_reversals_from_enterprise_unenrollment_refund_period_ended(
         'ContentMetadataApi'
     )
     @mock.patch(
-        'enterprise_subsidy.apps.transaction.signals.handlers.EnterpriseApiClient'
+        'enterprise_subsidy.apps.transaction.api.EnterpriseApiClient'
     )
     @ddt.data(True, False)
     def test_write_reversals_from_enterprise_unenrollments(
@@ -628,7 +627,7 @@ def test_write_reversals_from_enterprise_unenrollments(
         'ContentMetadataApi'
     )
     @mock.patch(
-        'enterprise_subsidy.apps.transaction.signals.handlers.EnterpriseApiClient'
+        'enterprise_subsidy.apps.transaction.api.EnterpriseApiClient'
     )
     @override_settings(ENTERPRISE_SUBSIDY_AUTOMATIC_EXTERNAL_CANCELLATION=False)
     def test_write_reversals_from_geag_enterprise_unenrollments_disabled_setting(
@@ -720,8 +719,7 @@ def test_write_reversals_from_geag_enterprise_unenrollments_disabled_setting(
         assert Reversal.objects.count() == 0
 
     @mock.patch(
-        'enterprise_subsidy.apps.transaction.management.commands.write_reversals_from_enterprise_unenrollments.'
-        'GetSmarterEnterpriseApiClient'
+        'enterprise_subsidy.apps.fulfillment.api.GetSmarterEnterpriseApiClient'
     )
     @mock.patch(
         'enterprise_subsidy.apps.transaction.management.commands.write_reversals_from_enterprise_unenrollments.'
@@ -732,7 +730,7 @@ def test_write_reversals_from_geag_enterprise_unenrollments_disabled_setting(
         'ContentMetadataApi'
     )
     @mock.patch(
-        'enterprise_subsidy.apps.transaction.signals.handlers.EnterpriseApiClient'
+        'enterprise_subsidy.apps.transaction.api.EnterpriseApiClient'
     )
     @override_settings(ENTERPRISE_SUBSIDY_AUTOMATIC_EXTERNAL_CANCELLATION=True)
     def test_write_reversals_from_geag_enterprise_unenrollments_enabled_setting(
@@ -836,7 +834,7 @@ def test_write_reversals_from_geag_enterprise_unenrollments_enabled_setting(
         'ContentMetadataApi'
     )
     @mock.patch(
-        'enterprise_subsidy.apps.transaction.signals.handlers.EnterpriseApiClient'
+        'enterprise_subsidy.apps.transaction.api.EnterpriseApiClient'
     )
     @override_settings(ENTERPRISE_SUBSIDY_AUTOMATIC_EXTERNAL_CANCELLATION=True)
     def test_write_reversals_from_geag_enterprise_unenrollments_unknown_provider(
@@ -846,7 +844,9 @@ def test_write_reversals_from_geag_enterprise_unenrollments_unknown_provider(
         mock_fetch_recent_unenrollments_client,
     ):
         """
-        Test the write_reversals_from_enterprise_unenrollments management command's ability to create a reversal.
+        Test that write_reversals_from_enterprise_unenrollments management command
+        does not do anything with an external reference provider that it doesn't know
+        how to un-fulfill or reverse.
         """
         # Reversal creation will trigger a signal handler that will make a call to enterprise
         mock_signal_client.return_value = mock.MagicMock()
@@ -921,10 +921,11 @@ def test_write_reversals_from_geag_enterprise_unenrollments_unknown_provider(
             'active': False
         }
 
-        assert Reversal.objects.count() == 0
-        with self.assertRaises(FulfillmentException):
-            call_command('write_reversals_from_enterprise_unenrollments')
-        assert Reversal.objects.count() == 0
+        self.assertIsNone(self.unknown_transaction.get_reversal())
+
+        call_command('write_reversals_from_enterprise_unenrollments')
+
+        self.assertIsNone(self.unknown_transaction.get_reversal())
 
     @mock.patch("enterprise_subsidy.apps.subsidy.models.Subsidy.lms_user_client")
     @mock.patch("enterprise_subsidy.apps.content_metadata.api.ContentMetadataApi.get_content_summary")

enterprise_subsidy/apps/transaction/tests/test_signal_handlers.py

@@ -6,9 +6,16 @@
 import pytest
 from django.test import TestCase
 from openedx_ledger.signals.signals import TRANSACTION_REVERSED
-from openedx_ledger.test_utils.factories import LedgerFactory, ReversalFactory, TransactionFactory
+from openedx_ledger.test_utils.factories import (
+    ExternalFulfillmentProviderFactory,
+    ExternalTransactionReferenceFactory,
+    LedgerFactory,
+    ReversalFactory,
+    TransactionFactory
+)
 
 from enterprise_subsidy.apps.api_client.enterprise import EnterpriseApiClient
+from enterprise_subsidy.apps.fulfillment.api import GEAGFulfillmentHandler
 from test_utils.utils import MockResponse
 
 
@@ -32,6 +39,35 @@ def test_transaction_reversed_signal_handler_catches_event(self, mock_oauth_clie
             f"{transaction.fulfillment_identifier}/cancel-fulfillment",
         )
 
+    @mock.patch('enterprise_subsidy.apps.fulfillment.api.GetSmarterEnterpriseApiClient')
+    @mock.patch('enterprise_subsidy.apps.api_client.base_oauth.OAuthAPIClient', return_value=mock.MagicMock())
+    def test_reversed_signal_causes_internal_and_external_unfulfillment(self, mock_oauth_client, mock_geag_client):
+        """
+        Tests that the signal handler cancels internal and external fulfillments
+        related to the reversed transaction.
+        """
+        mock_oauth_client.return_value.post.return_value = MockResponse({}, 201)
+        ledger = LedgerFactory()
+        transaction = TransactionFactory(ledger=ledger, quantity=100, fulfillment_identifier='foobar')
+        geag_provider = ExternalFulfillmentProviderFactory(
+            slug=GEAGFulfillmentHandler.EXTERNAL_FULFILLMENT_PROVIDER_SLUG,
+        )
+        geag_reference = ExternalTransactionReferenceFactory(
+            external_fulfillment_provider=geag_provider,
+            transaction=transaction,
+        )
+
+        reversal = ReversalFactory(transaction=transaction)
+        TRANSACTION_REVERSED.send(sender=self, reversal=reversal)
+
+        assert mock_oauth_client.return_value.post.call_args.args == (
+            EnterpriseApiClient.enterprise_subsidy_fulfillment_endpoint +
+            f"{transaction.fulfillment_identifier}/cancel-fulfillment",
+        )
+        mock_geag_client().cancel_enterprise_allocation.assert_called_once_with(
+            geag_reference.external_reference_id,
+        )
+
     @mock.patch('enterprise_subsidy.apps.api_client.base_oauth.OAuthAPIClient', return_value=mock.MagicMock())
     def test_transaction_reversed_signal_without_fulfillment_identifier(self, mock_oauth_client):
         """