Merge pull request #10898 from edx/ormsbee/dashboard_thumbnails

Modify CourseOverviews to create course image thumbnails.

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)

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

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"):
       <a href="#video-modal" class="media" rel="leanModal">
         <div class="hero">
-          <img src="${course_image_url(course)}" alt="" />
+          <img src="${course_image_urls['large']}" alt="" />
           <div class="play-intro"></div>
         </div>
       </a>
       %else:
       <div class="media">
         <div class="hero">
-          <img src="${course_image_url(course)}" alt="" />
+          <img src="${course_image_urls['large']}" alt="" />
         </div>
       </div>
       % endif

lms/templates/dashboard/_dashboard_course_listing.html

@@ -63,16 +63,16 @@ from student.helpers import (
       % if show_courseware_link:
         % if not is_course_blocked:
             <a href="${course_target}" data-course-key="${enrollment.course_id}" class="cover">
-              <img src="${course_overview.course_image_url}" class="course-image" alt="${_('{course_number} {course_name} Home Page').format(course_number=course_overview.number, course_name=course_overview.display_name_with_default) |h}" />
+              <img src="${course_overview.image_urls['small']}" class="course-image" alt="${_('{course_number} {course_name} Home Page').format(course_number=course_overview.number, course_name=course_overview.display_name_with_default) |h}" />
             </a>
         % else:
             <a class="fade-cover">
-              <img src="${course_overview.course_image_url}" class="course-image" alt="${_('{course_number} {course_name} Cover Image').format(course_number=course_overview.number, course_name=course_overview.display_name_with_default) |h}" />
+              <img src="${course_overview.image_urls['small']}" class="course-image" alt="${_('{course_number} {course_name} Cover Image').format(course_number=course_overview.number, course_name=course_overview.display_name_with_default) |h}" />
             </a>
         % endif
       % else:
         <a class="cover">
-          <img src="${course_overview.course_image_url}" class="course-image" alt="${_('{course_number} {course_name} Cover Image').format(course_number=course_overview.number, course_name=course_overview.display_name_with_default) | h}" />
+          <img src="${course_overview.image_urls['small']}" class="course-image" alt="${_('{course_number} {course_name} Cover Image').format(course_number=course_overview.number, course_name=course_overview.display_name_with_default) | h}" />
         </a>
       % 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:
         <div class="message message-status wrapper-message-primary is-shown">

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)

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,
+            },
+        ),
+    ]

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,
+            },
+        ),
+    ]

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
+        )

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

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)