Merge pull request #2234 from openedx/pwnage101/ENT-9213-2

fix: send LEARNER_CREDIT_COURSE_ENROLLMENT_REVOKED from the correct place.

CHANGELOG.rst

@@ -17,6 +17,10 @@ Unreleased
 ----------
 * nothing unreleased
 
+[4.25.9]
+----------
+* fix: send LEARNER_CREDIT_COURSE_ENROLLMENT_REVOKED from the correct place.
+
 [4.25.8]
 ----------
 * feat: added migration for removing unencrypted client credentials

enterprise/__init__.py

@@ -2,4 +2,4 @@
 Your project description goes here.
 """
 
-__version__ = "4.25.8"
+__version__ = "4.25.9"

enterprise/models.py

@@ -2097,14 +2097,37 @@ def is_audit_enrollment(self):
     @property
     def license(self):
         """
-        Returns the license associated with this enterprise course enrollment if one exists.
+        Returns the license fulfillment associated with this enterprise course enrollment if one exists.
         """
         try:
             associated_license = self.licensedenterprisecourseenrollment_enrollment_fulfillment  # pylint: disable=no-member
         except LicensedEnterpriseCourseEnrollment.DoesNotExist:
             associated_license = None
         return associated_license
 
+    @property
+    def learner_credit_fulfillment(self):
+        """
+        Returns the Learner Credit fulfillment associated with this enterprise course enrollment if one exists.
+        """
+        try:
+            associated_fulfillment = self.learnercreditenterprisecourseenrollment_enrollment_fulfillment  # pylint: disable=no-member
+        except LearnerCreditEnterpriseCourseEnrollment.DoesNotExist:
+            associated_fulfillment = None
+        return associated_fulfillment
+
+    @property
+    def fulfillments(self):
+        """
+        Find and return the related EnterpriseFulfillmentSource subclass, or empty list if there are none.
+
+        Returns:
+            list of EnterpriseFulfillmentSource subclass instances: all existing related fulfillments
+        """
+        possible_fulfillments = [self.license, self.learner_credit_fulfillment]
+        existing_fulfillments = [f for f in possible_fulfillments if f]
+        return existing_fulfillments
+
     @cached_property
     def course_enrollment(self):
         """
@@ -2196,6 +2219,48 @@ def get_enterprise_uuids_with_user_and_course(cls, user_id, course_run_id, is_cu
             )
             return []
 
+    def set_unenrolled(self, desired_unenrolled):
+        """
+        Idempotently set this object's fields to appear (un)enrolled and (un)saved-for-later.
+
+        Also, attempt to revoke any related fulfillment, which in turn is also idempotent.
+
+        This method and the fulfillment's revoke() call each other!!! If you edit either method, make sure to preserve
+        base cases that terminate infinite recursion.
+
+        TODO: revoke entitlements as well?
+        """
+        changed = False
+        if desired_unenrolled:
+            if not self.unenrolled or not self.saved_for_later:
+                self.saved_for_later = True
+                self.unenrolled = True
+                self.unenrolled_at = localized_utcnow()
+                changed = True
+        else:
+            if self.unenrolled or self.saved_for_later:
+                self.saved_for_later = False
+                self.unenrolled = False
+                self.unenrolled_at = None
+                changed = True
+        if changed:
+            LOGGER.info(
+                f"Marking EnterpriseCourseEnrollment as unenrolled={desired_unenrolled} "
+                f"for LMS user {self.enterprise_customer_user.user_id} "
+                f"and course {self.course_id}"
+            )
+            self.save()
+            # Find and revoke/reactivate any related fulfillment if unenrolling the EnterpriseCourseEnrollment.
+            # By only updating the related object on updates to self, we prevent infinite recursion.
+            if desired_unenrolled:
+                for fulfillment in self.fulfillments:
+                    if not fulfillment.is_revoked:  # redundant base case to terminate loops.
+                        fulfillment.revoke()
+            # Fulfillment reactivation on ECE reenrollment is unsupported. We'd need to collect a
+            # transaction UUID from the caller, but the caller at the time of writing is not aware of any
+            # transaction. Furthermore, we wouldn't know which fulfillment to reactivate, if there were multiple
+            # related fulfillment types.
+
     def __str__(self):
         """
         Create string representation of the enrollment.
@@ -2285,34 +2350,50 @@ def enterprise_customer_user(self):
 
     def revoke(self):
         """
