Reduce mongo calls on course outline and container pages

cms/djangoapps/contentstore/views/component.py

@@ -11,7 +11,7 @@ from django.conf import settings
 from xmodule.modulestore.exceptions import ItemNotFoundError
 from edxmako.shortcuts import render_to_response
 
-from xmodule.modulestore import ModuleStoreEnum
+from xmodule.modulestore import PublishState
 from xmodule.modulestore.django import modulestore
 
 from xblock.core import XBlock
@@ -97,7 +97,7 @@ def subsection_handler(request, usage_key_string):
         can_view_live = False
         subsection_units = item.get_children()
         for unit in subsection_units:
-            has_published = modulestore().has_item(unit.location, revision=ModuleStoreEnum.RevisionOption.published_only)
+            has_published = modulestore().compute_publish_state(unit) != PublishState.private
             if has_published:
                 can_view_live = True
                 break

cms/djangoapps/contentstore/views/course.py

@@ -211,7 +211,8 @@ def course_handler(request, course_key_string=None):
     response_format = request.REQUEST.get('format', 'html')
     if response_format == 'json' or 'application/json' in request.META.get('HTTP_ACCEPT', 'application/json'):
         if request.method == 'GET':
-            return JsonResponse(_course_outline_json(request, CourseKey.from_string(course_key_string)))
+            course_module = _get_course_module(CourseKey.from_string(course_key_string), request.user, depth=None)
+            return JsonResponse(_course_outline_json(request, course_module))
         elif request.method == 'POST':  # not sure if this is only post. If one will have ids, it goes after access
             return _create_or_rerun_course(request)
         elif not has_course_access(request.user, CourseKey.from_string(course_key_string)):
@@ -231,11 +232,10 @@ def course_handler(request, course_key_string=None):
         return HttpResponseNotFound()
 
 
-def _course_outline_json(request, course_key):
+def _course_outline_json(request, course_module):
     """
     Returns a JSON representation of the course module and recursively all of its children.
     """
