Merge pull request #14810 from edx/mrehan/create-or-update-overview-on-course-publish

TNL-6601 – Create or update course overview on a course publish

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

@@ -101,9 +101,9 @@ class CourseOverview(TimeStampedModel):
     eligible_for_financial_aid = BooleanField(default=True)
 
     @classmethod
-    def _create_from_course(cls, course):
+    def _create_or_update(cls, course):
         """
-        Creates a CourseOverview object from a CourseDescriptor.
+        Creates or updates a CourseOverview object from a CourseDescriptor.
 
         Does not touch the database, simply constructs and returns an overview
         from the given course.
@@ -112,13 +112,11 @@ class CourseOverview(TimeStampedModel):
             course (CourseDescriptor): any course descriptor object
 
         Returns:
-            CourseOverview: overview extracted from the given course
+            CourseOverview: created or updated overview extracted from the given course
         """
         from lms.djangoapps.certificates.api import get_active_web_certificate
         from openedx.core.lib.courses import course_image_url
 
-        log.info('Creating course overview for %s.', unicode(course.id))
-
         # Workaround for a problem discovered in https://openedx.atlassian.net/browse/TNL-2806.
         # If the course has a malformed grading policy such that
         # course._grading_policy['GRADE_CUTOFFS'] = {}, then
@@ -141,54 +139,62 @@ class CourseOverview(TimeStampedModel):
             end = ccx.due
             max_student_enrollments_allowed = ccx.max_student_enrollments_allowed
 
