Merge pull request #1762 from MITx/feature/cdodge/course-overview-perf

Feature/cdodge/course overview perf

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'])
 

cms/djangoapps/contentstore/utils.py

+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

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)
-
-    # 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
+    item = modulestore().get_item(location, depth=1)
 
-    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)
 

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

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
     }
 }
 

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)

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

common/lib/xmodule/xmodule/modulestore/draft.py

 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

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
-            if children:
-                query = {
-                    '_id': {'$in': [namedtuple_to_son(Location(child)) for child in children]}
-                }
-                to_process = self.collection.find(query)
-            else:
             to_process = []
+            if children:
+                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

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
+