-    course_module = _get_course_module(course_key, request.user, depth=None)
     return create_xblock_info(
         course_module,
         include_child_info=True,
@@ -369,10 +369,12 @@ def course_index(request, course_key):
 
     org, course, name: Attributes of the Location for the item to edit
     """
-    course_module = _get_course_module(course_key, request.user, depth=3)
+    # A depth of None implies the whole course. The course outline needs this in order to compute has_changes.
+    # A unit may not have a draft version, but one of its components could, and hence the unit itself has changes.
+    course_module = _get_course_module(course_key, request.user, depth=None)
     lms_link = get_lms_link_for_item(course_module.location)
     sections = course_module.get_children()
-    course_structure = _course_outline_json(request, course_key)
+    course_structure = _course_outline_json(request, course_module)
     locator_to_show = request.REQUEST.get('show', None)
 
     try:

cms/djangoapps/contentstore/views/item.py

@@ -24,7 +24,7 @@ from xblock.fragment import Fragment
 
 import xmodule
 from xmodule.tabs import StaticTab, CourseTabList
-from xmodule.modulestore import ModuleStoreEnum
+from xmodule.modulestore import ModuleStoreEnum, PublishState
 from xmodule.modulestore.django import modulestore
 from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError
 from xmodule.modulestore.inheritance import own_metadata
@@ -380,7 +380,7 @@ def _save_xblock(user, xblock, data=None, children=None, metadata=None, nullout=
     # new item should be republished. This is used by staff locking to ensure that changing the draft
     # value of the staff lock will also update the published version.
     if publish == 'republish':
-        published = modulestore().has_item(xblock.location, revision=ModuleStoreEnum.RevisionOption.published_only)
+        published = modulestore().compute_publish_state(xblock) != PublishState.private
         if published:
             publish = 'make_public'
 
@@ -624,7 +624,7 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F
         return None
 
     is_xblock_unit = is_unit(xblock, parent_xblock)
-    is_unit_with_changes = is_xblock_unit and modulestore().has_changes(xblock.location)
+    is_unit_with_changes = is_xblock_unit and modulestore().has_changes(xblock)
 
     if graders is None:
         graders = CourseGradingModel.fetch(xblock.location.course_key).graders
@@ -643,7 +643,7 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F
 
     # Treat DEFAULT_START_DATE as a magic number that means the release date has not been set
     release_date = get_default_time_display(xblock.start) if xblock.start != DEFAULT_START_DATE else None
-    published = modulestore().has_item(xblock.location, revision=ModuleStoreEnum.RevisionOption.published_only)
+    published = modulestore().compute_publish_state(xblock) != PublishState.private
 
     xblock_info = {
         "id": unicode(xblock.location),

cms/djangoapps/contentstore/views/tests/test_item.py

@@ -541,14 +541,14 @@ class TestEditItem(ItemTest):
         Verifies the item with given location has a published version and no draft (unpublished changes).
         """
         self.assertTrue(self._is_location_published(location))
-        self.assertFalse(modulestore().has_changes(location))
+        self.assertFalse(modulestore().has_changes(modulestore().get_item(location)))
 
     def _verify_published_with_draft(self, location):
         """
         Verifies the item with given location has a published version and also a draft version (unpublished changes).
         """
         self.assertTrue(self._is_location_published(location))
-        self.assertTrue(modulestore().has_changes(location))
+        self.assertTrue(modulestore().has_changes(modulestore().get_item(location)))
 
     def test_make_public(self):
         """ Test making a private problem public (publishing it). """

common/lib/xmodule/xmodule/modulestore/draft_and_published.py

@@ -80,7 +80,7 @@ class ModuleStoreDraftAndPublished(BranchSettingMixin):
         raise NotImplementedError
 
     @abstractmethod
-    def has_changes(self, usage_key):
+    def has_changes(self, xblock):
         raise NotImplementedError
 
     @abstractmethod

common/lib/xmodule/xmodule/modulestore/mixed.py

@@ -474,14 +474,14 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
         store = self._verify_modulestore_support(location.course_key, 'convert_to_draft')
         return store.convert_to_draft(location, user_id)
 
-    def has_changes(self, usage_key):
+    def has_changes(self, xblock):
         """
         Checks if the given block has unpublished changes
-        :param usage_key: the block to check
+        :param xblock: the block to check
         :return: True if the draft and published versions differ
         """
-        store = self._verify_modulestore_support(usage_key.course_key, 'has_changes')
-        return store.has_changes(usage_key)
+        store = self._verify_modulestore_support(xblock.location.course_key, 'has_changes')
+        return store.has_changes(xblock)
 
     def _verify_modulestore_support(self, course_key, method):
         """

common/lib/xmodule/xmodule/modulestore/mongo/base.py

@@ -297,6 +297,19 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
                         value[key] = self._convert_reference_to_key(subvalue)
         return jsonfields
 
+    def lookup_item(self, location):
+        """
+        Returns the JSON payload of the xblock at location.
+        """
+
+        try:
+            json = self.module_data[location]
+        except KeyError:
+            json = self.modulestore._find_one(location)
+            self.module_data[location] = json
+
+        return json
+
 
 # The only thing using this w/ wildcards is contentstore.mongo for asset retrieval
 def location_to_query(location, wildcard=True, tag='i4x'):

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

@@ -599,25 +599,19 @@ class DraftModuleStore(MongoModuleStore):
         _internal([root_usage.to_deprecated_son() for root_usage in root_usages])
         self.collection.remove({'_id': {'$in': to_be_deleted}}, safe=self.collection.safe)
 
-    def has_changes(self, location):
+    def has_changes(self, xblock):
         """
         Check if the xblock or its children have been changed since the last publish.
-        :param location: location to check
+        :param xblock: xblock to check
         :return: True if the draft and published versions differ
         """
 
-        try:
-            item = self.get_item(location)
-        # defensively check that the parent's child actually exists
-        except ItemNotFoundError:
-            return False
-
         # don't check children if this block has changes (is not public)
-        if self.compute_publish_state(item) != PublishState.public:
+        if self.compute_publish_state(xblock) != PublishState.public:
             return True
         # if this block doesn't have changes, then check its children
-        elif item.has_children:
-            return any([self.has_changes(child) for child in item.children])
+        elif xblock.has_children:
+            return any([self.has_changes(child) for child in xblock.get_children()])
         # otherwise there are no changes
         else:
             return False
@@ -799,13 +793,11 @@ class DraftModuleStore(MongoModuleStore):
         """
         if getattr(xblock, 'is_draft', False):
             published_xblock_location = as_published(xblock.location)
-            published_item = self.collection.find_one(
-                {'_id': published_xblock_location.to_deprecated_son()}
-            )
-            if published_item is None:
+            try:
+                xblock.runtime.lookup_item(published_xblock_location)
+            except ItemNotFoundError:
                 return PublishState.private
-            else:
-                return PublishState.draft
+            return PublishState.draft
         else:
             return PublishState.public
 

common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py

@@ -194,16 +194,16 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
         location = self._map_revision_to_branch(location, revision=revision)
         return SplitMongoModuleStore.get_parent_location(self, location, **kwargs)
 
-    def has_changes(self, usage_key):
+    def has_changes(self, xblock):
         """
         Checks if the given block has unpublished changes
-        :param usage_key: the block to check
+        :param xblock: the block to check
         :return: True if the draft and published versions differ
         """
         # TODO for better performance: lookup the courses and get the block entry, don't create the instances
-        draft = self.get_item(usage_key.for_branch(ModuleStoreEnum.BranchName.draft))
+        draft = self.get_item(xblock.location.for_branch(ModuleStoreEnum.BranchName.draft))
         try:
-            published = self.get_item(usage_key.for_branch(ModuleStoreEnum.BranchName.published))
+            published = self.get_item(xblock.location.for_branch(ModuleStoreEnum.BranchName.published))
         except ItemNotFoundError:
             return True
 

common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py

@@ -374,8 +374,8 @@ class TestMixedModuleStore(unittest.TestCase):
         )
 
         # Check that neither xblock has changes
