From 6b9797522b80f3f975234f5fcaaaea358b4d4e2d Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Tue, 1 Oct 2013 16:03:26 -0400 Subject: [PATCH 1/5] Write restful service to find all orphans To help fix recent bugs re orphaned discussions and to prototype a more restful json oriented api. --- CHANGELOG.rst | 8 + .../contentstore/tests/test_orphan.py | 58 +++++++ cms/djangoapps/contentstore/views/item.py | 19 ++- cms/urls.py | 2 + .../xmodule/xmodule/modulestore/mongo/base.py | 35 ++++- .../xmodule/modulestore/split_mongo/split.py | 18 +++ .../xmodule/modulestore/tests/test_orphan.py | 147 ++++++++++++++++++ 7 files changed, 285 insertions(+), 2 deletions(-) create mode 100644 cms/djangoapps/contentstore/tests/test_orphan.py create mode 100644 common/lib/xmodule/xmodule/modulestore/tests/test_orphan.py diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 07ce908ba9..43968d6716 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -9,6 +9,14 @@ Blades: When start time and end time are specified for a video, a visual range will be shown on the time slider to highlight the place in the video that will be played. +Studio: added restful interface for finding orphans in courses. +An orphan is an xblock to which no children relation points and whose type is not +in the set contentstore.views.item.DETACHED_CATEGORIES nor 'course'. + GET http://host/orphan/org.course returns json array of ids. + Requires course author access. + DELETE http://orphan/org.course deletes all the orphans in that course. + Requires is_staff access + Studio: Bug fix for text loss in Course Updates when the text exists before the first tag. diff --git a/cms/djangoapps/contentstore/tests/test_orphan.py b/cms/djangoapps/contentstore/tests/test_orphan.py new file mode 100644 index 0000000000..c6f16fd47b --- /dev/null +++ b/cms/djangoapps/contentstore/tests/test_orphan.py @@ -0,0 +1,58 @@ +""" +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 django.core.urlresolvers import reverse + +class TestOrphan(CourseTestCase): + """ + Test finding orphans via view and django config + """ + def setUp(self): + super(TestOrphan, self).setUp() + + runtime = self.course.runtime + + self._create_item('chapter', 'Chapter1', {}, {'display_name': 'Chapter 1'}, 'course', self.course.location.name, runtime) + self._create_item('chapter', 'Chapter2', {}, {'display_name': 'Chapter 2'}, 'course', self.course.location.name, runtime) + self._create_item('chapter', 'OrphanChapter', {}, {'display_name': 'Orphan Chapter'}, None, None, runtime) + self._create_item('vertical', 'Vert1', {}, {'display_name': 'Vertical 1'}, 'chapter', 'Chapter1', runtime) + self._create_item('vertical', 'OrphanVert', {}, {'display_name': 'Orphan Vertical'}, None, None, runtime) + self._create_item('html', 'Html1', "

Goodbye

", {'display_name': 'Parented Html'}, 'vertical', 'Vert1', runtime) + self._create_item('html', 'OrphanHtml', "

Hello

", {'display_name': 'Orphan html'}, None, None, runtime) + self._create_item('static_tab', 'staticuno', "

tab

", {'display_name': 'Tab uno'}, None, None, runtime) + self._create_item('about', 'overview', "

overview

", {}, None, None, runtime) + self._create_item('course_info', 'updates', "

  1. Sep 22

    test

