Skip to content

Commit

Permalink
Add deserialize logic for metadata fields.
Browse files Browse the repository at this point in the history
STUD-640

Handle the case of the double-quotes appearing within the string.

We don't need to worry about the whole concatenated ID being
double-quoted, since the concatenation was done by overriding xml_module's
export method.

Use the method already fined in XmlDescriptor.
  • Loading branch information
cahrens committed Aug 14, 2013
1 parent a2225aa commit bf6ebcc
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 6 deletions.
56 changes: 56 additions & 0 deletions common/lib/xmodule/xmodule/tests/test_video.py
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,62 @@ def test_from_xml_no_attributes(self):
'data': ''
})

def test_from_xml_double_quotes(self):
"""
Make sure we can handle the double-quoted string format (which was used for exporting for
a few weeks).
"""
module_system = DummySystem(load_error_modules=True)
xml_data ='''
<video display_name="&quot;display_name&quot;"
html5_sources="[&quot;source_1&quot;, &quot;source_2&quot;]"
show_captions="false"
source="&quot;http://download_video&quot;"
sub="&quot;html5_subtitles&quot;"
track="&quot;http://download_track&quot;"
youtube_id_0_75="&quot;OEoXaMPEzf65&quot;"
youtube_id_1_25="&quot;OEoXaMPEzf125&quot;"
youtube_id_1_5="&quot;OEoXaMPEzf15&quot;"
youtube_id_1_0="&quot;OEoXaMPEzf10&quot;"
/>
'''
output = VideoDescriptor.from_xml(xml_data, module_system)
self.assert_attributes_equal(output, {
'youtube_id_0_75': 'OEoXaMPEzf65',
'youtube_id_1_0': 'OEoXaMPEzf10',
'youtube_id_1_25': 'OEoXaMPEzf125',
'youtube_id_1_5': 'OEoXaMPEzf15',
'show_captions': False,
'start_time': 0.0,
'end_time': 0.0,
'track': 'http://download_track',
'source': 'http://download_video',
'html5_sources': ["source_1", "source_2"],
'data': ''
})

def test_from_xml_double_quote_concatenated_youtube(self):
module_system = DummySystem(load_error_modules=True)
xml_data = '''
<video display_name="Test Video"
youtube="1.0:&quot;p2Q6BrNhdh8&quot;,1.25:&quot;1EeWXzPdhSA&quot;">
</video>
'''
output = VideoDescriptor.from_xml(xml_data, module_system)
self.assert_attributes_equal(output, {
'youtube_id_0_75': '',
'youtube_id_1_0': 'p2Q6BrNhdh8',
'youtube_id_1_25': '1EeWXzPdhSA',
'youtube_id_1_5': '',
'show_captions': True,
'start_time': 0.0,
'end_time': 0.0,
'track': '',
'source': '',
'html5_sources': [],
'data': ''
})