-        return cls(
-            version=cls.VERSION,
-            id=course.id,
-            _location=course.location,
-            org=course.location.org,
-            display_name=display_name,
-            display_number_with_default=course.display_number_with_default,
-            display_org_with_default=course.display_org_with_default,
-
-            start=start,
-            end=end,
-            advertised_start=course.advertised_start,
-            announcement=course.announcement,
-
-            course_image_url=course_image_url(course),
-            social_sharing_url=course.social_sharing_url,
-
-            certificates_display_behavior=course.certificates_display_behavior,
-            certificates_show_before_end=course.certificates_show_before_end,
-            cert_html_view_enabled=course.cert_html_view_enabled,
-            has_any_active_web_certificate=(get_active_web_certificate(course) is not None),
-            cert_name_short=course.cert_name_short,
-            cert_name_long=course.cert_name_long,
-            lowest_passing_grade=lowest_passing_grade,
-            end_of_course_survey_url=course.end_of_course_survey_url,
-
-            days_early_for_beta=course.days_early_for_beta,
-            mobile_available=course.mobile_available,
-            visible_to_staff_only=course.visible_to_staff_only,
-            _pre_requisite_courses_json=json.dumps(course.pre_requisite_courses),
-
-            enrollment_start=course.enrollment_start,
-            enrollment_end=course.enrollment_end,
-            enrollment_domain=course.enrollment_domain,
-            invitation_only=course.invitation_only,
-            max_student_enrollments_allowed=max_student_enrollments_allowed,
-
-            catalog_visibility=course.catalog_visibility,
-            short_description=CourseDetails.fetch_about_attribute(course.id, 'short_description'),
-            effort=CourseDetails.fetch_about_attribute(course.id, 'effort'),
-            course_video_url=CourseDetails.fetch_video_url(course.id),
-            self_paced=course.self_paced,
-        )
+        course_overview = cls.objects.filter(id=course.id)
+        if course_overview.exists():
+            log.info('Updating course overview for %s.', unicode(course.id))
+            course_overview = course_overview.first()
+        else:
+            log.info('Creating course overview for %s.', unicode(course.id))
+            course_overview = cls()
+
+        course_overview.version = cls.VERSION
+        course_overview.id = course.id
+        course_overview._location = course.location
+        course_overview.org = course.location.org
+        course_overview.display_name = display_name
+        course_overview.display_number_with_default = course.display_number_with_default
+        course_overview.display_org_with_default = course.display_org_with_default
+
+        course_overview.start = start
+        course_overview.end = end
+        course_overview.advertised_start = course.advertised_start
+        course_overview.announcement = course.announcement
+
+        course_overview.course_image_url = course_image_url(course)
+        course_overview.social_sharing_url = course.social_sharing_url
+
+        course_overview.certificates_display_behavior = course.certificates_display_behavior
+        course_overview.certificates_show_before_end = course.certificates_show_before_end
+        course_overview.cert_html_view_enabled = course.cert_html_view_enabled
+        course_overview.has_any_active_web_certificate = (get_active_web_certificate(course) is not None)
+        course_overview.cert_name_short = course.cert_name_short
+        course_overview.cert_name_long = course.cert_name_long
+        course_overview.lowest_passing_grade = lowest_passing_grade
+        course_overview.end_of_course_survey_url = course.end_of_course_survey_url
+
+        course_overview.days_early_for_beta = course.days_early_for_beta
+        course_overview.mobile_available = course.mobile_available
+        course_overview.visible_to_staff_only = course.visible_to_staff_only
+        course_overview._pre_requisite_courses_json = json.dumps(course.pre_requisite_courses)
+
+        course_overview.enrollment_start = course.enrollment_start
+        course_overview.enrollment_end = course.enrollment_end
+        course_overview.enrollment_domain = course.enrollment_domain
+        course_overview.invitation_only = course.invitation_only
+        course_overview.max_student_enrollments_allowed = max_student_enrollments_allowed
+
+        course_overview.catalog_visibility = course.catalog_visibility
+        course_overview.short_description = CourseDetails.fetch_about_attribute(course.id, 'short_description')
+        course_overview.effort = CourseDetails.fetch_about_attribute(course.id, 'effort')
+        course_overview.course_video_url = CourseDetails.fetch_video_url(course.id)
+        course_overview.self_paced = course.self_paced
+
+        return course_overview
 
     @classmethod
     def load_from_module_store(cls, course_id):
         """
-        Load a CourseDescriptor, create a new CourseOverview from it, cache the
+        Load a CourseDescriptor, create or update a CourseOverview from it, cache the
         overview, and return it.
 
         Arguments:
@@ -207,15 +213,17 @@ class CourseOverview(TimeStampedModel):
         with store.bulk_operations(course_id):
             course = store.get_course(course_id)
             if isinstance(course, CourseDescriptor):
-                course_overview = cls._create_from_course(course)
+                course_overview = cls._create_or_update(course)
                 try:
                     with transaction.atomic():
                         course_overview.save()
+                        # Remove and recreate all the course tabs
+                        CourseOverviewTab.objects.filter(course_overview=course_overview).delete()
                         CourseOverviewTab.objects.bulk_create([
                             CourseOverviewTab(tab_id=tab.tab_id, course_overview=course_overview)
                             for tab in course.tabs
                         ])
-                        CourseOverviewImageSet.create_for_course(course_overview, course)
+                        CourseOverviewImageSet.create_or_update(course_overview, course)
 
                 except IntegrityError:
                     # There is a rare race condition that will occur if
@@ -272,7 +280,7 @@ class CourseOverview(TimeStampedModel):
         # they were never generated, or because they were flushed out after
         # a change to CourseOverviewImageConfig.
         if course_overview and not hasattr(course_overview, 'image_set'):
-            CourseOverviewImageSet.create_for_course(course_overview)
+            CourseOverviewImageSet.create_or_update(course_overview)
 
         return course_overview or cls.load_from_module_store(course_id)
 
@@ -696,11 +704,11 @@ class CourseOverviewImageSet(TimeStampedModel):
     large_url = models.TextField(blank=True, default="")
 
     @classmethod
-    def create_for_course(cls, course_overview, course=None):
+    def create_or_update(cls, course_overview, course=None):
         """
-        Create thumbnail images for this CourseOverview.
+        Create or update thumbnail images for this CourseOverview.
 
-        This will save the CourseOverviewImageSet it creates before it returns.
+        This will save the CourseOverviewImageSet before it returns.
         """
         from openedx.core.lib.courses import create_course_image_thumbnail
 
@@ -716,7 +724,11 @@ class CourseOverviewImageSet(TimeStampedModel):
         if not course:
             course = modulestore().get_course(course_overview.id)
 
-        image_set = CourseOverviewImageSet(course_overview=course_overview)
+        if hasattr(course_overview, 'image_set'):
+            image_set = course_overview.image_set
+        else:
+            image_set = cls(course_overview=course_overview)
+
         if course.course_image:
             # Try to create a thumbnails of the course image. If this fails for any
             # reason (weird format, non-standard URL, etc.), the URLs will default

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

@@ -13,7 +13,6 @@ def _listen_for_course_publish(sender, course_key, **kwargs):  # pylint: disable
     Catches the signal that a course has been published in Studio and
     updates the corresponding CourseOverview cache entry.
     """
-    CourseOverview.objects.filter(id=course_key).delete()
     CourseOverview.load_from_module_store(course_key)
 
 

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

