Merge pull request #1293 from MITx/feature/christina/misc

Fix bug related to creation of thumbnails for non-image files.

cms/djangoapps/contentstore/views.py

@@ -760,14 +760,16 @@ def upload_asset(request, org, course, coursename):
     content_loc = StaticContent.compute_location(org, course, filename)
     content = StaticContent(content_loc, filename, mime_type, filedata)
 
-    # first let's save a thumbnail so we can get back a thumbnail location
-    thumbnail_content = contentstore().generate_thumbnail(content)
+    # first let's see if a thumbnail can be created
+    (thumbnail_content, thumbnail_location) = contentstore().generate_thumbnail(content)
 
+    # delete cached thumbnail even if one couldn't be created this time (else the old thumbnail will continue to show)
+    del_cached_content(thumbnail_location)
+    # now store thumbnail location only if we could create it
     if thumbnail_content is not None:
-        content.thumbnail_location = thumbnail_content.location
-        del_cached_content(thumbnail_content.location)
+        content.thumbnail_location = thumbnail_location
 
-    #then commit the content 
+    #then commit the content
     contentstore().save(content)
     del_cached_content(content.location)
 
@@ -777,7 +779,7 @@ def upload_asset(request, org, course, coursename):
     response_payload = {'displayname' : content.name, 
         'uploadDate' : get_date_display(readback.last_modified_at), 
         'url' : StaticContent.get_url_path_from_location(content.location),
-        'thumb_url' : StaticContent.get_url_path_from_location(thumbnail_content.location) if thumbnail_content is not None else None,
+        'thumb_url' : StaticContent.get_url_path_from_location(thumbnail_location) if thumbnail_content is not None else None,
         'msg' : 'Upload completed'
         }
 

common/lib/xmodule/xmodule/contentstore/content.py

@@ -18,7 +18,7 @@ class StaticContent(object):
         self.content_type = content_type
         self.data = data
         self.last_modified_at = last_modified_at
-        self.thumbnail_location = Location(thumbnail_location)
+        self.thumbnail_location = Location(thumbnail_location) if thumbnail_location is not None else None
         # optional information about where this file was imported from. This is needed to support import/export 
         # cycles
         self.import_path = import_path
@@ -113,6 +113,12 @@ class ContentStore(object):
 
     def generate_thumbnail(self, content):
         thumbnail_content = None
+        # use a naming convention to associate originals with the thumbnail
+        thumbnail_name = StaticContent.generate_thumbnail_name(content.location.name)
+
+        thumbnail_file_location = StaticContent.compute_location(content.location.org, content.location.course,
+            thumbnail_name, is_thumbnail = True)
+
         # if we're uploading an image, then let's generate a thumbnail so that we can
         # serve it up when needed without having to rescale on the fly
         if content.content_type is not None and content.content_type.split('/')[0] == 'image':
@@ -131,13 +137,8 @@ class ContentStore(object):
                 thumbnail_file = StringIO.StringIO()
                 im.save(thumbnail_file, 'JPEG')
                 thumbnail_file.seek(0)
-            
-                # use a naming convention to associate originals with the thumbnail
-                thumbnail_name = StaticContent.generate_thumbnail_name(content.location.name)
 
-                # then just store this thumbnail as any other piece of content
-                thumbnail_file_location = StaticContent.compute_location(content.location.org, content.location.course, 
-                                                                                  thumbnail_name, is_thumbnail = True)
+                # store this thumbnail as any other piece of content
                 thumbnail_content = StaticContent(thumbnail_file_location, thumbnail_name, 
                                                   'image/jpeg', thumbnail_file)
 
@@ -147,7 +148,7 @@ class ContentStore(object):
                 # log and continue as thumbnails are generally considered as optional
                 logging.exception("Failed to generate thumbnail for {0}. Exception: {1}".format(content.location, str(e)))
 
-        return thumbnail_content
+        return thumbnail_content, thumbnail_file_location
 
 
 

common/lib/xmodule/xmodule/modulestore/xml_importer.py

@@ -35,10 +35,10 @@ def import_static_content(modules, course_loc, course_data_path, static_content_
                 content = StaticContent(content_loc, filename, mime_type, data, import_path = fullname_with_subpath)
 
                 # first let's save a thumbnail so we can get back a thumbnail location
-                thumbnail_content = static_content_store.generate_thumbnail(content)
+                (thumbnail_content, thumbnail_location) = static_content_store.generate_thumbnail(content)
 
                 if thumbnail_content is not None:
-                    content.thumbnail_location = thumbnail_content.location
+                    content.thumbnail_location = thumbnail_location
 
                 #then commit the content
                 static_content_store.save(content)
@@ -69,10 +69,10 @@ def verify_content_links(module, base_dir, static_content_store, link, remap_dic
                 content = StaticContent(content_loc, filename, mime_type, data, import_path = path) 
 
                 # first let's save a thumbnail so we can get back a thumbnail location
-                thumbnail_content = static_content_store.generate_thumbnail(content)
+                (thumbnail_content, thumbnail_location) = static_content_store.generate_thumbnail(content)
 
                 if thumbnail_content is not None:
-                    content.thumbnail_location = thumbnail_content.location
+                    content.thumbnail_location = thumbnail_location
 
                 #then commit the content
                 static_content_store.save(content)   

common/lib/xmodule/xmodule/tests/test_content.py

+import unittest
+from xmodule.contentstore.content import StaticContent
+from xmodule.contentstore.content import ContentStore
+from xmodule.modulestore import Location
+
+class Content:
+    def __init__(self, location, content_type):
+        self.location = location
+        self.content_type = content_type
+
+class ContentTest(unittest.TestCase):
+    def test_thumbnail_none(self):
+        # We had a bug where a thumbnail location of None was getting transformed into a Location tuple, with
+        # all elements being None. It is important that the location be just None for rendering.
+        content = StaticContent('loc', 'name', 'content_type', 'data', None, None, None)
+        self.assertIsNone(content.thumbnail_location)
+
+        content = StaticContent('loc', 'name', 'content_type', 'data')
+        self.assertIsNone(content.thumbnail_location)
+    def test_generate_thumbnail_nonimage(self):
+        contentStore = ContentStore()
+        content = Content(Location(u'c4x', u'mitX', u'800', u'asset', u'monsters.jpg'), None)
+        (thumbnail_content, thumbnail_file_location) = contentStore.generate_thumbnail(content)
+        self.assertIsNone(thumbnail_content)
+        self.assertEqual(Location(u'c4x', u'mitX', u'800', u'thumbnail', u'monsters.jpg'), thumbnail_file_location)
\ No newline at end of file