-        Marks this object as revoked and marks the associated EnterpriseCourseEnrollment
-        as "saved for later".  This object and the associated EnterpriseCourseEnrollment are both saved.
+        Idempotently unenroll/revoke this fulfillment and associated EnterpriseCourseEnrollment.
 
-        Subclasses may override this function to additionally emit revocation events.
+        This method and EnterpriseCourseEnrollment.set_unenrolled() call each other!!! If you edit either method, make
+        sure to preserve base cases that terminate infinite recursion.
 
-        TODO: revoke entitlements as well?
-        """
-        if self.enterprise_course_enrollment:
-            self.enterprise_course_enrollment.saved_for_later = True
-            self.enterprise_course_enrollment.unenrolled = True
-            self.enterprise_course_enrollment.unenrolled_at = localized_utcnow()
-            self.enterprise_course_enrollment.save()
+        Notes:
+        * This object and the associated EnterpriseCourseEnrollment may both be saved.
+        * Subclasses may override this function to additionally emit revocation events.
 
-        self.is_revoked = True
-        self.save()
+        Returns:
+            bool: True if self.is_revoked was changed.
+        """
+        changed = False
+        if not self.is_revoked:
+            LOGGER.info(f"Marking fulfillment {str(self)} as revoked.")
+            changed = True
+            self.is_revoked = True
+            self.save()
+            # Find and unenroll any related EnterpriseCourseEnrollment.
+            # By only updating the related object on updates to self, we prevent infinite recursion.
+            if ece := self.enterprise_course_enrollment:
+                if not ece.unenrolled:  # redundant base case to terminate loops.
+                    ece.set_unenrolled(True)
+        return changed
 
     def reactivate(self, **kwargs):
         """
         Idempotently reactivates this enterprise fulfillment source.
-        """
-        if self.enterprise_course_enrollment:
-            self.enterprise_course_enrollment.saved_for_later = False
-            self.enterprise_course_enrollment.unenrolled = False
-            self.enterprise_course_enrollment.unenrolled_at = None
-            self.enterprise_course_enrollment.save()
 
-        self.is_revoked = False
-        self.save()
+        Returns:
+            bool: True if self.is_revoked was changed.
+        """
+        changed = False
+        if self.is_revoked:
+            LOGGER.info(f"Marking fulfillment {str(self)} as reactivated.")
+            changed = True
+            self.is_revoked = False
+            self.save()
+            # Find and REenroll any related EnterpriseCourseEnrollment.
+            # By only updating the related object on updates to self, we prevent infinite recursion.
+            if ece := self.enterprise_course_enrollment:
+                if ece.unenrolled:  # redundant base case to terminate loops.
+                    ece.set_unenrolled(False)
+        return changed
 
     def __str__(self):
         """
@@ -2337,8 +2418,9 @@ def revoke(self):
         """
         Revoke this LearnerCreditEnterpriseCourseEnrollment, and emit a revoked event.
         """
