From 0967cb0d9d20435590f3d1520e9a4a18669f46c0 Mon Sep 17 00:00:00 2001 From: Peter Fogg Date: Wed, 24 Jul 2013 15:55:41 -0400 Subject: [PATCH 1/3] Fix word cloud modules not exporting correctly. --- .../contentstore/tests/test_contentstore.py | 28 +++++++++++++++++++ .../lib/xmodule/xmodule/word_cloud_module.py | 8 ++++++ 2 files changed, 36 insertions(+) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index b15c05b984ee..39e686edd587 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -855,6 +855,34 @@ def test_export_course_with_metadata_only_video(self): shutil.rmtree(root_dir) + def test_export_course_with_metadata_only_word_cloud(self): + """ + Similar to `test_export_course_with_metadata_only_video`. + """ + module_store = modulestore('direct') + draft_store = modulestore('draft') + content_store = contentstore() + + import_from_xml(module_store, 'common/test/data/', ['word_cloud']) + location = CourseDescriptor.id_to_location('HarvardX/ER22x/2013_Spring') + + verticals = module_store.get_items(['i4x', 'HarvardX', 'ER22x', 'vertical', None, None]) + + self.assertGreater(len(verticals), 0) + + parent = verticals[0] + + ItemFactory.create(parent_location=parent.location, category="word_cloud", display_name="untitled") + + root_dir = path(mkdtemp_clean()) + + print 'Exporting to tempdir = {0}'.format(root_dir) + + # export out to a tempdir + export_to_xml(module_store, content_store, location, root_dir, 'test_export', draft_modulestore=draft_store) + + shutil.rmtree(root_dir) + def test_course_handouts_rewrites(self): module_store = modulestore('direct') diff --git a/common/lib/xmodule/xmodule/word_cloud_module.py b/common/lib/xmodule/xmodule/word_cloud_module.py index 004e6ed32064..c8aaeba14ccb 100644 --- a/common/lib/xmodule/xmodule/word_cloud_module.py +++ b/common/lib/xmodule/xmodule/word_cloud_module.py @@ -9,6 +9,8 @@ import json import logging +from lxml import etree + from pkg_resources import resource_string from xmodule.raw_module import RawDescriptor from xmodule.editing_module import MetadataOnlyEditingDescriptor @@ -244,3 +246,9 @@ class WordCloudDescriptor(WordCloudFields, MetadataOnlyEditingDescriptor, RawDes """Descriptor for WordCloud Xmodule.""" module_class = WordCloudModule template_dir_name = 'word_cloud' + + def definition_to_xml(self, resource_fs): + """ + Since there's no real data, just return an empty XML tag. + """ + return etree.fromstring('') From f355e4a8f4b114c8de05b56556207ac98c9dc8d1 Mon Sep 17 00:00:00 2001 From: Peter Fogg Date: Thu, 25 Jul 2013 10:26:11 -0400 Subject: [PATCH 2/3] More general solution to word cloud export problems. --- common/lib/xmodule/xmodule/raw_module.py | 2 ++ common/lib/xmodule/xmodule/video_module.py | 7 ------- common/lib/xmodule/xmodule/word_cloud_module.py | 8 -------- 3 files changed, 2 insertions(+), 15 deletions(-) diff --git a/common/lib/xmodule/xmodule/raw_module.py b/common/lib/xmodule/xmodule/raw_module.py index 4c6c719224c9..58dacc16eb8c 100644 --- a/common/lib/xmodule/xmodule/raw_module.py +++ b/common/lib/xmodule/xmodule/raw_module.py @@ -20,6 +20,8 @@ def definition_from_xml(cls, xml_object, system): return {'data': etree.tostring(xml_object, pretty_print=True, encoding='unicode')}, [] def definition_to_xml(self, resource_fs): + if not self.data: + return etree.fromstring('<{0} />'.format(self.category)) try: return etree.fromstring(self.data) except etree.XMLSyntaxError as err: diff --git a/common/lib/xmodule/xmodule/video_module.py b/common/lib/xmodule/xmodule/video_module.py index 6c5640346ee7..71a99a7a63c7 100644 --- a/common/lib/xmodule/xmodule/video_module.py +++ b/common/lib/xmodule/xmodule/video_module.py @@ -130,13 +130,6 @@ def from_xml(cls, xml_data, system, org=None, course=None): _parse_video_xml(video, video.data) return video - def definition_to_xml(self, resource_fs): - """ - Override the base implementation. We don't actually have anything in the 'data' field - (it's an empty string), so we just return a simple XML element - """ - return etree.Element('video') - def _parse_video_xml(video, xml_data): """ diff --git a/common/lib/xmodule/xmodule/word_cloud_module.py b/common/lib/xmodule/xmodule/word_cloud_module.py index c8aaeba14ccb..004e6ed32064 100644 --- a/common/lib/xmodule/xmodule/word_cloud_module.py +++ b/common/lib/xmodule/xmodule/word_cloud_module.py @@ -9,8 +9,6 @@ import json import logging -from lxml import etree - from pkg_resources import resource_string from xmodule.raw_module import RawDescriptor from xmodule.editing_module import MetadataOnlyEditingDescriptor @@ -246,9 +244,3 @@ class WordCloudDescriptor(WordCloudFields, MetadataOnlyEditingDescriptor, RawDes """Descriptor for WordCloud Xmodule.""" module_class = WordCloudModule template_dir_name = 'word_cloud' - - def definition_to_xml(self, resource_fs): - """ - Since there's no real data, just return an empty XML tag. - """ - return etree.fromstring('') From 512269f8423a6b262198511ea8e9833c690fdd47 Mon Sep 17 00:00:00 2001 From: Peter Fogg Date: Tue, 30 Jul 2013 11:52:50 -0400 Subject: [PATCH 3/3] Add descriptor for modules with empty XML data. This allows a more general approach to modules such as word_cloud and video which have no XML data (it's all store in metadata), but use XmlDescriptor for backwards compatibility. They now generate an empty tag on export, and clear out empty tags on import. Also a small change to the video module as a result -- if it's asked to parse empty XML data, it won't try to parse anything. --- .../contentstore/tests/test_contentstore.py | 34 +++++++++++++++++++ common/lib/xmodule/xmodule/raw_module.py | 21 ++++++++++-- common/lib/xmodule/xmodule/video_module.py | 7 ++-- .../lib/xmodule/xmodule/word_cloud_module.py | 4 +-- 4 files changed, 60 insertions(+), 6 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 39e686edd587..78ff575c35eb 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -883,6 +883,40 @@ def test_export_course_with_metadata_only_word_cloud(self): shutil.rmtree(root_dir) + def test_empty_data_roundtrip(self): + """ + Test that an empty `data` field is preserved through + export/import. + """ + module_store = modulestore('direct') + draft_store = modulestore('draft') + content_store = contentstore() + + import_from_xml(module_store, 'common/test/data/', ['toy']) + location = CourseDescriptor.id_to_location('edX/toy/2012_Fall') + + verticals = module_store.get_items(['i4x', 'edX', 'toy', 'vertical', None, None]) + + self.assertGreater(len(verticals), 0) + + parent = verticals[0] + + # Create a module, and ensure that its `data` field is empty + word_cloud = ItemFactory.create(parent_location=parent.location, category="word_cloud", display_name="untitled") + del word_cloud.data + self.assertEquals(word_cloud.data, '') + + # Export the course + root_dir = path(mkdtemp_clean()) + export_to_xml(module_store, content_store, location, root_dir, 'test_roundtrip', draft_modulestore=draft_store) + + # Reimport and get the video back + import_from_xml(module_store, root_dir) + imported_word_cloud = module_store.get_item(Location(['i4x', 'edX', 'toy', 'word_cloud', 'untitled', None])) + + # It should now contain empty data + self.assertEquals(imported_word_cloud.data, '') + def test_course_handouts_rewrites(self): module_store = modulestore('direct') diff --git a/common/lib/xmodule/xmodule/raw_module.py b/common/lib/xmodule/xmodule/raw_module.py index 58dacc16eb8c..b972d7c8eb3a 100644 --- a/common/lib/xmodule/xmodule/raw_module.py +++ b/common/lib/xmodule/xmodule/raw_module.py @@ -20,8 +20,6 @@ def definition_from_xml(cls, xml_object, system): return {'data': etree.tostring(xml_object, pretty_print=True, encoding='unicode')}, [] def definition_to_xml(self, resource_fs): - if not self.data: - return etree.fromstring('<{0} />'.format(self.category)) try: return etree.fromstring(self.data) except etree.XMLSyntaxError as err: @@ -34,3 +32,22 @@ def definition_to_xml(self, resource_fs): context=lines[line - 1][offset - 40:offset + 40], loc=self.location)) raise Exception, msg, sys.exc_info()[2] + + +class EmptyDataRawDescriptor(XmlDescriptor, XMLEditingDescriptor): + """ + Version of RawDescriptor for modules which may have no XML data, + but use XMLEditingDescriptor for import/export handling. + """ + data = String(default='', scope=Scope.content) + + @classmethod + def definition_from_xml(cls, xml_object, system): + if len(xml_object) == 0 and len(xml_object.items()) == 0: + return {'data': ''}, [] + return {'data': etree.tostring(xml_object, pretty_print=True, encoding='unicode')}, [] + + def definition_to_xml(self, resource_fs): + if self.data: + return etree.fromstring(self.data) + return etree.Element(self.category) diff --git a/common/lib/xmodule/xmodule/video_module.py b/common/lib/xmodule/xmodule/video_module.py index 71a99a7a63c7..763975fc3bc9 100644 --- a/common/lib/xmodule/xmodule/video_module.py +++ b/common/lib/xmodule/xmodule/video_module.py @@ -12,7 +12,7 @@ from django.http import Http404 from xmodule.x_module import XModule -from xmodule.raw_module import RawDescriptor +from xmodule.raw_module import EmptyDataRawDescriptor from xmodule.editing_module import MetadataOnlyEditingDescriptor from xblock.core import Integer, Scope, String, Float, Boolean @@ -97,7 +97,7 @@ def get_html(self): class VideoDescriptor(VideoFields, MetadataOnlyEditingDescriptor, - RawDescriptor): + EmptyDataRawDescriptor): module_class = VideoModule def __init__(self, *args, **kwargs): @@ -136,6 +136,9 @@ def _parse_video_xml(video, xml_data): Parse video fields out of xml_data. The fields are set if they are present in the XML. """ + if not xml_data: + return + xml = etree.fromstring(xml_data) display_name = xml.get('display_name') diff --git a/common/lib/xmodule/xmodule/word_cloud_module.py b/common/lib/xmodule/xmodule/word_cloud_module.py index 004e6ed32064..900bded760f9 100644 --- a/common/lib/xmodule/xmodule/word_cloud_module.py +++ b/common/lib/xmodule/xmodule/word_cloud_module.py @@ -10,7 +10,7 @@ import logging from pkg_resources import resource_string -from xmodule.raw_module import RawDescriptor +from xmodule.raw_module import EmptyDataRawDescriptor from xmodule.editing_module import MetadataOnlyEditingDescriptor from xmodule.x_module import XModule @@ -240,7 +240,7 @@ def get_html(self): return self.content -class WordCloudDescriptor(WordCloudFields, MetadataOnlyEditingDescriptor, RawDescriptor): +class WordCloudDescriptor(WordCloudFields, MetadataOnlyEditingDescriptor, EmptyDataRawDescriptor): """Descriptor for WordCloud Xmodule.""" module_class = WordCloudModule template_dir_name = 'word_cloud'