Merge pull request #7489 from edx/hotfix/2015-03-26

default discussion modules' discussion_id field to xblock's UNIQUE_FIELD...

cms/djangoapps/contentstore/tests/test_contentstore.py

@@ -5,6 +5,7 @@ import mock
 from mock import patch
 import shutil
 import lxml.html
+import ddt
 
 from datetime import timedelta
 from fs.osfs import OSFS
@@ -976,6 +977,7 @@ class MiscCourseTests(ContentStoreTestCase):
             self.assertEqual(resp.status_code, 200)
 
 
+@ddt.ddt
 class ContentStoreTest(ContentStoreTestCase):
     """
     Tests for the CMS ContentStore application.
@@ -1433,13 +1435,29 @@ class ContentStoreTest(ContentStoreTestCase):
         # make sure we found the item (e.g. it didn't error while loading)
         self.assertTrue(did_load_item)
 
-    def test_forum_id_generation(self):
-        course = CourseFactory.create()
+    @ddt.data(ModuleStoreEnum.Type.split, ModuleStoreEnum.Type.mongo)
+    def test_forum_id_generation(self, default_store):
+        """
+        Test that a discussion item, even if it doesn't set its discussion_id,
+        consistently generates the same one
+        """
+        course = CourseFactory.create(default_store=default_store)
 
-        # crate a new module and add it as a child to a vertical
-        new_discussion_item = self.store.create_item(self.user.id, course.id, 'discussion', 'new_component')
+        # create a discussion item
+        discussion_item = self.store.create_item(self.user.id, course.id, 'discussion', 'new_component')
+
+        # now fetch it from the modulestore to instantiate its descriptor
+        fetched = self.store.get_item(discussion_item.location)
+
+        # refetch it to be safe
+        refetched = self.store.get_item(discussion_item.location)
+
+        # and make sure the same discussion items have the same discussion ids
+        self.assertEqual(fetched.discussion_id, discussion_item.discussion_id)
+        self.assertEqual(fetched.discussion_id, refetched.discussion_id)
 
-        self.assertNotEquals(new_discussion_item.discussion_id, '$$GUID$$')
+        # and make sure that the id isn't the old "$$GUID$$"
+        self.assertNotEqual(discussion_item.discussion_id, '$$GUID$$')
 
     def test_metadata_inheritance(self):
         course_items = import_course_from_xml(

common/lib/xmodule/xmodule/discussion_module.py

@@ -3,7 +3,7 @@ from pkg_resources import resource_string
 from xmodule.x_module import XModule
 from xmodule.raw_module import RawDescriptor
 from xmodule.editing_module import MetadataOnlyEditingDescriptor
-from xblock.fields import String, Scope
+from xblock.fields import String, Scope, UNIQUE_ID
 from uuid import uuid4
 
 # Make '_' a no-op so we can scrape strings
@@ -15,7 +15,7 @@ class DiscussionFields(object):
         display_name=_("Discussion Id"),
         help=_("The id is a unique identifier for the discussion. It is non editable."),
         scope=Scope.settings,
-        default="$$GUID$$")
+        default=UNIQUE_ID)
     display_name = String(
         display_name=_("Display Name"),
         help=_("Display name for this module"),
@@ -73,13 +73,6 @@ class DiscussionModule(DiscussionFields, XModule):
 
 class DiscussionDescriptor(DiscussionFields, MetadataOnlyEditingDescriptor, RawDescriptor):
 
-    def __init__(self, *args, **kwargs):
-        super(DiscussionDescriptor, self).__init__(*args, **kwargs)
-        # is this too late? i.e., will it get persisted and stay static w/ the first value
-        # any code references. I believe so.
-        if self.discussion_id == '$$GUID$$':
-            self.discussion_id = uuid4().hex
-
     module_class = DiscussionModule
     # The discussion XML format uses `id` and `for` attributes,
     # but these would overload other module attributes, so we prefix them

common/lib/xmodule/xmodule/modulestore/tests/test_split_migrator.py

@@ -8,7 +8,7 @@ import uuid
 import mock
 from nose.plugins.attrib import attr
 
-from xblock.fields import Reference, ReferenceList, ReferenceValueDict
+from xblock.fields import Reference, ReferenceList, ReferenceValueDict, UNIQUE_ID
 from xmodule.modulestore.split_migrator import SplitMigrator
 from xmodule.modulestore.tests.test_split_w_old_mongo import SplitWMongoCourseBoostrapper
 
@@ -164,7 +164,14 @@ class TestMigration(SplitWMongoCourseBoostrapper):
             self.assertEqual(presplit_dag_root.location.block_id, split_dag_root.location.block_id)
         # compare all fields but references
         for name, field in presplit_dag_root.fields.iteritems():
-            if not isinstance(field, (Reference, ReferenceList, ReferenceValueDict)):
+            # fields generated from UNIQUE_IDs are unique to an XBlock's scope,
+            # so if such a field is unset on an XBlock, we don't expect it
+            # to persist across courses
+            field_generated_from_unique_id = not field.is_set_on(presplit_dag_root) and field.default == UNIQUE_ID
+            should_check_field = not (
+                field_generated_from_unique_id or isinstance(field, (Reference, ReferenceList, ReferenceValueDict))
+            )
+            if should_check_field:
                 self.assertEqual(
                     getattr(presplit_dag_root, name),
                     getattr(split_dag_root, name),