Merge pull request #2249 from cpennington/non-ascii-course-images

Fix non-ascii course image names

cms/djangoapps/contentstore/tests/test_utils.py

@@ -1,14 +1,14 @@
 """ Tests for utils. """
-from contentstore import utils
-import mock
 import collections
 import copy
+import mock
 
 from django.test import TestCase
-from xmodule.modulestore.tests.factories import CourseFactory
 from django.test.utils import override_settings
 
+from contentstore import utils
 from xmodule.modulestore import Location
+from xmodule.modulestore.tests.factories import CourseFactory
 
 
 class LMSLinksTestCase(TestCase):
@@ -166,3 +166,22 @@ class CourseImageTestCase(TestCase):
         course = CourseFactory.create(org='edX', course='999')
         url = utils.course_image_url(course)
         self.assertEquals(url, '/c4x/edX/999/asset/{0}'.format(course.course_image))
+
+    def test_non_ascii_image_name(self):
+        # Verify that non-ascii image names are cleaned
+        course = CourseFactory.create(course_image=u'before_\N{SNOWMAN}_after.jpg')
+        self.assertEquals(
+            utils.course_image_url(course),
+            '/c4x/{org}/{course}/asset/before___after.jpg'.format(org=course.location.org, course=course.location.course)
+        )
+
+    def test_spaces_in_image_name(self):
+        # Verify that image names with spaces in them are cleaned
+        course = CourseFactory.create(course_image=u'before after.jpg')
+        self.assertEquals(
+            utils.course_image_url(course),
+            '/c4x/{org}/{course}/asset/before_after.jpg'.format(
+                org=course.location.org,
+                course=course.location.course
+            )
+        )

cms/djangoapps/contentstore/utils.py

@@ -191,7 +191,7 @@ def get_lms_link_for_about_page(location):
 
 def course_image_url(course):
     """Returns the image url for the course."""
-    loc = course.location._replace(tag='c4x', category='asset', name=course.course_image)
+    loc = StaticContent.compute_location(course.location.org, course.location.course, course.course_image)
     path = StaticContent.get_url_path_from_location(loc)
     return path
 

common/lib/xmodule/xmodule/tests/xml/__init__.py

@@ -3,6 +3,7 @@ Xml parsing tests for XModules
 """
 import pprint
 from mock import Mock
+from unittest import TestCase
 
 from xmodule.x_module import XMLParsingSystem, policy_key
 from xmodule.mako_module import MakoDescriptorSystem
@@ -54,7 +55,7 @@ class InMemorySystem(XMLParsingSystem, MakoDescriptorSystem):  # pylint: disable
         return self._descriptors[Location(location).url()]
 
 
-class XModuleXmlImportTest(object):
+class XModuleXmlImportTest(TestCase):
     """Base class for tests that use basic XML parsing"""
     def process_xml(self, xml_import_data):
         """Use the `xml_import_data` to import an :class:`XBlock` from XML."""

common/lib/xmodule/xmodule/tests/xml/factories.py

@@ -119,6 +119,10 @@ class XmlImportFactory(Factory):
 class CourseFactory(XmlImportFactory):
     """Factory for <course> nodes"""
     tag = 'course'
+    org = 'edX'
+    course = 'xml_test_course'
+    name = '101'
+    static_asset_path = 'xml_test_course'
 
 
 class SequenceFactory(XmlImportFactory):

lms/djangoapps/courseware/courses.py

@@ -106,7 +106,7 @@ def course_image_url(course):
     if course.static_asset_path or modulestore().get_modulestore_type(course.location.course_id) == XML_MODULESTORE_TYPE:
         return '/static/' + (course.static_asset_path or getattr(course, 'data_dir', '')) + "/images/course_image.jpg"
     else:
-        loc = course.location.replace(tag='c4x', category='asset', name=course.course_image)
+        loc = StaticContent.compute_location(course.location.org, course.location.course, course.course_image)
         _path = StaticContent.get_url_path_from_location(loc)
         return _path
 

lms/djangoapps/courseware/tests/test_courses.py

@@ -4,12 +4,15 @@ Tests for course access
 """
 import mock
 
-from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from django.http import Http404
 from django.test.utils import override_settings
-from courseware.courses import get_course_by_id, get_course, get_cms_course_link
 from xmodule.modulestore.django import get_default_store_name_for_current_request
+from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory
+from xmodule.tests.xml import factories as xml
+from xmodule.tests.xml import XModuleXmlImportTest
+
+from courseware.courses import get_course_by_id, get_course, get_cms_course_link, course_image_url
 from courseware.tests.tests import TEST_DATA_MONGO_MODULESTORE
 
 
@@ -79,3 +82,56 @@ class CoursesTest(ModuleStoreTestCase):
     )
     def test_default_modulestore_published_mapping(self):
         self.assertEqual(get_default_store_name_for_current_request(), 'default')
+
+
+@override_settings(
+    MODULESTORE=TEST_DATA_MONGO_MODULESTORE, CMS_BASE=CMS_BASE_TEST
+)
+class MongoCourseImageTestCase(ModuleStoreTestCase):
+    """Tests for course image URLs when using a mongo modulestore."""
+
+    def test_get_image_url(self):
+        """Test image URL formatting."""
+        course = CourseFactory.create(org='edX', course='999')
+        self.assertEquals(course_image_url(course), '/c4x/edX/999/asset/{0}'.format(course.course_image))
+
+    def test_non_ascii_image_name(self):
+        # Verify that non-ascii image names are cleaned
+        course = CourseFactory.create(course_image=u'before_\N{SNOWMAN}_after.jpg')
+        self.assertEquals(
+            course_image_url(course),
+            '/c4x/{org}/{course}/asset/before___after.jpg'.format(
+                org=course.location.org,
+                course=course.location.course
+            )
+        )
+
+    def test_spaces_in_image_name(self):
+        # Verify that image names with spaces in them are cleaned
+        course = CourseFactory.create(course_image=u'before after.jpg')
+        self.assertEquals(
+            course_image_url(course),
+            '/c4x/{org}/{course}/asset/before_after.jpg'.format(
+                org=course.location.org,
+                course=course.location.course
+            )
+        )
+
+
+class XmlCourseImageTestCase(XModuleXmlImportTest):
+    """Tests for course image URLs when using an xml modulestore."""
+
+    def test_get_image_url(self):
+        """Test image URL formatting."""
+        course = self.process_xml(xml.CourseFactory.build())
+        self.assertEquals(course_image_url(course), '/static/xml_test_course/images/course_image.jpg')
+
+    def test_non_ascii_image_name(self):
+        # XML Course images are always stored at /images/course_image.jpg
+        course = self.process_xml(xml.CourseFactory.build(course_image=u'before_\N{SNOWMAN}_after.jpg'))
+        self.assertEquals(course_image_url(course), '/static/xml_test_course/images/course_image.jpg')
+
+    def test_spaces_in_image_name(self):
+        # XML Course images are always stored at /images/course_image.jpg
+        course = self.process_xml(xml.CourseFactory.build(course_image=u'before after.jpg'))
+        self.assertEquals(course_image_url(course), '/static/xml_test_course/images/course_image.jpg')