From 8fd8574357502cf111ae11e68282192ccdc2ba48 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Thu, 11 Apr 2013 22:10:30 -0400 Subject: [PATCH 1/3] get_items has an query ambiguity with respect to revision=None. None has a special semantic in get_items as a wildcard, so if we query with 'None' then we'll get back both draft and non-draft items. --- common/lib/xmodule/xmodule/modulestore/mongo.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index 653a7ca22a..9f55c6a1be 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -203,7 +203,9 @@ def location_to_query(location, wildcard=True): if wildcard: for key, value in query.items(): - if value is None: + # don't allow wildcards on revision, since public is set as None, so + # its ambiguous between None as a real value versus None=wildcard + if value is None and key != 'revision': del query[key] return query From 1a3c622bb66b9e7a4c435fd2e26b650b1b633c9a Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Thu, 11 Apr 2013 22:32:35 -0400 Subject: [PATCH 2/3] actually the query parameter is '_id.revision' not just 'revision' --- common/lib/xmodule/xmodule/modulestore/mongo.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index 9f55c6a1be..dd51be3bf6 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -205,7 +205,7 @@ def location_to_query(location, wildcard=True): for key, value in query.items(): # don't allow wildcards on revision, since public is set as None, so # its ambiguous between None as a real value versus None=wildcard - if value is None and key != 'revision': + if value is None and key != '_id.revision': del query[key] return query From 4345c8e09c00ffbe41ff11a16a10294859987b17 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Fri, 12 Apr 2013 09:48:11 -0400 Subject: [PATCH 3/3] add unit test to assert that get_items() is not using revision=None as a wildcard --- .../contentstore/tests/test_contentstore.py | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index faec60f3e8..1d3fca1bcc 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -94,6 +94,33 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): return cnt + def test_get_items(self): + ''' + This verifies a bug we had where the None setting in get_items() meant 'wildcard' + Unfortunately, None = published for the revision field, so get_items() would return + both draft and non-draft copies. + ''' + store = modulestore() + draft_store = modulestore('draft') + import_from_xml(store, 'common/test/data/', ['simple']) + + html_module = draft_store.get_item(['i4x', 'edX', 'simple', 'html', 'test_html', None]) + + draft_store.clone_item(html_module.location, html_module.location) + + # now query get_items() to get this location with revision=None, this should just + # return back a single item (not 2) + + items = store.get_items(['i4x', 'edX', 'simple', 'html', 'test_html', None]) + self.assertEqual(len(items), 1) + self.assertFalse(getattr(items[0], 'is_draft', False)) + + # now refetch from the draft store. Note that even though we pass + # None in the revision field, the draft store will replace that with 'draft' + items = draft_store.get_items(['i4x', 'edX', 'simple', 'html', 'test_html', None]) + self.assertEqual(len(items), 1) + self.assertTrue(getattr(items[0], 'is_draft', False)) + def test_draft_metadata(self): ''' This verifies a bug we had where inherited metadata was getting written to the