Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

Merge pull request #13095 from open-craft/discussion-export-hotfix

Browse Source 
Discussion import/export fix (TNL-5001)
This commit is contained in:
alisan617
2016-07-26 09:07:28 -04:00
committed by GitHub
parent bc6a0603d4 1f9dfe6573
commit b5dc83f101
4 changed files with 264 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py
View File

@@ -121,6 +121,9 @@ class CrossStoreXMLRoundtrip(CourseComparisonTest, PartitionTestCase):
self.exclude_field(None, 'wiki_slug')
self.exclude_field(None, 'xml_attributes')
self.exclude_field(None, 'parent')
# discussion_ids are auto-generated based on usage_id, so they should change across
# modulestores - see TNL-5001
self.exclude_field(None, 'discussion_id')
self.ignore_asset_key('_id')
self.ignore_asset_key('uploadDate')
self.ignore_asset_key('content_son')

22
common/lib/xmodule/xmodule/xml_module.py
View File

@@ -335,7 +335,7 @@ class XmlParserMixin(object):
"""
# VS[compat] -- just have the url_name lookup, once translation is done
url_name = node.get('url_name', node.get('slug'))
url_name = cls._get_url_name(node)
def_id = id_generator.create_definition(node.tag, url_name)
usage_id = id_generator.create_usage(def_id)
aside_children = []
@@ -344,8 +344,7 @@ class XmlParserMixin(object):
if is_pointer_tag(node):
# new style:
# read the actual definition file--named using url_name.replace(':','/')
filepath = cls._format_filepath(node.tag, name_to_pathname(url_name))
definition_xml = cls.load_file(filepath, runtime.resources_fs, def_id)
definition_xml, filepath = cls.load_definition_xml(node, runtime, def_id)
aside_children = runtime.parse_asides(definition_xml, def_id, usage_id, id_generator)
else:
filepath = None
@@ -408,6 +407,23 @@ class XmlParserMixin(object):
return xblock
@classmethod
def _get_url_name(cls, node):
"""
Reads url_name attribute from the node
"""
return node.get('url_name', node.get('slug'))
@classmethod
def load_definition_xml(cls, node, runtime, def_id):
"""
Loads definition_xml stored in a dedicated file
"""
url_name = cls._get_url_name(node)
filepath = cls._format_filepath(node.tag, name_to_pathname(url_name))
definition_xml = cls.load_file(filepath, runtime.resources_fs, def_id)
return definition_xml, filepath
@classmethod
def _format_filepath(cls, category, name):
return u'{category}/{name}.{ext}'.format(category=category,

181
openedx/core/lib/xblock_builtin/xblock_discussion/tests.py Normal file
View File

@@ -0,0 +1,181 @@
""" Tests for DiscussionXBLock"""
from collections import namedtuple
import ddt
import itertools
import mock
from nose.plugins.attrib import attr
import random
import string
from unittest import TestCase
from safe_lxml import etree
from openedx.core.lib.xblock_builtin.xblock_discussion.xblock_discussion import DiscussionXBlock
from xblock.field_data import DictFieldData
from xblock.fields import ScopeIds, UNIQUE_ID, NO_CACHE_VALUE
from xblock.runtime import Runtime
AttributePair = namedtuple("AttributePair", ["name", "value"])
ID_ATTR_NAMES = ("discussion_id", "id",)
CATEGORY_ATTR_NAMES = ("discussion_category",)
TARGET_ATTR_NAMES = ("discussion_target", "for", )
def _random_string():
"""
Generates random string
"""
return ''.join(random.choice(string.lowercase, ) for _ in xrange(12))
def _make_attribute_test_cases():
"""
Builds test cases for attribute-dependent tests
"""
attribute_names = itertools.product(ID_ATTR_NAMES, CATEGORY_ATTR_NAMES, TARGET_ATTR_NAMES)
for id_attr, category_attr, target_attr in attribute_names:
yield (
AttributePair(id_attr, _random_string()),
AttributePair(category_attr, _random_string()),
AttributePair(target_attr, _random_string())
)
@attr('shard2')
@ddt.ddt
class DiscussionXBlockImportExportTests(TestCase):
"""
Import and export tests
"""
DISCUSSION_XBLOCK_LOCATION = "openedx.core.lib.xblock_builtin.xblock_discussion.xblock_discussion.DiscussionXBlock"
def setUp(self):
"""
Set up method
"""
super(DiscussionXBlockImportExportTests, self).setUp()
self.keys = ScopeIds("any_user", "discussion", "def_id", "usage_id")
self.runtime_mock = mock.Mock(spec=Runtime)
self.runtime_mock.construct_xblock_from_class = mock.Mock(side_effect=self._construct_xblock_mock)
self.runtime_mock.get_policy = mock.Mock(return_value={})
self.id_gen_mock = mock.Mock()
def _construct_xblock_mock(self, cls, keys): # pylint: disable=unused-argument
"""
Builds target xblock instance (DiscussionXBlock)
Signature-compatible with runtime.construct_xblock_from_class - can be used as a mock side-effect
"""
return DiscussionXBlock(self.runtime_mock, scope_ids=keys, field_data=DictFieldData({}))
@mock.patch(DISCUSSION_XBLOCK_LOCATION + ".load_definition_xml")
@ddt.unpack
@ddt.data(*list(_make_attribute_test_cases()))
def test_xblock_export_format(self, id_pair, category_pair, target_pair, patched_load_definition_xml):
"""
Test that xblock export XML format can be parsed preserving field values
"""
xblock_xml = """
<discussion
url_name="82bb87a2d22240b1adac2dfcc1e7e5e4" xblock-family="xblock.v1"
{id_attr}="{id_value}"
{category_attr}="{category_value}"
{target_attr}="{target_value}"
/>
""".format(
id_attr=id_pair.name, id_value=id_pair.value,
category_attr=category_pair.name, category_value=category_pair.value,
target_attr=target_pair.name, target_value=target_pair.value,
)
node = etree.fromstring(xblock_xml)
patched_load_definition_xml.side_effect = Exception("Irrelevant")
block = DiscussionXBlock.parse_xml(node, self.runtime_mock, self.keys, self.id_gen_mock)
try:
self.assertEqual(block.discussion_id, id_pair.value)
self.assertEqual(block.discussion_category, category_pair.value)
self.assertEqual(block.discussion_target, target_pair.value)
except AssertionError:
print xblock_xml
raise
@mock.patch(DISCUSSION_XBLOCK_LOCATION + ".load_definition_xml")
@ddt.unpack
@ddt.data(*(_make_attribute_test_cases()))
def test_legacy_export_format(self, id_pair, category_pair, target_pair, patched_load_definition_xml):
"""
Test that legacy export XML format can be parsed preserving field values
"""
xblock_xml = """<discussion url_name="82bb87a2d22240b1adac2dfcc1e7e5e4"/>"""
xblock_definition_xml = """
<discussion
{id_attr}="{id_value}"
{category_attr}="{category_value}"
{target_attr}="{target_value}"
/>""".format(
id_attr=id_pair.name, id_value=id_pair.value,
category_attr=category_pair.name, category_value=category_pair.value,
target_attr=target_pair.name, target_value=target_pair.value,
)
node = etree.fromstring(xblock_xml)
definition_node = etree.fromstring(xblock_definition_xml)
patched_load_definition_xml.return_value = (definition_node, "irrelevant")
block = DiscussionXBlock.parse_xml(node, self.runtime_mock, self.keys, self.id_gen_mock)
try:
self.assertEqual(block.discussion_id, id_pair.value)
self.assertEqual(block.discussion_category, category_pair.value)
self.assertEqual(block.discussion_target, target_pair.value)
except AssertionError:
print xblock_xml, xblock_definition_xml
raise
def test_export_default_discussion_id(self):
"""
Test that default discussion_id values are not exported.
Historically, the OLX format allowed omitting discussion ID values; in such case, the IDs are generated
deterministically based on the course ID and the usage ID. Moreover, Studio does not allow course authors
to edit discussion_id, so all courses authored in Studio have discussion_id omitted in OLX.
Forcing Studio to always export discussion_id can cause data loss when switching between an older and newer
export, in a course with a course ID different from the one from which the export was created - because the
discussion ID would be different.
"""
target_node = etree.Element('dummy')
block = DiscussionXBlock(self.runtime_mock, scope_ids=self.keys, field_data=DictFieldData({}))
discussion_id_field = block.fields['discussion_id']
# precondition checks - discussion_id does not have a value and uses UNIQUE_ID
self.assertEqual(
discussion_id_field._get_cached_value(block), # pylint: disable=protected-access
NO_CACHE_VALUE
)
self.assertEqual(discussion_id_field.default, UNIQUE_ID)
block.add_xml_to_node(target_node)
self.assertEqual(target_node.tag, "discussion")
self.assertNotIn("discussion_id", target_node.attrib)
@ddt.data("jediwannabe", "iddqd", "itisagooddaytodie")
def test_export_custom_discussion_id(self, discussion_id):
"""
Test that custom discussion_id values are exported
"""
target_node = etree.Element('dummy')
block = DiscussionXBlock(self.runtime_mock, scope_ids=self.keys, field_data=DictFieldData({}))
block.discussion_id = discussion_id
# precondition check
self.assertEqual(block.discussion_id, discussion_id)
block.add_xml_to_node(target_node)
self.assertEqual(target_node.tag, "discussion")
self.assertTrue(target_node.attrib["discussion_id"], discussion_id)

63
openedx/core/lib/xblock_builtin/xblock_discussion/xblock_discussion.py
View File

@@ -6,10 +6,12 @@ import logging
from xblockutils.resources import ResourceLoader
from xblockutils.studio_editable import StudioEditableXBlockMixin
from xmodule.raw_module import RawDescriptor
from xblock.core import XBlock
from xblock.fields import Scope, String, UNIQUE_ID
from xblock.fragment import Fragment
from xmodule.xml_module import XmlParserMixin
log = logging.getLogger(__name__)
loader = ResourceLoader(__name__) # pylint: disable=invalid-name
@@ -24,11 +26,11 @@ def _(text):
@XBlock.needs('user')
@XBlock.needs('i18n')
class DiscussionXBlock(XBlock, StudioEditableXBlockMixin):
class DiscussionXBlock(XBlock, StudioEditableXBlockMixin, XmlParserMixin):
"""
Provides a discussion forum that is inline with other content in the courseware.
"""
discussion_id = String(scope=Scope.settings, default=UNIQUE_ID, force_export=True)
discussion_id = String(scope=Scope.settings, default=UNIQUE_ID)
display_name = String(
display_name=_("Display Name"),
help=_("Display name for this component"),
@@ -59,6 +61,11 @@ class DiscussionXBlock(XBlock, StudioEditableXBlockMixin):
has_author_view = True # Tells Studio to use author_view
# support for legacy OLX format - consumed by XmlParserMixin.load_metadata
metadata_translations = dict(RawDescriptor.metadata_translations)
metadata_translations['id'] = 'discussion_id'
metadata_translations['for'] = 'discussion_target'
@property
def course_key(self):
"""
@@ -134,3 +141,55 @@ class DiscussionXBlock(XBlock, StudioEditableXBlockMixin):
Returns a JSON representation of the student_view of this XBlock.
"""
return {'topic_id': self.discussion_id}
@classmethod
def parse_xml(cls, node, runtime, keys, id_generator):
"""
Parses OLX into XBlock.
This method is overridden here to allow parsing legacy OLX, coming from discussion XModule.
XBlock stores all the associated data, fields and children in a XML element inlined into vertical XML file
XModule stored only minimal data on the element included into vertical XML and used a dedicated "discussion"
folder in OLX to store fields and children. Also, some info was put into "policy.json" file.
If no external data sources are found (file in "discussion" folder), it is exactly equivalent to base method
XBlock.parse_xml. Otherwise this method parses file in "discussion" folder (known as definition_xml), applies
policy.json and updates fields accordingly.
"""
block = super(DiscussionXBlock, cls).parse_xml(node, runtime, keys, id_generator)
cls._apply_translations_to_node_attributes(block, node)
cls._apply_metadata_and_policy(block, node, runtime)
return block
@classmethod
def _apply_translations_to_node_attributes(cls, block, node):
"""
Applies metadata translations for attributes stored on an inlined XML element.
"""
for old_attr, target_attr in cls.metadata_translations.iteritems():
if old_attr in node.attrib and hasattr(block, target_attr):
setattr(block, target_attr, node.attrib[old_attr])
@classmethod
def _apply_metadata_and_policy(cls, block, node, runtime):
"""
Attempt to load definition XML from "discussion" folder in OLX, than parse it and update block fields
"""
try:
definition_xml, _ = cls.load_definition_xml(node, runtime, block.scope_ids.def_id)
except Exception as err: # pylint: disable=broad-except
log.info(
"Exception %s when trying to load definition xml for block %s - assuming XBlock export format",
err,
block
)
return
metadata = cls.load_metadata(definition_xml)
cls.apply_policy(metadata, runtime.get_policy(block.scope_ids.usage_id))
for field_name, value in metadata.iteritems():
if field_name in block.fields:
setattr(block, field_name, value)