-        super().revoke()
-        send_learner_credit_course_enrollment_revoked_event(self)
+        if changed := super().revoke():
+            send_learner_credit_course_enrollment_revoked_event(self)
+        return changed
 
     def reactivate(self, transaction_id=None, **kwargs):
         """
@@ -2354,7 +2436,7 @@ def reactivate(self, transaction_id=None, **kwargs):
                 f"getting this enrollment for free."
             )
         self.transaction_id = transaction_id
-        super().reactivate()
+        return super().reactivate()
 
     transaction_id = models.UUIDField(
         primary_key=False,

enterprise/signals.py

@@ -17,7 +17,6 @@
 from enterprise.utils import (
     NotConnectedToOpenEdX,
     get_default_catalog_content_filter,
-    localized_utcnow,
     unset_enterprise_learner_language,
     unset_language_of_all_enterprise_learners,
 )
@@ -364,14 +363,7 @@ def course_enrollment_changed_receiver(sender, **kwargs):     # pylint: disable=
         enterprise_customer_user__user_id=enrollment.user.id,
     ).first()
     if enterprise_enrollment and enrollment.is_active:
-        logger.info(
-            f"Marking EnterpriseCourseEnrollment as enrolled (unenrolled_at=NULL) for user {enrollment.user} and "
-            f"course {enrollment.course.course_key}"
-        )
-        enterprise_enrollment.unenrolled = False
-        enterprise_enrollment.unenrolled_at = None
-        enterprise_enrollment.saved_for_later = False
-        enterprise_enrollment.save()
+        enterprise_enrollment.set_unenrolled(False)
     # Note: If the CourseEnrollment is being flipped to is_active=False, then this handler is a no-op.
     # In that case, the `enterprise_unenrollment_receiver` signal handler below will run.
 
@@ -386,13 +378,7 @@ def enterprise_unenrollment_receiver(sender, **kwargs):     # pylint: disable=un
         enterprise_customer_user__user_id=enrollment.user.id,
     ).first()
     if enterprise_enrollment:
-        logger.info(
-            f"Marking EnterpriseCourseEnrollment as unenrolled for user {enrollment.user} and "
-            f"course {enrollment.course.course_key}"
-        )
-        enterprise_enrollment.unenrolled = True
-        enterprise_enrollment.unenrolled_at = localized_utcnow()
-        enterprise_enrollment.save()
+        enterprise_enrollment.set_unenrolled(True)
 
 
 def create_enterprise_enrollment_receiver(sender, instance, **kwargs):     # pylint: disable=unused-argument

tests/test_enterprise/api/test_views.py

@@ -4152,6 +4152,7 @@ def test_requested_recently_unenrolled_subsidy_fulfillment(self):
         # user. Because the requesting user is an operator user, they should be able to see this enrollment.
         second_lc_enrollment = factories.LearnerCreditEnterpriseCourseEnrollmentFactory(
             enterprise_course_enrollment=second_enterprise_course_enrollment,
+            is_revoked=True,
         )
 
         self.enterprise_course_enrollment.unenrolled = True
@@ -4209,6 +4210,7 @@ def test_recently_unenrolled_fulfillment_endpoint_can_filter_for_modified_after(
         )
         old_learner_credit_enrollment = factories.LearnerCreditEnterpriseCourseEnrollmentFactory(
             enterprise_course_enrollment=old_enterprise_course_enrollment,
+            is_revoked=True,
         )
         response = self.client.get(
             reverse('enterprise-subsidy-fulfillment-unenrolled') + self.unenrolled_after_filter

tests/test_enterprise/test_signals.py

@@ -41,6 +41,7 @@
     EnterpriseCustomerFactory,
     EnterpriseCustomerUserFactory,
     EnterpriseGroupMembershipFactory,
+    LearnerCreditEnterpriseCourseEnrollmentFactory,
     PendingEnrollmentFactory,
     PendingEnterpriseCustomerAdminUserFactory,
     PendingEnterpriseCustomerUserFactory,
@@ -830,6 +831,7 @@ def test_delete_enterprise_learner_role_assignment_no_user_associated(self):
 
 
 @mark.django_db
+@ddt.ddt
 class TestCourseEnrollmentSignals(TestCase):
     """
     Tests signals associated with CourseEnrollments (that are found in edx-platform).
