Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions lms/djangoapps/course_api/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
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


Expand All @@ -32,6 +33,40 @@ 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)

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")}

@bradenmacdonald bradenmacdonald Nov 6, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this hack is required because you're declaring the field within this method, instead of as a member of this serializer.

I think that if you just declare the field like this:

class _AbsolutMediaSerializer(_MediaSerializer):
    requires_context = True
    url = AbsoluteURLField(source="get_url")

    def get_url(self):
        ...

then it should work without context hacks.

Edit: Hmm, never mind I guess that won't work because it won't call the method on the serializer. Maybe you can override __init__ to get data and call apply_cdn_to_url on the data there? I think that's cleaner than overriding __call__ and passing the context around like this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this cannot be done, because the BaseSerializer does not set the self.initial_data if data argument was empty. Although serializers has a get_initial method, the method will return an empty OrderedDict since the serializer did not receive data at all. So we need to pass the context around, but I'm opened to other ideas as well. When I did a research in this topic when I opened the PR, I did not find any other solution which is cleaner.


return field.to_representation(cdn_applied_uri)


class ImageSerializer(serializers.Serializer): # pylint: disable=abstract-method
"""
Collection of URLs pointing to images of various sizes.
Expand All @@ -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')
Expand Down
11 changes: 9 additions & 2 deletions lms/djangoapps/course_api/tests/test_serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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_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,
},
Expand All @@ -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):
Expand Down
Original file line number Diff line number Diff line change
@@ -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(),
),
]
29 changes: 21 additions & 8 deletions openedx/core/djangoapps/content/course_overviews/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -86,6 +86,8 @@ class Meta(object):
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()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this platform enhancement.
Where may I find documentation on how a "banner image" differs from a "course image"?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where may I find documentation on how a "banner image" differs from a "course image"?

@nasthagiri I see some context here, but not sure that it qualifies as "documentation" : https://github.com/edx/edx-platform/blob/7afee25ce2150443eefe0ae60b81f3e4f6e61900/common/lib/xmodule/xmodule/course_module.py#L589-L609

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a consumer perspective, it would be useful to know when the difference between the 2 images. Perhaps it would help to describe where each image is displayed?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @gabor-boros - can you answer that question ^ ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nasthagiri & @bradenmacdonald Apologies for replying that late, I probably missed the notification about this comment. As far as I can see, the banner_image is part of the extended_course_details. The original addition was part of edx@f7f281b as far as I can see, but I cannot find more information about that either.

From consumer perspective - in my mind - the difference is that the course image is kind of a thumbnail of a course while the banner image is like a cover image. So something like this:

Screenshot 2020-12-08 at 17 24 17

course_image_url = TextField()
social_sharing_url = TextField(null=True)
end_of_course_survey_url = TextField(null=True)
Expand Down Expand Up @@ -196,6 +198,7 @@ def _create_or_update(cls, course):
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

Expand Down Expand Up @@ -728,6 +731,22 @@ 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):
"""
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.
Expand All @@ -738,14 +757,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()
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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, 4, 4), (ModuleStoreEnum.Type.split, 3, 3))
@ddt.unpack
def test_versioning(self, modulestore_type, min_mongo_calls, max_mongo_calls):
"""
Expand Down Expand Up @@ -789,6 +789,28 @@ 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 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))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test has the same docstring as the previous test and it's hard to tell the difference. It took me a while to figure out that this is testing a slightly different method. Plus it doesn't actually test what it says in the docstring, because this test case doesn't test the case where start_url is already an absolute URL.

Maybe combine the tests, or change the docstring to say "Test the apply_cdn_to_url method directly when used with a single URL"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it seems I overlooked the docstring here. Thanks for pointing out! I'll fix this as well

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you forgot to address this review comment ^

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you are totally right. I'm doing it just right now. Sorry about that.


@ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
def test_error_generating_thumbnails(self, modulestore_type):
"""
Expand Down