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 7ebef5e4d5..042198113f 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 eacfb2037f..fa70cc3634 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 0000000000..b02e089041 --- /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 = """ + + """.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 = """""" + xblock_definition_xml = """ + """.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 50e1c57840..0f55f48bd8 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)