diff --git a/openedx/core/djangoapps/catalog/management/commands/sync_course_runs.py b/openedx/core/djangoapps/catalog/management/commands/sync_course_runs.py index f053c3fe75..ea2540cabe 100644 --- a/openedx/core/djangoapps/catalog/management/commands/sync_course_runs.py +++ b/openedx/core/djangoapps/catalog/management/commands/sync_course_runs.py @@ -1,6 +1,7 @@ """ Sync course runs from catalog service. """ +from collections import namedtuple import logging from django.core.management.base import BaseCommand @@ -15,33 +16,30 @@ log = logging.getLogger(__name__) class Command(BaseCommand): """ Purpose is to sync course runs data from catalog service to make it accessible in edx-platform. - It just happens to only be syncing marketing URLs from catalog course runs for now. """ help = 'Refresh marketing urls from catalog service.' - def update_course_overviews(self, course_runs): - """ - Refresh marketing urls for the given catalog course runs. + CourseRunField = namedtuple('CourseRunField', 'catalog_name course_overview_name') + course_run_fields = ( + CourseRunField(catalog_name='marketing_url', course_overview_name='marketing_url'), + CourseRunField(catalog_name='eligible_for_financial_aid', course_overview_name='eligible_for_financial_aid'), + CourseRunField(catalog_name='content_language', course_overview_name='language'), + ) + + def handle(self, *args, **options): + log.info('[sync_course_runs] Fetching course runs from catalog service.') + course_runs = get_course_runs() - Arguments: - course_runs: A list containing catalog course runs. - """ # metrics for observability - # number of catalog course runs retrieved. - catalog_course_runs_retrieved = len(course_runs) - # number of catalog course runs found in course overview. - course_runs_found_in_cache = 0 - # number of course overview records actually get updated. - course_metadata_updated = 0 + num_runs_found_in_catalog = len(course_runs) + num_runs_found_in_course_overview = 0 + num_course_overviews_updated = 0 for course_run in course_runs: - is_course_metadata_updated = False - marketing_url = course_run['marketing_url'] - eligible_for_financial_aid = course_run['eligible_for_financial_aid'] course_key = CourseKey.from_string(course_run['key']) try: course_overview = CourseOverview.objects.get(id=course_key) - course_runs_found_in_cache += 1 + num_runs_found_in_course_overview += 1 except CourseOverview.DoesNotExist: log.info( '[sync_course_runs] course overview record not found for course run: %s', @@ -49,32 +47,25 @@ class Command(BaseCommand): ) continue - # Check whether course overview's marketing url is outdated - this saves a db hit. - if course_overview.marketing_url != marketing_url: - course_overview.marketing_url = marketing_url - is_course_metadata_updated = True - - # Check whether course overview's eligible for financial aid is outdated - if course_overview.eligible_for_financial_aid != eligible_for_financial_aid: - course_overview.eligible_for_financial_aid = eligible_for_financial_aid - is_course_metadata_updated = True + is_course_metadata_updated = False + for field in self.course_run_fields: + catalog_value = course_run.get(field.catalog_name) + if getattr(course_overview, field.course_overview_name) != catalog_value: + setattr(course_overview, field.course_overview_name, catalog_value) + is_course_metadata_updated = True if is_course_metadata_updated: course_overview.save() - course_metadata_updated += 1 - - return catalog_course_runs_retrieved, course_runs_found_in_cache, course_metadata_updated - - def handle(self, *args, **options): - log.info('[sync_course_runs] Fetching course runs from catalog service.') - course_runs = get_course_runs() - course_runs_retrieved, course_runs_found, course_metadata_updated = self.update_course_overviews(course_runs) + num_course_overviews_updated += 1 log.info( - ('[sync_course_runs] course runs retrieved: %d, course runs found in course overview: %d,' - ' course runs not found in course overview: %d, course overviews metadata updated: %d,'), - course_runs_retrieved, - course_runs_found, - course_runs_retrieved - course_runs_found, - course_metadata_updated, + '[sync_course_runs] ' + 'course runs found in catalog: %d, ' + 'course runs found in course overview: %d, ' + 'course runs not found in course overview: %d, ' + 'course overviews updated: %d', + num_runs_found_in_catalog, + num_runs_found_in_course_overview, + num_runs_found_in_catalog - num_runs_found_in_course_overview, + num_course_overviews_updated, ) diff --git a/openedx/core/djangoapps/catalog/management/commands/tests/test_sync_course_runs.py b/openedx/core/djangoapps/catalog/management/commands/tests/test_sync_course_runs.py index bc5402ee95..37184930a8 100644 --- a/openedx/core/djangoapps/catalog/management/commands/tests/test_sync_course_runs.py +++ b/openedx/core/djangoapps/catalog/management/commands/tests/test_sync_course_runs.py @@ -7,6 +7,7 @@ import mock from django.core.management import call_command from openedx.core.djangoapps.catalog.tests.factories import CourseRunFactory +from openedx.core.djangoapps.catalog.management.commands.sync_course_runs import Command as sync_command from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory @@ -25,7 +26,7 @@ class TestSyncCourseRunsCommand(ModuleStoreTestCase): # create mongo course self.course = CourseFactory.create() # load this course into course overview - CourseOverview.get_from_id(self.course.id) + self.course_overview = CourseOverview.get_from_id(self.course.id) # create a catalog course run with the same course id. self.catalog_course_run = CourseRunFactory( key=unicode(self.course.id), @@ -33,31 +34,34 @@ class TestSyncCourseRunsCommand(ModuleStoreTestCase): eligible_for_financial_aid=False ) - def get_course_overview_marketing_url(self, course_id): - """ - Get course overview marketing url. - """ - return CourseOverview.objects.get(id=course_id).marketing_url - def test_course_run_sync(self, mock_catalog_course_runs): """ Verify on executing management command course overview data is updated with course run data from course discovery. """ mock_catalog_course_runs.return_value = [self.catalog_course_run] - earlier_marketing_url = self.get_course_overview_marketing_url(self.course.id) - course_overview = CourseOverview.objects.get(id=self.course.id) - earlier_eligible_for_financial_aid = course_overview.eligible_for_financial_aid call_command('sync_course_runs') - course_overview.refresh_from_db() - updated_marketing_url = self.get_course_overview_marketing_url(self.course.id) - updated_eligible_for_financial_aid = course_overview.eligible_for_financial_aid - # Assert that the Marketing URL has changed. - self.assertNotEqual(earlier_marketing_url, updated_marketing_url) - self.assertNotEqual(earlier_eligible_for_financial_aid, updated_eligible_for_financial_aid) - self.assertEqual(updated_marketing_url, 'test_marketing_url') - self.assertEqual(updated_eligible_for_financial_aid, False) + updated_course_overview = CourseOverview.objects.get(id=self.course.id) + + # assert fields have updated + for field in sync_command.course_run_fields: + course_overview_field_name = field.course_overview_name + catalog_field_name = field.catalog_name + + previous_course_overview_value = getattr(self.course_overview, course_overview_field_name), + updated_course_overview_value = getattr(updated_course_overview, course_overview_field_name) + + # course overview value matches catalog value + self.assertEqual( + updated_course_overview_value, + self.catalog_course_run.get(catalog_field_name), + ) + # new value doesn't match old value + self.assertNotEqual( + updated_course_overview_value, + previous_course_overview_value, + ) @mock.patch(COMMAND_MODULE + '.log.info') def test_course_overview_does_not_exist(self, mock_log_info, mock_catalog_course_runs): @@ -73,7 +77,7 @@ class TestSyncCourseRunsCommand(ModuleStoreTestCase): '[sync_course_runs] course overview record not found for course run: %s', nonexistent_course_run['key'], ) - updated_marketing_url = self.get_course_overview_marketing_url(self.course.id) + updated_marketing_url = CourseOverview.objects.get(id=self.course.id).marketing_url self.assertEqual(updated_marketing_url, 'test_marketing_url') @mock.patch(COMMAND_MODULE + '.log.info') @@ -81,17 +85,22 @@ class TestSyncCourseRunsCommand(ModuleStoreTestCase): """ Verify logging at start and end of the command. """ + def _assert_logs(num_updates): + mock_log_info.assert_any_call('[sync_course_runs] Fetching course runs from catalog service.') + mock_log_info.assert_any_call( + '[sync_course_runs] course runs found in catalog: %d, course runs found in course overview: %d,' + ' course runs not found in course overview: %d, course overviews updated: %d', + 3, + 1, + 2, + num_updates, + ) + mock_log_info.reset_mock() + mock_catalog_course_runs.return_value = [self.catalog_course_run, CourseRunFactory(), CourseRunFactory()] call_command('sync_course_runs') - # Assert the logs at the start of the command. - mock_log_info.assert_any_call('[sync_course_runs] Fetching course runs from catalog service.') - # Assert the log metrics at it's completion. - mock_log_info.assert_any_call( - ('[sync_course_runs] course runs retrieved: %d, course runs found in course overview: %d,' - ' course runs not found in course overview: %d, course overviews metadata updated: %d,'), - 3, - 1, - 2, - 1, - ) + _assert_logs(num_updates=1) + + call_command('sync_course_runs') + _assert_logs(num_updates=0) diff --git a/openedx/core/djangoapps/content/course_overviews/models.py b/openedx/core/djangoapps/content/course_overviews/models.py index 171c44cfac..cb694b9482 100644 --- a/openedx/core/djangoapps/content/course_overviews/models.py +++ b/openedx/core/djangoapps/content/course_overviews/models.py @@ -17,6 +17,7 @@ from opaque_keys.edx.keys import CourseKey from config_models.models import ConfigurationModel from lms.djangoapps import django_comment_client +from openedx.core.djangoapps.catalog.models import CatalogIntegration from openedx.core.djangoapps.models.course_details import CourseDetails from static_replace.models import AssetBaseUrlConfig from xmodule import course_metadata_utils, block_metadata_utils @@ -194,7 +195,8 @@ class CourseOverview(TimeStampedModel): course_overview.course_video_url = CourseDetails.fetch_video_url(course.id) course_overview.self_paced = course.self_paced - course_overview.language = course.language + if not CatalogIntegration.is_enabled(): + course_overview.language = course.language return course_overview diff --git a/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py b/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py index c42f285f77..197493a149 100644 --- a/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py +++ b/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py @@ -17,6 +17,7 @@ from django.utils import timezone from PIL import Image from lms.djangoapps.certificates.api import get_active_web_certificate +from openedx.core.djangoapps.catalog.tests.mixins import CatalogIntegrationMixin from openedx.core.djangoapps.models.course_details import CourseDetails from openedx.core.lib.courses import course_image_url from static_replace.models import AssetBaseUrlConfig @@ -40,7 +41,7 @@ from ..models import CourseOverview, CourseOverviewImageSet, CourseOverviewImage @attr(shard=3) @ddt.ddt -class CourseOverviewTestCase(ModuleStoreTestCase): +class CourseOverviewTestCase(CatalogIntegrationMixin, ModuleStoreTestCase): """ Tests for CourseOverview model. """ @@ -270,6 +271,24 @@ class CourseOverviewTestCase(ModuleStoreTestCase): course = CourseFactory.create(default_store=modulestore_type, run="TestRun", **kwargs) self.check_course_overview_against_course(course) + @ddt.data(True, False) + def test_language_field(self, catalog_integration_enabled): + """ + Test that the language field is not updated from the modulestore + when catalog integration is enabled. In that case, it gets updated + by the sync_course_runs management command, which synchronizes with + the Catalog service. + """ + self.create_catalog_integration(enabled=catalog_integration_enabled) + + course = CourseFactory.create(language='en') + course_overview = CourseOverview.get_from_id(course.id) + + if catalog_integration_enabled: + self.assertNotEqual(course_overview.language, course.language) + else: + self.assertEqual(course_overview.language, course.language) + @ddt.data(ModuleStoreEnum.Type.split, ModuleStoreEnum.Type.mongo) def test_get_non_existent_course(self, modulestore_type): """