fix: properly avoid race condition when provisioning licenses

license_manager/apps/subscriptions/admin.py

@@ -391,16 +391,17 @@ def save_model(self, request, obj, form, change):
         if not change:
             obj.desired_num_licenses = form.cleaned_data.get('num_licenses', 0)
 
-        # If the desired number of licenses is large enough, we trigger an async celery task
-        # after the creation of this record.  We wrap that creation in a transaction here
-        # to force a commit, so that the async task does not encounter a race condition
-        # where the plan it expects to read from the DB does not yet exist.
-        with transaction.atomic():
-            super().save_model(request, obj, form, change)
+        super().save_model(request, obj, form, change)
 
-        # Finally, if we're creating the model instance, go ahead and create the related license records.
+        # ``not change`` implies that we're creating the model instance,
+        # so go ahead and create the related license records.
         if not change:
-            obj.provision_licenses()
+            # If the desired number of licenses is large enough, ``provision_licenses()` will
+            # submit an async celery task after this record is saved.
+            # We defer submitting the task until a successful commit
+            # occurs, so that the async task does not encounter a race condition
+            # where the plan it expects to read from the DB does not yet exist.
+            transaction.on_commit(obj.provision_licenses)
 
 
 @admin.register(CustomSubscriptionExpirationMessaging)

license_manager/apps/subscriptions/tests/test_admin.py

@@ -4,7 +4,7 @@
 import pytest
 from django.contrib import messages
 from django.contrib.admin.sites import AdminSite
