diff --git a/lms/djangoapps/course_api/serializers.py b/lms/djangoapps/course_api/serializers.py index 931fbe83f8cc..84da7ab23adc 100644 --- a/lms/djangoapps/course_api/serializers.py +++ b/lms/djangoapps/course_api/serializers.py @@ -31,6 +31,38 @@ def get_uri(self, course_overview): return getattr(course_overview, self.uri_attribute) +class _AbsolutMediaSerializer(_MediaSerializer): # pylint: disable=abstract-method + """ + Nested serializer to represent a media object and its absolute path. + """ + requires_context = True + + def __call__(self, serializer_field): + self.context = serializer_field.context + return super(self).__call__(serializer_field) + + url = serializers.SerializerMethodField(source="*") + + def get_url(self, course_overview): + """ + Convert the media resource's URI to an absolute URL + """ + uri = getattr(course_overview, self.uri_attribute) + + if not uri: + return + + url = course_overview.apply_cdn_to_url(uri) + field = AbsoluteURLField() + + # In order to use the AbsoluteURLField to have the same + # behaviour what ImageSerializer provides, we need to set + # the request for the field + field._context = {"request": self.context.get("request")} + + return field.to_representation(url) + + class ImageSerializer(serializers.Serializer): # pylint: disable=abstract-method """ Collection of URLs pointing to images of various sizes. @@ -47,6 +79,7 @@ class _CourseApiMediaCollectionSerializer(serializers.Serializer): # pylint: di """ Nested serializer to represent a collection of media objects """ + banner_image = _AbsolutMediaSerializer(source='*', uri_attribute='banner_image_url') course_image = _MediaSerializer(source='*', uri_attribute='course_image_url') course_video = _MediaSerializer(source='*', uri_attribute='course_video_url') image = ImageSerializer(source='image_urls') diff --git a/lms/djangoapps/course_api/tests/test_serializers.py b/lms/djangoapps/course_api/tests/test_serializers.py index 587c42043d6f..84b296e186a3 100644 --- a/lms/djangoapps/course_api/tests/test_serializers.py +++ b/lms/djangoapps/course_api/tests/test_serializers.py @@ -39,15 +39,22 @@ def setUp(self): self.honor_user = self.create_user('honor', is_staff=False) self.request_factory = APIRequestFactory() + course_id = u'edX/toy/2012_Fall' + banner_image_path = u'/c4x/edX/toy/asset/images_course_image.jpg' + banner_image_url = u'http://testserver' + banner_image_path image_path = u'/c4x/edX/toy/asset/just_a_test.jpg' image_url = u'http://testserver' + image_path self.expected_data = { - 'id': u'edX/toy/2012_Fall', + 'id': course_id, 'name': u'Toy Course', 'number': u'toy', 'org': u'edX', 'short_description': u'A course about toys.', 'media': { + 'banner_image': { + 'uri': banner_image_path, + 'url': banner_image_url, + }, 'course_image': { 'uri': image_path, }, @@ -74,7 +81,7 @@ def setUp(self): 'invitation_only': False, # 'course_id' is a deprecated field, please use 'id' instead. - 'course_id': u'edX/toy/2012_Fall', + 'course_id': course_id, } def _get_request(self, user=None): diff --git a/openedx/core/djangoapps/content/course_overviews/migrations/0023_auto_20200922_1245.py b/openedx/core/djangoapps/content/course_overviews/migrations/0023_auto_20200922_1245.py new file mode 100644 index 000000000000..1a5d829af909 --- /dev/null +++ b/openedx/core/djangoapps/content/course_overviews/migrations/0023_auto_20200922_1245.py @@ -0,0 +1,23 @@ +# Generated by Django 2.2.16 on 2020-09-22 12:45 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('course_overviews', '0022_courseoverviewtab_is_hidden'), + ] + + operations = [ + migrations.AddField( + model_name='courseoverview', + name='banner_image_url', + field=models.TextField(null=True), + ), + migrations.AddField( + model_name='historicalcourseoverview', + name='banner_image_url', + field=models.TextField(null=True), + ), + ] diff --git a/openedx/core/djangoapps/content/course_overviews/models.py b/openedx/core/djangoapps/content/course_overviews/models.py index 27198452d06a..9176b717de78 100644 --- a/openedx/core/djangoapps/content/course_overviews/models.py +++ b/openedx/core/djangoapps/content/course_overviews/models.py @@ -63,7 +63,7 @@ class Meta(object): app_label = 'course_overviews' # IMPORTANT: Bump this whenever you modify this model and/or add a migration. - VERSION = 11 # this one goes to eleven + VERSION = 12 # this one goes to thirteen # Cache entry versioning. version = IntegerField() @@ -86,6 +86,7 @@ class Meta(object): announcement = DateTimeField(null=True) # URLs + banner_image_url = TextField(null=True) course_image_url = TextField() social_sharing_url = TextField(null=True) end_of_course_survey_url = TextField(null=True) @@ -196,6 +197,7 @@ def _create_or_update(cls, course): course_overview.advertised_start = course.advertised_start course_overview.announcement = course.announcement + course_overview.banner_image_url = CourseDetails.fetch_banner_image_url(course.id) course_overview.course_image_url = course_image_url(course) course_overview.social_sharing_url = course.social_sharing_url @@ -728,6 +730,21 @@ def closest_released_language(self): """ return get_closest_released_language(self.language) if self.language else None + def apply_cdn_to_url(self, image_url): + """ + Given a resolution -> url, return a copy with CDN applied. + + If CDN does not exist or is disabled, just returns the original. The + URL that we store in CourseOverviewImageSet is already top level path, + so we don't need to go through the /static remapping magic that happens + with other course assets. We just need to add the CDN server if appropriate. + """ + cdn_config = AssetBaseUrlConfig.current() + if not cdn_config.enabled: + return image_url + + return self._apply_cdn_to_url(image_url, cdn_config.base_url) + def apply_cdn_to_urls(self, image_urls): """ Given a dict of resolutions -> urls, return a copy with CDN applied. @@ -738,14 +755,8 @@ def apply_cdn_to_urls(self, image_urls): happens with other course assets. We just need to add the CDN server if appropriate. """ - cdn_config = AssetBaseUrlConfig.current() - if not cdn_config.enabled: - return image_urls - - base_url = cdn_config.base_url - return { - resolution: self._apply_cdn_to_url(url, base_url) + resolution: self.apply_cdn_to_url(url) for resolution, url in image_urls.items() } 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 05bb64cb2243..1bc5d413740d 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 @@ -382,7 +382,7 @@ def test_malformed_grading_policy(self): 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)) + @ddt.data((ModuleStoreEnum.Type.mongo, 5, 5), (ModuleStoreEnum.Type.split, 3, 3)) @ddt.unpack def test_versioning(self, modulestore_type, min_mongo_calls, max_mongo_calls): """ @@ -789,6 +789,25 @@ def test_cdn_with_external_image(self, modulestore_type): self.assertTrue(modified_urls['small'].startswith(expected_cdn_url)) self.assertEqual(modified_urls['large'], start_urls['large']) + @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) + def test_cdn_with_a_single_external_image(self, modulestore_type): + """ + Test that we return CDN prefixed URLs unless they're absolute. + """ + with self.store.default_store(modulestore_type): + course = CourseFactory.create(default_store=modulestore_type) + overview = CourseOverview.get_from_id(course.id) + + # Now enable the CDN... + AssetBaseUrlConfig.objects.create(enabled=True, base_url='fakecdn.edx.org') + expected_cdn_url = "//fakecdn.edx.org" + + start_url = "/static/overview.png" + modified_url = overview.apply_cdn_to_url(start_url) + + self.assertNotEqual(start_url, modified_url) + self.assertTrue(modified_url.startswith(expected_cdn_url)) + @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) def test_error_generating_thumbnails(self, modulestore_type): """ diff --git a/openedx/core/djangoapps/models/course_details.py b/openedx/core/djangoapps/models/course_details.py index e3624f55cb04..773467f2989f 100644 --- a/openedx/core/djangoapps/models/course_details.py +++ b/openedx/core/djangoapps/models/course_details.py @@ -91,6 +91,13 @@ def fetch_about_attribute(cls, course_key, attribute): value = None return value + @classmethod + def fetch_banner_image_url(cls, course_key): + """ + Fetch only the banner image url of the given course from the descriptor. + """ + return course_image_url(modulestore().get_course(course_key), 'banner_image') + @classmethod def fetch(cls, course_key): """ diff --git a/openedx/core/djangoapps/models/tests/test_course_details.py b/openedx/core/djangoapps/models/tests/test_course_details.py index c79e2ca19c70..082246ddae05 100644 --- a/openedx/core/djangoapps/models/tests/test_course_details.py +++ b/openedx/core/djangoapps/models/tests/test_course_details.py @@ -179,3 +179,12 @@ def test_fetch_video(self): self.assertEqual(CourseDetails.fetch_youtube_video_id(self.course.id), video_value) video_url = CourseDetails.fetch_video_url(self.course.id) self.assertRegex(video_url, r'http://.*{}'.format(video_value)) + + def test_fetch_banner_image_url(self): + course_details = CourseDetails.fetch(self.course.id) + banner_image_asset_path = CourseDetails.fetch_banner_image_url(self.course.id) + + self.assertEqual( + course_details.banner_image_asset_path, + banner_image_asset_path + )