-        self.assertFalse(self.store.has_changes(test_course.location))
-        self.assertFalse(self.store.has_changes(chapter.location))
+        self.assertFalse(self.store.has_changes(test_course))
+        self.assertFalse(self.store.has_changes(chapter))
 
     @ddt.data('draft', 'split')
     def test_has_changes(self, default_ms):
@@ -395,22 +395,22 @@ class TestMixedModuleStore(unittest.TestCase):
         )
 
         # Not yet published, so changes are present
-        self.assertTrue(self.store.has_changes(xblock.location))
+        self.assertTrue(self.store.has_changes(xblock))
 
         # Publish and verify that there are no unpublished changes
-        self.store.publish(xblock.location, self.user_id)
-        self.assertFalse(self.store.has_changes(xblock.location))
+        newXBlock = self.store.publish(xblock.location, self.user_id)
+        self.assertFalse(self.store.has_changes(newXBlock))
 
         # Change the component, then check that there now are changes
         component = self.store.get_item(xblock.location)
         component.display_name = 'Changed Display Name'
 
         component = self.store.update_item(component, self.user_id)
-        self.assertTrue(self.store.has_changes(component.location))
+        self.assertTrue(self.store.has_changes(component))
 
         # Publish and verify again
-        self.store.publish(component.location, self.user_id)
-        self.assertFalse(self.store.has_changes(component.location))
+        component = self.store.publish(component.location, self.user_id)
+        self.assertFalse(self.store.has_changes(component))
 
     @ddt.data(('draft', 7, 2), ('split', 13, 4))
     @ddt.unpack