", {}, None, None, runtime) + + def _create_item(self, category, name, data, metadata, parent_category, parent_name, runtime): + location = self.course.location.replace(category=category, name=name) + editable_modulestore('direct').create_and_save_xmodule(location, data, metadata, runtime) + if parent_name: + # add child to parent in mongo + 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) + + def test_mongo_orphan(self): + """ + Test that old mongo finds the orphans + """ + orphans = json.loads( + self.client.get( + reverse( + 'orphan', + kwargs={'course_id': '{}.{}'.format(self.course.location.org, self.course.location.course)} + ), + HTTP_ACCEPT='application/json' + ).content + ) + self.assertEqual(len(orphans), 3, "Wrong # {}".format(orphans)) + location = self.course.location.replace(category='chapter', name='OrphanChapter') + self.assertIn(location.url(), orphans) + location = self.course.location.replace(category='vertical', name='OrphanVert') + self.assertIn(location.url(), orphans) + location = self.course.location.replace(category='html', name='OrphanHtml') + self.assertIn(location.url(), orphans) diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 1ebdc8fb4f..816b05c16b 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 -__all__ = ['save_item', 'create_item', 'delete_item'] +__all__ = ['save_item', 'create_item', 'delete_item', 'orphan'] log = logging.getLogger(__name__) @@ -200,3 +200,20 @@ def delete_item(request): modulestore('direct').update_children(parent.location, parent.children) return JsonResponse() + + +@login_required +def orphan(request, course_id): + """ + View for handling orphan related requests. A get gets all of the current orphans. + DELETE, PUT and POST are meaningless for now. + + 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 + + :param request: + :param course_id: Locator syntax course_id + """ + # dhm: I'd add DELETE but I'm not sure what type of authentication/authorization we'd need + if request.method == 'GET': + return JsonResponse(modulestore().get_orphans(course_id, DETACHED_CATEGORIES, 'draft')) diff --git a/cms/urls.py b/cms/urls.py index 9bbf1664f8..c22b2c0cf5 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -130,6 +130,8 @@ urlpatterns += patterns( url(r'^login_post$', 'student.views.login_user', name='login_post'), url(r'^logout$', 'student.views.logout_user', name='logout'), + + url(r'^(?P[^/]+)/orphan', 'contentstore.views.orphan', name='orphan') ) # restful api diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index 352ebce61a..b4488e6497 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -34,6 +34,7 @@ from xblock.fields import Scope, ScopeIds from xmodule.modulestore import ModuleStoreBase, Location, MONGO_MODULESTORE_TYPE from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.modulestore.inheritance import own_metadata, InheritanceMixin, inherit_metadata, InheritanceKeyValueStore +import re log = logging.getLogger(__name__) @@ -697,7 +698,7 @@ class MongoModuleStore(ModuleStoreBase): course.tabs = existing_tabs # Save any changes to the course to the MongoKeyValueStore course.save() - self.update_metadata(course.location, course.xblock_kvs._metadata) + self.update_metadata(course.location, course.get_explicitly_set_fields_by_scope(Scope.settings)) def fire_updated_modulestore_signal(self, course_id, location): """ @@ -854,6 +855,38 @@ class MongoModuleStore(ModuleStoreBase): """ return MONGO_MODULESTORE_TYPE + COURSE_ID_RE = re.compile(r'(?P[^.]+)\.(?P.+)') + def parse_course_id(self, course_id): + """ + 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' + """ + 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.category': {'$nin': detached_categories} + }) + all_reachable = set() + item_locs = set() + for item in all_items: + if item['_id']['category'] != 'course': + item_locs.add(Location(item['_id']).replace(revision=None).url()) + all_reachable = all_reachable.union(item.get('definition', {}).get('children', [])) + item_locs -= all_reachable + return list(item_locs) + 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 diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index f310586c66..68aa883441 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -421,6 +421,24 @@ class SplitMongoModuleStore(ModuleStoreBase): items.append(BlockUsageLocator(url=locator.as_course_locator(), usage_id=parent_id)) return items + def get_orphans(self, course_id, detached_categories, branch): + """ + Return a dict of all of the orphans in the course. + + :param course_id: + """ + course = self._lookup_course(CourseLocator(course_id=course_id, branch=branch)) + items = set(course['structure']['blocks'].keys()) + items.remove(course['structure']['root']) + for block_id, block_data in course['structure']['blocks'].iteritems(): + items.difference_update(block_data.get('fields', {}).get('children', [])) + if block_data['category'] in detached_categories: + items.discard(block_id) + return [ + BlockUsageLocator(course_id=course_id, branch=branch, usage_id=block_id) + for block_id in items + ] + def get_course_index_info(self, course_locator): """ The index records the initial creation of the indexed course and tracks the current version diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_orphan.py b/common/lib/xmodule/xmodule/modulestore/tests/test_orphan.py new file mode 100644 index 0000000000..f8f7e73218 --- /dev/null +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_orphan.py @@ -0,0 +1,147 @@ +import uuid +import mock +import unittest +import random +import datetime + +from xmodule.modulestore.inheritance import InheritanceMixin +from xmodule.modulestore.mongo import MongoModuleStore +from xmodule.modulestore.split_mongo import SplitMongoModuleStore +from xmodule.modulestore import Location +from xmodule.fields import Date +from xmodule.modulestore.locator import BlockUsageLocator, CourseLocator + + +class TestOrphan(unittest.TestCase): + """ + Test the orphan finding code + """ + + # Snippet of what would be in the django settings envs file + db_config = { + 'host': 'localhost', + 'db': 'test_xmodule', + } + + modulestore_options = dict({ + 'default_class': 'xmodule.raw_module.RawDescriptor', + 'fs_root': '', + 'render_template': mock.Mock(return_value=""), + 'xblock_mixins': (InheritanceMixin,) + }, **db_config) + + split_course_id = 'test_org.test_course.runid' + + def setUp(self): + self.modulestore_options['collection'] = 'modulestore{0}'.format(uuid.uuid4().hex) + + self.userid = random.getrandbits(32) + super(TestOrphan, self).setUp() + self.split_mongo = SplitMongoModuleStore( + **self.modulestore_options + ) + self.addCleanup(self.tearDownSplit) + self.old_mongo = MongoModuleStore(**self.modulestore_options) + self.addCleanup(self.tearDownMongo) + self.course_location = None + self._create_course() + + def tearDownSplit(self): + 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): + 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): + location = Location('i4x', 'test_org', 'test_course', category, name) + self.old_mongo.create_and_save_xmodule(location, data, metadata, runtime) + if isinstance(data, basestring): + fields = {'data': data} + else: + fields = data.copy() + fields.update(metadata) + if parent_name: + # add child to parent in mongo + 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) + # create pointer for split + course_or_parent_locator = BlockUsageLocator( + course_id=self.split_course_id, + branch='draft', + usage_id=parent_name + ) + else: + course_or_parent_locator = CourseLocator( + course_id='test_org.test_course.runid', + branch='draft', + ) + self.split_mongo.create_item(course_or_parent_locator, category, self.userid, usage_id=name, fields=fields) + + def _create_course(self): + """ + * some detached items + * some attached children + * some orphans + """ + date_proxy = Date() + metadata = { + 'start': date_proxy.to_json(datetime.datetime(2000, 3, 13, 4)), + 'display_name': 'Migration test course', + } + data = { + 'wiki_slug': 'test_course_slug' + } + fields = metadata.copy() + fields.update(data) + # split requires the course to be created separately from creating items + self.split_mongo.create_course( + 'test_org', 'my course', self.userid, self.split_course_id, fields=fields, root_usage_id='runid' + ) + self.course_location = Location('i4x', 'test_org', 'test_course', 'course', 'runid') + self.old_mongo.create_and_save_xmodule(self.course_location, data, metadata) + runtime = self.old_mongo.get_item(self.course_location).runtime + + self._create_item('chapter', 'Chapter1', {}, {'display_name': 'Chapter 1'}, 'course', 'runid', runtime) + self._create_item('chapter', 'Chapter2', {}, {'display_name': 'Chapter 2'}, 'course', 'runid', runtime) + self._create_item('chapter', 'OrphanChapter', {}, {'display_name': 'Orphan Chapter'}, None, None, runtime) + self._create_item('vertical', 'Vert1', {}, {'display_name': 'Vertical 1'}, 'chapter', 'Chapter1', runtime) + self._create_item('vertical', 'OrphanVert', {}, {'display_name': 'Orphan Vertical'}, None, None, runtime) + self._create_item('html', 'Html1', "

Goodbye

", {'display_name': 'Parented Html'}, 'vertical', 'Vert1', runtime) + self._create_item('html', 'OrphanHtml', "

Hello

", {'display_name': 'Orphan html'}, None, None, runtime) + self._create_item('static_tab', 'staticuno', "

tab

", {'display_name': 'Tab uno'}, None, None, runtime) + self._create_item('about', 'overview', "

overview

", {}, None, None, runtime) + self._create_item('course_info', 'updates', "

  1. Sep 22

    test

", {}, None, None, runtime) + + def test_mongo_orphan(self): + """ + Test that old mongo finds the orphans + """ + orphans = self.old_mongo.get_orphans('test_org.test_course', ['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) + location = self.course_location.replace(category='vertical', name='OrphanVert') + self.assertIn(location.url(), orphans) + location = self.course_location.replace(category='html', name='OrphanHtml') + self.assertIn(location.url(), orphans) + + def test_split_orphan(self): + """ + Test that old mongo finds the orphans + """ + orphans = self.split_mongo.get_orphans(self.split_course_id, ['static_tab', 'about', 'course_info'], 'draft') + self.assertEqual(len(orphans), 3, "Wrong # {}".format(orphans)) + location = BlockUsageLocator(course_id=self.split_course_id, branch='draft', usage_id='OrphanChapter') + self.assertIn(location, orphans) + location = BlockUsageLocator(course_id=self.split_course_id, branch='draft', usage_id='OrphanVert') + self.assertIn(location, orphans) + location = BlockUsageLocator(course_id=self.split_course_id, branch='draft', usage_id='OrphanHtml') + self.assertIn(location, orphans) From e9c70633c6eedc02ae50cfb054e709176d8efd07 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Tue, 8 Oct 2013 11:43:20 -0400 Subject: [PATCH 2/5] Add delete if staff member --- cms/djangoapps/contentstore/tests/test_orphan.py | 14 ++++++++++++++ cms/djangoapps/contentstore/views/item.py | 8 +++++++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/tests/test_orphan.py b/cms/djangoapps/contentstore/tests/test_orphan.py index c6f16fd47b..566cc0cf56 100644 --- a/cms/djangoapps/contentstore/tests/test_orphan.py +++ b/cms/djangoapps/contentstore/tests/test_orphan.py @@ -56,3 +56,17 @@ class TestOrphan(CourseTestCase): self.assertIn(location.url(), orphans) location = self.course.location.replace(category='html', name='OrphanHtml') self.assertIn(location.url(), orphans) + + def test_mongo_orphan_delete(self): + """ + 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) + orphans = json.loads( + self.client.get(url, HTTP_ACCEPT='application/json').content + ) + self.assertEqual(len(orphans), 0, "Orphans not deleted {}".format(orphans)) diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 816b05c16b..273cc6ebd9 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -20,6 +20,7 @@ from ..utils import get_modulestore 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 __all__ = ['save_item', 'create_item', 'delete_item', 'orphan'] @@ -203,6 +204,7 @@ def delete_item(request): @login_required +@require_http_methods(("GET", "DELETE")) def orphan(request, course_id): """ View for handling orphan related requests. A get gets all of the current orphans. @@ -214,6 +216,10 @@ def orphan(request, course_id): :param request: :param course_id: Locator syntax course_id """ - # dhm: I'd add DELETE but I'm not sure what type of authentication/authorization we'd need if request.method == 'GET': return JsonResponse(modulestore().get_orphans(course_id, DETACHED_CATEGORIES, 'draft')) + if request.method == 'DELETE' and request.user.is_staff: + items = modulestore().get_orphans(course_id, DETACHED_CATEGORIES, 'draft') + for item in items: + modulestore('draft').delete_item(item, True) + return JsonResponse({'deleted': items}) From 4e455fd87fd567c6ebac9d593c56e499bc015c92 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Mon, 21 Oct 2013 14:38:03 -0400 Subject: [PATCH 3/5] Limit read access to people with write access. Add unit tests for auth --- .../contentstore/tests/test_orphan.py | 16 +++++++++++++ cms/djangoapps/contentstore/tests/utils.py | 2 +- cms/djangoapps/contentstore/views/item.py | 23 +++++++++++++------ 3 files changed, 33 insertions(+), 8 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_orphan.py b/cms/djangoapps/contentstore/tests/test_orphan.py index 566cc0cf56..c51c808f00 100644 --- a/cms/djangoapps/contentstore/tests/test_orphan.py +++ b/cms/djangoapps/contentstore/tests/test_orphan.py @@ -5,6 +5,7 @@ import json from contentstore.tests.utils import CourseTestCase from xmodule.modulestore.django import editable_modulestore from django.core.urlresolvers import reverse +from student.models import CourseEnrollment class TestOrphan(CourseTestCase): """ @@ -70,3 +71,18 @@ class TestOrphan(CourseTestCase): self.client.get(url, HTTP_ACCEPT='application/json').content ) self.assertEqual(len(orphans), 0, "Orphans not deleted {}".format(orphans)) + + def test_not_permitted(self): + """ + Test that auth restricts get and delete appropriately + """ + 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) + self.assertEqual(response.status_code, 403) + response = test_user_client.delete(url) + self.assertEqual(response.status_code, 403) diff --git a/cms/djangoapps/contentstore/tests/utils.py b/cms/djangoapps/contentstore/tests/utils.py index 6c00ab4111..03ad159822 100644 --- a/cms/djangoapps/contentstore/tests/utils.py +++ b/cms/djangoapps/contentstore/tests/utils.py @@ -65,7 +65,7 @@ class CourseTestCase(ModuleStoreTestCase): def createNonStaffAuthedUserClient(self): """ - Create a non-staff user, log them in, and return the client to use for testing. + Create a non-staff user, log them in, and return the client, user to use for testing. """ uname = 'teststudent' password = 'foo' diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 273cc6ebd9..16fbb32a6b 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -7,7 +7,7 @@ from django.core.exceptions import PermissionDenied from django.contrib.auth.decorators import login_required from xmodule.modulestore import Location -from xmodule.modulestore.django import modulestore +from xmodule.modulestore.django import modulestore, loc_mapper from xmodule.modulestore.inheritance import own_metadata from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError @@ -21,6 +21,8 @@ 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 student.models import CourseEnrollment __all__ = ['save_item', 'create_item', 'delete_item', 'orphan'] @@ -216,10 +218,17 @@ def orphan(request, course_id): :param request: :param course_id: Locator syntax course_id """ + course_loc = CourseLocator(course_id=course_id) if request.method == 'GET': - return JsonResponse(modulestore().get_orphans(course_id, DETACHED_CATEGORIES, 'draft')) - if request.method == 'DELETE' and request.user.is_staff: - items = modulestore().get_orphans(course_id, DETACHED_CATEGORIES, 'draft') - for item in items: - modulestore('draft').delete_item(item, True) - return JsonResponse({'deleted': items}) + if has_access(request.user, course_loc): + return JsonResponse(modulestore().get_orphans(course_id, DETACHED_CATEGORIES, 'draft')) + else: + raise PermissionDenied() + if request.method == 'DELETE': + if request.user.is_staff: + items = modulestore().get_orphans(course_id, DETACHED_CATEGORIES, 'draft') + for item in items: + modulestore('draft').delete_item(item, True) + return JsonResponse({'deleted': items}) + else: + raise PermissionDenied() From 8bbaf0065bb47e3cb26f9272df597910a645eff5 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Tue, 22 Oct 2013 11:08:52 -0400 Subject: [PATCH 4/5] Separate doc_store_config --- .../lib/xmodule/xmodule/modulestore/tests/test_orphan.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_orphan.py b/common/lib/xmodule/xmodule/modulestore/tests/test_orphan.py index f8f7e73218..6ebc81975e 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_orphan.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_orphan.py @@ -28,20 +28,21 @@ class TestOrphan(unittest.TestCase): 'fs_root': '', 'render_template': mock.Mock(return_value=""), 'xblock_mixins': (InheritanceMixin,) - }, **db_config) + }) split_course_id = 'test_org.test_course.runid' def setUp(self): - self.modulestore_options['collection'] = 'modulestore{0}'.format(uuid.uuid4().hex) + self.db_config['collection'] = 'modulestore{0}'.format(uuid.uuid4().hex) self.userid = random.getrandbits(32) super(TestOrphan, self).setUp() self.split_mongo = SplitMongoModuleStore( + self.db_config, **self.modulestore_options ) self.addCleanup(self.tearDownSplit) - self.old_mongo = MongoModuleStore(**self.modulestore_options) + self.old_mongo = MongoModuleStore(self.db_config, **self.modulestore_options) self.addCleanup(self.tearDownMongo) self.course_location = None self._create_course() From f45abe3d9fb738274ded35a25cae9eadb0c3a8fd Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Wed, 23 Oct 2013 08:50:45 -0400 Subject: [PATCH 5/5] 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)