From c4ea338035ae8745cc929184d295da91697ff62c Mon Sep 17 00:00:00 2001 From: Carson Gee Date: Wed, 16 Apr 2014 18:05:33 -0400 Subject: [PATCH 1/3] Additional logic to handle more course_image URL edge cases This changes logic to allow more missed use cases of course_image to work properly. The cases are: . XML courses with the course_image attribute set . Mongo courses that are imported without a contentstore . Mongo courses that have course_image set but don't have a content store It also exports default images_static_course.jpg to images/static_course.jpg to handle a use case where a course author uploaded an image to the default location in studio without using the studio interface for adding course images, they then export the course, and then import it without a contentstore --- .../xmodule/modulestore/tests/test_mongo.py | 50 +++++++++++++++++- .../xmodule/modulestore/xml_exporter.py | 22 ++++++++ .../simple/static/images_course_image.jpg | Bin 0 -> 547 bytes common/test/data/toy/course/2012_Fall.xml | 2 +- common/test/data/toy/static/just_a_test.jpg | Bin 0 -> 547 bytes lms/djangoapps/courseware/courses.py | 9 +++- .../courseware/tests/test_courses.py | 29 ++++++++-- 7 files changed, 105 insertions(+), 7 deletions(-) create mode 100644 common/test/data/simple/static/images_course_image.jpg create mode 100644 common/test/data/toy/static/just_a_test.jpg diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py index 0663f9fdea..8ed44a1f5e 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py @@ -1,11 +1,14 @@ from pprint import pprint # pylint: disable=E0611 from nose.tools import assert_equals, assert_raises, \ - assert_not_equals, assert_false + assert_not_equals, assert_false, assert_true from itertools import ifilter # pylint: enable=E0611 +from path import path import pymongo import logging +import shutil +from tempfile import mkdtemp from uuid import uuid4 from xblock.fields import Scope @@ -16,6 +19,7 @@ from xmodule.tests import DATA_DIR from xmodule.modulestore import Location, MONGO_MODULESTORE_TYPE from xmodule.modulestore.mongo import MongoModuleStore, MongoKeyValueStore from xmodule.modulestore.draft import DraftModuleStore +from xmodule.modulestore.xml_exporter import export_to_xml from xmodule.modulestore.xml_importer import import_from_xml, perform_xlint from xmodule.contentstore.mongo import MongoContentStore @@ -310,6 +314,50 @@ class TestMongoModuleStore(object): assert_equals(len(course_locations), 1) assert_in(Location('i4x', 'edX', 'simple', 'course', '2012_Fall'), course_locations) + def test_export_course_image(self): + """ + Test to make sure that we have a course image in the contentstore, + then export it to ensure it gets copied to both file locations. + """ + location = Location('c4x', 'edX', 'simple', 'asset', 'images_course_image.jpg') + course_location = Location('i4x', 'edX', 'simple', 'course', '2012_Fall') + + # This will raise if the course image is missing + self.content_store.find(location) + + root_dir = path(mkdtemp()) + export_to_xml(self.store, self.content_store, course_location, root_dir, 'test_export') + assert_true(path(root_dir / 'test_export/static/images/course_image.jpg').isfile()) + assert_true(path(root_dir / 'test_export/static/images_course_image.jpg').isfile()) + shutil.rmtree(root_dir) + + def test_export_course_image_nondefault(self): + """ + Make sure that if a non-default image path is specified that we + don't export it to the static default location + """ + course = self.get_course_by_id('edX/toy/2012_Fall') + assert_true(course.course_image, 'just_a_test.jpg') + + root_dir = path(mkdtemp()) + export_to_xml(self.store, self.content_store, course.location, root_dir, 'test_export') + assert_true(path(root_dir / 'test_export/static/just_a_test.jpg').isfile()) + assert_false(path(root_dir / 'test_export/static/images/course_image.jpg').isfile()) + shutil.rmtree(root_dir) + + def test_course_without_image(self): + """ + Make sure we elegantly passover our code when there isn't a static + image + """ + course = self.get_course_by_id('edX/simple_with_draft/2012_Fall') + root_dir = path(mkdtemp()) + export_to_xml(self.store, self.content_store, course.location, root_dir, 'test_export') + assert_false(path(root_dir / 'test_export/static/images/course_image.jpg').isfile()) + assert_false(path(root_dir / 'test_export/static/images_course_image.jpg').isfile()) + shutil.rmtree(root_dir) + + class TestMongoKeyValueStore(object): """ diff --git a/common/lib/xmodule/xmodule/modulestore/xml_exporter.py b/common/lib/xmodule/xmodule/modulestore/xml_exporter.py index b442e2ef96..00c759385a 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_exporter.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_exporter.py @@ -5,6 +5,8 @@ Methods for exporting course data to XML import logging import lxml.etree from xblock.fields import Scope +from xmodule.contentstore.content import StaticContent +from xmodule.exceptions import NotFoundError from xmodule.modulestore import Location from xmodule.modulestore.inheritance import own_metadata from fs.osfs import OSFS @@ -79,6 +81,26 @@ def export_to_xml(modulestore, contentstore, course_location, root_dir, course_d root_dir + '/' + course_dir + '/policies/assets.json', ) + # If we are using the default course image, export it to the + # legacy location to support backwards compatibility. + if course.course_image == course.fields['course_image'].default: + try: + course_image = contentstore.find( + StaticContent.compute_location( + course.location.org, + course.location.course, + course.course_image + ), + ) + except NotFoundError: + pass + else: + output_dir = root_dir + '/' + course_dir + '/static/images/' + if not os.path.isdir(output_dir): + os.makedirs(output_dir) + with OSFS(output_dir).open('course_image.jpg', 'wb') as course_image_file: + course_image_file.write(course_image.data) + # export the static tabs export_extra_content(export_fs, modulestore, course_id, course_location, 'static_tab', 'tabs', '.html') diff --git a/common/test/data/simple/static/images_course_image.jpg b/common/test/data/simple/static/images_course_image.jpg new file mode 100644 index 0000000000000000000000000000000000000000..6bb7f377a03e406c6a448c7adfc4994f03294874 GIT binary patch literal 547 zcmbu4y$ZrW5QJy$P%JF&z*6vhk`#f&2mxOqXh0hU30UUc#E0;w@kv}S1O>&IBD3A{ z?aqvx + diff --git a/common/test/data/toy/static/just_a_test.jpg b/common/test/data/toy/static/just_a_test.jpg new file mode 100644 index 0000000000000000000000000000000000000000..6bb7f377a03e406c6a448c7adfc4994f03294874 GIT binary patch literal 547 zcmbu4y$ZrW5QJy$P%JF&z*6vhk`#f&2mxOqXh0hU30UUc#E0;w@kv}S1O>&IBD3A{ z?aqvx Date: Mon, 12 May 2014 13:41:23 -0400 Subject: [PATCH 2/3] Corrected test names and doc strings --- lms/djangoapps/courseware/tests/test_courses.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lms/djangoapps/courseware/tests/test_courses.py b/lms/djangoapps/courseware/tests/test_courses.py index ba258e6017..7990da6cee 100644 --- a/lms/djangoapps/courseware/tests/test_courses.py +++ b/lms/djangoapps/courseware/tests/test_courses.py @@ -124,7 +124,7 @@ class MongoCourseImageTestCase(ModuleStoreTestCase): ) ) - def test_static_asset_path_default(self): + def test_static_asset_path_course_image_default(self): """ Test that without course_image being set, but static_asset_path being set that we get the right course_image url. @@ -135,10 +135,10 @@ class MongoCourseImageTestCase(ModuleStoreTestCase): '/static/foo/images/course_image.jpg' ) - def test_static_asset_path_set(self): + def test_static_asset_path_course_image_set(self): """ - Test that without course_image being set, but static_asset_path - being set that we get the right course_image url. + Test that with course_image and static_asset_path both + being set, that we get the right course_image url. """ course = CourseFactory.create(course_image=u'things_stuff.jpg', static_asset_path="foo") From 2f7adda00ddf102d383adfc0ecaf25281470a3ab Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Mon, 12 May 2014 15:52:57 -0400 Subject: [PATCH 3/3] Simple fixes - Ensure temp files are cleaned up. - Refactor a function with three return statements. --- .../xmodule/modulestore/tests/test_mongo.py | 30 +++++++++++-------- lms/djangoapps/courseware/courses.py | 9 +++--- 2 files changed, 23 insertions(+), 16 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py index 8ed44a1f5e..e52af84d37 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py @@ -326,10 +326,12 @@ class TestMongoModuleStore(object): self.content_store.find(location) root_dir = path(mkdtemp()) - export_to_xml(self.store, self.content_store, course_location, root_dir, 'test_export') - assert_true(path(root_dir / 'test_export/static/images/course_image.jpg').isfile()) - assert_true(path(root_dir / 'test_export/static/images_course_image.jpg').isfile()) - shutil.rmtree(root_dir) + try: + export_to_xml(self.store, self.content_store, course_location, root_dir, 'test_export') + assert_true(path(root_dir / 'test_export/static/images/course_image.jpg').isfile()) + assert_true(path(root_dir / 'test_export/static/images_course_image.jpg').isfile()) + finally: + shutil.rmtree(root_dir) def test_export_course_image_nondefault(self): """ @@ -340,10 +342,12 @@ class TestMongoModuleStore(object): assert_true(course.course_image, 'just_a_test.jpg') root_dir = path(mkdtemp()) - export_to_xml(self.store, self.content_store, course.location, root_dir, 'test_export') - assert_true(path(root_dir / 'test_export/static/just_a_test.jpg').isfile()) - assert_false(path(root_dir / 'test_export/static/images/course_image.jpg').isfile()) - shutil.rmtree(root_dir) + try: + export_to_xml(self.store, self.content_store, course.location, root_dir, 'test_export') + assert_true(path(root_dir / 'test_export/static/just_a_test.jpg').isfile()) + assert_false(path(root_dir / 'test_export/static/images/course_image.jpg').isfile()) + finally: + shutil.rmtree(root_dir) def test_course_without_image(self): """ @@ -352,10 +356,12 @@ class TestMongoModuleStore(object): """ course = self.get_course_by_id('edX/simple_with_draft/2012_Fall') root_dir = path(mkdtemp()) - export_to_xml(self.store, self.content_store, course.location, root_dir, 'test_export') - assert_false(path(root_dir / 'test_export/static/images/course_image.jpg').isfile()) - assert_false(path(root_dir / 'test_export/static/images_course_image.jpg').isfile()) - shutil.rmtree(root_dir) + try: + export_to_xml(self.store, self.content_store, course.location, root_dir, 'test_export') + assert_false(path(root_dir / 'test_export/static/images/course_image.jpg').isfile()) + assert_false(path(root_dir / 'test_export/static/images_course_image.jpg').isfile()) + finally: + shutil.rmtree(root_dir) diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index 51192088fe..3937860f08 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -110,14 +110,15 @@ def course_image_url(course): # set different than the default, return that path so that # courses can use custom course image paths, otherwise just # return the default static path. + url = '/static/' + (course.static_asset_path or getattr(course, 'data_dir', '')) if hasattr(course, 'course_image') and course.course_image != course.fields['course_image'].default: - return '/static/' + (course.static_asset_path or getattr(course, 'data_dir', '')) + '/' + course.course_image + url += '/' + course.course_image else: - return '/static/' + (course.static_asset_path or getattr(course, 'data_dir', '')) + "/images/course_image.jpg" + url += '/images/course_image.jpg' else: loc = StaticContent.compute_location(course.location.org, course.location.course, course.course_image) - _path = StaticContent.get_url_path_from_location(loc) - return _path + url = StaticContent.get_url_path_from_location(loc) + return url def find_file(filesystem, dirs, filename):