From aab5e0441b9bef5ded7e038884e057fc8e9c2c4d Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Fri, 28 Feb 2014 15:32:01 -0500 Subject: [PATCH] Mixed modulestore handles most modulestore functions, cont. --- .../lib/xmodule/xmodule/modulestore/mixed.py | 8 ++-- .../xmodule/xmodule/modulestore/mongo/base.py | 2 +- .../tests/test_mixed_modulestore.py | 42 +++++++------------ 3 files changed, 21 insertions(+), 31 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index 92a577a686..03fd14b489 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/common/lib/xmodule/xmodule/modulestore/mixed.py @@ -125,7 +125,7 @@ class MixedModuleStore(ModuleStoreWriteBase): # order the modulestores and ensure no dupes (default may be a dupe of a named store) # remove 'draft' as we know it's a functional dupe of 'direct' (ugly hardcoding) stores = set([value for key, value in self.modulestores.iteritems() if key != 'draft']) - stores = sorted(stores, _compare_stores) + stores = sorted(stores, cmp=_compare_stores) courses = {} # a dictionary of stringified course locations to course objects has_locators = any(issubclass(CourseLocator, store.reference_type) for store in stores) @@ -214,9 +214,9 @@ class MixedModuleStore(ModuleStoreWriteBase): pass try: course = store._get_course_for_item(block.scope_ids.usage_id) - if course: + if course is not None: return course.scope_ids.usage_id.course_id - except: # sorry, that method just raises vanilla Exception if it doesn't find course + except Exception: # sorry, that method just raises vanilla Exception if it doesn't find course pass def _infer_course_id_try(self, location): @@ -436,7 +436,7 @@ class MixedModuleStore(ModuleStoreWriteBase): """ store = self.modulestores[store_name] if store.reference_type != Location: - raise NotImplementedError(u"Cannot create maps from %s" % store.reference_type) + raise ValueError(u"Cannot create maps from %s" % store.reference_type) for course in store.get_courses(): loc_mapper().translate_location(course.location.course_id, course.location) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index b97b1e1686..3fa8c4531e 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -681,7 +681,7 @@ class MongoModuleStore(ModuleStoreWriteBase): dbmodel, ) for key, value in fields.iteritems(): - xmodule[key] = value + setattr(xmodule, key, value) # decache any pending field settings from init xmodule.save() return xmodule diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index 39073e4952..a82ffbb050 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -33,7 +33,7 @@ class TestMixedModuleStore(LocMapperSetupSansDjango): DEFAULT_CLASS = 'xmodule.raw_module.RawDescriptor' RENDER_TEMPLATE = lambda t_n, d, ctx = None, nsp = 'main': '' - IMPORT_COURSEID = 'MITx/999/2013_Spring' + MONGO_COURSEID = 'MITx/999/2013_Spring' XML_COURSEID1 = 'edX/toy/2012_Fall' XML_COURSEID2 = 'edX/simple/2012_Fall' @@ -51,7 +51,7 @@ class TestMixedModuleStore(LocMapperSetupSansDjango): 'mappings': { XML_COURSEID1: 'xml', XML_COURSEID2: 'xml', - IMPORT_COURSEID: 'default' + MONGO_COURSEID: 'default' }, 'stores': { 'xml': { @@ -124,17 +124,7 @@ class TestMixedModuleStore(LocMapperSetupSansDjango): course.location, item_location.category, location=item_location, block_id=item_location.name ) if isinstance(course.location, CourseLocator): - # add_entry is false b/c this is a test that the right thing happened w/o - # wanting any additional side effects - lookup_map = loc_mapper().translate_location( - course_location.course_id, course_location, add_entry_if_missing=False - ) - self.assertEqual(lookup_map, course.location) - self.course_locations[self.IMPORT_COURSEID] = course.location.version_agnostic() - lookup_map = loc_mapper().translate_location( - course_location.course_id, item_location, add_entry_if_missing=False - ) - self.assertEqual(lookup_map, chapter.location) + self.course_locations[self.MONGO_COURSEID] = course.location.version_agnostic() self.import_chapter_location = chapter.location.version_agnostic() else: self.assertEqual(course.location, course_location) @@ -160,17 +150,17 @@ class TestMixedModuleStore(LocMapperSetupSansDjango): self.course_locations = { course_id: generate_location(course_id) - for course_id in [self.IMPORT_COURSEID, self.XML_COURSEID1, self.XML_COURSEID2] + for course_id in [self.MONGO_COURSEID, self.XML_COURSEID1, self.XML_COURSEID2] } self.fake_location = Location('i4x', 'foo', 'bar', 'vertical', 'baz') - self.import_chapter_location = self.course_locations[self.IMPORT_COURSEID].replace( + self.import_chapter_location = self.course_locations[self.MONGO_COURSEID].replace( category='chapter', name='Overview' ) self.xml_chapter_location = self.course_locations[self.XML_COURSEID1].replace( category='chapter', name='Overview' ) # grab old style location b4 possibly converted - import_location = self.course_locations[self.IMPORT_COURSEID] + import_location = self.course_locations[self.MONGO_COURSEID] # get Locators and set up the loc mapper if app is Locator based if default == 'split': self.fake_location = loc_mapper().translate_location('foo/bar/2012_Fall', self.fake_location) @@ -186,7 +176,7 @@ class TestMixedModuleStore(LocMapperSetupSansDjango): self.assertEqual(self.store.get_modulestore_type(self.XML_COURSEID1), XML_MODULESTORE_TYPE) self.assertEqual(self.store.get_modulestore_type(self.XML_COURSEID2), XML_MODULESTORE_TYPE) mongo_ms_type = MONGO_MODULESTORE_TYPE if default_ms == 'direct' else SPLIT_MONGO_MODULESTORE_TYPE - self.assertEqual(self.store.get_modulestore_type(self.IMPORT_COURSEID), mongo_ms_type) + self.assertEqual(self.store.get_modulestore_type(self.MONGO_COURSEID), mongo_ms_type) # try an unknown mapping, it should be the 'default' store self.assertEqual(self.store.get_modulestore_type('foo/bar/2012_Fall'), mongo_ms_type) @@ -201,7 +191,7 @@ class TestMixedModuleStore(LocMapperSetupSansDjango): self.XML_COURSEID1, self.course_locations[self.XML_COURSEID1].replace(name='not_findable', category='problem') )) - self.assertFalse(self.store.has_item(self.IMPORT_COURSEID, self.fake_location)) + self.assertFalse(self.store.has_item(self.MONGO_COURSEID, self.fake_location)) @ddt.data('direct', 'split') def test_get_item(self, default_ms): @@ -222,7 +212,7 @@ class TestMixedModuleStore(LocMapperSetupSansDjango): self.course_locations[self.XML_COURSEID1].replace(name='not_findable', category='problem') ) with self.assertRaises(ItemNotFoundError): - self.store.get_instance(self.IMPORT_COURSEID, self.fake_location) + self.store.get_instance(self.MONGO_COURSEID, self.fake_location) @ddt.data('direct', 'split') def test_get_items(self, default_ms): @@ -252,10 +242,10 @@ class TestMixedModuleStore(LocMapperSetupSansDjango): self.store.update_item(course, None) # now do it for a r/w db # get_course api's are inconsistent: one takes Locators the other an old style course id - if hasattr(self.course_locations[self.IMPORT_COURSEID], 'as_course_locator'): - locn = self.course_locations[self.IMPORT_COURSEID] + if hasattr(self.course_locations[self.MONGO_COURSEID], 'as_course_locator'): + locn = self.course_locations[self.MONGO_COURSEID] else: - locn = self.IMPORT_COURSEID + locn = self.MONGO_COURSEID course = self.store.get_course(locn) # if following raised, then the test is really a noop, change it self.assertFalse(course.show_calculator, "Default changed making test meaningless") @@ -276,7 +266,7 @@ class TestMixedModuleStore(LocMapperSetupSansDjango): self.store.delete_item(self.import_chapter_location, '**replace_user**') # verify it's gone with self.assertRaises(ItemNotFoundError): - self.store.get_instance(self.IMPORT_COURSEID, self.import_chapter_location) + self.store.get_instance(self.MONGO_COURSEID, self.import_chapter_location) @ddt.data('direct', 'split') def test_get_courses(self, default_ms): @@ -289,7 +279,7 @@ class TestMixedModuleStore(LocMapperSetupSansDjango): if hasattr(course.location, 'version_agnostic') else course.location for course in courses ] - self.assertIn(self.course_locations[self.IMPORT_COURSEID], course_ids) + self.assertIn(self.course_locations[self.MONGO_COURSEID], course_ids) self.assertIn(self.course_locations[self.XML_COURSEID1], course_ids) self.assertIn(self.course_locations[self.XML_COURSEID2], course_ids) @@ -325,10 +315,10 @@ class TestMixedModuleStore(LocMapperSetupSansDjango): self.initdb(default_ms) parents = self.store.get_parent_locations( self.import_chapter_location, - self.IMPORT_COURSEID + self.MONGO_COURSEID ) self.assertEqual(len(parents), 1) - self.assertEqual(parents[0], self.course_locations[self.IMPORT_COURSEID]) + self.assertEqual(parents[0], self.course_locations[self.MONGO_COURSEID]) parents = self.store.get_parent_locations( self.xml_chapter_location,