From 18c45879bcc7e8ae5c4dc62248e26c23772cc167 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Wed, 2 Oct 2013 11:19:02 -0400 Subject: [PATCH 1/2] Test reproducing orphaning of children when draft is deleted and then parent published. ensure moving doesn't cause deletion --- .../xmodule/modulestore/tests/test_publish.py | 189 ++++++++++++++++++ 1 file changed, 189 insertions(+) create mode 100644 common/lib/xmodule/xmodule/modulestore/tests/test_publish.py diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py b/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py new file mode 100644 index 0000000000..3b535c6738 --- /dev/null +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py @@ -0,0 +1,189 @@ +""" +Test the publish code (primary causing orphans) +""" +import uuid +import mock +import unittest +import datetime +import random + +from xmodule.modulestore.inheritance import InheritanceMixin +from xmodule.modulestore.mongo import MongoModuleStore, DraftMongoModuleStore +from xmodule.modulestore import Location +from xmodule.fields import Date +from xmodule.modulestore.exceptions import ItemNotFoundError +from xmodule.modulestore.mongo.draft import DIRECT_ONLY_CATEGORIES + + +class TestPublish(unittest.TestCase): + """ + Test the publish code (primary causing orphans) + """ + + # Snippet of what would be in the django settings envs file + db_config = { + 'host': 'localhost', + 'db': 'test_xmodule', + } + + modulestore_options = dict({ + 'default_class': 'xmodule.raw_module.RawDescriptor', + 'fs_root': '', + 'render_template': mock.Mock(return_value=""), + 'xblock_mixins': (InheritanceMixin,) + }, **db_config) + + def setUp(self): + self.modulestore_options['collection'] = 'modulestore{0}'.format(uuid.uuid4().hex) + + self.old_mongo = MongoModuleStore(**self.modulestore_options) + self.draft_mongo = DraftMongoModuleStore(**self.modulestore_options) + self.addCleanup(self.tear_down_mongo) + self.course_location = None + + def tear_down_mongo(self): + # old_mongo doesn't give a db attr, but all of the dbs are the same and draft and pub use same collection + dbref = self.old_mongo.collection.database + dbref.drop_collection(self.old_mongo.collection) + dbref.connection.close() + + def _create_item(self, category, name, data, metadata, parent_category, parent_name, runtime): + """ + Create the item in either draft or direct based on category and attach to its parent. + """ + location = self.course_location.replace(category=category, name=name) + if category in DIRECT_ONLY_CATEGORIES: + mongo = self.old_mongo + else: + mongo = self.draft_mongo + mongo.create_and_save_xmodule(location, data, metadata, runtime) + if isinstance(data, basestring): + fields = {'data': data} + else: + fields = data.copy() + fields.update(metadata) + if parent_name: + # add child to parent in mongo + parent_location = self.course_location.replace(category=parent_category, name=parent_name) + parent = self.draft_mongo.get_item(parent_location) + parent.children.append(location.url()) + if parent_category in DIRECT_ONLY_CATEGORIES: + mongo = self.old_mongo + else: + mongo = self.draft_mongo + mongo.update_children(parent_location, parent.children) + + def _create_course(self): + """ + Create the course, publish all verticals + * some detached items + """ + date_proxy = Date() + metadata = { + 'start': date_proxy.to_json(datetime.datetime(2000, 3, 13, 4)), + 'display_name': 'Migration test course', + } + data = { + 'wiki_slug': 'test_course_slug' + } + fields = metadata.copy() + fields.update(data) + + self.course_location = Location('i4x', 'test_org', 'test_course', 'course', 'runid') + self.old_mongo.create_and_save_xmodule(self.course_location, data, metadata) + runtime = self.draft_mongo.get_item(self.course_location).runtime + + self._create_item('chapter', 'Chapter1', {}, {'display_name': 'Chapter 1'}, 'course', 'runid', runtime) + self._create_item('chapter', 'Chapter2', {}, {'display_name': 'Chapter 2'}, 'course', 'runid', runtime) + self._create_item('vertical', 'Vert1', {}, {'display_name': 'Vertical 1'}, 'chapter', 'Chapter1', runtime) + self._create_item('vertical', 'Vert2', {}, {'display_name': 'Vertical 2'}, 'chapter', 'Chapter1', runtime) + self._create_item('html', 'Html1', "

Goodbye