-from django.test import RequestFactory
+from django.test import RequestFactory, TestCase
 
 from license_manager.apps.subscriptions.admin import (
     CustomerAgreementAdmin,
@@ -29,78 +29,82 @@
 from ..constants import REVOKED
 
 
-@pytest.mark.django_db
-def test_licenses_subscription_creation():
+class SubscriptionCreationTests(TestCase):
     """
-    Verify that creating a SubscriptionPlan creates its associated Licenses after it is created.
+    Tests cases for creating new plans/licenses.
     """
-    subscription_admin = SubscriptionPlanAdmin(SubscriptionPlan, AdminSite())
-    request = RequestFactory()
-    request.user = UserFactory()
-    num_licenses = 5
-    form = make_bound_subscription_form(num_licenses=num_licenses)
-    obj = form.save()  # Get the object returned from saving the form to save to the database
-    assert obj.licenses.count() == 0  # Verify no Licenses have been created yet
-    change = False
-    subscription_admin.save_model(request, obj, form, change)
-    assert obj.licenses.count() == num_licenses
-
-
-@pytest.mark.django_db
-@mock.patch('license_manager.apps.subscriptions.admin.messages.add_message')
-def test_subscription_licenses_create_action(mock_add_message):
-    """
-    Verify that running the create licenses action will create licenses.
-    """
-    # Setup an existing plan
-    customer_agreement = CustomerAgreementFactory()
-    subscription_plan = SubscriptionPlanFactory.create(
-        customer_agreement=customer_agreement,
-        desired_num_licenses=10,
-    )
-    assert subscription_plan.licenses.count() == 0  # Verify no Licenses have been created yet
-
-    # setup the admin form
-    subscription_admin = SubscriptionPlanAdmin(SubscriptionPlan, AdminSite())
-    request = RequestFactory()
-    request.user = UserFactory()
-
-    # doesn't really matter what we put for num_licenses in here, save_model
-    # will read the desired number of license from the existing object on save.
-    form = make_bound_subscription_form(num_licenses=10)
-    form.save()
-
-    # save the form as a modify instead of create
-    subscription_admin.save_model(request, subscription_plan, form, True)
-    subscription_plan.refresh_from_db()
-    # Desired number of licenses won't actually be created until we run the action
-    assert subscription_plan.licenses.count() == 0
-
-    # Now run the action...
-    subscription_admin.create_actual_licenses_action(
-        request,
-        SubscriptionPlan.objects.filter(uuid=subscription_plan.uuid),
-    )
-    subscription_plan.refresh_from_db()
-    # Actual number of licenses should now equal the desired number (ten)
-    assert subscription_plan.licenses.count() == 10
-
-    mock_add_message.assert_called_once_with(
-        request, messages.SUCCESS, 'Successfully created license records for selected Subscription Plans.',
-    )
-
-    # check that freezing the plan means running the create licenses action has no effect
-    subscription_plan.last_freeze_timestamp = localized_utcnow()
-    subscription_plan.desired_num_licenses = 5000
-    subscription_plan.save()
-
-    subscription_admin.create_actual_licenses_action(
-        request,
-        SubscriptionPlan.objects.filter(uuid=subscription_plan.uuid),
-    )
-    subscription_plan.refresh_from_db()
-    # Actual number of licenses should STILL equal 10, because the plan is frozen
-    assert subscription_plan.licenses.count() == 10
+    def test_licenses_subscription_creation(self):
+        """
+        Verify that creating a SubscriptionPlan creates its associated Licenses after it is created.
+        """
+        subscription_admin = SubscriptionPlanAdmin(SubscriptionPlan, AdminSite())
+        request = RequestFactory()
+        request.user = UserFactory()
+        num_licenses = 5
+        form = make_bound_subscription_form(num_licenses=num_licenses)
+        obj = form.save()  # Get the object returned from saving the form to save to the database
+        assert obj.licenses.count() == 0  # Verify no Licenses have been created yet
+        change = False
+
+        with self.captureOnCommitCallbacks(execute=True):
+            subscription_admin.save_model(request, obj, form, change)
+
+        assert obj.licenses.count() == num_licenses
+
+    @mock.patch('license_manager.apps.subscriptions.admin.messages.add_message')
+    def test_subscription_licenses_create_action(self, mock_add_message):
+        """
+        Verify that running the create licenses action will create licenses.
+        """
+        # Setup an existing plan
+        customer_agreement = CustomerAgreementFactory()
+        subscription_plan = SubscriptionPlanFactory.create(
+            customer_agreement=customer_agreement,
+            desired_num_licenses=10,
+        )
+        assert subscription_plan.licenses.count() == 0  # Verify no Licenses have been created yet
+
+        # setup the admin form
+        subscription_admin = SubscriptionPlanAdmin(SubscriptionPlan, AdminSite())
+        request = RequestFactory()
+        request.user = UserFactory()
+
+        # doesn't really matter what we put for num_licenses in here, save_model
+        # will read the desired number of license from the existing object on save.
+        form = make_bound_subscription_form(num_licenses=10)
+        form.save()
+
+        # save the form as a modify instead of create
+        subscription_admin.save_model(request, subscription_plan, form, True)
+        subscription_plan.refresh_from_db()
+        # Desired number of licenses won't actually be created until we run the action
+        assert subscription_plan.licenses.count() == 0
+
+        # Now run the action...
+        subscription_admin.create_actual_licenses_action(
+            request,
+            SubscriptionPlan.objects.filter(uuid=subscription_plan.uuid),
+        )
+        subscription_plan.refresh_from_db()
+        # Actual number of licenses should now equal the desired number (ten)
+        assert subscription_plan.licenses.count() == 10
+
+        mock_add_message.assert_called_once_with(
+            request, messages.SUCCESS, 'Successfully created license records for selected Subscription Plans.',
+        )
+
+        # check that freezing the plan means running the create licenses action has no effect
+        subscription_plan.last_freeze_timestamp = localized_utcnow()
+        subscription_plan.desired_num_licenses = 5000
+        subscription_plan.save()
+
+        subscription_admin.create_actual_licenses_action(
+            request,
+            SubscriptionPlan.objects.filter(uuid=subscription_plan.uuid),
+        )
+        subscription_plan.refresh_from_db()
+        # Actual number of licenses should STILL equal 10, because the plan is frozen
+        assert subscription_plan.licenses.count() == 10
 
 
 @pytest.mark.django_db