diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py b/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py
index 7ebef5e4d51df46157209ebdd35f654a786debcb..042198113fbceb510a351167db6644a4a282114e 100644
--- a/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py
+++ b/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py
@@ -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')
diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py
index eacfb2037f803faede4ef8e63d7261266509c3c0..fa70cc36344b03b7ed431dd7ef582c4b841ed806 100644
--- a/common/lib/xmodule/xmodule/xml_module.py
+++ b/common/lib/xmodule/xmodule/xml_module.py
@@ -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,
diff --git a/openedx/core/lib/xblock_builtin/xblock_discussion/tests.py b/openedx/core/lib/xblock_builtin/xblock_discussion/tests.py
new file mode 100644
index 0000000000000000000000000000000000000000..b02e08904129f40045b5c53ff60519df39ae7652
--- /dev/null
+++ b/openedx/core/lib/xblock_builtin/xblock_discussion/tests.py
@@ -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)
diff --git a/openedx/core/lib/xblock_builtin/xblock_discussion/xblock_discussion.py b/openedx/core/lib/xblock_builtin/xblock_discussion/xblock_discussion.py
index 50e1c578401a043a795ab1ec1d09425cec68d7ac..0f55f48bd81ee11646d94962e35f1113aa6bde88 100644
--- a/openedx/core/lib/xblock_builtin/xblock_discussion/xblock_discussion.py
+++ b/openedx/core/lib/xblock_builtin/xblock_discussion/xblock_discussion.py
@@ -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)