", {'display_name': 'Parented Html'}, 'vertical', 'Vert1', runtime) + self._create_item( + 'discussion', 'Discussion1', + "discussion discussion_category=\"Lecture 1\" discussion_id=\"a08bfd89b2aa40fa81f2c650a9332846\" discussion_target=\"Lecture 1\"/>\n", + { + "discussion_category": "Lecture 1", + "discussion_target": "Lecture 1", + "display_name": "Lecture 1 Discussion", + "discussion_id": "a08bfd89b2aa40fa81f2c650a9332846" + }, + 'vertical', 'Vert1', runtime + ) + self._create_item('html', 'Html2', "

Hellow

", {'display_name': 'Hollow Html'}, 'vertical', 'Vert1', runtime) + self._create_item( + 'discussion', 'Discussion2', + "discussion discussion_category=\"Lecture 2\" discussion_id=\"b08bfd89b2aa40fa81f2c650a9332846\" discussion_target=\"Lecture 2\"/>\n", + { + "discussion_category": "Lecture 2", + "discussion_target": "Lecture 2", + "display_name": "Lecture 2 Discussion", + "discussion_id": "b08bfd89b2aa40fa81f2c650a9332846" + }, + 'vertical', 'Vert2', runtime + ) + self._create_item('static_tab', 'staticuno', "

tab

", {'display_name': 'Tab uno'}, None, None, runtime) + self._create_item('about', 'overview', "

overview

", {}, None, None, runtime) + self._create_item('course_info', 'updates', "

  1. Sep 22

    test

", {}, None, None, runtime) + + def _xmodule_recurse(self, item, action): + """ + Applies action depth-first down tree and to item last. + + A copy of cms.djangoapps.contentstore.views.requests._xmodule_recurse to reproduce its use and behavior + outside of django. + """ + for child in item.get_children(): + self._xmodule_recurse(child, action) + + action(item) + + def test_publish_draft_delete(self): + """ + To reproduce a bug (STUD-811) publish a vertical, convert to draft, delete a child, move a child, publish. + See if deleted and moved children still is connected or exists in db (bug was disconnected but existed) + """ + self._create_course() + userid = random.getrandbits(32) + location = self.course_location.replace(category='vertical', name='Vert1') + item = self.draft_mongo.get_item(location, 2) + self._xmodule_recurse( + item, + lambda i: self.draft_mongo.publish(i.location, userid) + ) + # verify status + item = self.draft_mongo.get_item(location, 0) + self.assertFalse(getattr(item, 'is_draft', False), "Item was published. Draft should not exist") + # however, children are still draft, but I'm not sure that's by design + + # convert back to draft + self.draft_mongo.convert_to_draft(location) + # both draft and published should exist + draft_vert = self.draft_mongo.get_item(location, 0) + self.assertTrue(getattr(draft_vert, 'is_draft', False), "Item was converted to draft but doesn't say so") + item = self.old_mongo.get_item(location, 0) + self.assertFalse(getattr(item, 'is_draft', False), "Published item doesn't say so") + + # delete the discussion (which oddly is not in draft mode) + location = self.course_location.replace(category='discussion', name='Discussion1') + self.draft_mongo.delete_item(location) + # remove pointer from draft vertical (verify presence first to ensure process is valid) + self.assertIn(location.url(), draft_vert.children) + draft_vert.children.remove(location.url()) + # move the other child + other_child_loc = self.course_location.replace(category='html', name='Html2') + draft_vert.children.remove(other_child_loc.url()) + other_vert = self.draft_mongo.get_item(self.course_location.replace(category='vertical', name='Vert2'), 0) + other_vert.children.append(other_child_loc.url()) + self.draft_mongo.update_children(draft_vert.location, draft_vert.children) + self.draft_mongo.update_children(other_vert.location, other_vert.children) + # publish + self._xmodule_recurse( + draft_vert, + lambda i: self.draft_mongo.publish(i.location, userid) + ) + item = self.old_mongo.get_item(draft_vert.location, 0) + self.assertNotIn(location.url(), item.children) + with self.assertRaises(ItemNotFoundError): + self.draft_mongo.get_item(location) + self.assertNotIn(other_child_loc.url(), item.children) + self.assertTrue(self.draft_mongo.has_item(None, other_child_loc), "Oops, lost moved item") From 5e1f03bb52262373bebabbadb369b10bbf36b1e8 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Wed, 2 Oct 2013 14:27:23 -0400 Subject: [PATCH 2/2] Publish removes deleted children --- .../lib/xmodule/xmodule/modulestore/mongo/draft.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index b816ab246d..f7a0250155 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py @@ -234,12 +234,26 @@ class DraftModuleStore(MongoModuleStore): """ Save a current draft to the underlying modulestore """ + try: + original_published = super(DraftModuleStore, self).get_item(location) + except ItemNotFoundError: + original_published = None + draft = self.get_item(location) draft.published_date = datetime.now(UTC) draft.published_by = published_by_id super(DraftModuleStore, self).update_item(location, draft.get_explicitly_set_fields_by_scope(Scope.content)) if draft.has_children: + if original_published is not None: + # see if children were deleted. 2 reasons for children lists to differ: + # 1) child deleted + # 2) child moved + for child in original_published.children: + if child not in draft.children: + rents = [Location(mom) for mom in self.get_parent_locations(child, None)] + if (len(rents) == 1 and rents[0] == Location(location)): # the 1 is this original_published + self.delete_item(child, True) super(DraftModuleStore, self).update_children(location, draft.children) super(DraftModuleStore, self).update_metadata(location, own_metadata(draft)) self.delete_item(location)