Prevent course image thumbnail race condition.

Without this change, we'd throw a TransactionManagementError in the case
where we have a race condition and generate multiple CourseOverviewImageSets
for the same CourseOverview concurrently.

openedx/core/djangoapps/content/course_overviews/models.py

@@ -680,8 +680,9 @@ class CourseOverviewImageSet(TimeStampedModel):
         # an error or the course has no source course_image), our url fields
         # just keep their blank defaults.
         try:
-            image_set.save()
-            course_overview.image_set = image_set
+            with transaction.atomic():
+                image_set.save()
+                course_overview.image_set = image_set
         except (IntegrityError, ValueError):
             # In the event of a race condition that tries to save two image sets
             # to the same CourseOverview, we'll just silently pass on the one

openedx/core/djangoapps/content/course_overviews/tests.py

@@ -32,7 +32,7 @@ from xmodule.modulestore.django import modulestore
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory, check_mongo_calls, check_mongo_calls_range
 
-from .models import CourseOverview, CourseOverviewImageConfig
+from .models import CourseOverview, CourseOverviewImageSet, CourseOverviewImageConfig
 
 
 @ddt.ddt
@@ -797,6 +797,43 @@ class CourseOverviewImageSetTestCase(ModuleStoreTestCase):
                 self.assertEqual(src_x, image_x)
                 self.assertEqual(src_y, image_y)
 
+    def test_image_creation_race_condition(self):
+        """
+        Test for race condition in CourseOverviewImageSet creation.
+
+        CourseOverviewTestCase already tests for race conditions with
+        CourseOverview as a whole, but we still need to test the case where a
+        CourseOverview already exists and we have a race condition purely in the
+        part that adds a new CourseOverviewImageSet.
+        """
+        # Set config to False so that we don't create the image yet
+        self.set_config(False)
+        course = CourseFactory.create()
+
+        # First create our CourseOverview
+        overview = CourseOverview.get_from_id(course.id)
+        self.assertFalse(hasattr(overview, 'image_set'))
+
+        # Now create an ImageSet by hand...
+        CourseOverviewImageSet.objects.create(course_overview=overview)
+
+        # Now do it the normal way -- this will cause an IntegrityError to be
+        # thrown and suppressed in create_for_course()
+        self.set_config(True)
+        CourseOverviewImageSet.create_for_course(overview)
+        self.assertTrue(hasattr(overview, 'image_set'))
+
+        # The following is actually very important for this test because
+        # set_config() does a model insert after create_for_course() has caught
+        # and supressed an IntegrityError above. If create_for_course() properly
+        # wraps that operation in a transaction.atomic() block, the following
+        # will execute fine. If create_for_course() doesn't use an atomic block,
+        # the following line will cause a TransactionManagementError because
+        # Django will detect that something has already been rolled back in this
+        # transaction. So we don't really care about setting the config -- it's
+        # just a convenient way to cause a database write operation to happen.
+        self.set_config(False)
+
     def _assert_image_urls_all_default(self, modulestore_type, raw_course_image_name, expected_url=None):
         """
         Helper for asserting that all image_urls are defaulting to a particular value.