@@ -994,7 +994,7 @@ class TestMixedModuleStore(unittest.TestCase):
         # Draft WITH changes
         item.display_name = 'new name'
         item = self.store.update_item(item, self.user_id)
-        self.assertTrue(self.store.has_changes(item.location))
+        self.assertTrue(self.store.has_changes(item))
         self.assertEquals(self.store.compute_publish_state(item), PublishState.draft)
 
     @ddt.data('draft', 'split')

common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py

@@ -515,10 +515,10 @@ class TestMongoModuleStore(unittest.TestCase):
             block_id=location.block_id
         )
         parent.children += [Location('edX', 'toy', '2012_Fall', 'vertical', 'does_not_exist')]
-        self.draft_store.update_item(parent, self.dummy_user)
+        parent = self.draft_store.update_item(parent, self.dummy_user)
 
         # Check the parent for changes should return False and not throw an exception
-        self.assertFalse(self.draft_store.has_changes(location))
+        self.assertFalse(self.draft_store.has_changes(parent))
 
     def _create_test_tree(self, name, user_id=None):
         """
@@ -569,6 +569,11 @@ class TestMongoModuleStore(unittest.TestCase):
 
         return locations
 
+    def _has_changes(self, location):
+        """ Helper that returns True if location has changes, False otherwise """
+        store = self.draft_store
+        return store.has_changes(store.get_item(location))
+
     def test_has_changes_ancestors(self):
         """
         Tests that has_changes() returns true on ancestors when a child is changed
@@ -577,7 +582,7 @@ class TestMongoModuleStore(unittest.TestCase):
 
         # Verify that there are no unpublished changes
         for key in locations:
-            self.assertFalse(self.draft_store.has_changes(locations[key]))
+            self.assertFalse(self._has_changes(locations[key]))
 
         # Change the child
         child = self.draft_store.get_item(locations['child'])
@@ -585,18 +590,18 @@ class TestMongoModuleStore(unittest.TestCase):
         self.draft_store.update_item(child, user_id=self.dummy_user)
 
         # All ancestors should have changes, but not siblings
-        self.assertTrue(self.draft_store.has_changes(locations['grandparent']))
-        self.assertTrue(self.draft_store.has_changes(locations['parent']))
-        self.assertTrue(self.draft_store.has_changes(locations['child']))
-        self.assertFalse(self.draft_store.has_changes(locations['parent_sibling']))
-        self.assertFalse(self.draft_store.has_changes(locations['child_sibling']))
+        self.assertTrue(self._has_changes(locations['grandparent']))
+        self.assertTrue(self._has_changes(locations['parent']))
+        self.assertTrue(self._has_changes(locations['child']))
+        self.assertFalse(self._has_changes(locations['parent_sibling']))
+        self.assertFalse(self._has_changes(locations['child_sibling']))
 
         # Publish the unit with changes
         self.draft_store.publish(locations['parent'], self.dummy_user)
 
         # Verify that there are no unpublished changes
         for key in locations:
