Merge pull request #2356 from edx/dhm/mixed_ms_wrapper

MixedModulestore wraps most getters, update_item, delete_item

CHANGELOG.rst

@@ -52,6 +52,12 @@ Blades: Fix comparison of float numbers. BLD-434.
 
 Blades: Allow regexp strings as the correct answer to a string response question. BLD-475.
 
+Common: MixedModulestore is now the only approved access to the persistence layer
+  - takes a new parameter 'reference_type' which can be 'Location' or 'Locator'. Mixed
+  then tries to ensure that every reference in any xblock gets converted to that type on
+  retrieval. Because we're moving to Locators, the default is Locator; so, you should change
+  all existing configurations to 'Location' (unless you're using split)
+
 Common: Add feature flags to allow developer use of pure XBlocks
   - ALLOW_ALL_ADVANCED_COMPONENTS disables the hard-coded list of advanced
     components in Studio, and allows any xblock to be added as an

cms/djangoapps/contentstore/course_info_model.py

@@ -56,7 +56,7 @@ def get_course_updates(location, provided_id):
     return course_upd_collection
 
 
-def update_course_updates(location, update, passed_id=None):
+def update_course_updates(location, update, passed_id=None, user=None):
     """
     Either add or update the given course update. It will add it if the passed_id is absent or None. It will update it if
     it has an passed_id which has a valid value. Until updates have distinct values, the passed_id is the location url + an index
@@ -102,7 +102,7 @@ def update_course_updates(location, update, passed_id=None):
 
     # update db record
     course_updates.data = html.tostring(course_html_parsed)
-    modulestore('direct').update_item(location, course_updates.data)
+    modulestore('direct').update_item(course_updates, user.id if user else None)
 
     return {
         "id": idx,
@@ -125,7 +125,7 @@ def _course_info_content(html_parsed):
 
 
 # pylint: disable=unused-argument
-def delete_course_update(location, update, passed_id):
+def delete_course_update(location, update, passed_id, user):
     """
     Delete the given course_info update from the db.
     Returns the resulting course_updates b/c their ids change.
@@ -158,7 +158,7 @@ def delete_course_update(location, update, passed_id):
         # update db record
         course_updates.data = html.tostring(course_html_parsed)
         store = modulestore('direct')
-        store.update_item(location, course_updates.data)
+        store.update_item(course_updates, user.id)
 
     return get_course_updates(location, None)
 

cms/djangoapps/contentstore/tests/test_checklists.py

@@ -54,7 +54,7 @@ class ChecklistTestCase(CourseTestCase):
         # Save the changed `checklists` to the underlying KeyValueStore before updating the modulestore
         self.course.save()
         modulestore = get_modulestore(self.course.location)
-        modulestore.update_metadata(self.course.location, own_metadata(self.course))
+        modulestore.update_item(self.course, self.user.id)
         self.assertEqual(self.get_persisted_checklists(), None)
         response = self.client.get(self.checklists_url)
         self.assertEqual(payload, response.content)

cms/djangoapps/contentstore/tests/test_contentstore.py

@@ -47,8 +47,6 @@ from xmodule.exceptions import NotFoundError
 
 from django_comment_common.utils import are_permissions_roles_seeded
 from xmodule.exceptions import InvalidVersionError
-import datetime
-from pytz import UTC
 from uuid import uuid4
 from pymongo import MongoClient
 from student.models import CourseEnrollment
@@ -126,11 +124,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
 
         course.advanced_modules = component_types
 
-        # Save the data that we've just changed to the underlying
-        # MongoKeyValueStore before we update the mongo datastore.
-        course.save()
-
-        store.update_metadata(course.location, own_metadata(course))
+        store.update_item(course, self.user.id)
 
         # just pick one vertical
         descriptor = store.get_items(Location('i4x', 'edX', 'simple', 'vertical', None, None))[0]
@@ -269,7 +263,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         self.assertIn('graceperiod', own_metadata(html_module))
         self.assertEqual(html_module.graceperiod, new_graceperiod)
 
-        draft_store.update_metadata(html_module.location, own_metadata(html_module))
+        draft_store.update_item(html_module, self.user.id)
 
         # read back to make sure it reads as 'own-metadata'
         html_module = draft_store.get_item(Location('i4x', 'edX', 'simple', 'html', 'test_html', None))
@@ -385,8 +379,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         self.assertEqual(course.tabs, expected_tabs)
 
         item.display_name = 'Updated'
-        item.save()
-        module_store.update_metadata(item.location, own_metadata(item))
+        module_store.update_item(item, self.user.id)
 
         course = module_store.get_item(course_location)
 
@@ -834,9 +827,9 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         html_module = module_store.get_instance(source_location.course_id, html_module_location)
 
         self.assertIsInstance(html_module.data, basestring)
-        new_data = html_module.data.replace('/static/', '/c4x/{0}/{1}/asset/'.format(
+        new_data = html_module.data = html_module.data.replace('/static/', '/c4x/{0}/{1}/asset/'.format(
             source_location.org, source_location.course))
-        module_store.update_item(html_module_location, new_data)
+        module_store.update_item(html_module, self.user.id)
 
         html_module = module_store.get_instance(source_location.course_id, html_module_location)
         self.assertEqual(new_data, html_module.data)
@@ -858,22 +851,18 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         draft_store = modulestore('draft')
         direct_store = modulestore('direct')
 
-        CourseFactory.create(org='MITx', course='999', display_name='Robot Super Course')
+        course = CourseFactory.create(org='MITx', course='999', display_name='Robot Super Course')
 
         location = Location('i4x://MITx/999/chapter/neuvo')
         # Ensure draft mongo store does not allow us to create chapters either directly or via convert to draft
         self.assertRaises(InvalidVersionError, draft_store.create_and_save_xmodule, location)
         direct_store.create_and_save_xmodule(location)
         self.assertRaises(InvalidVersionError, draft_store.convert_to_draft, location)
+        chapter = draft_store.get_instance(course.id, location)
+        chapter.data = 'chapter data'
 
-        self.assertRaises(InvalidVersionError, draft_store.update_item, location, 'chapter data')
-
-        # taking advantage of update_children and other functions never checking that the ids are valid
-        self.assertRaises(InvalidVersionError, draft_store.update_children, location,
-                          ['i4x://MITx/999/problem/doesntexist'])
-
-        self.assertRaises(InvalidVersionError, draft_store.update_metadata, location,
-                          {'due': datetime.datetime.now(UTC)})
+        with self.assertRaises(InvalidVersionError):
+            draft_store.update_item(chapter, self.user.id)
 
         self.assertRaises(InvalidVersionError, draft_store.unpublish, location)
 
@@ -992,8 +981,8 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         sequential = module_store.get_item(Location(['i4x', 'edX', 'toy',
                                            'sequential', 'vertical_sequential', None]))
         private_location_no_draft = private_vertical.location.replace(revision=None)
-        module_store.update_children(sequential.location, sequential.children +
-                                     [private_location_no_draft.url()])
+        sequential.children.append(private_location_no_draft.url())
+        module_store.update_item(sequential, self.user.id)
 
         # read back the sequential, to make sure we have a pointer to
         sequential = module_store.get_item(Location(['i4x', 'edX', 'toy',
@@ -1285,31 +1274,6 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         self.assertFalse(Location(['i4x', 'edX', 'toy', 'vertical', 'vertical_test', None])
                          in course.system.module_data)
 
-    def test_export_course_with_unknown_metadata(self):
-        module_store = modulestore('direct')
-        content_store = contentstore()
-
-        import_from_xml(module_store, 'common/test/data/', ['toy'])
-        location = CourseDescriptor.id_to_location('edX/toy/2012_Fall')
-
-        root_dir = path(mkdtemp_clean())
-
-        course = module_store.get_item(location)
-
-        metadata = own_metadata(course)
-        # add a bool piece of unknown metadata so we can verify we don't throw an exception
-        metadata['new_metadata'] = True
-
-        # Save the data that we've just changed to the underlying
-        # MongoKeyValueStore before we update the mongo datastore.
-        course.save()
-        module_store.update_metadata(location, metadata)
-
-        print 'Exporting to tempdir = {0}'.format(root_dir)
-
-        # export out to a tempdir
-        export_to_xml(module_store, content_store, location, root_dir, 'test_export')
-
     def test_export_course_without_content_store(self):
         module_store = modulestore('direct')
         content_store = contentstore()
@@ -1319,16 +1283,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         import_from_xml(module_store, 'common/test/data/', ['toy'])
         location = CourseDescriptor.id_to_location('edX/toy/2012_Fall')
 
-        # Add a sequence
-
         stub_location = Location(['i4x', 'edX', 'toy', 'sequential', 'vertical_sequential'])
-        sequential = module_store.get_item(stub_location)
-        module_store.update_children(sequential.location, sequential.children)
-
-        # Get course and export it without a content_store
-
-        course = module_store.get_item(location)
-        course.save()
 
         root_dir = path(mkdtemp_clean())
 
@@ -1343,7 +1298,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
             module_store, root_dir, ['test_export_no_content_store'],
             draft_store=None,
             static_content_store=None,
-            target_location_namespace=course.location
+            target_location_namespace=location
         )
 
         # Verify reimported course
@@ -1810,7 +1765,8 @@ class ContentStoreTest(ModuleStoreTestCase):
         # crate a new module and add it as a child to a vertical
         module_store.create_and_save_xmodule(new_component_location)
         parent = verticals[0]
-        module_store.update_children(parent.location, parent.children + [new_component_location.url()])
+        parent.children.append(new_component_location.url())
+        module_store.update_item(parent, self.user.id)
 
         # flush the cache
         module_store.refresh_cached_metadata_inheritance_tree(new_component_location)
@@ -1827,8 +1783,7 @@ class ContentStoreTest(ModuleStoreTestCase):
         # now let's define an override at the leaf node level
         #
         new_module.graceperiod = timedelta(1)
-        new_module.save()
-        module_store.update_metadata(new_module.location, own_metadata(new_module))
+        module_store.update_item(new_module, self.user.id)
 
         # flush the cache and refetch
         module_store.refresh_cached_metadata_inheritance_tree(new_component_location)
@@ -1942,10 +1897,7 @@ class MetadataSaveTestCase(ModuleStoreTestCase):
             delattr(self.video_descriptor, field_name)
 
         self.assertNotIn('html5_sources', own_metadata(self.video_descriptor))
-        get_modulestore(location).update_metadata(
-            location,
-            own_metadata(self.video_descriptor)
-        )
+        get_modulestore(location).update_item(self.video_descriptor, '**replace_user**')
         module = get_modulestore(location).get_item(location)
 
         self.assertNotIn('html5_sources', own_metadata(module))
@@ -2001,7 +1953,7 @@ def _course_factory_create_course():
     Creates a course via the CourseFactory and returns the locator for it.
     """
     course = CourseFactory.create(org='MITx', course='999', display_name='Robot Super Course')
-    return loc_mapper().translate_location(course.location.course_id, course.location, True, True)
+    return loc_mapper().translate_location(course.id, course.location, False, True)
 
 
 def _get_course_id(test_course_data):

cms/djangoapps/contentstore/tests/test_course_settings.py

@@ -73,32 +73,32 @@ class CourseDetailsTestCase(CourseTestCase):
         jsondetails.syllabus = "<a href='foo'>bar</a>"
         # encode - decode to convert date fields and other data which changes form
         self.assertEqual(
-            CourseDetails.update_from_json(self.course_locator, jsondetails.__dict__).syllabus,
+            CourseDetails.update_from_json(self.course_locator, jsondetails.__dict__, self.user).syllabus,
             jsondetails.syllabus, "After set syllabus"
         )
         jsondetails.overview = "Overview"
         self.assertEqual(
-            CourseDetails.update_from_json(self.course_locator, jsondetails.__dict__).overview,
+            CourseDetails.update_from_json(self.course_locator, jsondetails.__dict__, self.user).overview,
             jsondetails.overview, "After set overview"
         )
         jsondetails.intro_video = "intro_video"
         self.assertEqual(
-            CourseDetails.update_from_json(self.course_locator, jsondetails.__dict__).intro_video,
+            CourseDetails.update_from_json(self.course_locator, jsondetails.__dict__, self.user).intro_video,
             jsondetails.intro_video, "After set intro_video"
         )
         jsondetails.effort = "effort"
         self.assertEqual(
-            CourseDetails.update_from_json(self.course_locator, jsondetails.__dict__).effort,
+            CourseDetails.update_from_json(self.course_locator, jsondetails.__dict__, self.user).effort,
             jsondetails.effort, "After set effort"
         )
         jsondetails.start_date = datetime.datetime(2010, 10, 1, 0, tzinfo=UTC())
         self.assertEqual(
-            CourseDetails.update_from_json(self.course_locator, jsondetails.__dict__).start_date,
+            CourseDetails.update_from_json(self.course_locator, jsondetails.__dict__, self.user).start_date,
             jsondetails.start_date
         )
         jsondetails.course_image_name = "an_image.jpg"
         self.assertEqual(
-            CourseDetails.update_from_json(self.course_locator, jsondetails.__dict__).course_image_name,
+            CourseDetails.update_from_json(self.course_locator, jsondetails.__dict__, self.user).course_image_name,
             jsondetails.course_image_name
         )
 
@@ -120,8 +120,8 @@ class CourseDetailsTestCase(CourseTestCase):
 
             self.assertContains(response, "Introducing Your Course")
             self.assertContains(response, "Course Image")
-            self.assertNotContains(response,"Course Overview")
-            self.assertNotContains(response,"Course Introduction Video")
+            self.assertNotContains(response, "Course Overview")
+            self.assertNotContains(response, "Course Introduction Video")
             self.assertNotContains(response, "Requirements")
 
     def test_regular_site_fetch(self):
@@ -141,8 +141,8 @@ class CourseDetailsTestCase(CourseTestCase):
 
             self.assertContains(response, "Introducing Your Course")
             self.assertContains(response, "Course Image")
-            self.assertContains(response,"Course Overview")
-            self.assertContains(response,"Course Introduction Video")
+            self.assertContains(response, "Course Overview")
+            self.assertContains(response, "Course Introduction Video")
             self.assertContains(response, "Requirements")
 
 
@@ -241,67 +241,74 @@ class CourseGradingTest(CourseTestCase):
 
     def test_update_from_json(self):
         test_grader = CourseGradingModel.fetch(self.course_locator)
-        altered_grader = CourseGradingModel.update_from_json(self.course_locator, test_grader.__dict__)
+        altered_grader = CourseGradingModel.update_from_json(self.course_locator, test_grader.__dict__, self.user)
         self.assertDictEqual(test_grader.__dict__, altered_grader.__dict__, "Noop update")
 
         test_grader.graders[0]['weight'] = test_grader.graders[0].get('weight') * 2
-        altered_grader = CourseGradingModel.update_from_json(self.course_locator, test_grader.__dict__)
+        altered_grader = CourseGradingModel.update_from_json(self.course_locator, test_grader.__dict__, self.user)
         self.assertDictEqual(test_grader.__dict__, altered_grader.__dict__, "Weight[0] * 2")
 
         test_grader.grade_cutoffs['D'] = 0.3
-        altered_grader = CourseGradingModel.update_from_json(self.course_locator, test_grader.__dict__)
+        altered_grader = CourseGradingModel.update_from_json(self.course_locator, test_grader.__dict__, self.user)
         self.assertDictEqual(test_grader.__dict__, altered_grader.__dict__, "cutoff add D")
 
         test_grader.grace_period = {'hours': 4, 'minutes': 5, 'seconds': 0}
-        altered_grader = CourseGradingModel.update_from_json(self.course_locator, test_grader.__dict__)
+        altered_grader = CourseGradingModel.update_from_json(self.course_locator, test_grader.__dict__, self.user)
         self.assertDictEqual(test_grader.__dict__, altered_grader.__dict__, "4 hour grace period")
 
     def test_update_grader_from_json(self):
         test_grader = CourseGradingModel.fetch(self.course_locator)
-        altered_grader = CourseGradingModel.update_grader_from_json(self.course_locator, test_grader.graders[1])
+        altered_grader = CourseGradingModel.update_grader_from_json(
+            self.course_locator, test_grader.graders[1], self.user
+        )
         self.assertDictEqual(test_grader.graders[1], altered_grader, "Noop update")
 
         test_grader.graders[1]['min_count'] = test_grader.graders[1].get('min_count') + 2
-        altered_grader = CourseGradingModel.update_grader_from_json(self.course_locator, test_grader.graders[1])
+        altered_grader = CourseGradingModel.update_grader_from_json(
+            self.course_locator, test_grader.graders[1], self.user)
         self.assertDictEqual(test_grader.graders[1], altered_grader, "min_count[1] + 2")
 
         test_grader.graders[1]['drop_count'] = test_grader.graders[1].get('drop_count') + 1
-        altered_grader = CourseGradingModel.update_grader_from_json(self.course_locator, test_grader.graders[1])
+        altered_grader = CourseGradingModel.update_grader_from_json(
+            self.course_locator, test_grader.graders[1], self.user)
         self.assertDictEqual(test_grader.graders[1], altered_grader, "drop_count[1] + 2")
 
     def test_update_cutoffs_from_json(self):
         test_grader = CourseGradingModel.fetch(self.course_locator)
-        CourseGradingModel.update_cutoffs_from_json(self.course_locator, test_grader.grade_cutoffs)
+        CourseGradingModel.update_cutoffs_from_json(self.course_locator, test_grader.grade_cutoffs, self.user)
         # Unlike other tests, need to actually perform a db fetch for this test since update_cutoffs_from_json
         #  simply returns the cutoffs you send into it, rather than returning the db contents.
         altered_grader = CourseGradingModel.fetch(self.course_locator)
         self.assertDictEqual(test_grader.grade_cutoffs, altered_grader.grade_cutoffs, "Noop update")
 
         test_grader.grade_cutoffs['D'] = 0.3
-        CourseGradingModel.update_cutoffs_from_json(self.course_locator, test_grader.grade_cutoffs)
+        CourseGradingModel.update_cutoffs_from_json(self.course_locator, test_grader.grade_cutoffs, self.user)
         altered_grader = CourseGradingModel.fetch(self.course_locator)
         self.assertDictEqual(test_grader.grade_cutoffs, altered_grader.grade_cutoffs, "cutoff add D")
 
         test_grader.grade_cutoffs['Pass'] = 0.75
-        CourseGradingModel.update_cutoffs_from_json(self.course_locator, test_grader.grade_cutoffs)
+        CourseGradingModel.update_cutoffs_from_json(self.course_locator, test_grader.grade_cutoffs, self.user)
         altered_grader = CourseGradingModel.fetch(self.course_locator)
         self.assertDictEqual(test_grader.grade_cutoffs, altered_grader.grade_cutoffs, "cutoff change 'Pass'")
 
     def test_delete_grace_period(self):
         test_grader = CourseGradingModel.fetch(self.course_locator)
-        CourseGradingModel.update_grace_period_from_json(self.course_locator, test_grader.grace_period)
+        CourseGradingModel.update_grace_period_from_json(
+            self.course_locator, test_grader.grace_period, self.user
+        )
         # update_grace_period_from_json doesn't return anything, so query the db for its contents.
         altered_grader = CourseGradingModel.fetch(self.course_locator)
         self.assertEqual(test_grader.grace_period, altered_grader.grace_period, "Noop update")
 
         test_grader.grace_period = {'hours': 15, 'minutes': 5, 'seconds': 30}
-        CourseGradingModel.update_grace_period_from_json(self.course_locator, test_grader.grace_period)
+        CourseGradingModel.update_grace_period_from_json(
+            self.course_locator, test_grader.grace_period, self.user)
         altered_grader = CourseGradingModel.fetch(self.course_locator)
         self.assertDictEqual(test_grader.grace_period, altered_grader.grace_period, "Adding in a grace period")
 
         test_grader.grace_period = {'hours': 1, 'minutes': 10, 'seconds': 0}
         # Now delete the grace period
-        CourseGradingModel.delete_grace_period(self.course_locator)
+        CourseGradingModel.delete_grace_period(self.course_locator, self.user)
         # update_grace_period_from_json doesn't return anything, so query the db for its contents.
         altered_grader = CourseGradingModel.fetch(self.course_locator)
         # Once deleted, the grace period should simply be None
@@ -317,7 +324,7 @@ class CourseGradingTest(CourseTestCase):
         self.assertEqual(False, descriptor.graded)
 
         # Change the default grader type to Homework, which should also mark the section as graded
-        CourseGradingModel.update_section_grader_type(self.course, 'Homework')
+        CourseGradingModel.update_section_grader_type(self.course, 'Homework', self.user)
         descriptor = get_modulestore(self.course.location).get_item(self.course.location)
         section_grader_type = CourseGradingModel.get_section_grader_type(self.course_locator)
 
@@ -326,7 +333,7 @@ class CourseGradingTest(CourseTestCase):
         self.assertEqual(True, descriptor.graded)
 
         # Change the grader type back to notgraded, which should also unmark the section as graded
-        CourseGradingModel.update_section_grader_type(self.course, 'notgraded')
+        CourseGradingModel.update_section_grader_type(self.course, 'notgraded', self.user)
         descriptor = get_modulestore(self.course.location).get_item(self.course.location)
         section_grader_type = CourseGradingModel.get_section_grader_type(self.course_locator)
 
@@ -439,19 +446,27 @@ class CourseMetadataEditingTest(CourseTestCase):
         self.assertIn('xqa_key', test_model, 'xqa_key field ')
 
     def test_update_from_json(self):
-        test_model = CourseMetadata.update_from_json(self.course, {
-            "advertised_start": "start A",
-            "days_early_for_beta": 2
-        })
+        test_model = CourseMetadata.update_from_json(
+            self.course,
+            {
+                "advertised_start": "start A",
+                "days_early_for_beta": 2,
+            },
+            user=self.user
+         )
         self.update_check(test_model)
         # try fresh fetch to ensure persistence
         fresh = modulestore().get_item(self.course_location)
         test_model = CourseMetadata.fetch(fresh)
         self.update_check(test_model)
         # now change some of the existing metadata
-        test_model = CourseMetadata.update_from_json(fresh, {
-            "advertised_start": "start B",
-            "display_name": "jolly roger"}
+        test_model = CourseMetadata.update_from_json(
+            fresh,
+            {
+                "advertised_start": "start B",
+                "display_name": "jolly roger",
+            },
+            user=self.user
         )
         self.assertIn('display_name', test_model, 'Missing editable metadata field')
         self.assertEqual(test_model['display_name'], 'jolly roger', "not expected value")
@@ -468,9 +483,9 @@ class CourseMetadataEditingTest(CourseTestCase):
 
     def test_delete_key(self):
         test_model = CourseMetadata.update_from_json(
-            self.fullcourse, {
-                "unsetKeys": ['showanswer', 'xqa_key']
-            }
+            self.fullcourse,
+            {"unsetKeys": ['showanswer', 'xqa_key']},
+            user=self.user
         )
         # ensure no harm
         self.assertNotIn('graceperiod', test_model, 'blacklisted field leaked in')

cms/djangoapps/contentstore/tests/test_course_updates.py

@@ -123,7 +123,7 @@ class CourseUpdateTest(CourseTestCase):
         modulestore('direct').create_and_save_xmodule(location)
         course_updates = modulestore('direct').get_item(location)
         course_updates.data = 'bad news'
-        modulestore('direct').update_item(location, course_updates.data)
+        modulestore('direct').update_item(course_updates, self.user.id)
 
         init_content = '<iframe width="560" height="315" src="http://www.youtube.com/embed/RocY-Jd93XU" frameborder="0">'
         content = init_content + '</iframe>'

cms/djangoapps/contentstore/tests/test_crud.py

@@ -172,7 +172,7 @@ class TemplateTests(unittest.TestCase):
         )
         first_problem.max_attempts = 3
         first_problem.save()  # decache the above into the kvs
-        updated_problem = modulestore('split').update_item(first_problem, 'testbot')
+        updated_problem = modulestore('split').update_item(first_problem, '**replace_user**')
         self.assertIsNotNone(updated_problem.previous_version)
         self.assertEqual(updated_problem.previous_version, first_problem.update_version)
         self.assertNotEqual(updated_problem.update_version, first_problem.update_version)

cms/djangoapps/contentstore/tests/test_orphan.py

@@ -4,7 +4,6 @@ Test finding orphans via the view and django config
 import json
 from contentstore.tests.utils import CourseTestCase
 from xmodule.modulestore.django import editable_modulestore, loc_mapper
-from django.core.urlresolvers import reverse
 from student.models import CourseEnrollment
 
 class TestOrphan(CourseTestCase):
@@ -35,7 +34,7 @@ class TestOrphan(CourseTestCase):
             parent_location = self.course.location.replace(category=parent_category, name=parent_name)
             parent = editable_modulestore('direct').get_item(parent_location)
             parent.children.append(location.url())
-            editable_modulestore('direct').update_children(parent_location, parent.children)
+            editable_modulestore('direct').update_item(parent, self.user.id)
 
     def test_mongo_orphan(self):
         """

cms/djangoapps/contentstore/tests/test_textbooks.py

@@ -58,11 +58,8 @@ class TextbookIndexTestCase(CourseTestCase):
             }
         ]
         self.course.pdf_textbooks = content
-        # Save the data that we've just changed to the underlying
-        # MongoKeyValueStore before we update the mongo datastore.
-        self.course.save()
         store = get_modulestore(self.course.location)
-        store.update_metadata(self.course.location, own_metadata(self.course))
+        store.update_item(self.course, self.user.id)
 
         resp = self.client.get(
             self.url,
@@ -200,7 +197,7 @@ class TextbookDetailTestCase(CourseTestCase):
         # MongoKeyValueStore before we update the mongo datastore.
         self.course.save()
         self.store = get_modulestore(self.course.location)
-        self.store.update_metadata(self.course.location, own_metadata(self.course))
+        self.store.update_item(self.course, self.user.id)
         self.url_nonexist = self.course_locator.url_reverse("textbooks", "20")
 
     def test_get_1(self):

cms/djangoapps/contentstore/tests/test_transcripts.py

@@ -63,13 +63,13 @@ class Basetranscripts(CourseTestCase):
         self.item_locator, self.item_location = self._get_locator(resp)
         self.assertEqual(resp.status_code, 200)
 
+        self.item = modulestore().get_item(self.item_location)
         # hI10vDNYz4M - valid Youtube ID with transcripts.
         # JMD_ifUUfsU, AKqURZnYqpk, DYpADpL7jAY - valid Youtube IDs without transcripts.
-        data = '<video youtube="0.75:JMD_ifUUfsU,1.0:hI10vDNYz4M,1.25:AKqURZnYqpk,1.50:DYpADpL7jAY" />'
-        modulestore().update_item(self.item_location, data)
+        self.item.data = '<video youtube="0.75:JMD_ifUUfsU,1.0:hI10vDNYz4M,1.25:AKqURZnYqpk,1.50:DYpADpL7jAY" />'
+        modulestore().update_item(self.item, self.user.id)
 
         self.item = modulestore().get_item(self.item_location)
-
         # Remove all transcripts for current module.
         self.clear_subs_content()
 
@@ -130,14 +130,14 @@ class TestUploadtranscripts(Basetranscripts):
         self.bad_name_srt_file.seek(0)
 
     def test_success_video_module_source_subs_uploading(self):
-        data = textwrap.dedent("""
+        self.item.data = textwrap.dedent("""
             <video youtube="">
                 <source src="http://www.quirksmode.org/html5/videos/big_buck_bunny.mp4"/>
                 <source src="http://www.quirksmode.org/html5/videos/big_buck_bunny.webm"/>
                 <source src="http://www.quirksmode.org/html5/videos/big_buck_bunny.ogv"/>
             </video>
         """)
-        modulestore().update_item(self.item_location, data)
+        modulestore().update_item(self.item, self.user.id)
 
         link = reverse('upload_transcripts')
         filename = os.path.splitext(os.path.basename(self.good_srt_file.name))[0]
@@ -212,8 +212,9 @@ class TestUploadtranscripts(Basetranscripts):
         }
         resp = self.client.ajax_post('/xblock', data)
         item_locator, item_location = self._get_locator(resp)
-        data = '<non_video youtube="0.75:JMD_ifUUfsU,1.0:hI10vDNYz4M" />'
-        modulestore().update_item(item_location, data)
+        item = modulestore().get_item(item_location)
+        item.data = '<non_video youtube="0.75:JMD_ifUUfsU,1.0:hI10vDNYz4M" />'
+        modulestore().update_item(item, self.user.id)
 
         # non_video module: testing
 
@@ -232,8 +233,8 @@ class TestUploadtranscripts(Basetranscripts):
         self.assertEqual(json.loads(resp.content).get('status'), 'Transcripts are supported only for "video" modules.')
 
     def test_fail_bad_xml(self):
-        data = '<<<video youtube="0.75:JMD_ifUUfsU,1.25:AKqURZnYqpk,1.50:DYpADpL7jAY" />'
-        modulestore().update_item(self.item_location, data)
+        self.item.data = '<<<video youtube="0.75:JMD_ifUUfsU,1.25:AKqURZnYqpk,1.50:DYpADpL7jAY" />'
+        modulestore().update_item(self.item, self.user.id)
 
         link = reverse('upload_transcripts')
         filename = os.path.splitext(os.path.basename(self.good_srt_file.name))[0]
@@ -344,8 +345,8 @@ class TestDownloadtranscripts(Basetranscripts):
             pass
 
     def test_success_download_youtube(self):
-        data = '<video youtube="1:JMD_ifUUfsU" />'
-        modulestore().update_item(self.item_location, data)
+        self.item.data = '<video youtube="1:JMD_ifUUfsU" />'
+        modulestore().update_item(self.item, self.user.id)
 
         subs = {
             'start': [100, 200, 240],
@@ -365,14 +366,14 @@ class TestDownloadtranscripts(Basetranscripts):
 
     def test_success_download_nonyoutube(self):
         subs_id = str(uuid4())
-        data = textwrap.dedent("""
+        self.item.data = textwrap.dedent("""
             <video youtube="" sub="{}">
                 <source src="http://www.quirksmode.org/html5/videos/big_buck_bunny.mp4"/>
                 <source src="http://www.quirksmode.org/html5/videos/big_buck_bunny.webm"/>
                 <source src="http://www.quirksmode.org/html5/videos/big_buck_bunny.ogv"/>
             </video>
         """.format(subs_id))
-        modulestore().update_item(self.item_location, data)
+        modulestore().update_item(self.item, self.user.id)
 
         subs = {
             'start': [100, 200, 240],
@@ -424,14 +425,15 @@ class TestDownloadtranscripts(Basetranscripts):
         resp = self.client.ajax_post('/xblock', data)
         item_locator, item_location = self._get_locator(resp)
         subs_id = str(uuid4())
-        data = textwrap.dedent("""
+        item = modulestore().get_item(item_location)
+        item.data = textwrap.dedent("""
             <videoalpha youtube="" sub="{}">
                 <source src="http://www.quirksmode.org/html5/videos/big_buck_bunny.mp4"/>
                 <source src="http://www.quirksmode.org/html5/videos/big_buck_bunny.webm"/>
                 <source src="http://www.quirksmode.org/html5/videos/big_buck_bunny.ogv"/>
             </videoalpha>
         """.format(subs_id))
-        modulestore().update_item(item_location, data)
+        modulestore().update_item(item, self.user.id)
 
         subs = {
             'start': [100, 200, 240],
@@ -449,28 +451,28 @@ class TestDownloadtranscripts(Basetranscripts):
         self.assertEqual(resp.status_code, 404)
 
     def test_fail_nonyoutube_subs_dont_exist(self):
-        data = textwrap.dedent("""
+        self.item.data = textwrap.dedent("""
             <video youtube="" sub="UNDEFINED">
                 <source src="http://www.quirksmode.org/html5/videos/big_buck_bunny.mp4"/>
                 <source src="http://www.quirksmode.org/html5/videos/big_buck_bunny.webm"/>
                 <source src="http://www.quirksmode.org/html5/videos/big_buck_bunny.ogv"/>
             </video>
         """)
-        modulestore().update_item(self.item_location, data)
+        modulestore().update_item(self.item, self.user.id)
 
         link = reverse('download_transcripts')
         resp = self.client.get(link, {'locator': self.item_locator})
         self.assertEqual(resp.status_code, 404)
 
     def test_empty_youtube_attr_and_sub_attr(self):
-        data = textwrap.dedent("""
+        self.item.data = textwrap.dedent("""
             <video youtube="">
                 <source src="http://www.quirksmode.org/html5/videos/big_buck_bunny.mp4"/>
                 <source src="http://www.quirksmode.org/html5/videos/big_buck_bunny.webm"/>
                 <source src="http://www.quirksmode.org/html5/videos/big_buck_bunny.ogv"/>
             </video>
         """)
-        modulestore().update_item(self.item_location, data)
+        modulestore().update_item(self.item, self.user.id)
 
         link = reverse('download_transcripts')
         resp = self.client.get(link, {'locator': self.item_locator})
@@ -479,14 +481,14 @@ class TestDownloadtranscripts(Basetranscripts):
 
     def test_fail_bad_sjson_subs(self):
         subs_id = str(uuid4())
-        data = textwrap.dedent("""
+        self.item.data = textwrap.dedent("""
             <video youtube="" sub="{}">
                 <source src="http://www.quirksmode.org/html5/videos/big_buck_bunny.mp4"/>
                 <source src="http://www.quirksmode.org/html5/videos/big_buck_bunny.webm"/>
                 <source src="http://www.quirksmode.org/html5/videos/big_buck_bunny.ogv"/>
             </video>
         """.format(subs_id))
-        modulestore().update_item(self.item_location, data)
+        modulestore().update_item(self.item, self.user.id)
 
         subs = {
             'start': [100, 200, 240],
@@ -532,14 +534,14 @@ class TestChecktranscripts(Basetranscripts):
 
     def test_success_download_nonyoutube(self):
         subs_id = str(uuid4())
-        data = textwrap.dedent("""
+        self.item.data = textwrap.dedent("""
             <video youtube="" sub="{}">
                 <source src="http://www.quirksmode.org/html5/videos/big_buck_bunny.mp4"/>
                 <source src="http://www.quirksmode.org/html5/videos/big_buck_bunny.webm"/>
                 <source src="http://www.quirksmode.org/html5/videos/big_buck_bunny.ogv"/>
             </video>
         """.format(subs_id))
-        modulestore().update_item(self.item_location, data)
+        modulestore().update_item(self.item, self.user.id)
 
         subs = {
             'start': [100, 200, 240],
@@ -582,8 +584,8 @@ class TestChecktranscripts(Basetranscripts):
         transcripts_utils.remove_subs_from_store(subs_id, self.item)
 
     def test_check_youtube(self):
-        data = '<video youtube="1:JMD_ifUUfsU" />'
-        modulestore().update_item(self.item_location, data)
+        self.item.data = '<video youtube="1:JMD_ifUUfsU" />'
+        modulestore().update_item(self.item, self.user.id)
 
         subs = {
             'start': [100, 200, 240],
@@ -674,14 +676,15 @@ class TestChecktranscripts(Basetranscripts):
         resp = self.client.ajax_post('/xblock', data)
         item_locator, item_location = self._get_locator(resp)
         subs_id = str(uuid4())
-        data = textwrap.dedent("""
+        item = modulestore().get_item(item_location)
+        item.data = textwrap.dedent("""
             <not_video youtube="" sub="{}">
                 <source src="http://www.quirksmode.org/html5/videos/big_buck_bunny.mp4"/>
                 <source src="http://www.quirksmode.org/html5/videos/big_buck_bunny.webm"/>
                 <source src="http://www.quirksmode.org/html5/videos/big_buck_bunny.ogv"/>
             </videoalpha>
         """.format(subs_id))
-        modulestore().update_item(item_location, data)
+        modulestore().update_item(item, self.user.id)
 
         subs = {
             'start': [100, 200, 240],

cms/djangoapps/contentstore/transcripts_utils.py

@@ -280,16 +280,16 @@ def generate_srt_from_sjson(sjson_subs, speed):
     return output
 
 
-def save_module(item):
+def save_module(item, user):
     """
     Proceed with additional save operations.
     """
     item.save()
     store = get_modulestore(Location(item.id))
-    store.update_metadata(item.id, own_metadata(item))
+    store.update_item(item, user.id if user else None)
 
 
-def copy_or_rename_transcript(new_name, old_name, item, delete_old=False):
+def copy_or_rename_transcript(new_name, old_name, item, delete_old=False, user=None):
     """
     Renames `old_name` transcript file in storage to `new_name`.
 
@@ -303,12 +303,12 @@ def copy_or_rename_transcript(new_name, old_name, item, delete_old=False):
     transcripts = contentstore().find(content_location).data
     save_subs_to_store(json.loads(transcripts), new_name, item)
     item.sub = new_name
-    save_module(item)
+    save_module(item, user)
     if delete_old:
         remove_subs_from_store(old_name, item)
 
 
-def manage_video_subtitles_save(old_item, new_item):
+def manage_video_subtitles_save(old_item, new_item, user):
     """
     Does some specific things, that can be done only on save.
 
@@ -340,7 +340,7 @@ def manage_video_subtitles_save(old_item, new_item):
         # copy_or_rename_transcript changes item.sub of module
         try:
             # updates item.sub with `video_id`, if it is successful.
-            copy_or_rename_transcript(video_id, sub_name, new_item)
+            copy_or_rename_transcript(video_id, sub_name, new_item, user=user)
         except NotFoundError:
             # subtitles file `sub_name` is not presented in the system. Nothing to copy or rename.
             log.debug(

cms/djangoapps/contentstore/utils.py

@@ -222,16 +222,6 @@ def compute_unit_state(unit):
         return UnitState.public
 
 
-def update_item(location, value):
-    """
-    If value is None, delete the db entry. Otherwise, update it using the correct modulestore.
-    """
-    if value is None:
-        get_modulestore(location).delete_item(location)
-    else:
-        get_modulestore(location).update_item(location, value)
-
-
 def add_extra_panel_tab(tab_type, course):
     """
     Used to add the panel tab to a course if it does not exist.

cms/djangoapps/contentstore/views/checklist.py

@@ -51,8 +51,7 @@ def checklists_handler(request, tag=None, package_id=None, branch=None, version_
         # from the template.
         if not course_module.checklists:
             course_module.checklists = CourseDescriptor.checklists.default
-            course_module.save()
-            modulestore.update_metadata(old_location, own_metadata(course_module))
+            modulestore.update_item(course_module, request.user.id)
 
         expanded_checklists = expand_all_action_urls(course_module)
         if json_request:
@@ -81,7 +80,7 @@ def checklists_handler(request, tag=None, package_id=None, branch=None, version_
             # not default
             course_module.checklists = course_module.checklists
             course_module.save()
-            modulestore.update_metadata(old_location, own_metadata(course_module))
+            modulestore.update_item(course_module, request.user.id)
             expanded_checklist = expand_checklist_action_url(course_module, persisted_checklist)
             return JsonResponse(expanded_checklist)
         else:

cms/djangoapps/contentstore/views/course.py

@@ -163,8 +163,7 @@ def course_listing(request):
     """
     List all courses available to the logged in user
     """
-    # there's an index on category which will be used if none of its antecedents are set
-    courses = modulestore('direct').get_items(Location(None, None, None, 'course', None))
+    courses = modulestore('direct').get_courses()
 
     # filter out courses that we don't have access too
     def course_filter(course):
@@ -331,7 +330,7 @@ def create_new_course(request):
         definition_data=overview_template.get('data')
     )
 
-    initialize_course_tabs(new_course)
+    initialize_course_tabs(new_course, request.user)
 
     new_location = loc_mapper().translate_location(new_course.location.course_id, new_course.location, False, True)
     # can't use auth.add_users here b/c it requires request.user to already have Instructor perms in this course
@@ -417,7 +416,7 @@ def course_info_update_handler(request, tag=None, package_id=None, branch=None,
         return JsonResponse(get_course_updates(updates_location, provided_id))
     elif request.method == 'DELETE':
         try:
-            return JsonResponse(delete_course_update(updates_location, request.json, provided_id))
+            return JsonResponse(delete_course_update(updates_location, request.json, provided_id, request.user))
         except:
             return HttpResponseBadRequest(
                 "Failed to delete",
@@ -426,7 +425,7 @@ def course_info_update_handler(request, tag=None, package_id=None, branch=None,
     # can be either and sometimes django is rewriting one to the other:
     elif request.method in ('POST', 'PUT'):
         try:
-            return JsonResponse(update_course_updates(updates_location, request.json, provided_id))
+            return JsonResponse(update_course_updates(updates_location, request.json, provided_id, request.user))
         except:
             return HttpResponseBadRequest(
                 "Failed to save",
@@ -479,7 +478,7 @@ def settings_handler(request, tag=None, package_id=None, branch=None, version_gu
             )
         else:  # post or put, doesn't matter.
             return JsonResponse(
-                CourseDetails.update_from_json(locator, request.json),
+                CourseDetails.update_from_json(locator, request.json, request.user),
                 encoder=CourseSettingsEncoder
             )
 
@@ -526,15 +525,15 @@ def grading_handler(request, tag=None, package_id=None, branch=None, version_gui
             # None implies update the whole model (cutoffs, graceperiod, and graders) not a specific grader
             if grader_index is None:
                 return JsonResponse(
-                    CourseGradingModel.update_from_json(locator, request.json),
+                    CourseGradingModel.update_from_json(locator, request.json, request.user),
                     encoder=CourseSettingsEncoder
                 )
             else:
                 return JsonResponse(
-                    CourseGradingModel.update_grader_from_json(locator, request.json)
+                    CourseGradingModel.update_grader_from_json(locator, request.json, request.user)
                 )
         elif request.method == "DELETE" and grader_index is not None:
-            CourseGradingModel.delete_grader(locator, grader_index)
+            CourseGradingModel.delete_grader(locator, grader_index, request.user)
             return JsonResponse()
 
 
@@ -625,7 +624,8 @@ def advanced_settings_handler(request, package_id=None, branch=None, version_gui
                 return JsonResponse(CourseMetadata.update_from_json(
                     course_module,
                     request.json,
-                    filter_tabs=filter_tabs
+                    filter_tabs=filter_tabs,
+                    user=request.user,
                 ))
             except (TypeError, ValueError) as err:
                 return HttpResponseBadRequest(
@@ -743,10 +743,7 @@ def textbooks_list_handler(request, tag=None, package_id=None, branch=None, vers
         if not any(tab['type'] == 'pdf_textbooks' for tab in course.tabs):
             course.tabs.append({"type": "pdf_textbooks"})
         course.pdf_textbooks = textbooks
-        store.update_metadata(
-            course.location,
-            own_metadata(course)
-        )
+        store.update_item(course, request.user.id)
         return JsonResponse(course.pdf_textbooks)
     elif request.method == 'POST':
         # create a new textbook for the course
@@ -764,7 +761,7 @@ def textbooks_list_handler(request, tag=None, package_id=None, branch=None, vers
             tabs = course.tabs
             tabs.append({"type": "pdf_textbooks"})
             course.tabs = tabs
-        store.update_metadata(course.location, own_metadata(course))
+        store.update_item(course, request.user.id)
         resp = JsonResponse(textbook, status=201)
         resp["Location"] = locator.url_reverse('textbooks', textbook["id"])
         return resp
@@ -815,10 +812,7 @@ def textbooks_detail_handler(request, tid, tag=None, package_id=None, branch=Non
             course.pdf_textbooks = new_textbooks
         else:
             course.pdf_textbooks.append(new_textbook)
-        store.update_metadata(
-            course.location,
-            own_metadata(course)
-        )
+        store.update_item(course, request.user.id)
         return JsonResponse(new_textbook, status=201)
     elif request.method == 'DELETE':
         if not textbook:
@@ -827,10 +821,7 @@ def textbooks_detail_handler(request, tid, tag=None, package_id=None, branch=Non
         new_textbooks = course.pdf_textbooks[0:i]
         new_textbooks.extend(course.pdf_textbooks[i + 1:])
         course.pdf_textbooks = new_textbooks
-        store.update_metadata(
-            course.location,
-            own_metadata(course)
-        )
+        store.update_item(course, request.user.id)
         return JsonResponse()
 
 

cms/djangoapps/contentstore/views/item.py

@@ -9,7 +9,6 @@ from functools import partial
 from static_replace import replace_static_urls
 from xmodule_modifiers import wrap_xblock
 
-from django.conf import settings
 from django.core.exceptions import PermissionDenied
 from django.contrib.auth.decorators import login_required
 from django.http import HttpResponseBadRequest, HttpResponse
@@ -18,9 +17,8 @@ from django.views.decorators.http import require_http_methods
 
 from xblock.fields import Scope
 from xblock.fragment import Fragment
-from xblock.core import XBlock
 
-import xmodule.x_module
+import xmodule
 from xmodule.modulestore.django import modulestore, loc_mapper
 from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError
 from xmodule.modulestore.inheritance import own_metadata
@@ -159,7 +157,7 @@ def xblock_handler(request, tag=None, package_id=None, branch=None, version_guid
             delete_children = str_to_bool(request.REQUEST.get('recurse', 'False'))
             delete_all_versions = str_to_bool(request.REQUEST.get('all_versions', 'False'))
 
-            return _delete_item_at_location(old_location, delete_children, delete_all_versions)
+            return _delete_item_at_location(old_location, delete_children, delete_all_versions, request.user)
         else:  # Since we have a package_id, we are updating an existing xblock.
             return _save_item(
                 request,
@@ -184,7 +182,8 @@ def xblock_handler(request, tag=None, package_id=None, branch=None, version_guid
             dest_location = _duplicate_item(
                 parent_location,
                 duplicate_source_location,
-                request.json.get('display_name')
+                request.json.get('display_name'),
+                request.user,
             )
             course_location = loc_mapper().translate_locator_to_location(BlockUsageLocator(parent_locator), get_course=True)
             dest_locator = loc_mapper().translate_location(course_location.course_id, dest_location, False, True)
@@ -232,7 +231,8 @@ def _save_item(request, usage_loc, item_location, data=None, children=None, meta
             modulestore().convert_to_draft(item_location)
 
     if data:
-        store.update_item(item_location, data)
+        # TODO Allow any scope.content fields not just "data" (exactly like the get below this)
+        existing_item.data = data
     else:
         data = existing_item.get_explicitly_set_fields_by_scope(Scope.content)
 
@@ -242,9 +242,9 @@ def _save_item(request, usage_loc, item_location, data=None, children=None, meta
             for child_locator
             in children
         ]
-        store.update_children(item_location, children_ids)
+        existing_item.children = children_ids
 
-    # cdodge: also commit any metadata which might have been passed along
+    # also commit any metadata which might have been passed along
     if nullout is not None or metadata is not None:
         # the postback is not the complete metadata, as there's system metadata which is
         # not presented to the end-user for editing. So let's use the original (existing_item) and
@@ -269,14 +269,11 @@ def _save_item(request, usage_loc, item_location, data=None, children=None, meta
                         return JsonResponse({"error": "Invalid data"}, 400)
                     field.write_to(existing_item, value)
 
-        # Save the data that we've just changed to the underlying
-        # MongoKeyValueStore before we update the mongo datastore.
-        existing_item.save()
-        # commit to datastore
-        store.update_metadata(item_location, own_metadata(existing_item))
-
         if existing_item.category == 'video':
-            manage_video_subtitles_save(existing_item, existing_item)
+            manage_video_subtitles_save(existing_item, existing_item, request.user)
+
+    # commit to datastore
+    store.update_item(existing_item, request.user.id)
 
     result = {
         'id': unicode(usage_loc),
@@ -285,7 +282,7 @@ def _save_item(request, usage_loc, item_location, data=None, children=None, meta
     }
 
     if grader_type is not None:
-        result.update(CourseGradingModel.update_section_grader_type(existing_item, grader_type))
+        result.update(CourseGradingModel.update_section_grader_type(existing_item, grader_type, request.user))
 
     # Make public after updating the xblock, in case the caller asked
     # for both an update and a publish.
@@ -339,14 +336,15 @@ def _create_item(request):
 
     # TODO replace w/ nicer accessor
     if not 'detached' in parent.runtime.load_block_type(category)._class_tags:
-        get_modulestore(parent.location).update_children(parent_location, parent.children + [dest_location.url()])
+        parent.children.append(dest_location.url())
+        get_modulestore(parent.location).update_item(parent, request.user.id)
 
     course_location = loc_mapper().translate_locator_to_location(parent_locator, get_course=True)
     locator = loc_mapper().translate_location(course_location.course_id, dest_location, False, True)
     return JsonResponse({"locator": unicode(locator)})
 
 
-def _duplicate_item(parent_location, duplicate_source_location, display_name=None):
+def _duplicate_item(parent_location, duplicate_source_location, display_name=None, user=None):
     """
     Duplicate an existing xblock as a child of the supplied parent_location.
     """
@@ -373,13 +371,15 @@ def _duplicate_item(parent_location, duplicate_source_location, display_name=Non
         system=source_item.runtime,
     )
 
+    dest_module = get_modulestore(category).get_item(dest_location)
     # Children are not automatically copied over (and not all xblocks have a 'children' attribute).
     # Because DAGs are not fully supported, we need to actually duplicate each child as well.
     if source_item.has_children:
-        copied_children = []
+        dest_module.children = []
         for child in source_item.children:
-            copied_children.append(_duplicate_item(dest_location, Location(child)).url())
-        get_modulestore(dest_location).update_children(dest_location, copied_children)
+            dupe = _duplicate_item(dest_location, Location(child), user=user)
+            dest_module.children.append(dupe.url())
+        get_modulestore(dest_location).update_item(dest_module, user.id if user else None)
 
     if not 'detached' in source_item.runtime.load_block_type(category)._class_tags:
         parent = get_modulestore(parent_location).get_item(parent_location)
@@ -390,12 +390,12 @@ def _duplicate_item(parent_location, duplicate_source_location, display_name=Non
             parent.children.insert(source_index + 1, dest_location.url())
         else:
             parent.children.append(dest_location.url())
-        get_modulestore(parent_location).update_children(parent_location, parent.children)
+        get_modulestore(parent_location).update_item(parent, user.id if user else None)
 
     return dest_location
 
 
-def _delete_item_at_location(item_location, delete_children=False, delete_all_versions=False):
+def _delete_item_at_location(item_location, delete_children=False, delete_all_versions=False, user=None):
     """
     Deletes the item at with the given Location.
 
@@ -406,22 +406,19 @@ def _delete_item_at_location(item_location, delete_children=False, delete_all_ve
     item = store.get_item(item_location)
 
     if delete_children:
-        _xmodule_recurse(item, lambda i: store.delete_item(i.location, delete_all_versions))
+        _xmodule_recurse(item, lambda i: store.delete_item(i.location, delete_all_versions=delete_all_versions))
     else:
-        store.delete_item(item.location, delete_all_versions)
+        store.delete_item(item.location, delete_all_versions=delete_all_versions)
 
     # cdodge: we need to remove our parent's pointer to us so that it is no longer dangling
     if delete_all_versions:
         parent_locs = modulestore('direct').get_parent_locations(item_location, None)
 
+        item_url = item_location.url()
         for parent_loc in parent_locs:
             parent = modulestore('direct').get_item(parent_loc)
-            item_url = item_location.url()
-            if item_url in parent.children:
-                children = parent.children
-                children.remove(item_url)
-                parent.children = children
-                modulestore('direct').update_children(parent.location, parent.children)
+            parent.children.remove(item_url)
+            modulestore('direct').update_item(parent, user.id if user else None)
 
     return JsonResponse()
 
@@ -452,7 +449,7 @@ def orphan_handler(request, tag=None, package_id=None, branch=None, version_guid
         if request.user.is_staff:
             items = modulestore().get_orphans(old_location, 'draft')
             for item in items:
-                modulestore('draft').delete_item(item, True)
+                modulestore('draft').delete_item(item, delete_all_versions=True)
             return JsonResponse({'deleted': items})
         else:
             raise PermissionDenied()

cms/djangoapps/contentstore/views/tabs.py

@@ -23,7 +23,7 @@ from django.utils.translation import ugettext as _
 __all__ = ['tabs_handler']
 
 
-def initialize_course_tabs(course):
+def initialize_course_tabs(course, user):
     """
     set up the default tabs
     I've added this because when we add static tabs, the LMS either expects a None for the tabs list or
@@ -47,7 +47,7 @@ def initialize_course_tabs(course):
         {"type": "progress", "name": _("Progress")},
     ]
 
-    modulestore('direct').update_metadata(course.location.url(), own_metadata(course))
+    modulestore('direct').update_item(course, user.id)
 
 @expect_json
 @login_required
@@ -123,14 +123,14 @@ def tabs_handler(request, tag=None, package_id=None, branch=None, version_guid=N
 
                 # OK, re-assemble the static tabs in the new order
                 course_item.tabs = reordered_tabs
-                modulestore('direct').update_metadata(course_item.location, own_metadata(course_item))
+                modulestore('direct').update_item(course_item, request.user.id)
                 return JsonResponse()
             else:
                 raise NotImplementedError('Creating or changing tab content is not supported.')
     elif request.method == 'GET':  # assume html
         # see tabs have been uninitialized (e.g. supporting courses created before tab support in studio)
         if course_item.tabs is None or len(course_item.tabs) == 0:
-            initialize_course_tabs(course_item)
+            initialize_course_tabs(course_item, request.user)
 
         # first get all static tabs from the tabs list
         # we do this because this is also the order in which items are displayed in the LMS
@@ -179,7 +179,7 @@ def primitive_delete(course, num):
     # Note for future implementations: if you delete a static_tab, then Chris Dodge
     # points out that there's other stuff to delete beyond this element.
     # This code happens to not delete static_tab so it doesn't come up.
-    modulestore('direct').update_metadata(course.location, own_metadata(course))
+    modulestore('direct').update_item(course, '**replace_user**')
 
 
 def primitive_insert(course, num, tab_type, name):
@@ -188,5 +188,5 @@ def primitive_insert(course, num, tab_type, name):
     new_tab = {u'type': unicode(tab_type), u'name': unicode(name)}
     tabs = course.tabs
     tabs.insert(num, new_tab)
-    modulestore('direct').update_metadata(course.location, own_metadata(course))
+    modulestore('direct').update_item(course, '**replace_user**')
 

cms/djangoapps/contentstore/views/transcripts_ajax.py

@@ -126,17 +126,17 @@ def upload_transcripts(request):
             video_name = video_dict['video']
             # We are creating transcripts for every video source,
             # for the case that in future, some of video sources can be deleted.
-            statuses[video_name] = copy_or_rename_transcript(video_name, sub_attr, item)
+            statuses[video_name] = copy_or_rename_transcript(video_name, sub_attr, item, user=request.user)
             try:
                 # updates item.sub with `video_name` if it is successful.
-                copy_or_rename_transcript(video_name, sub_attr, item)
+                copy_or_rename_transcript(video_name, sub_attr, item, user=request.user)
                 selected_name = video_name  # name to write to item.sub field, chosen at random.
             except NotFoundError:
                 # subtitles file `sub_attr` is not presented in the system. Nothing to copy or rename.
                 return error_response(response, "Can't find transcripts in storage for {}".format(sub_attr))
 
         item.sub = selected_name  # write one of  new subtitles names to item.sub attribute.
-        save_module(item)
+        save_module(item, request.user)
         response['subs'] = item.sub
         response['status'] = 'Success'
     else:
@@ -389,7 +389,7 @@ def choose_transcripts(request):
 
     if item.sub != html5_id:  # update sub value
         item.sub = html5_id
-        save_module(item)
+        save_module(item, request.user)
     response = {'status': 'Success',  'subs': item.sub}
     return JsonResponse(response)
 
@@ -420,7 +420,7 @@ def replace_transcripts(request):
         return error_response(response, e.message)
 
     item.sub = youtube_id
-    save_module(item)
+    save_module(item, request.user)
     response = {'status': 'Success',  'subs': item.sub}
     return JsonResponse(response)
 
@@ -483,7 +483,7 @@ def rename_transcripts(request):
     for new_name in videos['html5'].keys():  # copy subtitles for every HTML5 source
         try:
             # updates item.sub with new_name if it is successful.
-            copy_or_rename_transcript(new_name, old_name, item)
+            copy_or_rename_transcript(new_name, old_name, item, user=request.user)
         except NotFoundError:
             # subtitles file `item.sub` is not presented in the system. Nothing to copy or rename.
             error_response(response, "Can't find transcripts in storage for {}".format(old_name))
@@ -519,10 +519,10 @@ def save_transcripts(request):
         for metadata_key, value in metadata.items():
             setattr(item, metadata_key, value)
 
-        save_module(item)  # item becomes updated with new values
+        save_module(item, request.user)  # item becomes updated with new values
 
         if new_sub:
-            manage_video_subtitles_save(None, item)
+            manage_video_subtitles_save(None, item, request.user)
         else:
             # If `new_sub` is empty, it means that user explicitly does not want to use
             # transcripts for current video ids and we remove all transcripts from storage.

cms/djangoapps/models/settings/course_details.py

@@ -6,10 +6,8 @@ from json.encoder import JSONEncoder
 
 from xmodule.modulestore import Location
 from xmodule.modulestore.exceptions import ItemNotFoundError
-from xmodule.modulestore.inheritance import own_metadata
 from contentstore.utils import get_modulestore, course_image_url
 from models.settings import course_grading
-from contentstore.utils import update_item
 from xmodule.fields import Date
 from xmodule.modulestore.django import loc_mapper
 
@@ -75,7 +73,25 @@ class CourseDetails(object):
         return course
 
     @classmethod
-    def update_from_json(cls, course_locator, jsondict):
+    def update_about_item(cls, course_old_location, about_key, data, course, user):
+        """
+        Update the about item with the new data blob. If data is None, then
+        delete the about item.
+        """
+        temploc = Location(course_old_location).replace(category='about', name=about_key)
+        store = get_modulestore(temploc)
+        if data is None:
+            store.delete_item(temploc)
+        else:
+            try:
+                about_item = store.get_item(temploc)
+            except ItemNotFoundError:
+                about_item = store.create_xmodule(temploc, system=course.runtime)
+            about_item.data = data
+            store.update_item(about_item, user.id)
+
+    @classmethod
+    def update_from_json(cls, course_locator, jsondict, user):
         """
         Decode the json into CourseDetails and save any changed attrs to the db
         """
@@ -130,26 +146,15 @@ class CourseDetails(object):
             dirty = True
 
         if dirty:
-            # Save the data that we've just changed to the underlying
-            # MongoKeyValueStore before we update the mongo datastore.
-            descriptor.save()
-
-            get_modulestore(course_old_location).update_metadata(course_old_location, own_metadata(descriptor))
+            get_modulestore(course_old_location).update_item(descriptor, user.id)
 
         # NOTE: below auto writes to the db w/o verifying that any of the fields actually changed
         # to make faster, could compare against db or could have client send over a list of which fields changed.
-        temploc = Location(course_old_location).replace(category='about', name='syllabus')
-        update_item(temploc, jsondict['syllabus'])
+        for about_type in ['syllabus', 'overview', 'effort']:
+            cls.update_about_item(course_old_location, about_type, jsondict[about_type], descriptor, user)
 
-        temploc = temploc.replace(name='overview')
-        update_item(temploc, jsondict['overview'])
-
-        temploc = temploc.replace(name='effort')
-        update_item(temploc, jsondict['effort'])
-
-        temploc = temploc.replace(name='video')
         recomposed_video_tag = CourseDetails.recompose_video_tag(jsondict['intro_video'])
-        update_item(temploc, recomposed_video_tag)
+        cls.update_about_item(course_old_location, 'video', recomposed_video_tag, descriptor, user)
 
         # Could just return jsondict w/o doing any db reads, but I put the reads in as a means to confirm
         # it persisted correctly

cms/djangoapps/models/settings/course_grading.py

@@ -52,7 +52,7 @@ class CourseGradingModel(object):
                     }
 
     @staticmethod
-    def update_from_json(course_locator, jsondict):
+    def update_from_json(course_locator, jsondict, user):
         """
         Decode the json into CourseGradingModel and save any changes. Returns the modified model.
         Probably not the usual path for updates as it's too coarse grained.
@@ -65,16 +65,14 @@ class CourseGradingModel(object):
         descriptor.raw_grader = graders_parsed
         descriptor.grade_cutoffs = jsondict['grade_cutoffs']
 
-        get_modulestore(course_old_location).update_item(
-            course_old_location, descriptor.get_explicitly_set_fields_by_scope(Scope.content)
-        )
+        get_modulestore(course_old_location).update_item(descriptor, user.id)
 
-        CourseGradingModel.update_grace_period_from_json(course_locator, jsondict['grace_period'])
+        CourseGradingModel.update_grace_period_from_json(course_locator, jsondict['grace_period'], user)
 
         return CourseGradingModel.fetch(course_locator)
 
     @staticmethod
-    def update_grader_from_json(course_location, grader):
+    def update_grader_from_json(course_location, grader, user):
         """
         Create or update the grader of the given type (string key) for the given course. Returns the modified
         grader which is a full model on the client but not on the server (just a dict)
@@ -91,14 +89,12 @@ class CourseGradingModel(object):
         else:
             descriptor.raw_grader.append(grader)
 
-        get_modulestore(course_old_location).update_item(
-            course_old_location, descriptor.get_explicitly_set_fields_by_scope(Scope.content)
-        )
+        get_modulestore(course_old_location).update_item(descriptor, user.id)
 
         return CourseGradingModel.jsonize_grader(index, descriptor.raw_grader[index])
 
     @staticmethod
-    def update_cutoffs_from_json(course_location, cutoffs):
+    def update_cutoffs_from_json(course_location, cutoffs, user):
         """
         Create or update the grade cutoffs for the given course. Returns sent in cutoffs (ie., no extra
         db fetch).
@@ -107,14 +103,12 @@ class CourseGradingModel(object):
         descriptor = get_modulestore(course_old_location).get_item(course_old_location)
         descriptor.grade_cutoffs = cutoffs
 
-        get_modulestore(course_old_location).update_item(
-            course_old_location, descriptor.get_explicitly_set_fields_by_scope(Scope.content)
-        )
+        get_modulestore(course_old_location).update_item(descriptor, user.id)
 
         return cutoffs
 
     @staticmethod
-    def update_grace_period_from_json(course_location, graceperiodjson):
+    def update_grace_period_from_json(course_location, graceperiodjson, user):
         """
         Update the course's default grace period. Incoming dict is {hours: h, minutes: m} possibly as a
         grace_period entry in an enclosing dict. It is also safe to call this method with a value of
@@ -132,12 +126,10 @@ class CourseGradingModel(object):
             grace_timedelta = timedelta(**graceperiodjson)
             descriptor.graceperiod = grace_timedelta
 
-            get_modulestore(course_old_location).update_metadata(
-                course_old_location, descriptor.get_explicitly_set_fields_by_scope(Scope.settings)
-            )
+            get_modulestore(course_old_location).update_item(descriptor, user.id)
 
     @staticmethod
-    def delete_grader(course_location, index):
+    def delete_grader(course_location, index, user):
         """
         Delete the grader of the given type from the given course.
         """
@@ -150,12 +142,10 @@ class CourseGradingModel(object):
             # force propagation to definition
             descriptor.raw_grader = descriptor.raw_grader
 
-        get_modulestore(course_old_location).update_item(
-            course_old_location, descriptor.get_explicitly_set_fields_by_scope(Scope.content)
-        )
+        get_modulestore(course_old_location).update_item(descriptor, user.id)
 
     @staticmethod
-    def delete_grace_period(course_location):
+    def delete_grace_period(course_location, user):
         """
         Delete the course's grace period.
         """
@@ -164,9 +154,7 @@ class CourseGradingModel(object):
 
         del descriptor.graceperiod
 
-        get_modulestore(course_old_location).update_metadata(
-            course_old_location, descriptor.get_explicitly_set_fields_by_scope(Scope.settings)
-        )
+        get_modulestore(course_old_location).update_item(descriptor, user.id)
 
     @staticmethod
     def get_section_grader_type(location):
@@ -178,7 +166,7 @@ class CourseGradingModel(object):
         }
 
     @staticmethod
-    def update_section_grader_type(descriptor, grader_type):
+    def update_section_grader_type(descriptor, grader_type, user):
         if grader_type is not None and grader_type != u'notgraded':
             descriptor.format = grader_type
             descriptor.graded = True
@@ -186,9 +174,7 @@ class CourseGradingModel(object):
             del descriptor.format
             del descriptor.graded
 
-        get_modulestore(descriptor.location).update_metadata(
-            descriptor.location, descriptor.get_explicitly_set_fields_by_scope(Scope.settings)
-        )
+        get_modulestore(descriptor.location).update_item(descriptor, user.id)
         return {'graderType': grader_type}
 
     @staticmethod

cms/djangoapps/models/settings/course_metadata.py

 from xblock.fields import Scope
 
 from contentstore.utils import get_modulestore
-from xmodule.modulestore.inheritance import own_metadata
 from cms.lib.xblock.mixin import CmsBlockMixin
 
 
@@ -48,7 +47,7 @@ class CourseMetadata(object):
         return result
 
     @classmethod
-    def update_from_json(cls, descriptor, jsondict, filter_tabs=True):
+    def update_from_json(cls, descriptor, jsondict, filter_tabs=True, user=None):
         """
         Decode the json into CourseMetadata and save any changed attrs to the db.
 
@@ -78,6 +77,6 @@ class CourseMetadata(object):
                 setattr(descriptor, key, value)
 
         if dirty:
-            get_modulestore(descriptor.location).update_metadata(descriptor.location, own_metadata(descriptor))
+            get_modulestore(descriptor.location).update_item(descriptor, user.id if user else None)
 
         return cls.fetch(descriptor)

common/djangoapps/external_auth/tests/test_shib.py

@@ -330,9 +330,7 @@ class ShibSPTest(ModuleStoreTestCase):
         for domain in ["", "shib:https://idp.stanford.edu/"]:
             # set domains
             course.enrollment_domain = domain
-            metadata = own_metadata(course)
-            metadata['enrollment_domain'] = domain
-            self.store.update_metadata(course.location.url(), metadata)
+            self.store.update_item(course, '**replace_user**')
 
             # setting location to test that GET params get passed through
             login_request = self.request_factory.get('/course_specific_login/MITx/999/Robot_Super_Course' +
@@ -401,15 +399,11 @@ class ShibSPTest(ModuleStoreTestCase):
         # create 2 course, one with limited enrollment one without
         shib_course = CourseFactory.create(org='Stanford', number='123', display_name='Shib Only')
         shib_course.enrollment_domain = 'shib:https://idp.stanford.edu/'
-        metadata = own_metadata(shib_course)
-        metadata['enrollment_domain'] = shib_course.enrollment_domain
-        self.store.update_metadata(shib_course.location.url(), metadata)
+        self.store.update_item(shib_course, '**replace_user**')
 
         open_enroll_course = CourseFactory.create(org='MITx', number='999', display_name='Robot Super Course')
         open_enroll_course.enrollment_domain = ''
-        metadata = own_metadata(open_enroll_course)
-        metadata['enrollment_domain'] = open_enroll_course.enrollment_domain
-        self.store.update_metadata(open_enroll_course.location.url(), metadata)
+        self.store.update_item(open_enroll_course, '**replace_user**')
 
         # create 3 kinds of students, external_auth matching shib_course, external_auth not matching, no external auth
         shib_student = UserFactory.create()
@@ -475,9 +469,7 @@ class ShibSPTest(ModuleStoreTestCase):
 
         course = CourseFactory.create(org='Stanford', number='123', display_name='Shib Only')
         course.enrollment_domain = 'shib:https://idp.stanford.edu/'
-        metadata = own_metadata(course)
-        metadata['enrollment_domain'] = course.enrollment_domain
-        self.store.update_metadata(course.location.url(), metadata)
+        self.store.update_item(course, '**replace_user**')
 
         # use django test client for sessions and url processing
         # no enrollment before trying

common/djangoapps/student/tests/test_login.py

@@ -203,9 +203,7 @@ class ExternalAuthShibTest(ModuleStoreTestCase):
         self.course = CourseFactory.create(org='Stanford', number='456', display_name='NO SHIB')
         self.shib_course = CourseFactory.create(org='Stanford', number='123', display_name='Shib Only')
         self.shib_course.enrollment_domain = 'shib:https://idp.stanford.edu/'
-        metadata = own_metadata(self.shib_course)
-        metadata['enrollment_domain'] = self.shib_course.enrollment_domain
-        self.store.update_metadata(self.shib_course.location.url(), metadata)
+        self.store.update_item(self.shib_course, '**replace_user**')
         self.user_w_map = UserFactory.create(email='withmap@stanford.edu')
         self.extauth = ExternalAuthMap(external_id='withmap@stanford.edu',
                                        external_email='withmap@stanford.edu',

common/lib/xmodule/xmodule/modulestore/__init__.py

@@ -339,7 +339,7 @@ class ModuleStoreRead(object):
         pass
 
     @abstractmethod
-    def get_items(self, location, course_id=None, depth=0):
+    def get_items(self, location, course_id=None, depth=0, qualifiers=None):
         """
         Returns a list of XModuleDescriptor instances for the items
         that match location. Any element of location that is None is treated
@@ -379,6 +379,15 @@ class ModuleStoreRead(object):
         pass
 
     @abstractmethod
+    def get_orphans(self, course_location, branch):
+        """
+        Get all of the xblocks in the given course which have no parents and are not of types which are
+        usually orphaned. NOTE: may include xblocks which still have references via xblocks which don't
+        use children to point to their dependents.
+        """
+        pass
+
+    @abstractmethod
     def get_errored_courses(self):
         """
         Return a dictionary of course_dir -> [(msg, exception_str)], for each
@@ -404,44 +413,34 @@ class ModuleStoreWrite(ModuleStoreRead):
     __metaclass__ = ABCMeta
 
     @abstractmethod
-    def update_item(self, location, data, allow_not_found=False):
+    def update_item(self, xblock, user_id=None, allow_not_found=False, force=False):
         """
-        Set the data in the item specified by the location to
-        data
+        Update the given xblock's persisted repr. Pass the user's unique id which the persistent store
+        should save with the update if it has that ability.
 
-        location: Something that can be passed to Location
-        data: A nested dictionary of problem data
-        """
-        pass
+        :param allow_not_found: whether this method should raise an exception if the given xblock
+        has not been persisted before.
+        :param force: fork the structure and don't update the course draftVersion if there's a version
+        conflict (only applicable to version tracking and conflict detecting persistence stores)
 
-    @abstractmethod
-    def update_children(self, location, children):
-        """
-        Set the children for the item specified by the location to
-        children
-
-        location: Something that can be passed to Location
-        children: A list of child item identifiers
+        :raises VersionConflictError: if package_id and version_guid given and the current
+        version head != version_guid and force is not True. (only applicable to version tracking stores)
         """
         pass
 
     @abstractmethod
-    def update_metadata(self, location, metadata):
+    def delete_item(self, location, user_id=None, delete_all_versions=False, delete_children=False, force=False):
         """
-        Set the metadata for the item specified by the location to
-        metadata
+        Delete an item from persistence. Pass the user's unique id which the persistent store
+        should save with the update if it has that ability.
 
-        location: Something that can be passed to Location
-        metadata: A nested dictionary of module metadata
-        """
-        pass
+        :param delete_all_versions: removes both the draft and published version of this item from
+        the course if using draft and old mongo. Split may or may not implement this.
+        :param force: fork the structure and don't update the course draftVersion if there's a version
+        conflict (only applicable to version tracking and conflict detecting persistence stores)
 
-    @abstractmethod
-    def delete_item(self, location):
-        """
-        Delete an item from this modulestore
-
-        location: Something that can be passed to Location
+        :raises VersionConflictError: if package_id and version_guid given and the current
+        version head != version_guid and force is not True. (only applicable to version tracking stores)
         """
         pass
 

common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py

@@ -163,8 +163,14 @@ class LocMapperStore(object):
             else:
                 raise ItemNotFoundError(location)
         elif isinstance(block_id, dict):
-            # name is not unique, look through for the right category
-            if location.category in block_id:
+            # jump_to_id uses a None category.
+            if location.category is None:
+                if len(block_id) == 1:
+                    # unique match (most common case)
+                    block_id = block_id.values()[0]
+                else:
+                    raise InvalidLocationError()
+            elif location.category in block_id:
                 block_id = block_id[location.category]
             elif add_entry_if_missing:
                 block_id = self._add_to_block_map(location, location_id, entry['block_map'])

common/lib/xmodule/xmodule/modulestore/locator.py

@@ -488,6 +488,19 @@ class BlockUsageLocator(CourseLocator):
             raise ValueError('Could not parse "%s" as a package_id' % package_id)
         self._set_value(parse, 'block', self.set_block_id)
 
+    @classmethod
+    def make_relative(cls, course_locator, block_id):
+        """
+        Return a new instance which has the given block_id in the given course
+        :param course_locator: may be a BlockUsageLocator in the same snapshot
+        """
+        return BlockUsageLocator(
+            package_id=course_locator.package_id,
+            version_guid=course_locator.version_guid,
+            branch=course_locator.branch,
+            block_id=block_id
+        )
+
     def __unicode__(self):
         """
         Return a string representing this location.

common/lib/xmodule/xmodule/modulestore/mixed.py

@@ -3,29 +3,43 @@ MixedModuleStore allows for aggregation between multiple modulestores.
 
 In this way, courses can be served up both - say - XMLModuleStore or MongoModuleStore
 
-IMPORTANT: This modulestore only supports READONLY applications, e.g. LMS
 """
 
 from . import ModuleStoreWriteBase
-from xmodule.modulestore.django import create_modulestore_instance
+from xmodule.modulestore.django import create_modulestore_instance, loc_mapper
 import logging
+from xmodule.modulestore import Location
+from xblock.fields import Reference, ReferenceList, String
+from xmodule.modulestore.locator import CourseLocator, Locator, BlockUsageLocator
+from xmodule.modulestore.exceptions import InsufficientSpecificationError, ItemNotFoundError
+from xmodule.modulestore.parsers import ALLOWED_ID_CHARS
+import re
 
 log = logging.getLogger(__name__)
 
 
 class MixedModuleStore(ModuleStoreWriteBase):
     """
-    ModuleStore that can be backed by either XML or Mongo
+    ModuleStore knows how to route requests to the right persistence ms and how to convert any
+    references in the xblocks to the type required by the app and the persistence layer.
     """
-    def __init__(self, mappings, stores, **kwargs):
+    def __init__(self, mappings, stores, reference_type=None, **kwargs):
         """
         Initialize a MixedModuleStore. Here we look into our passed in kwargs which should be a
         collection of other modulestore configuration informations
+
+        :param reference_type: either Location or Locator to indicate what type of reference this app
+        uses.
         """
         super(MixedModuleStore, self).__init__(**kwargs)
 
         self.modulestores = {}
         self.mappings = mappings
+        # temporary code for transition period
+        if reference_type is None:
+            log.warn("reference_type not specified in MixedModuleStore settings. %s",
+                "Will default temporarily to the to-be-deprecated Location.")
+        self.use_locations = (reference_type != 'Locator')
         if 'default' not in stores:
             raise Exception('Missing a default modulestore in the MixedModuleStore __init__ method.')
 
@@ -45,60 +59,203 @@ class MixedModuleStore(ModuleStoreWriteBase):
         mapping = self.mappings.get(course_id, 'default')
         return self.modulestores[mapping]
 
-    def has_item(self, course_id, location):
-        return self._get_modulestore_for_courseid(course_id).has_item(course_id, location)
+    def _locator_to_location(self, reference):
+        """
+        Convert the referenced locator to a location casting to and from a string as necessary
+        """
+        stringify = isinstance(reference, basestring)
+        if stringify:
+            reference = BlockUsageLocator(url=reference)
+        location = loc_mapper().translate_locator_to_location(reference)
+        return location.url() if stringify else location
 
-    def get_item(self, location, depth=0):
+    def _location_to_locator(self, course_id, reference):
         """
-        This method is explicitly not implemented as we need a course_id to disambiguate
-        We should be able to fix this when the data-model rearchitecting is done
+        Convert the referenced location to a locator casting to and from a string as necessary
         """
-        raise NotImplementedError
+        stringify = isinstance(reference, basestring)
+        if stringify:
+            reference = Location(reference)
+        locator = loc_mapper().translate_location(course_id, reference, reference.revision == 'draft', True)
+        return unicode(locator) if stringify else locator
 
-    def get_instance(self, course_id, location, depth=0):
-        return self._get_modulestore_for_courseid(course_id).get_instance(course_id, location, depth)
+    def _incoming_reference_adaptor(self, store, course_id, reference):
+        """
+        Convert the reference to the type the persistence layer wants
+        """
+        if issubclass(store.reference_type, Location if self.use_locations else Locator):
+            return reference
+        if store.reference_type == Location:
+            return self._locator_to_location(reference)
+        return self._location_to_locator(course_id, reference)
 
-    def get_items(self, location, course_id=None, depth=0):
+    def _outgoing_reference_adaptor(self, store, course_id, reference):
         """
-        Returns a list of XModuleDescriptor instances for the items
-        that match location. Any element of location that is None is treated
-        as a wildcard that matches any value
+        Convert the reference to the type the application wants
+        """
+        if issubclass(store.reference_type, Location if self.use_locations else Locator):
+            return reference
+        if store.reference_type == Location:
+            return self._location_to_locator(course_id, reference)
+        return self._locator_to_location(reference)
 
-        location: Something that can be passed to Location
+    def _xblock_adaptor_iterator(self, adaptor, string_converter, store, course_id, xblock):
+        """
+        Change all reference fields in this xblock to the type expected by the receiving layer
+        """
+        for field in xblock.fields.itervalues():
+            if field.is_set_on(xblock):
+                if isinstance(field, Reference):
+                    field.write_to(
+                        xblock,
+                        adaptor(store, course_id, field.read_from(xblock))
+                    )
+                elif isinstance(field, ReferenceList):
+                    field.write_to(
+                        xblock,
+                        [
+                            adaptor(store, course_id, ref)
+                            for ref in field.read_from(xblock)
+                        ]
+                    )
+                elif isinstance(field, String):
+                    # replace links within the string
+                    string_converter(field, xblock)
+        return xblock
 
-        depth: An argument that some module stores may use to prefetch
-            descendents of the queried modules for more efficient results later
-            in the request. The depth is counted in the number of calls to
-            get_children() to cache. None indicates to cache all descendents
+    def _incoming_xblock_adaptor(self, store, course_id, xblock):
+        """
+        Change all reference fields in this xblock to the type expected by the persistence layer
+        """
+        string_converter = self._get_string_converter(
+            course_id, store.reference_type, xblock.location
+        )
+        return self._xblock_adaptor_iterator(
+            self._incoming_reference_adaptor, string_converter, store, course_id, xblock
+        )
+
+    def _outgoing_xblock_adaptor(self, store, course_id, xblock):
         """
-        if not course_id:
-            raise Exception("Must pass in a course_id when calling get_items() with MixedModuleStore")
+        Change all reference fields in this xblock to the type expected by the persistence layer
+        """
+        string_converter = self._get_string_converter(
+            course_id, xblock.location.__class__, xblock.location
+        )
+        return self._xblock_adaptor_iterator(
+            self._outgoing_reference_adaptor, string_converter, store, course_id, xblock
+        )
 
-        return self._get_modulestore_for_courseid(course_id).get_items(location, course_id, depth)
+    CONVERT_RE = re.compile(r"/jump_to_id/({}+)".format(ALLOWED_ID_CHARS))
 
-    def update_item(self, location, data, allow_not_found=False):
+    def _get_string_converter(self, course_id, reference_type, from_base_addr):
         """
-        MixedModuleStore is for read-only (aka LMS)
+        Return a closure which finds and replaces all embedded links in a string field
+        with the correct rewritten link for the target type
         """
-        raise NotImplementedError
+        if self.use_locations and reference_type == Location:
+            return lambda field, xblock: None
+        if not self.use_locations and issubclass(reference_type, Locator):
+            return lambda field, xblock: None
+        if isinstance(from_base_addr, Location):
+            def mapper(found_id):
+                """
+                Convert the found id to BlockUsageLocator block_id
+                """
+                location = from_base_addr.replace(category=None, name=found_id)
+                # NOTE without category, it cannot create a new mapping if there's not one already
+                return loc_mapper().translate_location(course_id, location).block_id
+        else:
+            def mapper(found_id):
+                """
+                Convert the found id to Location block_id
+                """
+                locator = BlockUsageLocator.make_relative(from_base_addr, found_id)
+                return loc_mapper().translate_locator_to_location(locator).name
 
-    def update_children(self, location, children):
+        def converter(field, xblock):
+            """
+            Find all of the ids in the block and replace them w/ their mapped values
+            """
+            value = field.read_from(xblock)
+            self.CONVERT_RE.sub(mapper, value)
+            field.write_to(xblock, value)
+        return converter
+
+    def has_item(self, course_id, reference):
         """
-        MixedModuleStore is for read-only (aka LMS)
+        Does the course include the xblock who's id is reference?
+
+        :param course_id: a course_id or package_id (slashed or dotted)
+        :param reference: a Location or BlockUsageLocator
         """
-        raise NotImplementedError
+        store = self._get_modulestore_for_courseid(course_id)
+        decoded_ref = self._incoming_reference_adaptor(store, course_id, reference)
+        return store.has_item(course_id, decoded_ref)
 
-    def update_metadata(self, location, metadata):
+    def get_item(self, location, depth=0):
         """
-        MixedModuleStore is for read-only (aka LMS)
+        This method is explicitly not implemented as we need a course_id to disambiguate
+        We should be able to fix this when the data-model rearchitecting is done
         """
         raise NotImplementedError
 
-    def delete_item(self, location):
+    def get_instance(self, course_id, location, depth=0):
+        store = self._get_modulestore_for_courseid(course_id)
+        decoded_ref = self._incoming_reference_adaptor(store, course_id, location)
+        xblock = store.get_instance(course_id, decoded_ref, depth)
+        return self._outgoing_xblock_adaptor(store, course_id, xblock)
+
+    def get_items(self, location, course_id=None, depth=0, qualifiers=None):
         """
-        MixedModuleStore is for read-only (aka LMS)
+        Returns a list of XModuleDescriptor instances for the items
+        that match location. Any element of location that is None is treated
+        as a wildcard that matches any value. NOTE: don't use this to look for courses
+        as the course_id is required. Use get_courses.
+
+        location: either a Location possibly w/ None as wildcards for category or name or
+        a Locator with at least a package_id and branch but possibly no block_id.
+
+        depth: An argument that some module stores may use to prefetch
+            descendents of the queried modules for more efficient results later
+            in the request. The depth is counted in the number of calls to
+            get_children() to cache. None indicates to cache all descendents
         """
-        raise NotImplementedError
+        if not (course_id or hasattr(location, 'package_id')):
+            raise Exception("Must pass in a course_id when calling get_items()")
+
+        store = self._get_modulestore_for_courseid(course_id or getattr(location, 'package_id'))
+        # translate won't work w/ missing fields so work around it
+        if store.reference_type == Location:
+            if not self.use_locations:
+                if getattr(location, 'block_id', False):
+                    location = self._incoming_reference_adaptor(store, course_id, location)
+                else:
+                    # get the course's location
+                    location = loc_mapper().translate_locator_to_location(location, get_course=True)
+                    # now remove the unknowns
+                    location = location.replace(
+                        category=qualifiers.get('category', None),
+                        name=None
+                    )
+        else:
+            if self.use_locations:
+                if not isinstance(location, Location):
+                    location = Location(location)
+                try:
+                    location.ensure_fully_specified()
+                    location = loc_mapper().translate_location(
+                        course_id, location, location.revision == 'published', True
+                    )
+                except InsufficientSpecificationError:
+                    # construct the Locator by hand
+                    if location.category is not None and qualifiers.get('category', False):
+                        qualifiers['category'] = location.category
+                    location = loc_mapper().translate_location_to_course_locator(
+                        course_id, location, location.revision == 'published'
+                    )
+        xblocks = store.get_items(location, course_id, depth, qualifiers)
+        xblocks = [self._outgoing_xblock_adaptor(store, course_id, xblock) for xblock in xblocks]
+        return xblocks
 
     def get_courses(self):
         '''
@@ -126,15 +283,50 @@ class MixedModuleStore(ModuleStoreWriteBase):
 
     def get_course(self, course_id):
         """
-        returns the course module associated with the course_id
+        returns the course module associated with the course_id. If no such course exists,
+        it returns None
+
+        :param course_id: must be either a string course_id or a CourseLocator
         """
-        return self._get_modulestore_for_courseid(course_id).get_course(course_id)
+        store = self._get_modulestore_for_courseid(
+            course_id.package_id if hasattr(course_id, 'package_id') else course_id)
+        try:
+            # translate won't work w/ missing fields so work around it
+            if store.reference_type == Location:
+                # takes the course_id: figure out if this is old or new style
+                if not self.use_locations:
+                    if isinstance(course_id, basestring):
+                        course_id = CourseLocator(package_id=course_id, branch='published')
+                    course_location = loc_mapper().translate_locator_to_location(course_id, get_course=True)
+                    course_id = course_location.course_id
+                xblock = store.get_course(course_id)
+            else:
+                # takes a courseLocator
+                if isinstance(course_id, CourseLocator):
+                    location = course_id
+                    course_id = None  # not an old style course_id; so, don't use it further
+                elif '/' in course_id:
+                    location = loc_mapper().translate_location_to_course_locator(course_id, None, True)
+                else:
+                    location = CourseLocator(package_id=course_id, branch='published')
+                    course_id = None  # not an old style course_id; so, don't use it further
+                xblock = store.get_course(location)
+        except ItemNotFoundError:
+            return None
+        if xblock is not None:
+            return self._outgoing_xblock_adaptor(store, course_id, xblock)
+        else:
+            return None
 
     def get_parent_locations(self, location, course_id):
         """
-        returns the parent locations for a given lcoation and course_id
+        returns the parent locations for a given location and course_id
         """
-        return self._get_modulestore_for_courseid(course_id).get_parent_locations(location, course_id)
+        store = self._get_modulestore_for_courseid(course_id)
+        decoded_ref = self._incoming_reference_adaptor(store, course_id, location)
+        parents = store.get_parent_locations(decoded_ref, course_id)
+        return [self._outgoing_reference_adaptor(store, course_id, reference)
+                for reference in parents]
 
     def get_modulestore_type(self, course_id):
         """
@@ -146,6 +338,17 @@ class MixedModuleStore(ModuleStoreWriteBase):
         """
         return self._get_modulestore_for_courseid(course_id).get_modulestore_type(course_id)
 
+    def get_orphans(self, course_location, branch):
+        """
+        Get all of the xblocks in the given course which have no parents and are not of types which are
+        usually orphaned. NOTE: may include xblocks which still have references via xblocks which don't
+        use children to point to their dependents.
+        """
+        course_id = getattr(course_location, 'course_id', getattr(course_location, 'package_id', None))
+        store = self._get_modulestore_for_courseid(course_id)
+        decoded_ref = self._incoming_reference_adaptor(store, course_id, course_location)
+        return store.get_orphans(decoded_ref, branch)
+
     def get_errored_courses(self):
         """
         Return a dictionary of course_dir -> [(msg, exception_str)], for each
@@ -155,3 +358,32 @@ class MixedModuleStore(ModuleStoreWriteBase):
         for store in self.modulestores.values():
             errs.update(store.get_errored_courses())
         return errs
+
+    def update_item(self, xblock, user_id, allow_not_found=False):
+        """
+        Update the xblock persisted to be the same as the given for all types of fields
+        (content, children, and metadata) attribute the change to the given user.
+        """
+        if self.use_locations:
+            raise NotImplementedError
+
+        locator = xblock.location
+        course_id = locator.package_id
+        store = self._get_modulestore_for_courseid(course_id)
+
+        # if an xblock, convert its contents to correct addr scheme
+        xblock = self._incoming_xblock_adaptor(store, course_id, xblock)
+        xblock = store.update_item(xblock, user_id)
+
+        return self._outgoing_xblock_adaptor(store, course_id, xblock)
+
+    def delete_item(self, location, **kwargs):
+        """
+        Delete the given item from persistence.
+        """
+        if self.use_locations:
+            raise NotImplementedError
+
+        store = self._get_modulestore_for_courseid(location.package_id)
+        decoded_ref = self._incoming_reference_adaptor(store, location.package_id, location)
+        return store.delete_item(decoded_ref, **kwargs)

common/lib/xmodule/xmodule/modulestore/mongo/base.py

@@ -38,10 +38,6 @@ from xblock.core import XBlock
 
 log = logging.getLogger(__name__)
 
-# TODO (cpennington): This code currently operates under the assumption that
-# there is only one revision for each item. Once we start versioning inside the CMS,
-# that assumption will have to change
-
 
 def get_course_id_no_run(location):
     '''
@@ -224,6 +220,7 @@ def namedtuple_to_son(namedtuple, prefix=''):
     Converts a namedtuple into a SON object with the same key order
     """
     son = SON()
+    # pylint: disable=protected-access
     for idx, field_name in enumerate(namedtuple._fields):
         son[prefix + field_name] = namedtuple[idx]
     return son
@@ -258,6 +255,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
     """
     A Mongodb backed ModuleStore
     """
+    reference_type = Location
 
     # TODO (cpennington): Enable non-filesystem filestores
     # pylint: disable=C0103
@@ -299,9 +297,11 @@ class MongoModuleStore(ModuleStoreWriteBase):
 
         # Force mongo to maintain an index over _id.* that is in the same order
         # that is used when querying by a location
+        # pylint: disable=no-member, protected_access
         self.collection.ensure_index(
             zip(('_id.' + field for field in Location._fields), repeat(1)),
         )
+        # pylint: enable=no-member, protected_access
 
         if default_class is not None:
             module_path, _, class_name = default_class.rpartition('.')
@@ -363,11 +363,6 @@ class MongoModuleStore(ModuleStoreWriteBase):
             """
             Helper method for computing inherited metadata for a specific location url
             """
-            # check for presence of metadata key. Note that a given module may not yet be fully formed.
-            # example: update_item -> update_children -> update_metadata sequence on new item create
-            # if we get called here without update_metadata called first then 'metadata' hasn't been set
-            # as we're not fully transactional at the DB layer. Same comment applies to below key name
-            # check
             my_metadata = results_by_url[url].get('metadata', {})
 
             # go through all the children and recurse, but only if we have
@@ -443,6 +438,9 @@ class MongoModuleStore(ModuleStoreWriteBase):
         del item['_id']
 
     def _query_children_for_cache_children(self, items):
+        """
+        Generate a pymongo in query for finding the items and return the payloads
+        """
         # first get non-draft in a round-trip
         query = {
             '_id': {'$in': [namedtuple_to_son(Location(item)) for item in items]}
@@ -531,8 +529,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
         '''
         Returns a list of course descriptors.
         '''
-        # TODO (vshnayder): Why do I have to specify i4x here?
-        course_filter = Location("i4x", category="course")
+        course_filter = Location(category="course")
         return [
             course
             for course
@@ -556,6 +553,16 @@ class MongoModuleStore(ModuleStoreWriteBase):
             raise ItemNotFoundError(location)
         return item
 
+    def get_course(self, course_id):
+        """
+        Get the course with the given courseid (org/course/run)
+        """
+        id_components = course_id.split('/')
+        try:
+            return self.get_item(Location('i4x', id_components[0], id_components[1], 'course', id_components[2]))
+        except ItemNotFoundError:
+            return None
+
     def has_item(self, course_id, location):
         """
         Returns True if location exists in this ModuleStore.
@@ -599,7 +606,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
         """
         return self.get_item(location, depth=depth)
 
-    def get_items(self, location, course_id=None, depth=0):
+    def get_items(self, location, course_id=None, depth=0, qualifiers=None):
         items = self.collection.find(
             location_to_query(location),
             sort=[('revision', pymongo.ASCENDING)],
@@ -664,13 +671,13 @@ class MongoModuleStore(ModuleStoreWriteBase):
         # Save any changes to the xmodule to the MongoKeyValueStore
         xmodule.save()
         self.collection.save({
-                '_id': xmodule.location.dict(),
-                'metadata': own_metadata(xmodule),
-                'definition': {
-                    'data': xmodule.get_explicitly_set_fields_by_scope(Scope.content),
-                    'children': xmodule.children if xmodule.has_children else []
-                }
-            })
+            '_id': namedtuple_to_son(xmodule.location),
+            'metadata': own_metadata(xmodule),
+            'definition': {
+                'data': xmodule.get_explicitly_set_fields_by_scope(Scope.content),
+                'children': xmodule.children if xmodule.has_children else []
+            }
+        })
         # recompute (and update) the metadata inheritance tree which is cached
         self.refresh_cached_metadata_inheritance_tree(xmodule.location)
         self.fire_updated_modulestore_signal(get_course_id_no_run(xmodule.location), xmodule.location)
@@ -708,7 +715,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
             course.tabs = existing_tabs
             # Save any changes to the course to the MongoKeyValueStore
             course.save()
-            self.update_metadata(course.location, course.get_explicitly_set_fields_by_scope(Scope.settings))
+            self.update_item(course, '**replace_user**')
 
     def fire_updated_modulestore_signal(self, course_id, location):
         """
@@ -754,7 +761,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
         # See http://www.mongodb.org/display/DOCS/Updating for
         # atomic update syntax
         result = self.collection.update(
-            {'_id': Location(location).dict()},
+            {'_id': namedtuple_to_son(Location(location))},
             {'$set': update},
             multi=False,
             upsert=True,
@@ -765,73 +772,56 @@ class MongoModuleStore(ModuleStoreWriteBase):
         if result['n'] == 0:
             raise ItemNotFoundError(location)
 
-    def update_item(self, location, data, allow_not_found=False):
+    def update_item(self, xblock, user, allow_not_found=False):
         """
-        Set the data in the item specified by the location to
-        data
+        Update the persisted version of xblock to reflect its current values.
 
         location: Something that can be passed to Location
         data: A nested dictionary of problem data
         """
         try:
-            self._update_single_item(location, {'definition.data': data})
+            definition_data = xblock.get_explicitly_set_fields_by_scope()
+            if len(definition_data) == 1 and 'data' in definition_data:
+                definition_data = definition_data['data']
+            payload = {
+                'definition.data': definition_data,
+                'metadata': own_metadata(xblock),
+            }
+            if xblock.has_children:
+                # convert all to urls
+                xblock.children = [child.url() if isinstance(child, Location) else child
+                                   for child in xblock.children]
+                payload.update({'definition.children': xblock.children})
+            self._update_single_item(xblock.location, payload)
+            # for static tabs, their containing course also records their display name
+            if xblock.category == 'static_tab':
+                course = self._get_course_for_item(xblock.location)
+                # find the course's reference to this tab and update the name.
+                for tab in course.tabs:
+                    if tab.get('url_slug') == xblock.location.name:
+                        # only update if changed
+                        if tab['name'] != xblock.display_name:
+                            tab['name'] = xblock.display_name
+                            self.update_item(course, user)
+                            break
+
+            # recompute (and update) the metadata inheritance tree which is cached
+            # was conditional on children or metadata having changed before dhm made one update to rule them all
+            self.refresh_cached_metadata_inheritance_tree(xblock.location)
+            # fire signal that we've written to DB
+            self.fire_updated_modulestore_signal(get_course_id_no_run(xblock.location), xblock.location)
         except ItemNotFoundError:
             if not allow_not_found:
                 raise
 
-    def update_children(self, location, children):
-        """
-        Set the children for the item specified by the location to
-        children
-
-        location: Something that can be passed to Location
-        children: A list of child item identifiers
-        """
-        # Normalize the children to urls
-        children = [Location(child).url() for child in children]
-
-        self._update_single_item(location, {'definition.children': children})
-        # recompute (and update) the metadata inheritance tree which is cached
-        self.refresh_cached_metadata_inheritance_tree(Location(location))
-        # fire signal that we've written to DB
-        self.fire_updated_modulestore_signal(get_course_id_no_run(Location(location)), Location(location))
-
-    def update_metadata(self, location, metadata):
-        """
-        Set the metadata for the item specified by the location to
-        metadata
-
-        location: Something that can be passed to Location
-        metadata: A nested dictionary of module metadata
-        """
-        # VS[compat] cdodge: This is a hack because static_tabs also have references from the course module, so
-        # if we add one then we need to also add it to the policy information (i.e. metadata)
-        # we should remove this once we can break this reference from the course to static tabs
-        loc = Location(location)
-        if loc.category == 'static_tab':
-            course = self._get_course_for_item(loc)
-            existing_tabs = course.tabs or []
-            for tab in existing_tabs:
-                if tab.get('url_slug') == loc.name:
-                    tab['name'] = metadata.get('display_name', tab.get('name'))
-                    break
-            course.tabs = existing_tabs
-            # Save the updates to the course to the MongoKeyValueStore
-            course.save()
-            self.update_metadata(course.location, own_metadata(course))
-
-        self._update_single_item(location, {'metadata': metadata})
-        # recompute (and update) the metadata inheritance tree which is cached
-        self.refresh_cached_metadata_inheritance_tree(loc)
-        self.fire_updated_modulestore_signal(get_course_id_no_run(Location(location)), Location(location))
-
-    def delete_item(self, location, delete_all_versions=False):
+    # pylint: disable=unused-argument
+    def delete_item(self, location, **kwargs):
         """
         Delete an item from this modulestore
 
         location: Something that can be passed to Location
-        delete_all_versions: is here because the DraftMongoModuleStore needs it and we need to keep the interface the same. It is unused.
         """
+        # pylint: enable=unused-argument
         # VS[compat] cdodge: This is a hack because static_tabs also have references from the course module, so
         # if we add one then we need to also add it to the policy information (i.e. metadata)
         # we should remove this once we can break this reference from the course to static tabs
@@ -840,9 +830,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
             course = self._get_course_for_item(item.location)
             existing_tabs = course.tabs or []
             course.tabs = [tab for tab in existing_tabs if tab.get('url_slug') != location.name]
-            # Save the updates to the course to the MongoKeyValueStore
-            course.save()
-            self.update_metadata(course.location, own_metadata(course))
+            self.update_item(course, '**replace_user**')
 
         # Must include this to avoid the django debug toolbar (which defines the deprecated "safe=False")
         # from overriding our default value set in the init method.
@@ -889,7 +877,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
         item_locs -= all_reachable
         return list(item_locs)
 
-    def _create_new_field_data(self, category, location, definition_data, metadata):
+    def _create_new_field_data(self, _category, _location, definition_data, metadata):
         """
         To instantiate a new xmodule which will be saved latter, set up the dbModel and kvs
         """

common/lib/xmodule/xmodule/modulestore/mongo/draft.py

@@ -11,11 +11,9 @@ from datetime import datetime
 from xmodule.exceptions import InvalidVersionError
 from xmodule.modulestore import Location
 from xmodule.modulestore.exceptions import ItemNotFoundError, DuplicateItemError
-from xmodule.modulestore.inheritance import own_metadata
 from xmodule.modulestore.mongo.base import location_to_query, namedtuple_to_son, get_course_id_no_run, MongoModuleStore
 import pymongo
 from pytz import UTC
-from xblock.fields import Scope
 
 DRAFT = 'draft'
 # Things w/ these categories should never be marked as version='draft'
@@ -109,7 +107,7 @@ class DraftModuleStore(MongoModuleStore):
         return super(DraftModuleStore, self).create_xmodule(draft_loc, definition_data, metadata, system)
 
 
-    def get_items(self, location, course_id=None, depth=0):
+    def get_items(self, location, course_id=None, depth=0, qualifiers=None):
         """
         Returns a list of XModuleDescriptor instances for the items
         that match location. Any element of location that is None is treated
@@ -146,7 +144,9 @@ class DraftModuleStore(MongoModuleStore):
         draft_location = as_draft(source_location)
         if draft_location.category in DIRECT_ONLY_CATEGORIES:
             raise InvalidVersionError(source_location)
-        original['_id'] = draft_location.dict()
+        if not original:
+            raise ItemNotFoundError
+        original['_id'] = namedtuple_to_son(draft_location)
         try:
             self.collection.insert(original)
         except pymongo.errors.DuplicateKeyError:
@@ -157,60 +157,27 @@ class DraftModuleStore(MongoModuleStore):
 
         return self._load_items([original])[0]
 
-    def update_item(self, location, data, allow_not_found=False):
+    def update_item(self, xblock, user, allow_not_found=False):
         """
-        Set the data in the item specified by the location to
-        data
+        Save the current values to persisted version of the xblock
 
         location: Something that can be passed to Location
         data: A nested dictionary of problem data
         """
-        draft_loc = as_draft(location)
+        draft_loc = as_draft(xblock.location)
         try:
-            draft_item = self.get_item(location)
-            if not getattr(draft_item, 'is_draft', False):
-                self.convert_to_draft(location)
-        except ItemNotFoundError, e:
+            if not self.has_item(None, draft_loc):
+                self.convert_to_draft(xblock.location)
+        except ItemNotFoundError:
             if not allow_not_found:
-                raise e
-
-        return super(DraftModuleStore, self).update_item(draft_loc, data)
-
-    def update_children(self, location, children):
-        """
-        Set the children for the item specified by the location to
-        children
-
-        location: Something that can be passed to Location
-        children: A list of child item identifiers
-        """
-        draft_loc = as_draft(location)
-        draft_item = self.get_item(location)
-        if not getattr(draft_item, 'is_draft', False):
-            self.convert_to_draft(location)
-
-        return super(DraftModuleStore, self).update_children(draft_loc, children)
-
-    def update_metadata(self, location, metadata):
-        """
-        Set the metadata for the item specified by the location to
-        metadata
-
-        location: Something that can be passed to Location
-        metadata: A nested dictionary of module metadata
-        """
-        draft_loc = as_draft(location)
-        draft_item = self.get_item(location)
-
-        if not getattr(draft_item, 'is_draft', False):
-            self.convert_to_draft(location)
-
-        if 'is_draft' in metadata:
-            del metadata['is_draft']
+                raise
 
-        return super(DraftModuleStore, self).update_metadata(draft_loc, metadata)
+        xblock.location = draft_loc
+        super(DraftModuleStore, self).update_item(xblock, user)
+        # don't allow locations to truly represent themselves as draft outside of this file
+        xblock.location = as_published(xblock.location)
 
-    def delete_item(self, location, delete_all_versions=False):
+    def delete_item(self, location, delete_all_versions=False, **kwargs):
         """
         Delete an item from this modulestore
 
@@ -243,7 +210,6 @@ class DraftModuleStore(MongoModuleStore):
 
         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:
@@ -254,8 +220,7 @@ class DraftModuleStore(MongoModuleStore):
                         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))
+        super(DraftModuleStore, self).update_item(draft, '**replace_user**')
         self.delete_item(location)
 
     def unpublish(self, location):

common/lib/xmodule/xmodule/modulestore/split_mongo/split.py

@@ -57,7 +57,9 @@ from pytz import UTC
 
 from xmodule.errortracker import null_error_tracker
 from xmodule.x_module import prefer_xmodules
-from xmodule.modulestore.locator import BlockUsageLocator, DefinitionLocator, CourseLocator, VersionTree, LocalId
+from xmodule.modulestore.locator import (
+    BlockUsageLocator, DefinitionLocator, CourseLocator, VersionTree, LocalId, Locator
+)
 from xmodule.modulestore.exceptions import InsufficientSpecificationError, VersionConflictError, DuplicateItemError
 from xmodule.modulestore import inheritance, ModuleStoreWriteBase, Location, SPLIT_MONGO_MODULESTORE_TYPE
 
@@ -98,6 +100,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
     A Mongodb backed ModuleStore supporting versions, inheritance,
     and sharing.
     """
+    reference_type = Locator
     def __init__(self, doc_store_config, fs_root, render_template,
                  default_class=None,
                  error_tracker=null_error_tracker,
@@ -936,7 +939,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         self.db_connection.insert_course_index(index_entry)
         return self.get_course(CourseLocator(package_id=new_id, branch=master_branch))
 
-    def update_item(self, descriptor, user_id, force=False):
+    def update_item(self, descriptor, user_id, allow_not_found=False, force=False):
         """
         Save the descriptor's fields. it doesn't descend the course dag to save the children.
         Return the new descriptor (updated location).
@@ -1115,14 +1118,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
                 if key not in new_keys or original_fields[key] != settings[key]:
                     return True
 
-    def update_children(self, location, children):
-        '''Deprecated, use update_item.'''
-        raise NotImplementedError('use update_item')
-
-    def update_metadata(self, location, metadata):
-        '''Deprecated, use update_item.'''
-        raise NotImplementedError('use update_item')
-
     def xblock_publish(self, user_id, source_course, destination_course, subtree_list, blacklist):
         """
         Publishes each xblock in subtree_list and those blocks descendants excluding blacklist
@@ -1211,7 +1206,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         """
         self.db_connection.update_course_index(updated_index_entry)
 
-    def delete_item(self, usage_locator, user_id, delete_children=False, force=False):
+    # TODO impl delete_all_versions
+    def delete_item(self, usage_locator, user_id, delete_all_versions=False, delete_children=False, force=False):
         """
         Delete the block or tree rooted at block (if delete_children) and any references w/in the course to the block
         from a new version of the course structure.
@@ -1361,6 +1357,16 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         else:
             return DefinitionLocator(definition['_id'])
 
+    def get_modulestore_type(self, course_id):
+        """
+        Returns an enumeration-like type reflecting the type of this modulestore
+        The return can be one of:
+        "xml" (for XML based courses),
+        "mongo" for old-style MongoDB backed courses,
+        "split" for new-style split MongoDB backed courses.
+        """
+        return SPLIT_MONGO_MODULESTORE_TYPE
+
     def internal_clean_children(self, course_locator):
         """
         Only intended for rather low level methods to use. Goes through the children attrs of
@@ -1509,16 +1515,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
             del fields['category']
         return fields
 
-    def get_modulestore_type(self, course_id):
-        """
-        Returns an enumeration-like type reflecting the type of this modulestore
-        The return can be one of:
-        "xml" (for XML based courses),
-        "mongo" for old-style MongoDB backed courses,
-        "split" for new-style split MongoDB backed courses.
-        """
-        return SPLIT_MONGO_MODULESTORE_TYPE
-
     def _new_structure(self, user_id, root_block_id,
                        root_category=None, block_fields=None, definition_id=None):
         """

common/lib/xmodule/xmodule/modulestore/store_utilities.py

 import re
 from xmodule.contentstore.content import StaticContent
 from xmodule.modulestore import Location
-from xmodule.modulestore.inheritance import own_metadata
 
 import logging
 
@@ -125,13 +124,10 @@ def _clone_modules(modulestore, modules, source_location, dest_location):
 
         print "Cloning module {0} to {1}....".format(original_loc, module.location)
 
-        # NOTE: usage of the the internal module.xblock_kvs._data does not include any 'default' values for the fields
-        data = module.xblock_kvs._data
-        if isinstance(data, basestring):
-            data = rewrite_nonportable_content_links(
-                source_location.course_id, dest_location.course_id, data)
-
-        modulestore.update_item(module.location, data)
+        if 'data' in module.fields and module.fields['data'].is_set_on(module) and isinstance(module.data, basestring):
+            module.data = rewrite_nonportable_content_links(
+                source_location.course_id, dest_location.course_id, module.data
+            )
 
         # repoint children
         if module.has_children:
@@ -145,10 +141,9 @@ def _clone_modules(modulestore, modules, source_location, dest_location):
                 )
                 new_children.append(child_loc.url())
 
-            modulestore.update_children(module.location, new_children)
+            module.children = new_children
 
-        # save metadata
-        modulestore.update_metadata(module.location, own_metadata(module))
+        modulestore.update_item(module, '**replace_user**')
 
 
 def clone_course(modulestore, contentstore, source_location, dest_location, delete_original=False):

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

@@ -33,6 +33,7 @@ def mixed_store_config(data_dir, mappings):
             'ENGINE': 'xmodule.modulestore.mixed.MixedModuleStore',
             'OPTIONS': {
                 'mappings': mappings,
+                'reference_type': 'Location',
                 'stores': {
                     'default': mongo_config['default'],
                     'xml': xml_config['default']
@@ -196,18 +197,15 @@ class ModuleStoreTestCase(TestCase):
     """
 
     @staticmethod
-    def update_course(course, data):
+    def update_course(course):
         """
         Updates the version of course in the modulestore
-        with the metadata in 'data' and returns the updated version.
 
         'course' is an instance of CourseDescriptor for which we want
         to update metadata.
-
-        'data' is a dictionary with an entry for each CourseField we want to update.
         """
         store = editable_modulestore('direct')
-        store.update_metadata(course.location, data)
+        store.update_item(course, '**replace_user**')
         updated_course = store.get_instance(course.id, course.location)
         return updated_course
 

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

-import datetime
-
 from factory import Factory, lazy_attribute_sequence, lazy_attribute
 from factory.containers import CyclicDefinitionError
 from uuid import uuid4
-from pytz import UTC
 
 from xmodule.modulestore import Location
 from xmodule.x_module import prefer_xmodules
@@ -127,7 +124,6 @@ class ItemFactory(XModuleFactory):
         # passed in via **kwargs. However, some of those aren't actual field values,
         # so pop those off for use separately
 
-        DETACHED_CATEGORIES = ['about', 'static_tab', 'course_info']
         # catch any old style users before they get into trouble
         assert 'template' not in kwargs
         parent_location = Location(kwargs.pop('parent_location', None))
@@ -155,7 +151,7 @@ class ItemFactory(XModuleFactory):
         # replace the display name with an optional parameter passed in from the caller
         if display_name is not None:
             metadata['display_name'] = display_name
-        module = store.create_and_save_xmodule(location, metadata=metadata, definition_data=data)
+        store.create_and_save_xmodule(location, metadata=metadata, definition_data=data)
 
         module = store.get_item(location)
 
@@ -165,8 +161,8 @@ class ItemFactory(XModuleFactory):
 
         store.save_xmodule(module)
 
-        if location.category not in DETACHED_CATEGORIES:
+        if 'detached' not in module._class_tags:
             parent.children.append(location.url())
-            store.update_children(parent_location, parent.children)
+            store.update_item(parent, '**replace_user**')
 
         return store.get_item(location)

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

@@ -7,7 +7,7 @@ import unittest
 import uuid
 from xmodule.modulestore import Location
 from xmodule.modulestore.locator import BlockUsageLocator
-from xmodule.modulestore.exceptions import ItemNotFoundError
+from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError
 from xmodule.modulestore.loc_mapper_store import LocMapperStore
 from mock import Mock
 
@@ -114,7 +114,7 @@ class TestLocationMapper(unittest.TestCase):
 
         new_style_package_id = '{}.geek_dept.{}.baz_run'.format(org, course)
         block_map = {
-            'abc123': {'problem': 'problem2'},
+            'abc123': {'problem': 'problem2', 'vertical': 'vertical2'},
             'def456': {'problem': 'problem4'},
             'ghi789': {'problem': 'problem7'},
         }
@@ -136,6 +136,14 @@ class TestLocationMapper(unittest.TestCase):
                 Location('i4x', org, course, 'problem', '1def23'),
                 add_entry_if_missing=False
             )
+        test_no_cat_locn = test_problem_locn.replace(category=None)
+        with self.assertRaises(InvalidLocationError):
+            loc_mapper().translate_location(
+                old_style_course_id, test_no_cat_locn, False, False
+            )
+        test_no_cat_locn = test_no_cat_locn.replace(name='def456')
+        # only one course matches
+        self.translate_n_check(test_no_cat_locn, old_style_course_id, new_style_package_id, 'problem4', 'published')
 
         # add a distractor course (note that abc123 has a different translation in this one)
         distractor_block_map = {

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

@@ -259,6 +259,24 @@ class LocatorTest(TestCase):
         testobj = BlockUsageLocator(package_id=package_id, branch=branch, block_id=block_id)
         self.check_block_locn_fields(testobj, 'Cannot handle colon', package_id=package_id, branch=branch, block=block_id)
 
+    def test_relative(self):
+        """
+        Test making a relative usage locator.
+        """
+        package_id = 'mit.eecs-1'
+        branch = 'foo'
+        baseobj = CourseLocator(package_id=package_id, branch=branch)
+        block_id = 'problem:with-colon~2'
+        testobj = BlockUsageLocator.make_relative(baseobj, block_id)
+        self.check_block_locn_fields(
+            testobj, 'Cannot make relative to course', package_id=package_id, branch=branch, block=block_id
+        )
+        block_id = 'completely_different'
+        testobj = BlockUsageLocator.make_relative(testobj, block_id)
+        self.check_block_locn_fields(
+            testobj, 'Cannot make relative to block usage', package_id=package_id, branch=branch, block=block_id
+        )
+
     def test_repr(self):
         testurn = 'mit.eecs.6002x/' + BRANCH_PREFIX + 'published/' + BLOCK_PREFIX + 'HW3'
         testobj = BlockUsageLocator(package_id=testurn)

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

@@ -13,6 +13,8 @@ from xmodule.modulestore.xml_importer import import_from_xml
 # Mixed modulestore depends on django, so we'll manually configure some django settings
 # before importing the module
 from django.conf import settings
+import unittest
+import copy
 if not settings.configured:
     settings.configure()
 
@@ -37,6 +39,7 @@ OPTIONS = {
         XML_COURSEID2: 'xml',
         IMPORT_COURSEID: 'default'
     },
+    'reference_type': 'Location',
     'stores': {
         'xml': {
             'ENGINE': 'xmodule.modulestore.xml.XMLModuleStore',
@@ -182,6 +185,7 @@ class TestMixedModuleStore(object):
             )
 
     def test_get_items(self):
+        # NOTE: use get_course if you just want the course. get_items only allows wildcarding of category and name
         modules = self.store.get_items(Location('i4x', None, None, 'course', None), IMPORT_COURSEID)
         assert_equals(len(modules), 1)
         assert_equals(modules[0].location.course, self.import_course)
@@ -190,21 +194,14 @@ class TestMixedModuleStore(object):
         assert_equals(len(modules), 1)
         assert_equals(modules[0].location.course, 'toy')
 
-        modules = self.store.get_items(Location('i4x', None, None, 'course', None), XML_COURSEID2)
+        modules = self.store.get_items(Location('i4x', 'edX', 'simple', 'course', None), XML_COURSEID2)
         assert_equals(len(modules), 1)
         assert_equals(modules[0].location.course, 'simple')
 
     def test_update_item(self):
+        # FIXME update
         with assert_raises(NotImplementedError):
-            self.store.update_item(self.fake_location, None)
-
-    def test_update_children(self):
-        with assert_raises(NotImplementedError):
-            self.store.update_children(self.fake_location, None)
-
-    def test_update_metadata(self):
-        with assert_raises(NotImplementedError):
-            self.store.update_metadata(self.fake_location, None)
+            self.store.update_item(self.fake_location, '**replace_user**')
 
     def test_delete_item(self):
         with assert_raises(NotImplementedError):
@@ -250,3 +247,25 @@ class TestMixedModuleStore(object):
         assert_equals(Location(parents[0]).org, 'edX')
         assert_equals(Location(parents[0]).course, 'toy')
         assert_equals(Location(parents[0]).name, '2012_Fall')
+
+class TestMixedMSInit(unittest.TestCase):
+    """
+    Test initializing w/o a reference_type
+    """
+    def setUp(self):
+        unittest.TestCase.setUp(self)
+        options = copy.copy(OPTIONS)
+        del options['reference_type']
+        self.connection = pymongo.MongoClient(
+            host=HOST,
+            port=PORT,
+            tz_aware=True,
+        )
+        self.store = MixedModuleStore(**options)
+
+    def test_use_locations(self):
+        """
+        Test that use_locations defaulted correctly
+        """
+        self.assertTrue(self.store.use_locations)
+

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

@@ -82,7 +82,7 @@ class TestOrphan(unittest.TestCase):
             parent_location = Location('i4x', 'test_org', 'test_course', parent_category, parent_name)
             parent = self.old_mongo.get_item(parent_location)
             parent.children.append(location.url())
-            self.old_mongo.update_children(parent_location, parent.children)
+            self.old_mongo.update_item(parent, self.userid)
             # create pointer for split
             course_or_parent_locator = BlockUsageLocator(
                 package_id=self.split_package_id,

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

@@ -71,7 +71,7 @@ class TestPublish(unittest.TestCase):
                 mongo = self.old_mongo
             else:
                 mongo = self.draft_mongo
-            mongo.update_children(parent_location, parent.children)
+            mongo.update_item(parent, '**replace_user**')
 
     def _create_course(self):
         """
@@ -174,8 +174,8 @@ class TestPublish(unittest.TestCase):
         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)
+        self.draft_mongo.update_item(draft_vert, '**replace_user**')
+        self.draft_mongo.update_item(other_vert, '**replace_user**')
         # publish
         self._xmodule_recurse(
             draft_vert,

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

@@ -102,7 +102,7 @@ class TestMigration(unittest.TestCase):
         location = location.replace(category='chapter', name=uuid.uuid4().hex)
         chapter2 = self._create_and_get_item(self.old_mongo, location, {}, {'display_name': 'Chapter 2'}, runtime)
         course_root.children.append(chapter2.location.url())
-        self.old_mongo.update_children(course_root.location, course_root.children)
+        self.old_mongo.update_item(course_root, '**replace_user**')
         # vertical in live only
         location = location.replace(category='vertical', name=uuid.uuid4().hex)
         live_vert = self._create_and_get_item(self.old_mongo, location, {}, {'display_name': 'Live vertical'}, runtime)
@@ -140,7 +140,7 @@ class TestMigration(unittest.TestCase):
         self.create_random_units(self.old_mongo, live_vert)
 
         # update the chapter
-        self.old_mongo.update_children(chapter1.location, chapter1.children)
+        self.old_mongo.update_item(chapter1, '**replace_user**')
 
         # now the other one w/ the conditional
         # first create some show children
@@ -169,7 +169,7 @@ class TestMigration(unittest.TestCase):
         # add direct children
         self.create_random_units(self.old_mongo, conditional)
         chapter2.children.append(conditional.location.url())
-        self.old_mongo.update_children(chapter2.location, chapter2.children)
+        self.old_mongo.update_item(chapter2, '**replace_user**')
 
         # and the ancillary docs (not children)
         location = location.replace(category='static_tab', name=uuid.uuid4().hex)
@@ -207,9 +207,9 @@ class TestMigration(unittest.TestCase):
                     cc_store, location, data, {'display_name': str(uuid.uuid4())}, parent.runtime
                 )
                 cc_parent.children.append(element.location.url())
-        store.update_children(parent.location, parent.children)
+        store.update_item(parent, '**replace_user**')
         if cc_store is not None:
-            cc_store.update_children(cc_parent.location, cc_parent.children)
+            cc_store.update_item(cc_parent, '**replace_user**')
 
     def compare_courses(self, presplit, published):
         # descend via children to do comparison

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

@@ -748,7 +748,7 @@ class TestItemCrud(SplitModuleTest):
 
         problem.max_attempts = 4
         problem.save()  # decache above setting into the kvs
-        updated_problem = modulestore().update_item(problem, 'changeMaven')
+        updated_problem = modulestore().update_item(problem, '**replace_user**')
         # check that course version changed and course's previous is the other one
         self.assertEqual(updated_problem.definition_locator.definition_id, pre_def_id)
         self.assertNotEqual(updated_problem.location.version_guid, pre_version_guid)
@@ -767,7 +767,7 @@ class TestItemCrud(SplitModuleTest):
         history_info = modulestore().get_course_history_info(current_course.location)
         self.assertEqual(history_info['previous_version'], pre_version_guid)
         self.assertEqual(str(history_info['original_version']), self.GUID_D3)
-        self.assertEqual(history_info['edited_by'], "changeMaven")
+        self.assertEqual(history_info['edited_by'], "**replace_user**")
         self.assertGreaterEqual(history_info['edited_on'], premod_time)
         self.assertLessEqual(history_info['edited_on'], datetime.datetime.now(UTC))
 
@@ -784,7 +784,7 @@ class TestItemCrud(SplitModuleTest):
         self.assertGreater(len(block.children), 0, "meaningless test")
         moved_child = block.children.pop()
         block.save()  # decache model changes
-        updated_problem = modulestore().update_item(block, 'childchanger')
+        updated_problem = modulestore().update_item(block, '**replace_user**')
         # check that course version changed and course's previous is the other one
         self.assertEqual(updated_problem.definition_locator.definition_id, pre_def_id)
         self.assertNotEqual(updated_problem.location.version_guid, pre_version_guid)
@@ -794,7 +794,7 @@ class TestItemCrud(SplitModuleTest):
         other_block = modulestore().get_item(locator)
         other_block.children.append(moved_child)
         other_block.save()  # decache model changes
-        other_updated = modulestore().update_item(other_block, 'childchanger')
+        other_updated = modulestore().update_item(other_block, '**replace_user**')
         self.assertIn(moved_child, other_updated.children)
 
     def test_update_definition(self):
@@ -808,7 +808,7 @@ class TestItemCrud(SplitModuleTest):
 
         block.grading_policy['GRADER'][0]['min_count'] = 13
         block.save()  # decache model changes
-        updated_block = modulestore().update_item(block, 'definition_changer')
+        updated_block = modulestore().update_item(block, '**replace_user**')
 
         self.assertNotEqual(updated_block.definition_locator.definition_id, pre_def_id)
         self.assertNotEqual(updated_block.location.version_guid, pre_version_guid)
@@ -846,7 +846,7 @@ class TestItemCrud(SplitModuleTest):
         block.advertised_start = "Soon"
 
         block.save()  # decache model changes
-        updated_block = modulestore().update_item(block, "test_update_manifold")
+        updated_block = modulestore().update_item(block, "**replace_user**")
         self.assertNotEqual(updated_block.definition_locator.definition_id, pre_def_id)
         self.assertNotEqual(updated_block.location.version_guid, pre_version_guid)
         self.assertEqual(updated_block.grading_policy['GRADER'][0]['min_count'], 13)

common/lib/xmodule/xmodule/modulestore/xml.py

@@ -377,6 +377,7 @@ class XMLModuleStore(ModuleStoreReadBase):
             self.default_class = class_
 
         self.parent_trackers = defaultdict(ParentTracker)
+        self.reference_type = Location
 
         # All field data will be stored in an inheriting field data.
         self.field_data = inheriting_field_data(kvs=DictKeyValueStore())
@@ -644,7 +645,7 @@ class XMLModuleStore(ModuleStoreReadBase):
         raise NotImplementedError("XMLModuleStores can't guarantee that definitions"
                                   " are unique. Use get_instance.")
 
-    def get_items(self, location, course_id=None, depth=0):
+    def get_items(self, location, course_id=None, depth=0, qualifiers=None):
         items = []
 
         def _add_get_items(self, location, modules):
@@ -676,33 +677,22 @@ class XMLModuleStore(ModuleStoreReadBase):
         """
         return dict((k, self.errored_courses[k].errors) for k in self.errored_courses)
 
-    def update_item(self, location, data):
+    def get_orphans(self, course_location, _branch):
         """
-        Set the data in the item specified by the location to
-        data
-
-        location: Something that can be passed to Location
-        data: A nested dictionary of problem data
+        Get all of the xblocks in the given course which have no parents and are not of types which are
+        usually orphaned. NOTE: may include xblocks which still have references via xblocks which don't
+        use children to point to their dependents.
         """
-        raise NotImplementedError("XMLModuleStores are read-only")
+        # here just to quell the abstractmethod. someone could write the impl if needed
+        raise NotImplementedError
 
-    def update_children(self, location, children):
+    def update_item(self, xblock, user, **kwargs):
         """
-        Set the children for the item specified by the location to
+        Set the data in the item specified by the location to
         data
 
         location: Something that can be passed to Location
-        children: A list of child item identifiers
-        """
-        raise NotImplementedError("XMLModuleStores are read-only")
-
-    def update_metadata(self, location, metadata):
-        """
-        Set the metadata for the item specified by the location to
-        metadata
-
-        location: Something that can be passed to Location
-        metadata: A nested dictionary of module metadata
+        data: A nested dictionary of problem data
         """
         raise NotImplementedError("XMLModuleStores are read-only")
 

common/lib/xmodule/xmodule/modulestore/xml_importer.py

@@ -4,14 +4,13 @@ import mimetypes
 from path import path
 import json
 
-from xblock.fields import Scope
-
 from .xml import XMLModuleStore, ImportSystem, ParentTracker
 from xmodule.modulestore import Location
 from xmodule.contentstore.content import StaticContent
 from .inheritance import own_metadata
 from xmodule.errortracker import make_error_tracker
 from .store_utilities import rewrite_nonportable_content_links
+import xblock
 
 log = logging.getLogger(__name__)
 
@@ -318,43 +317,13 @@ def import_module(
 
     logging.debug('processing import of module {}...'.format(module.location.url()))
 
-    content = {}
-    for field in module.fields.values():
-        if field.scope != Scope.content:
-            continue
-        try:
-            content[field.name] = module._field_data.get(module, field.name)
-        except KeyError:
-            # Ignore any missing keys in _field_data
-            pass
-
-    module_data = {}
-    if 'data' in content:
-        module_data = content['data']
-    else:
-        module_data = content
-
-    if isinstance(module_data, basestring) and do_import_static:
+    if do_import_static and 'data' in module.fields and isinstance(module.fields['data'], xblock.fields.String):
         # we want to convert all 'non-portable' links in the module_data
         # (if it is a string) to portable strings (e.g. /static/)
-        module_data = rewrite_nonportable_content_links(
+        module.data = rewrite_nonportable_content_links(
             source_course_location.course_id,
-            dest_course_location.course_id, module_data
-        )
-
-    if allow_not_found:
-        store.update_item(
-            module.location, module_data, allow_not_found=allow_not_found
+            dest_course_location.course_id, module.data
         )
-    else:
-        store.update_item(module.location, module_data)
-
-    if hasattr(module, 'children') and module.children != []:
-        store.update_children(module.location, module.children)
-
-    # NOTE: It's important to use own_metadata here to avoid writing
-    # inherited metadata everywhere.
-
     # remove any export/import only xml_attributes
     # which are used to wire together draft imports
     if 'parent_sequential_url' in getattr(module, 'xml_attributes', []):
@@ -362,9 +331,8 @@ def import_module(
 
     if 'index_in_children_list' in getattr(module, 'xml_attributes', []):
         del module.xml_attributes['index_in_children_list']
-    module.save()
 
-    store.update_metadata(module.location, dict(own_metadata(module)))
+    store.update_item(module, '**replace_user**', allow_not_found=allow_not_found)
 
 
 def import_course_draft(
@@ -409,7 +377,7 @@ def import_course_draft(
     # First it is necessary to order the draft items by their desired index in the child list
     # (order os.walk returns them in is not guaranteed).
     drafts = dict()
-    for dirname, dirnames, filenames in os.walk(draft_dir + "/vertical"):
+    for dirname, _dirnames, filenames in os.walk(draft_dir + "/vertical"):
         for filename in filenames:
             module_path = os.path.join(dirname, filename)
             with open(module_path, 'r') as f:
@@ -496,7 +464,7 @@ def import_course_draft(
 
                             if non_draft_location.url() not in sequential.children:
                                 sequential.children.insert(index, non_draft_location.url())
-                                store.update_children(sequential.location, sequential.children)
+                                store.update_item(sequential, '**replace_user**')
 
                         import_module(
                             module, draft_store, course_data_path,

lms/djangoapps/courseware/tests/test_submitting_problems.py

@@ -228,9 +228,9 @@ class TestCourseGrader(TestSubmittingProblems):
         Add a grading policy to the course.
         """
 
-        course_data = {'grading_policy': grading_policy}
+        self.course.grading_policy = grading_policy
         store = editable_modulestore('direct')
-        store.update_item(self.course.location, course_data)
+        store.update_item(self.course, '**replace_user**')
         self.refresh_course()
 
     def get_grade_summary(self):

lms/djangoapps/courseware/tests/test_view_authentication.py

@@ -25,6 +25,7 @@ from courseware.tests.factories import (
     OrgStaffFactory,
     OrgInstructorFactory,
 )
+from xmodule.modulestore.django import modulestore
 
 
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
@@ -84,7 +85,7 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
         urls.extend([
             reverse('book', kwargs={'course_id': course.id,
                                     'book_index': index})
-            for index, book in enumerate(course.textbooks)
+            for index in xrange(len(course.textbooks))
         ])
         for url in urls:
             check_for_get_code(self, 200, url)
@@ -112,6 +113,7 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
         self.course = CourseFactory.create(number='999', display_name='Robot_Super_Course')
         self.overview_chapter = ItemFactory.create(display_name='Overview')
         self.courseware_chapter = ItemFactory.create(display_name='courseware')
+        self.course = modulestore().get_course(self.course.id)
 
         self.test_course = CourseFactory.create(number='666', display_name='Robot_Sub_Course')
         self.other_org_course = CourseFactory.create(org='Other_Org_Course')
@@ -126,6 +128,7 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
             parent_location=self.overview_chapter.location,
             display_name='Welcome'
         )
+        self.test_course = modulestore().get_course(self.test_course.id)
 
         self.global_staff_user = GlobalStaffFactory()
         self.unenrolled_user = UserFactory(last_name="Unenrolled")
@@ -264,10 +267,10 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
         # Make courses start in the future
         now = datetime.datetime.now(pytz.UTC)
         tomorrow = now + datetime.timedelta(days=1)
-        course_data = {'start': tomorrow}
-        test_course_data = {'start': tomorrow}
-        self.course = self.update_course(self.course, course_data)
-        self.test_course = self.update_course(self.test_course, test_course_data)
+        self.course.start = tomorrow
+        self.test_course.start = tomorrow
+        self.course = self.update_course(self.course)
+        self.test_course = self.update_course(self.test_course)
 
         self.assertFalse(self.course.has_started())
         self.assertFalse(self.test_course.has_started())
@@ -289,10 +292,10 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
         """
         now = datetime.datetime.now(pytz.UTC)
         tomorrow = now + datetime.timedelta(days=1)
-        course_data = {'start': tomorrow}
-        test_course_data = {'start': tomorrow}
-        self.course = self.update_course(self.course, course_data)
-        self.test_course = self.update_course(self.test_course, test_course_data)
+        self.course.start = tomorrow
+        self.test_course.start = tomorrow
+        self.course = self.update_course(self.course)
+        self.test_course = self.update_course(self.test_course)
 
         self.login(self.instructor_user)
         # Enroll in the classes---can't see courseware otherwise.
@@ -312,10 +315,10 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
         """
         now = datetime.datetime.now(pytz.UTC)
         tomorrow = now + datetime.timedelta(days=1)
-        course_data = {'start': tomorrow}
-        test_course_data = {'start': tomorrow}
-        self.course = self.update_course(self.course, course_data)
-        self.test_course = self.update_course(self.test_course, test_course_data)
+        self.course.start = tomorrow
+        self.test_course.start = tomorrow
+        self.course = self.update_course(self.course)
+        self.test_course = self.update_course(self.test_course)
 
         self.login(self.global_staff_user)
         self.enroll(self.course, True)
@@ -336,13 +339,14 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
         nextday = tomorrow + datetime.timedelta(days=1)
         yesterday = now - datetime.timedelta(days=1)
 
-        course_data = {'enrollment_start': tomorrow, 'enrollment_end': nextday}
-        test_course_data = {'enrollment_start': yesterday, 'enrollment_end': tomorrow}
-
         # self.course's enrollment period hasn't started
-        self.course = self.update_course(self.course, course_data)
+        self.course.enrollment_start = tomorrow
+        self.course.enrollment_end = nextday
         # test_course course's has
-        self.test_course = self.update_course(self.test_course, test_course_data)
+        self.test_course.enrollment_start = yesterday
+        self.test_course.enrollment_end = tomorrow
+        self.course = self.update_course(self.course)
+        self.test_course = self.update_course(self.test_course)
 
         # First, try with an enrolled student
         self.login(self.unenrolled_user)

lms/djangoapps/courseware/views.py

@@ -232,13 +232,13 @@ def index(request, course_id, chapter=None, section=None,
      - HTTPresponse
     """
     user = User.objects.prefetch_related("groups").get(id=request.user.id)
-    request.user = user	# keep just one instance of User
+    request.user = user  # keep just one instance of User
     course = get_course_with_access(user, course_id, 'load', depth=2)
     staff_access = has_access(user, course, 'staff')
     registered = registered_for_course(course, user)
     if not registered:
         # TODO (vshnayder): do course instructors need to be registered to see course?
-        log.debug(u'User {0} tried to view course {1} but is not enrolled'.format(user, course.location.url()))
+        log.debug(u'User %s tried to view course %s but is not enrolled', user, course.location.url())
         return redirect(reverse('about_course', args=[course.id]))
 
     masq = setup_masquerade(request, staff_access)

lms/djangoapps/instructor_task/tests/test_base.py

@@ -208,7 +208,9 @@ class InstructorTaskModuleTestCase(InstructorTaskCourseTestCase):
                         'num_responses': 2}
         problem_xml = factory.build_xml(**factory_args)
         location = InstructorTaskTestCase.problem_location(problem_url_name)
-        self.module_store.update_item(location, problem_xml)
+        item = self.module_store.get_instance(self.course.id, location)
+        item.data = problem_xml
+        self.module_store.update_item(item, '**replace_user**')
 
     def get_student_module(self, username, descriptor):
         """Get StudentModule object for test course, given the `username` and the problem's `descriptor`."""

lms/djangoapps/instructor_task/tests/test_integration.py

@@ -288,7 +288,11 @@ class TestRescoringTask(TestIntegrationTask):
             """ % ('!=' if redefine else '=='))
         problem_xml = factory.build_xml(script=script, cfn="check_func", expect="42", num_responses=1)
         if redefine:
-            self.module_store.update_item(InstructorTaskModuleTestCase.problem_location(problem_url_name), problem_xml)
+            descriptor = self.module_store.get_instance(
+                self.course.id, InstructorTaskModuleTestCase.problem_location(problem_url_name)
+            )
+            descriptor.data = problem_xml
+            self.module_store.update_item(descriptor, '**replace_user**')
         else:
             # Use "per-student" rerandomization so that check-problem can be called more than once.
             # Using "always" means we cannot check a problem twice, but we want to call once to get the

lms/envs/acceptance.py

@@ -44,6 +44,7 @@ MODULESTORE = {
         'ENGINE': 'xmodule.modulestore.mixed.MixedModuleStore',
         'OPTIONS': {
             'mappings': {},
+            'reference_type': 'Location',
             'stores': {
                 'default': {
                     'ENGINE': 'xmodule.modulestore.mongo.MongoModuleStore',

lms/envs/bok_choy.auth.json

@@ -51,6 +51,7 @@
             "ENGINE": "xmodule.modulestore.mixed.MixedModuleStore",
             "OPTIONS": {
                 "mappings": {},
+                "reference_type": "Location",
                 "stores": {
                     "default": {
                         "DOC_STORE_CONFIG": {

lms/envs/cms/mixed_dev.py

@@ -12,6 +12,7 @@ MODULESTORE = {
     'default': {
         'ENGINE': 'xmodule.modulestore.mixed.MixedModuleStore',
         'OPTIONS': {
+            'reference_type': 'Location',
             'mappings': {
                 'MITx/2.01x/2013_Spring': 'xml'
             },