@@ -897,17 +899,27 @@ def test_create_ece_receiver_does_not_call_task_if_ecu_not_exists(self, mock_tas
         create_enterprise_enrollment_receiver(sender, instance, **kwargs)
         mock_task.assert_not_called()
 
-    def test_course_enrollment_changed_receiver(self):
+    @ddt.data(True, False)
+    def test_course_enrollment_changed_receiver(self, create_related_lc_fulfillment):
         """
         Test receiver that is supposed to handle course enrollments being reactivated (re-enrolled).
         """
         # Create an unenrolled EnterpriseCourseEnrollment.
         enterprise_enrollment = EnterpriseCourseEnrollmentFactory(
             enterprise_customer_user=self.enterprise_customer_user,
+            saved_for_later=True,
             unenrolled=True,
             unenrolled_at=datetime.now() - timedelta(days=1),
         )
 
+        # Optionally create a revoked LearnerCreditEnterpriseCourseEnrollment.
+        lc_fulfillment = None
+        if create_related_lc_fulfillment:
+            lc_fulfillment = LearnerCreditEnterpriseCourseEnrollmentFactory(
+                enterprise_course_enrollment=enterprise_enrollment,
+                is_revoked=True,
+            )
+
         # Simulate a previously inactive course enrollment being re-activated.
         mock_enrollment_data = mock.Mock()
         mock_enrollment_data.course.course_key = enterprise_enrollment.course_id
@@ -921,20 +933,36 @@ def test_course_enrollment_changed_receiver(self):
         # Make sure the previously inactive enterprise enrollment has now been re-activated in response to the system
         # enrollment being re-activated.
         enterprise_enrollment.refresh_from_db()
+        assert enterprise_enrollment.saved_for_later is False
         assert enterprise_enrollment.unenrolled is False
         assert enterprise_enrollment.unenrolled_at is None
 
-    def test_enterprise_unenrollment_receiver(self):
+        # Make sure the related LC fulfillment is SILL REVOKED. Fulfillment re-activation is unsupported.
+        if create_related_lc_fulfillment:
+            lc_fulfillment.refresh_from_db()
+            assert lc_fulfillment.is_revoked is True
+
+    @mock.patch('enterprise.models.send_learner_credit_course_enrollment_revoked_event')
+    @ddt.data(True, False)
+    def test_enterprise_unenrollment_receiver(self, create_related_lc_fulfillment, mock_send_revoked_event):
         """
         Test receiver that is supposed to handle course enrollments being deactivated (unenrolled).
         """
         # Create an enrolled EnterpriseCourseEnrollment.
         enterprise_enrollment = EnterpriseCourseEnrollmentFactory(
             enterprise_customer_user=self.enterprise_customer_user,
+            saved_for_later=False,
             unenrolled=None,
             unenrolled_at=None,
         )
 
+        # Optionally create an enrolled LearnerCreditEnterpriseCourseEnrollment.
+        lc_fulfillment = None
+        if create_related_lc_fulfillment:
+            lc_fulfillment = LearnerCreditEnterpriseCourseEnrollmentFactory(
+                enterprise_course_enrollment=enterprise_enrollment,
+            )
+
         # Simulate a previously active course enrollment being deactivated.
         mock_enrollment_data = mock.Mock()
         mock_enrollment_data.course.course_key = enterprise_enrollment.course_id
@@ -948,9 +976,20 @@ def test_enterprise_unenrollment_receiver(self):
         # Make sure the previously active enterprise enrollment has now been deactivated in response to the system
         # enrollment being deactivated.
         enterprise_enrollment.refresh_from_db()
+        assert enterprise_enrollment.saved_for_later is True
         assert enterprise_enrollment.unenrolled is True
         assert enterprise_enrollment.unenrolled_at is not None
 
+        # Make sure the related LC fulfillment is also revoked, and a signal sent.
+        if create_related_lc_fulfillment:
+            lc_fulfillment.refresh_from_db()
+            assert lc_fulfillment.is_revoked is True
+            mock_send_revoked_event.assert_called_once()
+            event_fulfillment = mock_send_revoked_event.call_args.args[0]
+            assert event_fulfillment.uuid == lc_fulfillment.uuid
+        else:
+            mock_send_revoked_event.assert_not_called()
+
 
 @mark.django_db
 class TestEnterpriseCatalogSignals(unittest.TestCase):