From f6417b4f71ee71926395518df6ed5117ce2d98dd Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Thu, 25 Apr 2013 15:24:31 -0400 Subject: [PATCH 1/5] add a delete-all-versions flag to the delete_item method in the modulestores. Extend tests to verify. Still wip --- .../contentstore/tests/test_contentstore.py | 34 +++++++++++++++---- cms/djangoapps/contentstore/views.py | 11 ++---- .../lib/xmodule/xmodule/modulestore/draft.py | 10 ++++-- .../lib/xmodule/xmodule/modulestore/mongo.py | 4 ++- 4 files changed, 41 insertions(+), 18 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index ed95d81d67..8a85eb75d9 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -264,15 +264,27 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): def test_delete(self): import_from_xml(modulestore(), 'common/test/data/', ['full']) - module_store = modulestore('direct') + direct_store = modulestore('direct') + draft_store = modulestore('draft') - sequential = module_store.get_item(Location(['i4x', 'edX', 'full', 'sequential', 'Administrivia_and_Circuit_Elements', None])) + sequential = direct_store.get_item(Location(['i4x', 'edX', 'full', 'sequential', 'Administrivia_and_Circuit_Elements', None])) - chapter = module_store.get_item(Location(['i4x', 'edX', 'full', 'chapter', 'Week_1', None])) + chapter = direct_store.get_item(Location(['i4x', 'edX', 'full', 'chapter', 'Week_1', None])) - # make sure the parent no longer points to the child object which was deleted + # make sure the parent points to the child object which is to be deleted self.assertTrue(sequential.location.url() in chapter.children) + # get a vertical (and components in it) to put into 'draft' + vertical = direct_store.get_item(Location(['i4x', 'edX', 'full', + 'vertical', 'vertical_66', None])) + + private_item_location = vertical.location._replace(name='private_vertical') + + draft_store.clone_item(vertical.location, private_item_location) + + direct_store.update_children(sequential.location, sequential.children + + [private_item_location.url()]) + self.client.post( reverse('delete_item'), json.dumps({'id': sequential.location.url(), 'delete_children': 'true', 'delete_all_versions': 'true'}), @@ -281,18 +293,28 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): found = False try: - module_store.get_item(Location(['i4x', 'edX', 'full', 'sequential', 'Administrivia_and_Circuit_Elements', None])) + draft_store.get_item(Location(['i4x', 'edX', 'full', 'sequential', 'Administrivia_and_Circuit_Elements', None])) found = True except ItemNotFoundError: pass self.assertFalse(found) - chapter = module_store.get_item(Location(['i4x', 'edX', 'full', 'chapter', 'Week_1', None])) + chapter = draft_store.get_item(Location(['i4x', 'edX', 'full', 'chapter', 'Week_1', None])) # make sure the parent no longer points to the child object which was deleted self.assertFalse(sequential.location.url() in chapter.children) + # make sure we can't look up the private vertical + found_private = False + try: + draft_store.get_item(private_item_location) + found_private = True + except ItemNotFoundError: + pass + + self.assertFalse(found_private) + def test_about_overrides(self): ''' This test case verifies that a course can use specialized override for about data, e.g. /about/Fall_2012/effort.html diff --git a/cms/djangoapps/contentstore/views.py b/cms/djangoapps/contentstore/views.py index 603010f5b4..3a1ac1ebd1 100644 --- a/cms/djangoapps/contentstore/views.py +++ b/cms/djangoapps/contentstore/views.py @@ -624,16 +624,9 @@ def delete_item(request): # if caller wants to only delete the draft than the caller should put item.location.revision='draft' if delete_children: - _xmodule_recurse(item, lambda i: store.delete_item(i.location)) + _xmodule_recurse(item, lambda i: store.delete_item(i.location, delete_all_versions)) else: - store.delete_item(item.location) - - # cdodge: this is a bit of a hack until I can talk with Cale about the - # semantics of delete_item whereby the store is draft aware. Right now calling - # delete_item on a vertical tries to delete the draft version leaving the - # requested delete to never occur - if item.location.revision is None and item.location.category == 'vertical' and delete_all_versions: - modulestore('direct').delete_item(item.location) + store.delete_item(item.location, 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: diff --git a/common/lib/xmodule/xmodule/modulestore/draft.py b/common/lib/xmodule/xmodule/modulestore/draft.py index 43eb050129..a1372b25f0 100644 --- a/common/lib/xmodule/xmodule/modulestore/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/draft.py @@ -4,6 +4,8 @@ from . import ModuleStoreBase, Location, namedtuple_to_son from .exceptions import ItemNotFoundError from .inheritance import own_metadata +import logging + DRAFT = 'draft' @@ -159,13 +161,17 @@ class DraftModuleStore(ModuleStoreBase): return super(DraftModuleStore, self).update_metadata(draft_loc, metadata) - def delete_item(self, location): + def delete_item(self, location, delete_all_versions=False): """ Delete an item from this modulestore location: Something that can be passed to Location """ - return super(DraftModuleStore, self).delete_item(as_draft(location)) + super(DraftModuleStore, self).delete_item(as_draft(location)) + if delete_all_versions: + super(DraftModuleStore, self).delete_item(location) + + return def get_parent_locations(self, location, course_id): '''Find all locations that are the parents of this location. Needed diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index 28ea1f2659..ca0750609f 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -694,12 +694,14 @@ class MongoModuleStore(ModuleStoreBase): 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): + def delete_item(self, location, delete_all_versions=False): """ 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. """ + logging.debug('delete_all_versions = {0}'.format(delete_all_versions)) # 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 From ead18f9fbb1fe11825164365c2869354719c230e Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Fri, 26 Apr 2013 10:30:39 -0400 Subject: [PATCH 2/5] undo the unit test, as per Jeh, the tests really aren't supposed to run the CMS with 'draft awareness' --- .../contentstore/tests/test_contentstore.py | 20 ------------------- 1 file changed, 20 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 8a85eb75d9..f4926acc29 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -274,17 +274,6 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): # make sure the parent points to the child object which is to be deleted self.assertTrue(sequential.location.url() in chapter.children) - # get a vertical (and components in it) to put into 'draft' - vertical = direct_store.get_item(Location(['i4x', 'edX', 'full', - 'vertical', 'vertical_66', None])) - - private_item_location = vertical.location._replace(name='private_vertical') - - draft_store.clone_item(vertical.location, private_item_location) - - direct_store.update_children(sequential.location, sequential.children + - [private_item_location.url()]) - self.client.post( reverse('delete_item'), json.dumps({'id': sequential.location.url(), 'delete_children': 'true', 'delete_all_versions': 'true'}), @@ -305,15 +294,6 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): # make sure the parent no longer points to the child object which was deleted self.assertFalse(sequential.location.url() in chapter.children) - # make sure we can't look up the private vertical - found_private = False - try: - draft_store.get_item(private_item_location) - found_private = True - except ItemNotFoundError: - pass - - self.assertFalse(found_private) def test_about_overrides(self): ''' From 2e64bc33ffc9e2e6d0e993588f7fb4d34475a5e9 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Fri, 26 Apr 2013 12:37:37 -0400 Subject: [PATCH 3/5] make sure delete_item always uses the draft store --- cms/djangoapps/contentstore/views.py | 6 ++++-- common/lib/xmodule/xmodule/modulestore/draft.py | 8 +++++++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/views.py b/cms/djangoapps/contentstore/views.py index 3a1ac1ebd1..a22d78900f 100644 --- a/cms/djangoapps/contentstore/views.py +++ b/cms/djangoapps/contentstore/views.py @@ -615,9 +615,11 @@ def delete_item(request): delete_children = request.POST.get('delete_children', False) delete_all_versions = request.POST.get('delete_all_versions', False) - item = modulestore().get_item(item_location) + logging.debug('delete_all_versions = {0}'.format(delete_all_versions)) - store = get_modulestore(item_loc) + store = modulestore() + + item = store.get_item(item_location) # @TODO: this probably leaves draft items dangling. My preferance would be for the semantic to be # if item.location.revision=None, then delete both draft and published version diff --git a/common/lib/xmodule/xmodule/modulestore/draft.py b/common/lib/xmodule/xmodule/modulestore/draft.py index a1372b25f0..d7300e1730 100644 --- a/common/lib/xmodule/xmodule/modulestore/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/draft.py @@ -15,6 +15,12 @@ def as_draft(location): """ return Location(location)._replace(revision=DRAFT) +def as_published(location): + """ + Returns the Location that is the published version for `location` + """ + return Location(location)._replace(revision=None) + def wrap_draft(item): """ @@ -169,7 +175,7 @@ class DraftModuleStore(ModuleStoreBase): """ super(DraftModuleStore, self).delete_item(as_draft(location)) if delete_all_versions: - super(DraftModuleStore, self).delete_item(location) + super(DraftModuleStore, self).delete_item(as_published(location)) return From 7d206e020d9c559d28eb11d25479ff4441a669b0 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Fri, 26 Apr 2013 16:13:26 -0400 Subject: [PATCH 4/5] remove comment which is no longer valid as well as debug log messages --- cms/djangoapps/contentstore/views.py | 6 ------ common/lib/xmodule/xmodule/modulestore/mongo.py | 1 - 2 files changed, 7 deletions(-) diff --git a/cms/djangoapps/contentstore/views.py b/cms/djangoapps/contentstore/views.py index a22d78900f..153e37ed82 100644 --- a/cms/djangoapps/contentstore/views.py +++ b/cms/djangoapps/contentstore/views.py @@ -615,16 +615,10 @@ def delete_item(request): delete_children = request.POST.get('delete_children', False) delete_all_versions = request.POST.get('delete_all_versions', False) - logging.debug('delete_all_versions = {0}'.format(delete_all_versions)) - store = modulestore() item = store.get_item(item_location) - # @TODO: this probably leaves draft items dangling. My preferance would be for the semantic to be - # if item.location.revision=None, then delete both draft and published version - # if caller wants to only delete the draft than the caller should put item.location.revision='draft' - if delete_children: _xmodule_recurse(item, lambda i: store.delete_item(i.location, delete_all_versions)) else: diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index ca0750609f..c8256422f8 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -701,7 +701,6 @@ class MongoModuleStore(ModuleStoreBase): 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. """ - logging.debug('delete_all_versions = {0}'.format(delete_all_versions)) # 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 From d7693d96b7720d2f4913ce63625b7c67e9533581 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Mon, 29 Apr 2013 09:18:49 -0400 Subject: [PATCH 5/5] unit test should just use direct store. Also remove unused import and logging. --- cms/djangoapps/contentstore/tests/test_contentstore.py | 5 ++--- common/lib/xmodule/xmodule/modulestore/draft.py | 2 -- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index f4926acc29..07b7032e60 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -265,7 +265,6 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): import_from_xml(modulestore(), 'common/test/data/', ['full']) direct_store = modulestore('direct') - draft_store = modulestore('draft') sequential = direct_store.get_item(Location(['i4x', 'edX', 'full', 'sequential', 'Administrivia_and_Circuit_Elements', None])) @@ -282,14 +281,14 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): found = False try: - draft_store.get_item(Location(['i4x', 'edX', 'full', 'sequential', 'Administrivia_and_Circuit_Elements', None])) + direct_store.get_item(Location(['i4x', 'edX', 'full', 'sequential', 'Administrivia_and_Circuit_Elements', None])) found = True except ItemNotFoundError: pass self.assertFalse(found) - chapter = draft_store.get_item(Location(['i4x', 'edX', 'full', 'chapter', 'Week_1', None])) + chapter = direct_store.get_item(Location(['i4x', 'edX', 'full', 'chapter', 'Week_1', None])) # make sure the parent no longer points to the child object which was deleted self.assertFalse(sequential.location.url() in chapter.children) diff --git a/common/lib/xmodule/xmodule/modulestore/draft.py b/common/lib/xmodule/xmodule/modulestore/draft.py index d7300e1730..c3f1b23688 100644 --- a/common/lib/xmodule/xmodule/modulestore/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/draft.py @@ -4,8 +4,6 @@ from . import ModuleStoreBase, Location, namedtuple_to_son from .exceptions import ItemNotFoundError from .inheritance import own_metadata -import logging - DRAFT = 'draft'