diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 1e206565cd9508eb2097156105e7d250e86efd40..823aced17b620ee1e1d204f331520af2ca3be403 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/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( diff --git a/common/lib/xmodule/xmodule/discussion_module.py b/common/lib/xmodule/xmodule/discussion_module.py index 4e952ec983647790137e27582314d50b999373a1..ed787fb296a3510455292ae62a788a48cd8b9d42 100644 --- a/common/lib/xmodule/xmodule/discussion_module.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_migrator.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_migrator.py index 442a98d857787dc4f5f362a70e544c79e268c32c..3b97df2b90b96d298ec5128fe7b864b2f115d0d3 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_migrator.py +++ b/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),