From f45abe3d9fb738274ded35a25cae9eadb0c3a8fd Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Wed, 23 Oct 2013 08:50:45 -0400 Subject: [PATCH] Use new restful pattern instead --- .../contentstore/tests/test_orphan.py | 30 ++++++++----------- cms/djangoapps/contentstore/views/item.py | 20 +++++++------ cms/urls.py | 2 +- .../xmodule/xmodule/modulestore/mongo/base.py | 22 +++----------- .../xmodule/modulestore/tests/test_orphan.py | 24 ++++++++++----- 5 files changed, 46 insertions(+), 52 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_orphan.py b/cms/djangoapps/contentstore/tests/test_orphan.py index c51c808f00..e7babd25fd 100644 --- a/cms/djangoapps/contentstore/tests/test_orphan.py +++ b/cms/djangoapps/contentstore/tests/test_orphan.py @@ -3,7 +3,7 @@ Test finding orphans via the view and django config """ import json from contentstore.tests.utils import CourseTestCase -from xmodule.modulestore.django import editable_modulestore +from xmodule.modulestore.django import editable_modulestore, loc_mapper from django.core.urlresolvers import reverse from student.models import CourseEnrollment @@ -41,12 +41,12 @@ class TestOrphan(CourseTestCase): """ Test that old mongo finds the orphans """ + locator = loc_mapper().translate_location(self.course.location.course_id, self.course.location, False, True) + orphan_url = locator.url_reverse('orphan/', '') + orphans = json.loads( self.client.get( - reverse( - 'orphan', - kwargs={'course_id': '{}.{}'.format(self.course.location.org, self.course.location.course)} - ), + orphan_url, HTTP_ACCEPT='application/json' ).content ) @@ -62,13 +62,11 @@ class TestOrphan(CourseTestCase): """ Test that old mongo deletes the orphans """ - url = reverse( - 'orphan', - kwargs={'course_id': '{}.{}'.format(self.course.location.org, self.course.location.course)} - ) - self.client.delete(url) + locator = loc_mapper().translate_location(self.course.location.course_id, self.course.location, False, True) + orphan_url = locator.url_reverse('orphan/', '') + self.client.delete(orphan_url) orphans = json.loads( - self.client.get(url, HTTP_ACCEPT='application/json').content + self.client.get(orphan_url, HTTP_ACCEPT='application/json').content ) self.assertEqual(len(orphans), 0, "Orphans not deleted {}".format(orphans)) @@ -78,11 +76,9 @@ class TestOrphan(CourseTestCase): """ test_user_client, test_user = self.createNonStaffAuthedUserClient() CourseEnrollment.enroll(test_user, self.course.location.course_id) - url = reverse( - 'orphan', - kwargs={'course_id': '{}.{}'.format(self.course.location.org, self.course.location.course)} - ) - response = test_user_client.get(url) + locator = loc_mapper().translate_location(self.course.location.course_id, self.course.location, False, True) + orphan_url = locator.url_reverse('orphan/', '') + response = test_user_client.get(orphan_url) self.assertEqual(response.status_code, 403) - response = test_user_client.delete(url) + response = test_user_client.delete(orphan_url) self.assertEqual(response.status_code, 403) diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 16fbb32a6b..cc71c677a8 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -21,7 +21,7 @@ from .access import has_access from .helpers import _xmodule_recurse from xmodule.x_module import XModuleDescriptor from django.views.decorators.http import require_http_methods -from xmodule.modulestore.locator import CourseLocator +from xmodule.modulestore.locator import CourseLocator, BlockUsageLocator from student.models import CourseEnrollment __all__ = ['save_item', 'create_item', 'delete_item', 'orphan'] @@ -204,13 +204,13 @@ def delete_item(request): return JsonResponse() - +# pylint: disable=W0613 @login_required @require_http_methods(("GET", "DELETE")) -def orphan(request, course_id): +def orphan(request, tag=None, course_id=None, branch=None, version_guid=None, block=None): """ - View for handling orphan related requests. A get gets all of the current orphans. - DELETE, PUT and POST are meaningless for now. + View for handling orphan related requests. GET gets all of the current orphans. + DELETE removes all orphans (requires is_staff access) An orphan is a block whose category is not in the DETACHED_CATEGORY list, is not the root, and is not reachable from the root via children @@ -218,15 +218,17 @@ def orphan(request, course_id): :param request: :param course_id: Locator syntax course_id """ - course_loc = CourseLocator(course_id=course_id) + location = BlockUsageLocator(course_id=course_id, branch=branch, version_guid=version_guid, usage_id=block) + # DHM: when split becomes back-end, move or conditionalize this conversion + old_location = loc_mapper().translate_locator_to_location(location) if request.method == 'GET': - if has_access(request.user, course_loc): - return JsonResponse(modulestore().get_orphans(course_id, DETACHED_CATEGORIES, 'draft')) + if has_access(request.user, old_location): + return JsonResponse(modulestore().get_orphans(old_location, DETACHED_CATEGORIES, 'draft')) else: raise PermissionDenied() if request.method == 'DELETE': if request.user.is_staff: - items = modulestore().get_orphans(course_id, DETACHED_CATEGORIES, 'draft') + items = modulestore().get_orphans(old_location, DETACHED_CATEGORIES, 'draft') for item in items: modulestore('draft').delete_item(item, True) return JsonResponse({'deleted': items}) diff --git a/cms/urls.py b/cms/urls.py index c22b2c0cf5..a9228eab3b 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -131,7 +131,6 @@ urlpatterns += patterns( url(r'^logout$', 'student.views.logout_user', name='logout'), - url(r'^(?P[^/]+)/orphan', 'contentstore.views.orphan', name='orphan') ) # restful api @@ -142,6 +141,7 @@ urlpatterns += patterns( # (?ix) == ignore case and verbose (multiline regex) url(r'(?ix)^course/{}$'.format(parsers.URL_RE_SOURCE), 'course_handler'), url(r'(?ix)^checklists/{}(/)?(?P\d+)?$'.format(parsers.URL_RE_SOURCE), 'checklists_handler'), + url(r'(?ix)^orphan/{}$'.format(parsers.URL_RE_SOURCE), 'orphan') ) js_info_dict = { diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index b4488e6497..086cbb6bb7 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -855,27 +855,13 @@ class MongoModuleStore(ModuleStoreBase): """ return MONGO_MODULESTORE_TYPE - COURSE_ID_RE = re.compile(r'(?P[^.]+)\.(?P.+)') - def parse_course_id(self, course_id): + def get_orphans(self, course_location, detached_categories, _branch): """ - Parse a Locator style course_id into a dict w/ the org and course_id - :param course_id: a string looking like 'org.course.id.part' + Return an array all of the locations for orphans in the course. """ - match = self.COURSE_ID_RE.match(course_id) - if match is None: - raise ValueError(course_id) - return match.groupdict() - - def get_orphans(self, course_id, detached_categories, _branch): - """ - Return a dict of all of the orphans in the course. - - :param course_id: - """ - locator_dict = self.parse_course_id(course_id) all_items = self.collection.find({ - '_id.org': locator_dict['org'], - '_id.course': locator_dict['course_id'], + '_id.org': course_location.org, + '_id.course': course_location.course, '_id.category': {'$nin': detached_categories} }) all_reachable = set() diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_orphan.py b/common/lib/xmodule/xmodule/modulestore/tests/test_orphan.py index 6ebc81975e..073531094f 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_orphan.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_orphan.py @@ -23,12 +23,12 @@ class TestOrphan(unittest.TestCase): 'db': 'test_xmodule', } - modulestore_options = dict({ + modulestore_options = { 'default_class': 'xmodule.raw_module.RawDescriptor', 'fs_root': '', 'render_template': mock.Mock(return_value=""), 'xblock_mixins': (InheritanceMixin,) - }) + } split_course_id = 'test_org.test_course.runid' @@ -41,25 +41,35 @@ class TestOrphan(unittest.TestCase): self.db_config, **self.modulestore_options ) - self.addCleanup(self.tearDownSplit) + self.addCleanup(self.tear_down_split) self.old_mongo = MongoModuleStore(self.db_config, **self.modulestore_options) - self.addCleanup(self.tearDownMongo) + self.addCleanup(self.tear_down_mongo) self.course_location = None self._create_course() - def tearDownSplit(self): + def tear_down_split(self): + """ + Remove the test collections, close the db connection + """ split_db = self.split_mongo.db split_db.drop_collection(split_db.course_index) split_db.drop_collection(split_db.structures) split_db.drop_collection(split_db.definitions) split_db.connection.close() - def tearDownMongo(self): + def tear_down_mongo(self): + """ + Remove the test collections, close the db connection + """ split_db = self.split_mongo.db # old_mongo doesn't give a db attr, but all of the dbs are the same split_db.drop_collection(self.old_mongo.collection) def _create_item(self, category, name, data, metadata, parent_category, parent_name, runtime): + """ + Create the item of the given category and block id in split and old mongo, add it to the optional + parent. The parent category is only needed because old mongo requires it for the id. + """ location = Location('i4x', 'test_org', 'test_course', category, name) self.old_mongo.create_and_save_xmodule(location, data, metadata, runtime) if isinstance(data, basestring): @@ -125,7 +135,7 @@ class TestOrphan(unittest.TestCase): """ Test that old mongo finds the orphans """ - orphans = self.old_mongo.get_orphans('test_org.test_course', ['static_tab', 'about', 'course_info'], None) + orphans = self.old_mongo.get_orphans(self.course_location, ['static_tab', 'about', 'course_info'], None) self.assertEqual(len(orphans), 3, "Wrong # {}".format(orphans)) location = self.course_location.replace(category='chapter', name='OrphanChapter') self.assertIn(location.url(), orphans)