From 88c7d58313513b6df49875aa5f8a6323dc5306b9 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 8 Dec 2015 17:05:02 -0500 Subject: [PATCH] Modify CourseOverviews to create course image thumbnails. Course teams occasionally upload very large files as their course image. Before this commit, those images would be used directly in the student's dashboard, sometimes leading to MBs worth of image data on that page. With this commit, we now auto-generate small and large thumbnails of configurable size. The Student Dashboard and Course About pages will make use of this new functionality (CourseOverview.image_urls), but the behavior of CourseOverview.course_image_url will not change. Note that the thumbnails are still created in the contentstore, and sit alongside their originals. What's included: 1. Multiple sizes, currently starting with "raw", "small", and "large". This falls back to the current behavior automatically in the case where thumbnails don't exist or this feature has been disabled in configuration. 2. Django admin based configuration for image sizes and whether to enable the functionality at all. Note that to regenerate images, you'd need to wipe the CourseOverviewImageSet model rows -- it doesn't do that automatically. This is partly because it's a very rare operation, and partly because I'm not entirely sure what the longer term invalidation strategy should be in a world where we might potentially have multiple themes. The flexible configuration was intended to allow better customization and theming. 3. The Course About pages also use the new thumbnail functionality, as an example of "large". This is in addition to the "small" used on the student dashboard. Things I'm punting on for now (followup PRs welcome!): 1. Bringing the thumbnails to course discovery. A quick attempt to do so showed that it wasn't getting properly invalidated and updated when publishes happen (so the old image still showed up). It probably has something to do with when we do the re-indexing because it stores this data in elasticsearch, but I'm not going to chase it down right now. 2. Center-cropping. While this is a nice-to-have feature, the behavior in this PR is no worse than what already exists in master in terms of image distortion (letting the browser handle it). 3. Automated invalidation of the images when a new config is created. --- .../xmodule/xmodule/contentstore/content.py | 41 ++- lms/djangoapps/courseware/views.py | 7 +- lms/templates/courseware/course_about.html | 4 +- .../dashboard/_dashboard_course_listing.html | 8 +- .../content/course_overviews/admin.py | 50 ++- .../migrations/0006_courseoverviewimageset.py | 30 ++ .../0007_courseoverviewimageconfig.py | 34 ++ .../content/course_overviews/models.py | 230 +++++++++++- .../content/course_overviews/tests.py | 344 +++++++++++++++++- openedx/core/lib/courses.py | 18 +- 10 files changed, 742 insertions(+), 24 deletions(-) create mode 100644 openedx/core/djangoapps/content/course_overviews/migrations/0006_courseoverviewimageset.py create mode 100644 openedx/core/djangoapps/content/course_overviews/migrations/0007_courseoverviewimageconfig.py diff --git a/common/lib/xmodule/xmodule/contentstore/content.py b/common/lib/xmodule/xmodule/contentstore/content.py index 544cef6b61..9337b176e8 100644 --- a/common/lib/xmodule/xmodule/contentstore/content.py +++ b/common/lib/xmodule/xmodule/contentstore/content.py @@ -39,13 +39,23 @@ class StaticContent(object): return self.location.category == 'thumbnail' @staticmethod - def generate_thumbnail_name(original_name): + def generate_thumbnail_name(original_name, dimensions=None): + """ + - original_name: Name of the asset (typically its location.name) + - dimensions: `None` or a tuple of (width, height) in pixels + """ name_root, ext = os.path.splitext(original_name) if not ext == XASSET_THUMBNAIL_TAIL_NAME: name_root = name_root + ext.replace(u'.', u'-') + + if dimensions: + width, height = dimensions # pylint: disable=unpacking-non-sequence + name_root += "-{}x{}".format(width, height) + return u"{name_root}{extension}".format( name_root=name_root, - extension=XASSET_THUMBNAIL_TAIL_NAME,) + extension=XASSET_THUMBNAIL_TAIL_NAME, + ) @staticmethod def compute_location(course_key, path, revision=None, is_thumbnail=False): @@ -248,11 +258,25 @@ class ContentStore(object): """ raise NotImplementedError - def generate_thumbnail(self, content, tempfile_path=None): + def generate_thumbnail(self, content, tempfile_path=None, dimensions=None): + """Create a thumbnail for a given image. + + Returns a tuple of (StaticContent, AssetKey) + + `content` is the StaticContent representing the image you want to make a + thumbnail out of. + + `tempfile_path` is a string path to the location of a file to read from + in order to grab the image data, instead of relying on `content.data` + + `dimensions` is an optional param that represents (width, height) in + pixels. It defaults to None. + """ thumbnail_content = None # use a naming convention to associate originals with the thumbnail - thumbnail_name = StaticContent.generate_thumbnail_name(content.location.name) - + thumbnail_name = StaticContent.generate_thumbnail_name( + content.location.name, dimensions=dimensions + ) thumbnail_file_location = StaticContent.compute_location( content.location.course_key, thumbnail_name, is_thumbnail=True ) @@ -273,8 +297,11 @@ class ContentStore(object): # I've seen some exceptions from the PIL library when trying to save palletted # PNG files to JPEG. Per the google-universe, they suggest converting to RGB first. im = im.convert('RGB') - size = 128, 128 - im.thumbnail(size, Image.ANTIALIAS) + + if not dimensions: + dimensions = (128, 128) + + im.thumbnail(dimensions, Image.ANTIALIAS) thumbnail_file = StringIO.StringIO() im.save(thumbnail_file, 'JPEG') thumbnail_file.seek(0) diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 72c47b6592..f061e8e937 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -54,6 +54,7 @@ from openedx.core.djangoapps.credit.api import ( is_user_eligible_for_credit, is_credit_course ) +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from courseware.models import StudentModuleHistory from courseware.model_data import FieldDataCache, ScoresClient from .module_render import toc_for_course, get_module_for_descriptor, get_module, get_module_by_usage_id @@ -866,6 +867,9 @@ def course_about(request, course_id): # get prerequisite courses display names pre_requisite_courses = get_prerequisite_courses_display(course) + # Overview + overview = CourseOverview.get_from_id(course.id) + return render_to_response('courseware/course_about.html', { 'course': course, 'staff_access': staff_access, @@ -887,7 +891,8 @@ def course_about(request, course_id): 'disable_courseware_header': True, 'can_add_course_to_cart': can_add_course_to_cart, 'cart_link': reverse('shoppingcart.views.show_cart'), - 'pre_requisite_courses': pre_requisite_courses + 'pre_requisite_courses': pre_requisite_courses, + 'course_image_urls': overview.image_urls, }) diff --git a/lms/templates/courseware/course_about.html b/lms/templates/courseware/course_about.html index 1d009726d4..e16259e3e4 100644 --- a/lms/templates/courseware/course_about.html +++ b/lms/templates/courseware/course_about.html @@ -173,14 +173,14 @@ from openedx.core.lib.courses import course_image_url % if get_course_about_section(request, course, "video"):
- +
%else:
- +
% endif diff --git a/lms/templates/dashboard/_dashboard_course_listing.html b/lms/templates/dashboard/_dashboard_course_listing.html index 0f1337700a..cab9f83c66 100644 --- a/lms/templates/dashboard/_dashboard_course_listing.html +++ b/lms/templates/dashboard/_dashboard_course_listing.html @@ -63,16 +63,16 @@ from student.helpers import ( % if show_courseware_link: % if not is_course_blocked: - ${_('{course_number} {course_name} Home Page').format(course_number=course_overview.number, course_name=course_overview.display_name_with_default) |h} + ${_('{course_number} {course_name} Home Page').format(course_number=course_overview.number, course_name=course_overview.display_name_with_default) |h} % else: - ${_('{course_number} {course_name} Cover Image').format(course_number=course_overview.number, course_name=course_overview.display_name_with_default) |h} + ${_('{course_number} {course_name} Cover Image').format(course_number=course_overview.number, course_name=course_overview.display_name_with_default) |h} % endif % else: - ${_('{course_number} {course_name} Cover Image').format(course_number=course_overview.number, course_name=course_overview.display_name_with_default) | h} + ${_('{course_number} {course_name} Cover Image').format(course_number=course_overview.number, course_name=course_overview.display_name_with_default) | h} % endif % if settings.FEATURES.get('ENABLE_VERIFIED_CERTIFICATES') and course_verified_certs.get('display_mode') != 'audit': @@ -281,7 +281,7 @@ from student.helpers import ( % if credit_status is not None: <%include file="_dashboard_credit_info.html" args="credit_status=credit_status"/> - % endif + % endif % if verification_status.get('status') in [VERIFY_STATUS_NEED_TO_VERIFY, VERIFY_STATUS_SUBMITTED, VERIFY_STATUS_APPROVED, VERIFY_STATUS_NEED_TO_REVERIFY] and not is_course_blocked:
diff --git a/openedx/core/djangoapps/content/course_overviews/admin.py b/openedx/core/djangoapps/content/course_overviews/admin.py index 17f8fa5e77..7b1c90355a 100644 --- a/openedx/core/djangoapps/content/course_overviews/admin.py +++ b/openedx/core/djangoapps/content/course_overviews/admin.py @@ -5,7 +5,8 @@ name, and start dates, but don't actually need to crawl into course content. """ from django.contrib import admin -from .models import CourseOverview +from config_models.admin import ConfigurationModelAdmin +from .models import CourseOverview, CourseOverviewImageConfig, CourseOverviewImageSet class CourseOverviewAdmin(admin.ModelAdmin): @@ -35,4 +36,51 @@ class CourseOverviewAdmin(admin.ModelAdmin): search_fields = ['id', 'display_name'] +class CourseOverviewImageConfigAdmin(ConfigurationModelAdmin): + """ + Basic configuration for CourseOverview Image thumbnails. + + By default this is disabled. If you change the dimensions of the images with + a new config after thumbnails have already been generated, you need to clear + the entries in CourseOverviewImageSet manually for new entries to be + created. + """ + list_display = [ + 'change_date', + 'changed_by', + 'enabled', + 'large_width', + 'large_height', + 'small_width', + 'small_height' + ] + + def get_list_display(self, request): + """ + Restore default list_display behavior. + + ConfigurationModelAdmin overrides this, but in a way that doesn't + respect the ordering. This lets us customize it the usual Django admin + way. + """ + return self.list_display + + +class CourseOverviewImageSetAdmin(admin.ModelAdmin): + """ + Thumbnail images associated with CourseOverviews. This should be used for + debugging purposes only -- e.g. don't edit these values. + """ + list_display = [ + 'course_overview', + 'small_url', + 'large_url', + ] + search_fields = ['course_overview__id'] + readonly_fields = ['course_overview_id'] + fields = ('course_overview_id', 'small_url', 'large_url') + + admin.site.register(CourseOverview, CourseOverviewAdmin) +admin.site.register(CourseOverviewImageConfig, CourseOverviewImageConfigAdmin) +admin.site.register(CourseOverviewImageSet, CourseOverviewImageSetAdmin) diff --git a/openedx/core/djangoapps/content/course_overviews/migrations/0006_courseoverviewimageset.py b/openedx/core/djangoapps/content/course_overviews/migrations/0006_courseoverviewimageset.py new file mode 100644 index 0000000000..609950bbbb --- /dev/null +++ b/openedx/core/djangoapps/content/course_overviews/migrations/0006_courseoverviewimageset.py @@ -0,0 +1,30 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import migrations, models +import django.utils.timezone +import model_utils.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('course_overviews', '0005_delete_courseoverviewgeneratedhistory'), + ] + + operations = [ + migrations.CreateModel( + name='CourseOverviewImageSet', + fields=[ + ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)), + ('created', model_utils.fields.AutoCreatedField(default=django.utils.timezone.now, verbose_name='created', editable=False)), + ('modified', model_utils.fields.AutoLastModifiedField(default=django.utils.timezone.now, verbose_name='modified', editable=False)), + ('small_url', models.TextField(default=b'', blank=True)), + ('large_url', models.TextField(default=b'', blank=True)), + ('course_overview', models.OneToOneField(related_name='image_set', to='course_overviews.CourseOverview')), + ], + options={ + 'abstract': False, + }, + ), + ] diff --git a/openedx/core/djangoapps/content/course_overviews/migrations/0007_courseoverviewimageconfig.py b/openedx/core/djangoapps/content/course_overviews/migrations/0007_courseoverviewimageconfig.py new file mode 100644 index 0000000000..8185fc1783 --- /dev/null +++ b/openedx/core/djangoapps/content/course_overviews/migrations/0007_courseoverviewimageconfig.py @@ -0,0 +1,34 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import migrations, models +import django.db.models.deletion +from django.conf import settings + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('course_overviews', '0006_courseoverviewimageset'), + ] + + operations = [ + migrations.CreateModel( + name='CourseOverviewImageConfig', + fields=[ + ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)), + ('change_date', models.DateTimeField(auto_now_add=True, verbose_name='Change date')), + ('enabled', models.BooleanField(default=False, verbose_name='Enabled')), + ('small_width', models.IntegerField(default=375)), + ('small_height', models.IntegerField(default=200)), + ('large_width', models.IntegerField(default=750)), + ('large_height', models.IntegerField(default=400)), + ('changed_by', models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, editable=False, to=settings.AUTH_USER_MODEL, null=True, verbose_name='Changed by')), + ], + options={ + 'ordering': ('-change_date',), + 'abstract': False, + }, + ), + ] diff --git a/openedx/core/djangoapps/content/course_overviews/models.py b/openedx/core/djangoapps/content/course_overviews/models.py index a4149820d5..659bf21ee2 100644 --- a/openedx/core/djangoapps/content/course_overviews/models.py +++ b/openedx/core/djangoapps/content/course_overviews/models.py @@ -9,11 +9,14 @@ from django.db.models.fields import BooleanField, DateTimeField, DecimalField, T from django.db.utils import IntegrityError from django.template import defaultfilters from django.utils.translation import ugettext -from lms.djangoapps import django_comment_client + +from ccx_keys.locator import CCXLocator from model_utils.models import TimeStampedModel from opaque_keys.edx.keys import CourseKey -from openedx.core.djangoapps.models.course_details import CourseDetails +from config_models.models import ConfigurationModel +from lms.djangoapps import django_comment_client +from openedx.core.djangoapps.models.course_details import CourseDetails from util.date_utils import strftime_localized from xmodule import course_metadata_utils from xmodule.course_module import CourseDescriptor, DEFAULT_START_DATE @@ -21,9 +24,6 @@ from xmodule.error_module import ErrorDescriptor from xmodule.modulestore.django import modulestore from xmodule_django.models import CourseKeyField, UsageKeyField -from ccx_keys.locator import CCXLocator - - log = logging.getLogger(__name__) @@ -213,6 +213,8 @@ class CourseOverview(TimeStampedModel): CourseOverviewTab(tab_id=tab.tab_id, course_overview=course_overview) for tab in course.tabs ]) + CourseOverviewImageSet.create_for_course(course_overview, course) + except IntegrityError: # There is a rare race condition that will occur if # CourseOverview.get_from_id is called while a @@ -256,13 +258,20 @@ class CourseOverview(TimeStampedModel): course from the module store. """ try: - course_overview = cls.objects.get(id=course_id) + course_overview = cls.objects.select_related('image_set').get(id=course_id) if course_overview.version < cls.VERSION: # Throw away old versions of CourseOverview, as they might contain stale data. course_overview.delete() course_overview = None except cls.DoesNotExist: course_overview = None + + # Regenerate the thumbnail images if they're missing (either because + # 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) + return course_overview or cls.load_from_module_store(course_id) def clean_id(self, padding_char='='): @@ -489,6 +498,45 @@ class CourseOverview(TimeStampedModel): return True return False + @property + def image_urls(self): + """ + Return a dict with all known URLs for this course image. + + Current resolutions are: + raw = original upload from the user + small = thumbnail with dimensions CourseOverviewImageConfig.current().small + large = thumbnail with dimensions CourseOverviewImageConfig.current().large + + If no thumbnails exist, the raw (originally uploaded) image will be + returned for all resolutions. + """ + # This is either the raw image that the course team uploaded, or the + # settings.DEFAULT_COURSE_ABOUT_IMAGE_URL if they didn't specify one. + raw_image_url = self.course_image_url + + # Default all sizes to return the raw image if there is no + # CourseOverviewImageSet associated with this CourseOverview. This can + # happen because we're disabled via CourseOverviewImageConfig. + urls = { + 'raw': raw_image_url, + 'small': raw_image_url, + 'large': raw_image_url, + } + + # If we do have a CourseOverviewImageSet, we still default to the raw + # images if our thumbnails are blank (might indicate that there was a + # processing error of some sort while trying to generate thumbnails). + if hasattr(self, 'image_set') and CourseOverviewImageConfig.current().enabled: + urls['small'] = self.image_set.small_url or raw_image_url + urls['large'] = self.image_set.large_url or raw_image_url + + return urls + + def __unicode__(self): + """Represent ourselves with the course key.""" + return unicode(self.id) + class CourseOverviewTab(models.Model): """ @@ -496,3 +544,173 @@ class CourseOverviewTab(models.Model): """ tab_id = models.CharField(max_length=50) course_overview = models.ForeignKey(CourseOverview, db_index=True, related_name="tabs") + + +class CourseOverviewImageSet(TimeStampedModel): + """ + Model for Course overview images. Each column is an image type/size. + + You should basically never use this class directly. Read from + CourseOverview.image_urls instead. + + Special Notes on Deployment/Rollback/Changes: + + 1. By default, this functionality is disabled. To turn it on, you have to + create a CourseOverviewImageConfig entry via Django Admin and select + enabled=True. + + 2. If it is enabled in configuration, it will lazily create thumbnails as + individual CourseOverviews are requested. This is independent of the + CourseOverview's cls.VERSION scheme. This is to better support the use + case where someone might want to change the thumbnail resolutions for + their theme -- we didn't want to tie the code-based data schema of + CourseOverview to configuration changes. + + 3. A CourseOverviewImageSet is automatically deleted when the CourseOverview + it belongs to is deleted. So it will be regenerated whenever there's a + new publish or the CourseOverview schema version changes. It's not + particularly smart about this, and will just re-write the same thumbnails + over and over to the same location without checking to see if there were + changes. + + 4. Just because a CourseOverviewImageSet is successfully created does not + mean that any thumbnails exist. There might have been a processing error, + or there might simply be no source image to create a thumbnail out of. + In this case, accessing CourseOverview.image_urls will return the value + for course.course_image_url for all resolutions. CourseOverviewImageSet + will *not* try to regenerate if there is a model entry with blank values + for the URLs -- the assumption is that either there's no data there or + something has gone wrong and needs fixing in code. + + 5. If you want to change thumbnail resolutions, you need to create a new + CourseOverviewImageConfig with the desired dimensions and then wipe the + values in CourseOverviewImageSet. + + Logical next steps that I punted on for this first cut: + + 1. Converting other parts of the app to use this. + + Our first cut only affects About Pages and the Student Dashboard. But + most places that use course_image_url() should be converted -- e.g. + course discovery, mobile, etc. + + 2. Center cropping the image before scaling. + + This is desirable, but it involves a few edge cases (what the rounding + policy is, what to do with undersized images, etc.) The behavior that + we implemented is at least no worse than what was already there in terms + of distorting images. + + 3. Automatically invalidating entries based on CourseOverviewImageConfig. + + There are two basic paths I can think of for this. The first is to + completely wipe this table when the config changes. The second is to + actually tie the config as a foreign key from this model -- so you could + do the comparison to see if the image_set's config_id matched + CourseOverviewImageConfig.current() and invalidate it if they didn't + match. I punted on this mostly because it's just not something that + happens much at all in practice, there is an understood (if manual) + process to do it, and it can happen in a follow-on PR if anyone is + interested in extending this functionality. + + """ + course_overview = models.OneToOneField(CourseOverview, db_index=True, related_name="image_set") + small_url = models.TextField(blank=True, default="") + large_url = models.TextField(blank=True, default="") + + @classmethod + def create_for_course(cls, course_overview, course=None): + """ + Create thumbnail images for this CourseOverview. + + This will save the CourseOverviewImageSet it creates before it returns. + """ + from openedx.core.lib.courses import create_course_image_thumbnail + + # If image thumbnails are not enabled, do nothing. + config = CourseOverviewImageConfig.current() + if not config.enabled: + return + + # If a course object was provided, use that. Otherwise, pull it from + # CourseOverview's course_id. This happens because sometimes we are + # generated as part of the CourseOverview creation (course is available + # and passed in), and sometimes the CourseOverview already exists. + if not course: + course = modulestore().get_course(course_overview.id) + + image_set = CourseOverviewImageSet(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 + # to being blank. No matter what happens, we don't want to bubble up + # a 500 -- an image_set is always optional. + try: + image_set.small_url = create_course_image_thumbnail(course, config.small) + image_set.large_url = create_course_image_thumbnail(course, config.large) + except Exception: # pylint: disable=broad-except + log.exception( + "Could not create thumbnail for course %s with image %s (small=%s), (large=%s)", + course.id, + course.course_image, + config.small, + config.large + ) + + # Regardless of whether we created thumbnails or not, we need to save + # this record before returning. If no thumbnails were created (there was + # an error or the course has no source course_image), our url fields + # just keep their blank defaults. + try: + image_set.save() + course_overview.image_set = image_set + except (IntegrityError, ValueError): + # In the event of a race condition that tries to save two image sets + # to the same CourseOverview, we'll just silently pass on the one + # that fails. They should be the same data anyway. + # + # The ValueError above is to catch the following error that can + # happen in Django 1.8.4+ if the CourseOverview object fails to save + # (again, due to race condition). + # + # Example: ValueError: save() prohibited to prevent data loss due + # to unsaved related object 'course_overview'.") + pass + + def __unicode__(self): + return u"CourseOverviewImageSet({}, small_url={}, large_url={})".format( + self.course_overview_id, self.small_url, self.large_url + ) + + +class CourseOverviewImageConfig(ConfigurationModel): + """ + This sets the size of the thumbnail images that Course Overviews will generate + to display on the about, info, and student dashboard pages. If you make any + changes to this, you will have to regenerate CourseOverviews in order for it + to take effect. You might want to do this if you're doing precise theming of + your install of edx-platform... but really, you probably don't want to do this + at all at the moment, given how new this is. :-P + """ + # Small thumbnail, for things like the student dashboard + small_width = models.IntegerField(default=375) + small_height = models.IntegerField(default=200) + + # Large thumbnail, for things like the about page + large_width = models.IntegerField(default=750) + large_height = models.IntegerField(default=400) + + @property + def small(self): + """Tuple for small image dimensions in pixels -- (width, height)""" + return (self.small_width, self.small_height) + + @property + def large(self): + """Tuple for large image dimensions in pixels -- (width, height)""" + return (self.large_width, self.large_height) + + def __unicode__(self): + return u"CourseOverviewImageConfig(enabled={}, small={}, large={})".format( + self.enabled, self.small, self.large + ) diff --git a/openedx/core/djangoapps/content/course_overviews/tests.py b/openedx/core/djangoapps/content/course_overviews/tests.py index 51666bad93..a7a15c4b0a 100644 --- a/openedx/core/djangoapps/content/course_overviews/tests.py +++ b/openedx/core/djangoapps/content/course_overviews/tests.py @@ -1,6 +1,7 @@ """ Tests for course_overviews app. """ +from cStringIO import StringIO import datetime import ddt import itertools @@ -8,11 +9,17 @@ import math import mock import pytz +from django.conf import settings +from django.test.utils import override_settings from django.utils import timezone +from PIL import Image from lms.djangoapps.certificates.api import get_active_web_certificate from openedx.core.djangoapps.models.course_details import CourseDetails from openedx.core.lib.courses import course_image_url +from xmodule.assetstore.assetmgr import AssetManager +from xmodule.contentstore.django import contentstore +from xmodule.contentstore.content import StaticContent from xmodule.course_metadata_utils import DEFAULT_START_DATE from xmodule.course_module import ( CATALOG_VISIBILITY_CATALOG_AND_ABOUT, @@ -25,7 +32,7 @@ from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, check_mongo_calls, check_mongo_calls_range -from .models import CourseOverview +from .models import CourseOverview, CourseOverviewImageConfig @ddt.ddt @@ -340,7 +347,7 @@ class CourseOverviewTestCase(ModuleStoreTestCase): course_overview = CourseOverview._create_from_course(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, 4)) @ddt.unpack def test_versioning(self, modulestore_type, min_mongo_calls, max_mongo_calls): """ @@ -485,3 +492,336 @@ class CourseOverviewTestCase(ModuleStoreTestCase): {c.id for c in CourseOverview.get_all_courses(org='TEST_ORG_1')}, {c.id for c in org_courses[1]}, ) + + +@ddt.ddt +class CourseOverviewImageSetTestCase(ModuleStoreTestCase): + """ + Course thumbnail generation tests. + """ + + def setUp(self): + """Create an active CourseOverviewImageConfig with non-default values.""" + self.set_config(True) + super(CourseOverviewImageSetTestCase, self).setUp() + + def set_config(self, enabled): + """ + Enable or disable thumbnail generation config. + + Config models pick the most recent by date created, descending. I delete + entries here because that can sometimes screw up on MySQL, which only + has second-level granularity in this timestamp. + + This uses non-default values for the dimensions. + """ + CourseOverviewImageConfig.objects.all().delete() + CourseOverviewImageConfig.objects.create( + enabled=enabled, + small_width=200, + small_height=100, + large_width=400, + large_height=200 + ) + + @override_settings(DEFAULT_COURSE_ABOUT_IMAGE_URL='default_course.png') + @override_settings(STATIC_URL='static/') + @ddt.data( + *itertools.product( + [ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split], + [None, ''] + ) + ) + @ddt.unpack + def test_no_source_image(self, modulestore_type, course_image): + """ + Tests that we behave as expected if no source image was specified. + """ + # Because we're sending None and '', we expect to get the generic + # fallback URL for course images. + fallback_url = settings.STATIC_URL + settings.DEFAULT_COURSE_ABOUT_IMAGE_URL + course_overview = self._assert_image_urls_all_default(modulestore_type, course_image, fallback_url) + + # Even though there was no source image to generate, we should still + # have a CourseOverviewImageSet object associated with this overview. + self.assertTrue(hasattr(course_overview, 'image_set')) + + @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) + def test_disabled_no_prior_data(self, modulestore_type): + """ + Test behavior when we are disabled and no entries exist. + + 1. No CourseOverviewImageSet will be created. + 2. All resolutions should return the URL of the raw source image. + """ + # Disable model generation using config models... + self.set_config(enabled=False) + + # Since we're disabled, we should just return the raw source image back + # for every resolution in image_urls. + fake_course_image = 'sample_image.png' + course_overview = self._assert_image_urls_all_default(modulestore_type, fake_course_image) + + # Because we are disabled, no image set should have been generated. + self.assertFalse(hasattr(course_overview, 'image_set')) + + @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) + def test_disabled_with_prior_data(self, modulestore_type): + """ + Test behavior when entries have been created but we are disabled. + + This might happen because a strange bug was introduced -- e.g. we + corrupt the images somehow when making thumbnails. Expectations: + + 1. We ignore whatever was created for the thumbnails, and image_urls + returns the same as if no thumbnails had ever been generated. So + basically, we return the raw source image for every resolution. + 2. We keep the CourseOverviewImageSet data around for debugging + purposes. + """ + course_image = "my_course.jpg" + broken_small_url = "I am small!" + broken_large_url = "I am big!" + with self.store.default_store(modulestore_type): + course = CourseFactory.create( + default_store=modulestore_type, course_image=course_image + ) + course_overview_before = CourseOverview.get_from_id(course.id) + + # This initial seeding should create an entry for the image_set. + self.assertTrue(hasattr(course_overview_before, 'image_set')) + + # Now just throw in some fake data to this image set, something that + # couldn't possibly work. + course_overview_before.image_set.small_url = broken_small_url + course_overview_before.image_set.large_url = broken_large_url + course_overview_before.image_set.save() + + # Now disable the thumbnail feature + self.set_config(False) + + # Fetch a new CourseOverview + course_overview_after = CourseOverview.get_from_id(course.id) + + # Assert that the data still exists for debugging purposes + self.assertTrue(hasattr(course_overview_after, 'image_set')) + image_set = course_overview_after.image_set + self.assertEqual(image_set.small_url, broken_small_url) + self.assertEqual(image_set.large_url, broken_large_url) + + # But because we've disabled it, asking for image_urls should give us + # the raw source image for all resolutions, and not our broken images. + expected_url = course_image_url(course) + self.assertEqual( + course_overview_after.image_urls, + { + 'raw': expected_url, + 'small': expected_url, + 'large': expected_url + } + ) + + @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) + def test_error_generating_thumbnails(self, modulestore_type): + """ + Test a scenario where thumbnails cannot be generated. + + We need to make sure that: + + 1. We don't cause any 500s to leak out. A failure to generate thumbnails + should never cause CourseOverview generation to fail. + 2. We return the raw course image for all resolutions. + 3. We don't kill our CPU by trying over and over again. + """ + with mock.patch('openedx.core.lib.courses.create_course_image_thumbnail') as patched_create_thumbnail: + # Strictly speaking, this would fail anyway because there's no data + # backing sample_image.png, but we're going to make the side-effect + # more dramatic. ;-) + fake_course_image = 'sample_image.png' + patched_create_thumbnail.side_effect = Exception("Kaboom!") + + # This will generate a CourseOverview and verify that we get the + # source image back for all resolutions. + course_overview = self._assert_image_urls_all_default(modulestore_type, fake_course_image) + + # Make sure we were called (i.e. we tried to create the thumbnail) + patched_create_thumbnail.assert_called() + + # Now an image set does exist, even though it only has blank values for + # the small and large urls. + self.assertTrue(hasattr(course_overview, 'image_set')) + self.assertEqual(course_overview.image_set.small_url, '') + self.assertEqual(course_overview.image_set.large_url, '') + + # The next time we create a CourseOverview, the images are explicitly + # *not* regenerated. + with mock.patch('openedx.core.lib.courses.create_course_image_thumbnail') as patched_create_thumbnail: + course_overview = CourseOverview.get_from_id(course_overview.id) + patched_create_thumbnail.assert_not_called() + + @ddt.data( + *itertools.product( + [ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split], + [True, False], + ) + ) + @ddt.unpack + def test_happy_path(self, modulestore_type, create_after_overview): + """ + What happens when everything works like we expect it to. + + If `create_after_overview` is True, we will temporarily disable + thumbnail creation so that the initial CourseOverview is created without + an image_set, and the CourseOverviewImageSet is created afterwards. If + `create_after_overview` is False, we'll create the CourseOverviewImageSet + at the same time as the CourseOverview. + """ + # Create a real (oversized) image... + image = Image.new("RGB", (800, 400), "blue") + image_buff = StringIO() + image.save(image_buff, format="JPEG") + image_buff.seek(0) + image_name = "big_course_image.jpeg" + + with self.store.default_store(modulestore_type): + course = CourseFactory.create( + default_store=modulestore_type, course_image=image_name + ) + + # Save a real image here... + course_image_asset_key = StaticContent.compute_location(course.id, course.course_image) + course_image_content = StaticContent(course_image_asset_key, image_name, 'image/jpeg', image_buff) + contentstore().save(course_image_content) + + # If create_after_overview is True, disable thumbnail generation so + # that the CourseOverview object is created and saved without an + # image_set at first (it will be lazily created later). + if create_after_overview: + self.set_config(enabled=False) + + # Now generate the CourseOverview... + course_overview = CourseOverview.get_from_id(course.id) + + # If create_after_overview is True, no image_set exists yet. Verify + # that, then switch config back over to True and it should lazily + # create the image_set on the next get_from_id() call. + if create_after_overview: + self.assertFalse(hasattr(course_overview, 'image_set')) + self.set_config(enabled=True) + course_overview = CourseOverview.get_from_id(course.id) + + self.assertTrue(hasattr(course_overview, 'image_set')) + image_urls = course_overview.image_urls + config = CourseOverviewImageConfig.current() + + # Make sure the thumbnail names come out as expected... + self.assertTrue(image_urls['raw'].endswith('big_course_image.jpeg')) + self.assertTrue(image_urls['small'].endswith('big_course_image-jpeg-{}x{}.jpg'.format(*config.small))) + self.assertTrue(image_urls['large'].endswith('big_course_image-jpeg-{}x{}.jpg'.format(*config.large))) + + # Now make sure our thumbnails are of the sizes we expect... + for image_url, expected_size in [(image_urls['small'], config.small), (image_urls['large'], config.large)]: + image_key = StaticContent.get_location_from_path(image_url) + image_content = AssetManager.find(image_key) + image = Image.open(StringIO(image_content.data)) + self.assertEqual(image.size, expected_size) + + @ddt.data( + (800, 400), # Larger than both, correct ratio + (800, 600), # Larger than both, incorrect ratio + (300, 150), # In between small and large, correct ratio + (300, 180), # In between small and large, incorrect ratio + (100, 50), # Smaller than both, correct ratio + (100, 80), # Smaller than both, incorrect ratio + (800, 20), # Bizarrely wide + (20, 800), # Bizarrely tall + ) + def test_different_resolutions(self, src_dimensions): + """ + Test various resolutions of images to make thumbnails of. + + Note that our test sizes are small=(200, 100) and large=(400, 200). + + 1. Images should won't be blown up if it's too small, so a (100, 50) + resolution image will remain (100, 50). + 2. However, images *will* be converted using our format and quality + settings (JPEG, 75% -- the PIL default). This is because images with + relatively small dimensions not compressed properly. + 3. Image thumbnail naming will maintain the naming convention of the + target resolution, even if the image was not actually scaled to that + size (i.e. it was already smaller). This is mostly because it's + simpler to be consistent, but it also lets us more easily tell which + configuration a thumbnail was created under. + """ + # Create a source image... + image = Image.new("RGB", src_dimensions, "blue") + image_buff = StringIO() + image.save(image_buff, format="PNG") + image_buff.seek(0) + image_name = "src_course_image.png" + + course = CourseFactory.create(course_image=image_name) + + # Save the image to the contentstore... + course_image_asset_key = StaticContent.compute_location(course.id, course.course_image) + course_image_content = StaticContent(course_image_asset_key, image_name, 'image/png', image_buff) + contentstore().save(course_image_content) + + # Now generate the CourseOverview... + config = CourseOverviewImageConfig.current() + course_overview = CourseOverview.get_from_id(course.id) + image_urls = course_overview.image_urls + + for image_url, target in [(image_urls['small'], config.small), (image_urls['large'], config.large)]: + image_key = StaticContent.get_location_from_path(image_url) + image_content = AssetManager.find(image_key) + image = Image.open(StringIO(image_content.data)) + + # Naming convention for thumbnail + self.assertTrue(image_url.endswith('src_course_image-png-{}x{}.jpg'.format(*target))) + + # Actual thumbnail data + src_x, src_y = src_dimensions + target_x, target_y = target + image_x, image_y = image.size + + # I'm basically going to assume the image library knows how to do + # the right thing in terms of handling aspect ratio. We're just + # going to make sure that small images aren't blown up, and that + # we never exceed our target sizes + self.assertLessEqual(image_x, target_x) + self.assertLessEqual(image_y, target_y) + + if src_x < target_x and src_y < target_y: + self.assertEqual(src_x, image_x) + self.assertEqual(src_y, image_y) + + def _assert_image_urls_all_default(self, modulestore_type, raw_course_image_name, expected_url=None): + """ + Helper for asserting that all image_urls are defaulting to a particular value. + + Returns the CourseOverview created. This function is useful when you + know that the thumbnail generation process is going to fail in some way + (e.g. unspecified source image, disabled config, runtime error) and want + to verify that all the image URLs are a certain expected value (either + the source image, or the fallback URL). + """ + with self.store.default_store(modulestore_type): + course = CourseFactory.create( + default_store=modulestore_type, course_image=raw_course_image_name + ) + if expected_url is None: + expected_url = course_image_url(course) + + course_overview = CourseOverview.get_from_id(course.id) + + # All the URLs that come back should be for the expected_url + self.assertEqual( + course_overview.image_urls, + { + 'raw': expected_url, + 'small': expected_url, + 'large': expected_url, + } + ) + return course_overview diff --git a/openedx/core/lib/courses.py b/openedx/core/lib/courses.py index f420c1f54f..2936cc93d8 100644 --- a/openedx/core/lib/courses.py +++ b/openedx/core/lib/courses.py @@ -3,8 +3,10 @@ Common utility functions related to courses. """ from django.conf import settings -from xmodule.modulestore.django import modulestore +from xmodule.assetstore.assetmgr import AssetManager from xmodule.contentstore.content import StaticContent +from xmodule.contentstore.django import contentstore +from xmodule.modulestore.django import modulestore from xmodule.modulestore import ModuleStoreEnum @@ -27,4 +29,18 @@ def course_image_url(course): else: loc = StaticContent.compute_location(course.id, course.course_image) url = StaticContent.serialize_asset_key_with_slash(loc) + return url + + +def create_course_image_thumbnail(course, dimensions): + """Create a course image thumbnail and return the URL. + + - dimensions is a tuple of (width, height) + """ + course_image_asset_key = StaticContent.compute_location(course.id, course.course_image) + course_image = AssetManager.find(course_image_asset_key) # a StaticContent obj + + _content, thumb_loc = contentstore().generate_thumbnail(course_image, dimensions=dimensions) + + return StaticContent.serialize_asset_key_with_slash(thumb_loc)