Skip to content

Video xml serialization bugs #688

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
chrisndodge wants to merge 7 commits into from
Closed

Video xml serialization bugs #688

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
042f5f4
was getting unicode errors when exporting CB22x
Aug 15, 2013
8b358e9
export_to_xml() really should be definition_to_xml() since we'll rely…
Aug 15, 2013
69cde61
remove commented out line
Aug 15, 2013
a1d0f9b
update tests to call definition_to_xml rather than export_to_xml, whi…
Aug 15, 2013
2ef1e60
Make our deserializing consistent with serializing.
Aug 15, 2013
c9dabab
have XModuleDescriptor properly ensure category is set
Aug 15, 2013
c6e05e5
Merge branch 'fix/cdodge/unicode-error-in-video-xml-serilization' of …
Aug 15, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions cms/djangoapps/contentstore/views/assets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,8 @@ def generate_export_course(request, org, course, name):
try:
export_to_xml(modulestore('direct'), contentstore(), loc, root_dir, name, modulestore())
except SerializationError, e:
logging.exception('There was an error exporting course {0}. {1}'.format(course_module.location, unicode(e)))

unit = None
failed_item = None
parent = None
Expand Down Expand Up @@ -380,6 +382,7 @@ def generate_export_course(request, org, course, name):
})
})
except Exception, e:
logging.exception('There was an error exporting course {0}. {1}'.format(course_module.location, unicode(e)))
return render_to_response('export.html', {
'context_course': course_module,
'successful_import_redirect_url': '',
Expand Down
9 changes: 5 additions & 4 deletions common/lib/xmodule/xmodule/tests/test_video.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

import unittest
from . import LogicTest
from lxml import etree
from .import get_test_system
from xmodule.modulestore import Location
from xmodule.video_module import VideoDescriptor, _create_youtube_string
Expand Down Expand Up @@ -344,7 +345,7 @@ def test_export_to_xml(self):
desc.track = 'http://www.example.com/track'
desc.html5_sources = ['http://www.example.com/source.mp4', 'http://www.example.com/source.ogg']

xml = desc.export_to_xml(None) # We don't use the `resource_fs` parameter
xml = desc.definition_to_xml(None) # We don't use the `resource_fs` parameter
expected = dedent('''\
<video url_name="SampleProblem1" start_time="0:00:01" youtube="0.75:izygArpw-Qo,1.00:p2Q6BrNhdh8,1.25:1EeWXzPdhSA,1.50:rABDYkeK0x8" show_captions="false" end_time="0:01:00">
<source src="http://www.example.com/source.mp4"/>
Expand All @@ -353,15 +354,15 @@ def test_export_to_xml(self):
</video>
''')

self.assertEquals(expected, xml)
self.assertEquals(expected, etree.tostring(xml, pretty_print=True))

def test_export_to_xml_empty_parameters(self):
"""Test XML export with defaults."""
module_system = DummySystem(load_error_modules=True)
location = Location(["i4x", "edX", "video", "default", "SampleProblem1"])
desc = VideoDescriptor(module_system, {'location': location})

xml = desc.export_to_xml(None)
xml = desc.definition_to_xml(None)
expected = '<video url_name="SampleProblem1"/>\n'

self.assertEquals(expected, xml)
self.assertEquals(expected, etree.tostring(xml, pretty_print=True))
10 changes: 7 additions & 3 deletions common/lib/xmodule/xmodule/video_module.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,10 @@ def __init__(self, *args, **kwargs):
self._model_data.update(model_data)
del self.data

# I'm not sure how, but in some lifecycles we can loose the .category attribute
# this needs to be investigated
self.category = 'video'

@classmethod
def from_xml(cls, xml_data, system, org=None, course=None):
"""
Expand All @@ -240,7 +244,7 @@ def from_xml(cls, xml_data, system, org=None, course=None):
video = cls(system, model_data)
return video

def export_to_xml(self, resource_fs):
def definition_to_xml(self, resource_fs):

@cahrens cahrens Aug 15, 2013

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this approach means that we go through the serialization code in XmlModule, it is not necessary to wrap the value of "show_captions" with json.dumps (on line 259).

@cahrens cahrens Aug 15, 2013

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, when I removed json.dumps the boolean did not get serialized out. I guess that is because we are not sending the model data value, but the _from_json value (due to calling it as self.show_captions instead of getting it directly from the model data). So json.dumps stays for now. It might be better to change it to _to_json.

@cahrens cahrens Aug 15, 2013

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, my last note about this. The videoalpha code right below this expects value to be a string. In fact, a boolean value of False causes nothing to be written out:

 for key, value in attrs.items():
        # Mild workaround to ensure that tests pass -- if a field
        # is set to its default value, we don't need to write it out.
        if key in fields and fields[key].default == getattr(self, key):
            continue
        if value:      <------------------------------------ will not work with boolean value of False for value
            xml.set(key, unicode(value))

I'm not particularly sold on what is going on in this method, but I am inclined to leave it alone for now.

@dmitchell dmitchell Aug 15, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arggh. Of course, my current branch has tools to fix this... (ad nauseum). Both this line and the one above re video ids, should, given the current master, only grab the values from self if field.name in self._model_data. The way this code is written, we will not export explicitly set values if they so happen to be == default even if inheritance would overwrite them w/ something that's not the default. In this case, inheritance may not be an issue b/c these may all be Scope.content, but it's a bad pattern.

The loop should be over the explicitly set values and not have this if value: logic.

Given this rant, however, I think we can go ahead but someone should clean up these oddities throughout the code someday (i'm not sure how to capture, perhaps at least a # TODO?)

"""
Returns an xml string representing this module.
"""
Expand All @@ -266,7 +270,7 @@ def export_to_xml(self, resource_fs):
if key in fields and fields[key].default == getattr(self, key):
continue
if value:
xml.set(key, str(value))
xml.set(key, unicode(value))

for source in self.html5_sources:
ele = etree.Element('source')
Expand All @@ -277,7 +281,7 @@ def export_to_xml(self, resource_fs):
ele = etree.Element('track')
ele.set('src', self.track)
xml.append(ele)
return etree.tostring(xml, pretty_print=True)
return xml

@staticmethod
def _parse_youtube(data):
Expand Down