From fad53592d28bf944c26e8c49ad67c09db13bd8fd Mon Sep 17 00:00:00 2001 From: Qubad786 Date: Mon, 3 Apr 2017 19:44:40 +0500 Subject: [PATCH] Create or update course overview on a course publish event received from studio. --- .../content/course_overviews/models.py | 112 ++++++++++-------- .../content/course_overviews/signals.py | 1 - .../content/course_overviews/tests.py | 51 +++++++- 3 files changed, 107 insertions(+), 57 deletions(-) diff --git a/openedx/core/djangoapps/content/course_overviews/models.py b/openedx/core/djangoapps/content/course_overviews/models.py index 2b8b22bb21..3617db2d84 100644 --- a/openedx/core/djangoapps/content/course_overviews/models.py +++ b/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, + 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() - start=start, - end=end, - advertised_start=course.advertised_start, - announcement=course.announcement, + 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_image_url=course_image_url(course), - social_sharing_url=course.social_sharing_url, + course_overview.start = start + course_overview.end = end + course_overview.advertised_start = course.advertised_start + course_overview.announcement = course.announcement - 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, + course_overview.course_image_url = course_image_url(course) + course_overview.social_sharing_url = course.social_sharing_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), + 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 - 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, + 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) - 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.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 diff --git a/openedx/core/djangoapps/content/course_overviews/signals.py b/openedx/core/djangoapps/content/course_overviews/signals.py index 951ebbd65f..f32cf24848 100644 --- a/openedx/core/djangoapps/content/course_overviews/signals.py +++ b/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) diff --git a/openedx/core/djangoapps/content/course_overviews/tests.py b/openedx/core/djangoapps/content/course_overviews/tests.py index 6fcfb45336..f0b5d6b672 100644 --- a/openedx/core/djangoapps/content/course_overviews/tests.py +++ b/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)