@@ -11,6 +11,7 @@ from nose.plugins.attrib import attr
 import pytz
 
 from django.conf import settings
+from django.db.utils import IntegrityError
 from django.test.utils import override_settings
 from django.utils import timezone
 from PIL import Image
@@ -41,7 +42,7 @@ from .models import CourseOverview, CourseOverviewImageSet, CourseOverviewImageC
 @ddt.ddt
 class CourseOverviewTestCase(ModuleStoreTestCase):
     """
-    Tests for CourseOverviewDescriptor model.
+    Tests for CourseOverview model.
     """
 
     TODAY = timezone.now()
@@ -62,7 +63,7 @@ class CourseOverviewTestCase(ModuleStoreTestCase):
         Specifically, given a course, test that data within the following three
         objects match each other:
          - the CourseDescriptor itself
-         - a CourseOverview that was newly constructed from _create_from_course
+         - a CourseOverview that was newly constructed from _create_or_update
          - a CourseOverview that was loaded from the MySQL database
 
         Arguments:
@@ -83,7 +84,7 @@ class CourseOverviewTestCase(ModuleStoreTestCase):
             return math.floor((date_time - epoch).total_seconds())
 
         # Load the CourseOverview from the cache twice. The first load will be a cache miss (because the cache
-        # is empty) so the course will be newly created with CourseOverviewDescriptor.create_from_course. The second
+        # is empty) so the course will be newly created with CourseOverview._create_or_update. The second
         # load will be a cache hit, so the course will be loaded from the cache.
         course_overview_cache_miss = CourseOverview.get_from_id(course.id)
         course_overview_cache_hit = CourseOverview.get_from_id(course.id)
@@ -345,7 +346,7 @@ class CourseOverviewTestCase(ModuleStoreTestCase):
         course._grading_policy['GRADE_CUTOFFS'] = {}  # pylint: disable=protected-access
         with self.assertRaises(ValueError):
             __ = course.lowest_passing_grade
-        course_overview = CourseOverview._create_from_course(course)  # pylint: disable=protected-access
+        course_overview = CourseOverview._create_or_update(course)  # pylint: disable=protected-access
         self.assertEqual(course_overview.lowest_passing_grade, None)
 
     @ddt.data((ModuleStoreEnum.Type.mongo, 4, 4), (ModuleStoreEnum.Type.split, 3, 4))
@@ -920,9 +921,9 @@ class CourseOverviewImageSetTestCase(ModuleStoreTestCase):
         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()
+        # thrown and suppressed in create_or_update()
         self.set_config(True)
-        CourseOverviewImageSet.create_for_course(overview)
+        CourseOverviewImageSet.create_or_update(overview)
         self.assertTrue(hasattr(overview, 'image_set'))
 
         # The following is actually very important for this test because
@@ -965,3 +966,41 @@ class CourseOverviewImageSetTestCase(ModuleStoreTestCase):
                 }
             )
             return course_overview
+
+
+@attr(shard=3)
+@ddt.ddt
+class CourseOverviewTabTestCase(ModuleStoreTestCase):
+    """
+    Tests for CourseOverviewTab model.
+    """
+
+    ENABLED_SIGNALS = ['course_published']
+
+    @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
+    def test_tabs_deletion_rollback_on_integrity_error(self, modulestore_type):
+        """
+        Tests that course_overview tabs deletion is correctly rolled back if an Exception
+        occurs while updating the course_overview.
+        """
+        course = CourseFactory.create(default_store=modulestore_type)
+        course_overview = CourseOverview.get_from_id(course.id)
+        expected_tabs = {tab.tab_id for tab in course_overview.tabs.all()}
+
+        with mock.patch(
+            'openedx.core.djangoapps.content.course_overviews.models.CourseOverviewTab.objects.bulk_create'
+        ) as course_overview_tabs_bulk_create:
+            course_overview_tabs_bulk_create.side_effect = IntegrityError
+
+            # Update display name on the course descriptor
+            # This fires a course_published signal, which should be caught in signals.py,
+            # which should in turn load CourseOverview from modulestore.
+            course.display_name = u'Updated display name'
+            with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred):
+                self.store.update_item(course, ModuleStoreEnum.UserID.test)
+
+            # Asserts that the tabs deletion is properly rolled back to a save point and
+            # the course overview is not updated.
+            actual_tabs = {tab.tab_id for tab in course_overview.tabs.all()}
+            self.assertEqual(actual_tabs, expected_tabs)
+            self.assertNotEqual(course_overview.display_name, course.display_name)