[TNL-5001] Import-export issues when importing legacy discussion OLX format

Observed Problems:
1. Discussion categories and targets were missing
2. When imported over existing course Discussion IDs have changed unexpectedly - all posts were missing

Solutions:
* Parsing legacy discussion OLX
* Do not force exporting discussion ID

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')

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,

openedx/core/lib/xblock_builtin/xblock_discussion/tests.py

+""" 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)

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)