-            self.assertFalse(self.draft_store.has_changes(locations[key]))
+            self.assertFalse(self._has_changes(locations[key]))
 
     def test_has_changes_publish_ancestors(self):
         """
@@ -606,7 +611,7 @@ class TestMongoModuleStore(unittest.TestCase):
 
         # Verify that there are no unpublished changes
         for key in locations:
-            self.assertFalse(self.draft_store.has_changes(locations[key]))
+            self.assertFalse(self._has_changes(locations[key]))
 
         # Change both children
         child = self.draft_store.get_item(locations['child'])
@@ -617,22 +622,22 @@ class TestMongoModuleStore(unittest.TestCase):
         self.draft_store.update_item(child_sibling, user_id=self.dummy_user)
 
         # Verify that ancestors have changes
-        self.assertTrue(self.draft_store.has_changes(locations['grandparent']))
-        self.assertTrue(self.draft_store.has_changes(locations['parent']))
+        self.assertTrue(self._has_changes(locations['grandparent']))
+        self.assertTrue(self._has_changes(locations['parent']))
 
         # Publish one child
         self.draft_store.publish(locations['child_sibling'], self.dummy_user)
 
         # Verify that ancestors still have changes
-        self.assertTrue(self.draft_store.has_changes(locations['grandparent']))
-        self.assertTrue(self.draft_store.has_changes(locations['parent']))
+        self.assertTrue(self._has_changes(locations['grandparent']))
+        self.assertTrue(self._has_changes(locations['parent']))
 
         # Publish the other child
         self.draft_store.publish(locations['child'], self.dummy_user)
 
         # Verify that ancestors now have no changes
-        self.assertFalse(self.draft_store.has_changes(locations['grandparent']))
-        self.assertFalse(self.draft_store.has_changes(locations['parent']))
+        self.assertFalse(self._has_changes(locations['grandparent']))
+        self.assertFalse(self._has_changes(locations['parent']))
 
     def test_has_changes_add_remove_child(self):
         """
@@ -642,8 +647,8 @@ class TestMongoModuleStore(unittest.TestCase):
         locations = self._create_test_tree('has_changes_add_remove_child')
 
         # Test that the ancestors don't have changes
-        self.assertFalse(self.draft_store.has_changes(locations['grandparent']))
-        self.assertFalse(self.draft_store.has_changes(locations['parent']))
+        self.assertFalse(self._has_changes(locations['grandparent']))
+        self.assertFalse(self._has_changes(locations['parent']))
 
         # Create a new child and attach it to parent
         new_child_location = Location('edX', 'tree', 'has_changes_add_remove_child', 'vertical', 'new_child')
@@ -655,8 +660,8 @@ class TestMongoModuleStore(unittest.TestCase):
         )
 
         # Verify that the ancestors now have changes
-        self.assertTrue(self.draft_store.has_changes(locations['grandparent']))
-        self.assertTrue(self.draft_store.has_changes(locations['parent']))
+        self.assertTrue(self._has_changes(locations['grandparent']))
+        self.assertTrue(self._has_changes(locations['parent']))
 
         # Remove the child from the parent
         parent = self.draft_store.get_item(locations['parent'])
@@ -664,8 +669,8 @@ class TestMongoModuleStore(unittest.TestCase):
         self.draft_store.update_item(parent, user_id=self.dummy_user)
 
         # Verify that ancestors now have no changes
-        self.assertFalse(self.draft_store.has_changes(locations['grandparent']))
-        self.assertFalse(self.draft_store.has_changes(locations['parent']))
+        self.assertFalse(self._has_changes(locations['grandparent']))
+        self.assertFalse(self._has_changes(locations['parent']))
 
     def test_has_changes_non_direct_only_children(self):
         """
@@ -689,16 +694,16 @@ class TestMongoModuleStore(unittest.TestCase):
         self.draft_store.publish(parent_location, self.dummy_user)
 
         # Verify that there are no changes
-        self.assertFalse(self.draft_store.has_changes(parent_location))
-        self.assertFalse(self.draft_store.has_changes(child_location))
+        self.assertFalse(self._has_changes(parent_location))
+        self.assertFalse(self._has_changes(child_location))
 
         # Change the child
         child.display_name = 'Changed Display Name'
         self.draft_store.update_item(child, user_id=self.dummy_user)
 
         # Verify that both parent and child have changes
-        self.assertTrue(self.draft_store.has_changes(parent_location))
-        self.assertTrue(self.draft_store.has_changes(child_location))
+        self.assertTrue(self._has_changes(parent_location))
+        self.assertTrue(self._has_changes(child_location))
 
     def test_update_edit_info_ancestors(self):
         """