def test_old_video_format(self):
"""
Test backwards compatibility with VideoModule's XML format.
Expand Down
17 changes: 15 additions & 2 deletions common/lib/xmodule/xmodule/video_module.py
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,10 @@ def _parse_youtube(data):
pieces = video.split(':')
try:
speed = '%.2f' % float(pieces[0]) # normalize speed
youtube_id = pieces[1]
# Handle the fact that youtube IDs got double-quoted for a period of time.
# Note: we pass in "VideoFields.youtube_id_1_0" so we deserialize as a String--
# it doesn't matter what the actual speed is for the purposes of deserializing.
youtube_id = VideoDescriptor._deserialize(VideoFields.youtube_id_1_0.name, pieces[1])

This comment has been minimized.

Copy link
@vasylnakvasiuk

vasylnakvasiuk Aug 15, 2013

Contributor

Could we rework _deserialize method so, that method can pass types, like String, Integer, Boolean?
Cause, VideoFields.youtube_id_1_0.name confuses me.

This comment has been minimized.

Copy link
@cahrens

cahrens Aug 15, 2013

The method that _deserialize calls expects to get the name of the metadata attribute. It uses that (and the fields defined for the class), to determine what the type is.

When we are deserializing XML, all we have is the name of the attribute and the value. We don't know String, Integer, or Boolean at that level.

I agree that this particular usage is confusing-- I attempted to explain it with the comment above. In this usage, we don't have an attribute name that maps to the concatenation of all the youtube IDs-- we only have names for the individual speeds. The much more common case is below on line 357, where we actually have the name of the metadata attribute. Changing _deserialize to accept String, Integer, or Boolean means writing a 2nd version of the XMLDescriptor method, just for this usage.

ret[speed] = youtube_id
except (ValueError, IndexError):
log.warning('Invalid YouTube ID: %s' % video)
Expand All @@ -310,7 +313,6 @@ def _parse_video_xml(xml_data):
model_data = {}

conversions = {
'show_captions': json.loads,
'start_time': VideoDescriptor._parse_time,
'end_time': VideoDescriptor._parse_time
}
Expand Down Expand Up @@ -349,6 +351,11 @@ def _parse_video_xml(xml_data):
# Convert XML attrs into Python values.
if attr in conversions:
value = conversions[attr](value)
else:
# We export values with json.dumps (well, except for Strings, but
# for about a month we did it for Strings also).
value = VideoDescriptor._deserialize(attr, value)

model_data[attr] = value

return model_data
Expand All @@ -367,6 +374,12 @@ def _parse_time(str_time):
seconds=obj_time.tm_sec
).total_seconds()

@staticmethod
def _deserialize(attr, value):
"""
Handles deserializing values that may have been encoded with json.dumps.
"""
return VideoDescriptor.get_map_for_field(VideoDescriptor, attr).from_xml(value)

def _create_youtube_string(module):
"""
Expand Down
13 changes: 9 additions & 4 deletions common/lib/xmodule/xmodule/xml_module.py
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,13 @@ class XmlDescriptor(XModuleDescriptor):

metadata_to_export_to_policy = ('discussion_topics')

@classmethod
@staticmethod

This comment has been minimized.

Copy link
@vasylnakvasiuk

vasylnakvasiuk Aug 15, 2013

Contributor

Why you use @staticmethod, and then use this method, like a class method?

This comment has been minimized.

Copy link
@cahrens

cahrens Aug 15, 2013

Because I need to call it from static methods in VideoDescriptor. I didn't feel like making all of those static methods class methods.

def get_map_for_field(cls, attr):
"""
Returns a serialize/deserialize AttrMap for the given field of a class.
Searches through fields defined by cls to find one named attr.
"""
for field in set(cls.fields + cls.lms.fields):
if field.name == attr:
from_xml = lambda val: deserialize_field(field, val)
Expand Down Expand Up @@ -280,7 +285,7 @@ def load_metadata(cls, xml_object):
# don't load these
continue

attr_map = cls.get_map_for_field(attr)
attr_map = XmlDescriptor.get_map_for_field(cls, attr)
metadata[attr] = attr_map.from_xml(val)
return metadata

Expand All @@ -291,7 +296,7 @@ def apply_policy(cls, metadata, policy):
through the attrmap. Updates the metadata dict in place.
"""
for attr in policy:
attr_map = cls.get_map_for_field(attr)
attr_map = XmlDescriptor.get_map_for_field(cls, attr)
metadata[cls._translate(attr)] = attr_map.from_xml(policy[attr])

@classmethod
Expand Down Expand Up @@ -407,7 +412,7 @@ def val_for_xml(attr):
"""Get the value for this attribute that we want to store.
(Possible format conversion through an AttrMap).
"""
attr_map = self.get_map_for_field(attr)
attr_map = XmlDescriptor.get_map_for_field(self, attr)
return attr_map.to_xml(self._model_data[attr])

# Add the non-inherited metadata
Expand Down

0 comments on commit bf6ebcc

Please sign in to comment.