diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index ce5bf36559..355b840fdf 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -85,6 +85,43 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): def test_edit_unit_full(self): self.check_edit_unit('full') + def _get_draft_counts(self, item): + cnt = 1 if getattr(item, 'is_draft', False) else 0 + for child in item.get_children(): + cnt = cnt + self._get_draft_counts(child) + + return cnt + + def test_get_depth_with_drafts(self): + import_from_xml(modulestore(), 'common/test/data/', ['simple']) + + course = modulestore('draft').get_item(Location(['i4x', 'edX', 'simple', + 'course', '2012_Fall', None]), depth=None) + + # make sure no draft items have been returned + num_drafts = self._get_draft_counts(course) + self.assertEqual(num_drafts, 0) + + problem = modulestore('draft').get_item(Location(['i4x', 'edX', 'simple', + 'problem', 'ps01-simple', None])) + + # put into draft + modulestore('draft').clone_item(problem.location, problem.location) + + # make sure we can query that item and verify that it is a draft + draft_problem = modulestore('draft').get_item(Location(['i4x', 'edX', 'simple', + 'problem', 'ps01-simple', None])) + self.assertTrue(getattr(draft_problem,'is_draft', False)) + + #now requery with depth + course = modulestore('draft').get_item(Location(['i4x', 'edX', 'simple', + 'course', '2012_Fall', None]), depth=None) + + # make sure just one draft item have been returned + num_drafts = self._get_draft_counts(course) + self.assertEqual(num_drafts, 1) + + def test_static_tab_reordering(self): import_from_xml(modulestore(), 'common/test/data/', ['full']) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 63dfe5bf5f..bd820fd489 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -1,3 +1,4 @@ +import logging from django.conf import settings from xmodule.modulestore import Location from xmodule.modulestore.django import modulestore @@ -137,7 +138,7 @@ def compute_unit_state(unit): 'private' content is editabled and not visible in the LMS """ - if unit.cms.is_draft: + if getattr(unit, 'is_draft', False): try: modulestore('direct').get_item(unit.location) return UnitState.draft diff --git a/cms/djangoapps/contentstore/views.py b/cms/djangoapps/contentstore/views.py index 561708c833..945216d1db 100644 --- a/cms/djangoapps/contentstore/views.py +++ b/cms/djangoapps/contentstore/views.py @@ -188,7 +188,7 @@ def course_index(request, org, course, name): 'coursename': name }) - course = modulestore().get_item(location) + course = modulestore().get_item(location, depth=3) sections = course.get_children() return render_to_response('overview.html', { @@ -208,19 +208,14 @@ def course_index(request, org, course, name): @login_required def edit_subsection(request, location): # check that we have permissions to edit this item - if not has_access(request.user, location): + course = get_course_for_item(location) + if not has_access(request.user, course.location): raise PermissionDenied() - item = modulestore().get_item(location) + item = modulestore().get_item(location, depth=1) - # TODO: we need a smarter way to figure out what course an item is in - for course in modulestore().get_courses(): - if (course.location.org == item.location.org and - course.location.course == item.location.course): - break - - lms_link = get_lms_link_for_item(location) - preview_link = get_lms_link_for_item(location, preview=True) + lms_link = get_lms_link_for_item(location, course_id=course.location.course_id) + preview_link = get_lms_link_for_item(location, course_id=course.location.course_id, preview=True) # make sure that location references a 'sequential', otherwise return BadRequest if item.location.category != 'sequential': @@ -277,19 +272,13 @@ def edit_unit(request, location): id: A Location URL """ - # check that we have permissions to edit this item - if not has_access(request.user, location): + course = get_course_for_item(location) + if not has_access(request.user, course.location): raise PermissionDenied() - item = modulestore().get_item(location) + item = modulestore().get_item(location, depth=1) - # TODO: we need a smarter way to figure out what course an item is in - for course in modulestore().get_courses(): - if (course.location.org == item.location.org and - course.location.course == item.location.course): - break - - lms_link = get_lms_link_for_item(item.location) + lms_link = get_lms_link_for_item(item.location, course_id=course.location.course_id) component_templates = defaultdict(list) diff --git a/cms/envs/dev.py b/cms/envs/dev.py index 5612db1396..b8d4d14b9e 100644 --- a/cms/envs/dev.py +++ b/cms/envs/dev.py @@ -142,4 +142,4 @@ DEBUG_TOOLBAR_CONFIG = { # To see stacktraces for MongoDB queries, set this to True. # Stacktraces slow down page loads drastically (for pages with lots of queries). -DEBUG_TOOLBAR_MONGO_STACKTRACES = False +DEBUG_TOOLBAR_MONGO_STACKTRACES = True diff --git a/cms/envs/test.py b/cms/envs/test.py index d7992cb471..59664bfd40 100644 --- a/cms/envs/test.py +++ b/cms/envs/test.py @@ -58,6 +58,10 @@ MODULESTORE = { 'direct': { 'ENGINE': 'xmodule.modulestore.mongo.MongoModuleStore', 'OPTIONS': modulestore_options + }, + 'draft': { + 'ENGINE': 'xmodule.modulestore.mongo.DraftMongoModuleStore', + 'OPTIONS': modulestore_options } } diff --git a/cms/xmodule_namespace.py b/cms/xmodule_namespace.py index cad3110574..c9bb8f4c6e 100644 --- a/cms/xmodule_namespace.py +++ b/cms/xmodule_namespace.py @@ -40,7 +40,6 @@ class CmsNamespace(Namespace): """ Namespace with fields common to all blocks in Studio """ - is_draft = Boolean(help="Whether this module is a draft", default=False, scope=Scope.settings) published_date = DateTuple(help="Date when the module was published", scope=Scope.settings) published_by = String(help="Id of the user who published this module", scope=Scope.settings) empty = StringyBoolean(help="Whether this is an empty template", scope=Scope.settings, default=False) diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index 022e016a58..2593b04472 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -10,6 +10,7 @@ from collections import namedtuple from .exceptions import InvalidLocationError, InsufficientSpecificationError from xmodule.errortracker import ErrorLog, make_error_tracker +from bson.son import SON log = logging.getLogger('mitx.' + 'modulestore') @@ -457,3 +458,13 @@ class ModuleStoreBase(ModuleStore): if c.id == course_id: return c return None + + +def namedtuple_to_son(namedtuple, prefix=''): + """ + Converts a namedtuple into a SON object with the same key order + """ + son = SON() + for idx, field_name in enumerate(namedtuple._fields): + son[prefix + field_name] = namedtuple[idx] + return son diff --git a/common/lib/xmodule/xmodule/modulestore/draft.py b/common/lib/xmodule/xmodule/modulestore/draft.py index 71922c08df..cfce5eb7db 100644 --- a/common/lib/xmodule/xmodule/modulestore/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/draft.py @@ -1,7 +1,8 @@ from datetime import datetime -from . import ModuleStoreBase, Location +from . import ModuleStoreBase, Location, namedtuple_to_son from .exceptions import ItemNotFoundError +import logging DRAFT = 'draft' @@ -15,11 +16,11 @@ def as_draft(location): def wrap_draft(item): """ - Sets `item.cms.is_draft` to `True` if the item is a + Sets `item.is_draft` to `True` if the item is a draft, and `False` otherwise. Sets the item's location to the non-draft location in either case """ - item.cms.is_draft = item.location.revision == DRAFT + setattr(item, 'is_draft', item.location.revision == DRAFT) item.location = item.location._replace(revision=None) return item @@ -55,11 +56,10 @@ class DraftModuleStore(ModuleStoreBase): get_children() to cache. None indicates to cache all descendents """ - # cdodge: we're forcing depth=0 here as the Draft store is not handling caching well try: - return wrap_draft(super(DraftModuleStore, self).get_item(as_draft(location), depth=0)) + return wrap_draft(super(DraftModuleStore, self).get_item(as_draft(location), depth=depth)) except ItemNotFoundError: - return wrap_draft(super(DraftModuleStore, self).get_item(location, depth=0)) + return wrap_draft(super(DraftModuleStore, self).get_item(location, depth=depth)) def get_instance(self, course_id, location, depth=0): """ @@ -67,11 +67,10 @@ class DraftModuleStore(ModuleStoreBase): TODO (vshnayder): this may want to live outside the modulestore eventually """ - # cdodge: we're forcing depth=0 here as the Draft store is not handling caching well try: - return wrap_draft(super(DraftModuleStore, self).get_instance(course_id, as_draft(location), depth=0)) + return wrap_draft(super(DraftModuleStore, self).get_instance(course_id, as_draft(location), depth=depth)) except ItemNotFoundError: - return wrap_draft(super(DraftModuleStore, self).get_instance(course_id, location, depth=0)) + return wrap_draft(super(DraftModuleStore, self).get_instance(course_id, location, depth=depth)) def get_items(self, location, course_id=None, depth=0): """ @@ -88,9 +87,8 @@ class DraftModuleStore(ModuleStoreBase): """ draft_loc = as_draft(location) - # cdodge: we're forcing depth=0 here as the Draft store is not handling caching well - draft_items = super(DraftModuleStore, self).get_items(draft_loc, course_id=course_id, depth=0) - items = super(DraftModuleStore, self).get_items(location, course_id=course_id, depth=0) + draft_items = super(DraftModuleStore, self).get_items(draft_loc, course_id=course_id, depth=depth) + items = super(DraftModuleStore, self).get_items(location, course_id=course_id, depth=depth) draft_locs_found = set(item.location._replace(revision=None) for item in draft_items) non_draft_items = [ @@ -118,7 +116,7 @@ class DraftModuleStore(ModuleStoreBase): """ draft_loc = as_draft(location) draft_item = self.get_item(location) - if not draft_item.cms.is_draft: + if not getattr(draft_item, 'is_draft', False): self.clone_item(location, draft_loc) return super(DraftModuleStore, self).update_item(draft_loc, data) @@ -133,7 +131,7 @@ class DraftModuleStore(ModuleStoreBase): """ draft_loc = as_draft(location) draft_item = self.get_item(location) - if not draft_item.cms.is_draft: + if not getattr(draft_item, 'is_draft', False): self.clone_item(location, draft_loc) return super(DraftModuleStore, self).update_children(draft_loc, children) @@ -149,7 +147,7 @@ class DraftModuleStore(ModuleStoreBase): draft_loc = as_draft(location) draft_item = self.get_item(location) - if not draft_item.cms.is_draft: + if not getattr(draft_item, 'is_draft', False): self.clone_item(location, draft_loc) if 'is_draft' in metadata: @@ -192,3 +190,36 @@ class DraftModuleStore(ModuleStoreBase): """ super(DraftModuleStore, self).clone_item(location, as_draft(location)) super(DraftModuleStore, self).delete_item(location) + + def _query_children_for_cache_children(self, items): + # first get non-draft in a round-trip + queried_children = [] + to_process_non_drafts = super(DraftModuleStore, self)._query_children_for_cache_children(items) + + to_process_dict = {} + for non_draft in to_process_non_drafts: + to_process_dict[Location(non_draft["_id"])] = non_draft + + # now query all draft content in another round-trip + query = { + '_id': {'$in': [namedtuple_to_son(as_draft(Location(item))) for item in items]} + } + to_process_drafts = list(self.collection.find(query)) + + # now we have to go through all drafts and replace the non-draft + # with the draft. This is because the semantics of the DraftStore is to + # always return the draft - if available + for draft in to_process_drafts: + draft_loc = Location(draft["_id"]) + draft_as_non_draft_loc = draft_loc._replace(revision=None) + + # does non-draft exist in the collection + # if so, replace it + if draft_as_non_draft_loc in to_process_dict: + to_process_dict[draft_as_non_draft_loc] = draft + + # convert the dict - which is used for look ups - back into a list + for key, value in to_process_dict.iteritems(): + queried_children.append(value) + + return queried_children diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index 47e35cda93..f1e09b024a 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -3,7 +3,6 @@ import sys import logging import copy -from bson.son import SON from collections import namedtuple from fs.osfs import OSFS from itertools import repeat @@ -19,7 +18,7 @@ from xmodule.error_module import ErrorDescriptor from xblock.runtime import DbModel, KeyValueStore, InvalidScopeError from xblock.core import Scope -from . import ModuleStoreBase, Location +from . import ModuleStoreBase, Location, namedtuple_to_son from .draft import DraftModuleStore from .exceptions import (ItemNotFoundError, DuplicateItemError) @@ -202,16 +201,6 @@ def location_to_query(location, wildcard=True): return query -def namedtuple_to_son(ntuple, prefix=''): - """ - Converts a namedtuple into a SON object with the same key order - """ - son = SON() - for idx, field_name in enumerate(ntuple._fields): - son[prefix + field_name] = ntuple[idx] - return son - - metadata_cache_key = attrgetter('org', 'course') @@ -383,6 +372,13 @@ class MongoModuleStore(ModuleStoreBase): item['location'] = item['_id'] del item['_id'] + def _query_children_for_cache_children(self, items): + # first get non-draft in a round-trip + query = { + '_id': {'$in': [namedtuple_to_son(Location(item)) for item in items]} + } + return list(self.collection.find(query)) + def _cache_children(self, items, depth=0): """ Returns a dictionary mapping Location -> item data, populated with json data @@ -407,13 +403,10 @@ class MongoModuleStore(ModuleStoreBase): # Load all children by id. See # http://www.mongodb.org/display/DOCS/Advanced+Queries#AdvancedQueries-%24or # for or-query syntax + to_process = [] if children: - query = { - '_id': {'$in': [namedtuple_to_son(Location(child)) for child in children]} - } - to_process = self.collection.find(query) - else: - to_process = [] + to_process = self._query_children_for_cache_children(children) + # If depth is None, then we just recurse until we hit all the descendents if depth is not None: depth -= 1 diff --git a/common/lib/xmodule/xmodule/modulestore/store_utilities.py b/common/lib/xmodule/xmodule/modulestore/store_utilities.py index cb3cd375a7..2935069090 100644 --- a/common/lib/xmodule/xmodule/modulestore/store_utilities.py +++ b/common/lib/xmodule/xmodule/modulestore/store_utilities.py @@ -136,3 +136,4 @@ def delete_course(modulestore, contentstore, source_location, commit = False): modulestore.delete_item(source_location) return True +