From 2a35410fe4f85ea1828cb55d32c416efae73ec12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A1bor=20Boros?= Date: Tue, 22 Sep 2020 17:40:11 +0200 Subject: [PATCH] Expose banner image URL in course API Banner image URL is exposed on the course list and course details API endpoints. The new `banner_image_url` API field has both the relative and absolute URLs for the image it represents, if it is set. --- lms/djangoapps/course_api/serializers.py | 36 +++++++++++++++++++ .../course_api/tests/test_serializers.py | 11 ++++-- .../0023_courseoverview_banner_image_url.py | 23 ++++++++++++ .../content/course_overviews/models.py | 29 ++++++++++----- .../tests/test_course_overviews.py | 24 ++++++++++++- 5 files changed, 112 insertions(+), 11 deletions(-) create mode 100644 openedx/core/djangoapps/content/course_overviews/migrations/0023_courseoverview_banner_image_url.py diff --git a/lms/djangoapps/course_api/serializers.py b/lms/djangoapps/course_api/serializers.py index 56e69d1e0d..a98534290e 100644 --- a/lms/djangoapps/course_api/serializers.py +++ b/lms/djangoapps/course_api/serializers.py @@ -11,6 +11,7 @@ from django.urls import reverse from rest_framework import serializers from openedx.core.djangoapps.models.course_details import CourseDetails +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.lib.api.fields import AbsoluteURLField @@ -32,6 +33,40 @@ class _MediaSerializer(serializers.Serializer): # pylint: disable=abstract-meth 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) + + uri_absolute = serializers.SerializerMethodField(source="*") + + def get_uri_absolute(self, course_overview): + """ + Convert the media resource's URI to an absolute URI. + """ + uri = getattr(course_overview, self.uri_attribute) + + if not uri: + # Return empty string here, to keep the same + # response type in case uri is empty as well. + return "" + + cdn_applied_uri = 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(cdn_applied_uri) + + class ImageSerializer(serializers.Serializer): # pylint: disable=abstract-method """ Collection of URLs pointing to images of various sizes. @@ -48,6 +83,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 587c42043d..d6ea499998 100644 --- a/lms/djangoapps/course_api/tests/test_serializers.py +++ b/lms/djangoapps/course_api/tests/test_serializers.py @@ -39,15 +39,22 @@ class TestCourseSerializer(CourseApiFactoryMixin, ModuleStoreTestCase): self.honor_user = self.create_user('honor', is_staff=False) self.request_factory = APIRequestFactory() + course_id = u'edX/toy/2012_Fall' + banner_image_uri = u'/c4x/edX/toy/asset/images_course_image.jpg' + banner_image_absolute_uri = u'http://testserver' + banner_image_uri 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_uri, + 'uri_absolute': banner_image_absolute_uri, + }, 'course_image': { 'uri': image_path, }, @@ -74,7 +81,7 @@ class TestCourseSerializer(CourseApiFactoryMixin, ModuleStoreTestCase): '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_courseoverview_banner_image_url.py b/openedx/core/djangoapps/content/course_overviews/migrations/0023_courseoverview_banner_image_url.py new file mode 100644 index 0000000000..1862eb090a --- /dev/null +++ b/openedx/core/djangoapps/content/course_overviews/migrations/0023_courseoverview_banner_image_url.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(), + ), + migrations.AddField( + model_name='historicalcourseoverview', + name='banner_image_url', + field=models.TextField(), + ), + ] diff --git a/openedx/core/djangoapps/content/course_overviews/models.py b/openedx/core/djangoapps/content/course_overviews/models.py index 27198452d0..e2def26cd8 100644 --- a/openedx/core/djangoapps/content/course_overviews/models.py +++ b/openedx/core/djangoapps/content/course_overviews/models.py @@ -63,7 +63,7 @@ class CourseOverview(TimeStampedModel): 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,8 @@ class CourseOverview(TimeStampedModel): announcement = DateTimeField(null=True) # URLs + # Not allowing null per django convention; not sure why many TextFields in this model do allow null + banner_image_url = TextField() course_image_url = TextField() social_sharing_url = TextField(null=True) end_of_course_survey_url = TextField(null=True) @@ -196,6 +198,7 @@ class CourseOverview(TimeStampedModel): course_overview.advertised_start = course.advertised_start course_overview.announcement = course.announcement + course_overview.banner_image_url = course_image_url(course, 'banner_image') course_overview.course_image_url = course_image_url(course) course_overview.social_sharing_url = course.social_sharing_url @@ -728,6 +731,22 @@ class CourseOverview(TimeStampedModel): """ return get_closest_released_language(self.language) if self.language else None + def apply_cdn_to_url(self, image_url): + """ + Applies a new CDN/base URL to the given URLs if CDN configuration is + enabled. + + 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 +757,8 @@ class CourseOverview(TimeStampedModel): 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 05bb64cb22..b327177ab7 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 @@ class CourseOverviewTestCase(CatalogIntegrationMixin, ModuleStoreTestCase, Cache 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, 4, 4), (ModuleStoreEnum.Type.split, 3, 3)) @ddt.unpack def test_versioning(self, modulestore_type, min_mongo_calls, max_mongo_calls): """ @@ -789,6 +789,28 @@ class CourseOverviewImageSetTestCase(ModuleStoreTestCase): 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 CDN is applied for a URL when apply_cdn_to_url called directly. + + Apply CDN/base URL to the given URL if CDN configuration is enabled + and the URL is not 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): """