Commit f500b722 by Calen Pennington

Make sure that we have the right set of fields available during xml parsing

We had a bug where mixins weren't being applied before `load_from_xml`
was called. This meant that not all of the fields were being loaded
correctly. To fix it, we used the mixoligist from the runtime to apply
the mixins earlier in the process. However, that caused the mixins to be
applied twice.

The included fixes to xblock resolved the multiply-applied mixins, and
the fixes to the parsing code make it simpler to understand, and add
some unit tests of the parsing to boot.
parent 813795dd
......@@ -91,7 +91,6 @@ class ImportTestCase(BaseCourseTestCase):
self.assertNotEqual(descriptor1.location, descriptor2.location)
@unittest.skip('Temporarily disabled')
def test_reimport(self):
'''Make sure an already-exported error xml tag loads properly'''
......
# disable missing docstring
#pylint: disable=C0111
from xmodule.x_module import XModuleFields
from xblock.fields import Scope, String, Dict, Boolean, Integer, Float, Any, List
from xblock.field_data import DictFieldData
from xmodule.fields import Date, Timedelta
from xmodule.xml_module import XmlDescriptor, serialize_field, deserialize_field
import unittest
from nose.tools import assert_equals # pylint: disable=E0611
from mock import Mock
from xmodule.modulestore.inheritance import InheritanceKeyValueStore, InheritanceMixin
from nose.tools import assert_equals, assert_not_equals, assert_true, assert_false, assert_in, assert_not_in # pylint: disable=E0611
from xblock.field_data import DictFieldData
from xblock.fields import Scope, String, Dict, Boolean, Integer, Float, Any, List
from xblock.runtime import DbModel
from xmodule.fields import Date, Timedelta
from xmodule.modulestore.inheritance import InheritanceKeyValueStore, InheritanceMixin
from xmodule.x_module import XModuleFields
from xmodule.xml_module import XmlDescriptor, serialize_field, deserialize_field
from xmodule.course_module import CourseDescriptor
from xmodule.seq_module import SequenceDescriptor
from xmodule.tests import get_test_descriptor_system
from xmodule.tests.xml import XModuleXmlImportTest
from xmodule.tests.xml.factories import CourseFactory, SequenceFactory, ProblemFactory
class CrazyJsonString(String):
......@@ -379,3 +387,78 @@ class TestDeserializeTimedelta(TestDeserialize):
self.assertDeserializeEqual('1 day 12 hours 59 minutes 59 seconds',
'"1 day 12 hours 59 minutes 59 seconds"')
self.assertDeserializeNonString()
class TestXmlAttributes(XModuleXmlImportTest):
def test_unknown_attribute(self):
assert_false(hasattr(CourseDescriptor, 'unknown_attr'))
course = self.process_xml(CourseFactory.build(unknown_attr='value'))
assert_false(hasattr(course, 'unknown_attr'))
assert_equals('value', course.xml_attributes['unknown_attr'])
def test_known_attribute(self):
assert_true(hasattr(CourseDescriptor, 'show_chat'))
course = self.process_xml(CourseFactory.build(show_chat='true'))
assert_true(course.show_chat)
assert_not_in('show_chat', course.xml_attributes)
def test_rerandomize_in_policy(self):
# Rerandomize isn't a basic attribute of Sequence
assert_false(hasattr(SequenceDescriptor, 'rerandomize'))
root = SequenceFactory.build(policy={'rerandomize': 'never'})
ProblemFactory.build(parent=root)
seq = self.process_xml(root)
# Rerandomize is added to the constructed sequence via the InheritanceMixin
assert_equals('never', seq.rerandomize)
# Rerandomize is a known value coming from policy, and shouldn't appear
# in xml_attributes
assert_not_in('rerandomize', seq.xml_attributes)
def test_attempts_in_policy(self):
# attempts isn't a basic attribute of Sequence
assert_false(hasattr(SequenceDescriptor, 'attempts'))
root = SequenceFactory.build(policy={'attempts': '1'})
ProblemFactory.build(parent=root)
seq = self.process_xml(root)
# attempts isn't added to the constructed sequence, because
# it's not in the InheritanceMixin
assert_false(hasattr(seq, 'attempts'))
# attempts is an unknown attribute, so we should include it
# in xml_attributes so that it gets written out (despite the misleading
# name)
assert_in('attempts', seq.xml_attributes)
def test_inheritable_attribute(self):
# days_early_for_beta isn't a basic attribute of Sequence
assert_false(hasattr(SequenceDescriptor, 'days_early_for_beta'))
# days_early_for_beta is added by InheritanceMixin
assert_true(hasattr(InheritanceMixin, 'days_early_for_beta'))
root = SequenceFactory.build(policy={'days_early_for_beta': '2'})
ProblemFactory.build(parent=root)
# InheritanceMixin will be used when processing the XML
assert_in(InheritanceMixin, root.xblock_mixins)
seq = self.process_xml(root)
assert_equals(seq.unmixed_class, SequenceDescriptor)
assert_not_equals(type(seq), SequenceDescriptor)
# days_early_for_beta is added to the constructed sequence, because
# it's in the InheritanceMixin
assert_equals(2, seq.days_early_for_beta)
# days_early_for_beta is a known attribute, so we shouldn't include it
# in xml_attributes
assert_not_in('days_early_for_beta', seq.xml_attributes)
"""
Xml parsing tests for XModules
"""
import pprint
from mock import Mock
from xmodule.x_module import XMLParsingSystem, XModuleDescriptor
from xmodule.mako_module import MakoDescriptorSystem
class InMemorySystem(XMLParsingSystem, MakoDescriptorSystem): # pylint: disable=abstract-method
"""
The simplest possible XMLParsingSystem
"""
def __init__(self, xml_import_data):
self.org = xml_import_data.org
self.course = xml_import_data.course
self.default_class = xml_import_data.default_class
self._descriptors = {}
super(InMemorySystem, self).__init__(
policy=xml_import_data.policy,
process_xml=self.process_xml,
load_item=self.load_item,
error_tracker=Mock(),
resources_fs=xml_import_data.filesystem,
mixins=xml_import_data.xblock_mixins,
render_template=lambda template, context: pprint.pformat((template, context))
)
def process_xml(self, xml): # pylint: disable=method-hidden
"""Parse `xml` as an XModuleDescriptor, and add it to `self._descriptors`"""
descriptor = XModuleDescriptor.load_from_xml(xml, self, self.org, self.course, self.default_class)
self._descriptors[descriptor.location.url()] = descriptor
return descriptor
def load_item(self, location): # pylint: disable=method-hidden
"""Return the descriptor loaded for `location`"""
return self._descriptors[location]
class XModuleXmlImportTest(object):
"""Base class for tests that use basic `XModuleDescriptor.load_from_xml` xml parsing"""
def process_xml(self, xml_import_data):
"""Use the `xml_import_data` to import an :class:`XModuleDescriptor` from xml"""
system = InMemorySystem(xml_import_data)
return system.process_xml(xml_import_data.xml_string)
"""
Factories for generating edXML for testing XModule import
"""
import inspect
from fs.memoryfs import MemoryFS
from factory import Factory, lazy_attribute, post_generation, Sequence
from lxml import etree
from xmodule.modulestore.inheritance import InheritanceMixin
class XmlImportData(object):
"""
Class to capture all of the data needed to actually run an XML import,
so that the Factories have something to generate
"""
def __init__(self, xml_node, xml=None, org=None, course=None,
default_class=None, policy=None,
filesystem=None, parent=None,
xblock_mixins=()):
self._xml_node = xml_node
self._xml_string = xml
self.org = org
self.course = course
self.default_class = default_class
self.filesystem = filesystem
self.xblock_mixins = xblock_mixins
self.parent = parent
if policy is None:
self.policy = {}
else:
self.policy = policy
@property
def xml_string(self):
"""Return the stringified version of the generated xml"""
if self._xml_string is not None:
return self._xml_string
return etree.tostring(self._xml_node)
def __repr__(self):
return u"XmlImportData{!r}".format((
self._xml_node, self._xml_string, self.org,
self.course, self.default_class, self.policy,
self.filesystem, self.parent, self.xblock_mixins
))
# Extract all argument names used to construct XmlImportData objects,
# so that the factory doesn't treat them as XML attributes
XML_IMPORT_ARGS = inspect.getargspec(XmlImportData.__init__).args
class XmlImportFactory(Factory):
"""
Factory for generating XmlImportData's, which can hold all the data needed
to run an XModule XML import
"""
FACTORY_FOR = XmlImportData
filesystem = MemoryFS()
xblock_mixins = (InheritanceMixin,)
url_name = Sequence(str)
attribs = {}
policy = {}
tag = 'unknown'
@classmethod
def _adjust_kwargs(cls, **kwargs):
"""
Adjust the kwargs to be passed to the generated class.
Any kwargs that match :fun:`XmlImportData.__init__` will be passed
through. Any other unknown `kwargs` will be treated as XML attributes
:param tag: xml tag for the generated :class:`Element` node
:param text: (Optional) specifies the text of the generated :class:`Element`.
:param policy: (Optional) specifies data for the policy json file for this node
:type policy: dict
:param attribs: (Optional) specify attributes for the XML node
:type attribs: dict
"""
tag = kwargs.pop('tag', 'unknown')
kwargs['policy'] = {'{tag}/{url_name}'.format(tag=tag, url_name=kwargs['url_name']): kwargs['policy']}
kwargs['xml_node'].text = kwargs.pop('text', None)
kwargs['xml_node'].attrib.update(kwargs.pop('attribs', {}))
for key in kwargs.keys():
if key not in XML_IMPORT_ARGS:
kwargs['xml_node'].set(key, kwargs.pop(key))
return kwargs
@lazy_attribute
def xml_node(self):
"""An :class:`xml.etree.Element`"""
return etree.Element(self.tag)
@post_generation
def parent(self, _create, extracted, **_):
"""Hook to merge this xml into a parent xml node"""
if extracted is None:
return
extracted._xml_node.append(self._xml_node) # pylint: disable=no-member, protected-access
extracted.policy.update(self.policy)
class CourseFactory(XmlImportFactory):
"""Factory for <course> nodes"""
tag = 'course'
class SequenceFactory(XmlImportFactory):
"""Factory for <sequential> nodes"""
tag = 'sequential'
class ProblemFactory(XmlImportFactory):
"""Factory for <problem> nodes"""
tag = 'problem'
text = '<h1>Empty Problem!</h1>'
"""
Test that inherited fields work correctly when parsing XML
"""
from nose.tools import assert_equals # pylint: disable=no-name-in-module
from xmodule.tests.xml import XModuleXmlImportTest
from xmodule.tests.xml.factories import CourseFactory, SequenceFactory, ProblemFactory
class TestInheritedFieldParsing(XModuleXmlImportTest):
"""
Test that inherited fields work correctly when parsing XML
"""
def test_null_string(self):
# Test that the string inherited fields are passed through 'deserialize_field',
# which converts the string "null" to the python value None
root = CourseFactory.build(days_early_for_beta="null")
sequence = SequenceFactory.build(parent=root)
ProblemFactory.build(parent=sequence)
course = self.process_xml(root)
assert_equals(None, course.days_early_for_beta)
sequence = course.get_children()[0]
assert_equals(None, sequence.days_early_for_beta)
problem = sequence.get_children()[0]
assert_equals(None, problem.days_early_for_beta)
"""
Tests that policy json files import correctly when loading XML
"""
from nose.tools import assert_equals, assert_raises # pylint: disable=no-name-in-module
from xmodule.tests.xml.factories import CourseFactory
from xmodule.tests.xml import XModuleXmlImportTest
class TestPolicy(XModuleXmlImportTest):
"""
Tests that policy json files import correctly when loading xml
"""
def test_no_attribute_mapping(self):
# Policy files are json, and thus the values aren't passed through 'deserialize_field'
# Therefor, the string 'null' is passed unchanged to the Float field, which will trigger
# a ValueError
with assert_raises(ValueError):
course = self.process_xml(CourseFactory.build(policy={'days_early_for_beta': 'null'}))
# Trigger the exception by looking at the imported data
course.days_early_for_beta # pylint: disable=pointless-statement
def test_course_policy(self):
course = self.process_xml(CourseFactory.build(policy={'days_early_for_beta': None}))
assert_equals(None, course.days_early_for_beta)
course = self.process_xml(CourseFactory.build(policy={'days_early_for_beta': 9}))
assert_equals(9, course.days_early_for_beta)
......@@ -24,7 +24,7 @@ from django.conf import settings
from xmodule.x_module import XModule
from xmodule.editing_module import TabsEditingDescriptor
from xmodule.raw_module import EmptyDataRawDescriptor
from xmodule.xml_module import is_pointer_tag, name_to_pathname
from xmodule.xml_module import is_pointer_tag, name_to_pathname, deserialize_field
from xmodule.modulestore import Location
from xblock.fields import Scope, String, Boolean, Float, List, Integer, ScopeIds
......@@ -217,7 +217,7 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor
# For backwards compatibility -- if we've got XML data, parse
# it out and set the metadata fields
if self.data:
field_data = VideoDescriptor._parse_video_xml(self.data)
field_data = self._parse_video_xml(self.data)
self._field_data.set_many(self, field_data)
del self.data
......@@ -241,7 +241,7 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor
if is_pointer_tag(xml_object):
filepath = cls._format_filepath(xml_object.tag, name_to_pathname(url_name))
xml_data = etree.tostring(cls.load_file(filepath, system.resources_fs, location))
field_data = VideoDescriptor._parse_video_xml(xml_data)
field_data = cls._parse_video_xml(xml_data)
field_data['location'] = location
kvs = InheritanceKeyValueStore(initial_values=field_data)
field_data = DbModel(kvs)
......@@ -292,8 +292,8 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor
xml.append(ele)
return xml
@staticmethod
def _parse_youtube(data):
@classmethod
def _parse_youtube(cls, data):
"""
Parses a string of Youtube IDs such as "1.0:AXdE34_U,1.5:VO3SxfeD"
into a dictionary. Necessary for backwards compatibility with
......@@ -310,14 +310,14 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor
# 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])
youtube_id = deserialize_field(cls.youtube_id_1_0, pieces[1])
ret[speed] = youtube_id
except (ValueError, IndexError):
log.warning('Invalid YouTube ID: %s' % video)
return ret
@staticmethod
def _parse_video_xml(xml_data):
@classmethod
def _parse_video_xml(cls, xml_data):
"""
Parse video fields out of xml_data. The fields are set if they are
present in the XML.
......@@ -326,8 +326,8 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor
field_data = {}
conversions = {
'start_time': VideoDescriptor._parse_time,
'end_time': VideoDescriptor._parse_time
'start_time': cls._parse_time,
'end_time': cls._parse_time
}
# Convert between key names for certain attributes --
......@@ -349,10 +349,10 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor
for attr, value in xml.items():
if attr in compat_keys:
attr = compat_keys[attr]
if attr in VideoDescriptor.metadata_to_strip + ('url_name', 'name'):
if attr in cls.metadata_to_strip + ('url_name', 'name'):
continue
if attr == 'youtube':
speeds = VideoDescriptor._parse_youtube(value)
speeds = cls._parse_youtube(value)
for speed, youtube_id in speeds.items():
# should have made these youtube_id_1_00 for
# cleanliness, but hindsight doesn't need glasses
......@@ -367,20 +367,13 @@ class VideoDescriptor(VideoFields, TabsEditingDescriptor, EmptyDataRawDescriptor
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)
value = deserialize_field(cls.fields[attr], value)
field_data[attr] = value
return field_data
@classmethod
def _deserialize(cls, attr, value):
"""
Handles deserializing values that may have been encoded with json.dumps.
"""
return cls.get_map_for_field(attr).from_xml(value)
@staticmethod
def _parse_time(str_time):
def _parse_time(cls, str_time):
"""Converts s in '12:34:45' format to seconds. If s is
None, returns empty string"""
if not str_time:
......
......@@ -62,23 +62,6 @@ def get_metadata_from_xml(xml_object, remove=True):
xml_object.remove(meta)
return dmdata
_AttrMapBase = namedtuple('_AttrMap', 'from_xml to_xml')
class AttrMap(_AttrMapBase):
"""
A class that specifies two functions:
from_xml: convert value from the xml representation into
an internal python representation
to_xml: convert the internal python representation into
the value to store in the xml.
"""
def __new__(_cls, from_xml=lambda x: x,
to_xml=lambda x: x):
return _AttrMapBase.__new__(_cls, from_xml, to_xml)
def serialize_field(value):
"""
......@@ -167,20 +150,6 @@ class XmlDescriptor(XModuleDescriptor):
metadata_to_export_to_policy = ('discussion_topics', 'checklists')
@classmethod
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.
"""
if attr in cls.fields:
from_xml = lambda val: deserialize_field(cls.fields[attr], val)
to_xml = lambda val: serialize_field(val)
return AttrMap(from_xml, to_xml)
else:
return AttrMap()
@classmethod
def definition_from_xml(cls, xml_object, system):
"""
Return the definition to be passed to the newly created descriptor
......@@ -274,19 +243,19 @@ class XmlDescriptor(XModuleDescriptor):
Returns a dictionary {key: value}.
"""
metadata = {}
for attr in xml_object.attrib:
val = xml_object.get(attr)
if val is not None:
# VS[compat]. Remove after all key translations done
attr = cls._translate(attr)
if attr in cls.metadata_to_strip:
# don't load these
continue
attr_map = cls.get_map_for_field(attr)
metadata[attr] = attr_map.from_xml(val)
metadata = {'xml_attributes': {}}
for attr, val in xml_object.attrib.iteritems():
# VS[compat]. Remove after all key translations done
attr = cls._translate(attr)
if attr in cls.metadata_to_strip:
# don't load these
continue
if attr not in cls.fields:
metadata['xml_attributes'][attr] = val
else:
metadata[attr] = deserialize_field(cls.fields[attr], val)
return metadata
@classmethod
......@@ -295,9 +264,14 @@ class XmlDescriptor(XModuleDescriptor):
Add the keys in policy to metadata, after processing them
through the attrmap. Updates the metadata dict in place.
"""
for attr in policy:
attr_map = cls.get_map_for_field(attr)
metadata[cls._translate(attr)] = attr_map.from_xml(policy[attr])
for attr, value in policy.iteritems():
attr = cls._translate(attr)
if attr not in cls.fields:
# Store unknown attributes coming from policy.json
# in such a way that they will export to xml unchanged
metadata['xml_attributes'][attr] = value
else:
metadata[attr] = value
@classmethod
def from_xml(cls, xml_data, system, org=None, course=None):
......@@ -357,11 +331,7 @@ class XmlDescriptor(XModuleDescriptor):
field_data.update(definition)
field_data['children'] = children
field_data['xml_attributes'] = {}
field_data['xml_attributes']['filename'] = definition.get('filename', ['', None]) # for git link
for key, value in metadata.items():
if key not in cls.fields:
field_data['xml_attributes'][key] = value
field_data['location'] = location
field_data['category'] = xml_object.tag
kvs = InheritanceKeyValueStore(initial_values=field_data)
......@@ -415,18 +385,11 @@ class XmlDescriptor(XModuleDescriptor):
# Set the tag so we get the file path right
xml_object.tag = self.category
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)
return attr_map.to_xml(self._field_data.get(self, attr))
# Add the non-inherited metadata
for attr in sorted(own_metadata(self)):
# don't want e.g. data_dir
if attr not in self.metadata_to_strip and attr not in self.metadata_to_export_to_policy:
val = val_for_xml(attr)
val = serialize_field(self._field_data.get(self, attr))
try:
xml_object.set(attr, val)
except Exception, e:
......
......@@ -306,7 +306,7 @@ def get_module_for_descriptor_internal(user, descriptor, field_data_cache, cours
# Construct the key for the module
key = KeyValueStore.Key(
scope=Scope.user_state,
student_id=user.id,
user_id=user.id,
block_scope_id=descriptor.location,
field_name='grade'
)
......
......@@ -89,6 +89,9 @@ evaluation=10.0 - ((float(5 * error + warning + refactor + convention) / stateme
# evaluation report (RP0004).
comment=no
# Display symbolic names of messages in reports
symbols=yes
[TYPECHECK]
......@@ -120,7 +123,10 @@ generated-members=
content,
status_code,
# For factory_boy factories
create
create,
build,
# For xblocks
fields,
[BASIC]
......
......@@ -47,7 +47,7 @@ Pillow==1.7.8
pip>=1.4
polib==1.0.3
pycrypto>=2.6
pygments==1.5
pygments==1.6
pygraphviz==1.1
pymongo==2.4.1
pyparsing==1.5.6
......
......@@ -14,8 +14,8 @@
-e git+https://github.com/eventbrite/zendesk.git@d53fe0e81b623f084e91776bcf6369f8b7b63879#egg=zendesk
# Our libraries:
-e git+https://github.com/edx/XBlock.git@aa0d60627#egg=XBlock
-e git+https://github.com/edx/XBlock.git@a8de02c0#egg=XBlock
-e git+https://github.com/edx/codejail.git@0a1b468#egg=codejail
-e git+https://github.com/edx/diff-cover.git@v0.2.3#egg=diff_cover
-e git+https://github.com/edx/diff-cover.git@v0.2.4#egg=diff_cover
-e git+https://github.com/edx/js-test-tool.git@v0.0.7#egg=js_test_tool
-e git+https://github.com/edx/django-waffle.git@823a102e48#egg=django-waffle
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment