Reduce calls to expensive methods in create_xblock_info

Special-case the outline page requests.

STUD-1997

cms/djangoapps/contentstore/views/component.py

@@ -176,7 +176,7 @@ def container_handler(request, usage_key_string):
 
         # Fetch the XBlock info for use by the container page. Note that it includes information
         # about the block's ancestors and siblings for use by the Unit Outline.
-        xblock_info = create_xblock_info(xblock, include_ancestor_info=is_unit_page, include_edited_by=True, include_published_by=True)
+        xblock_info = create_xblock_info(xblock, include_ancestor_info=is_unit_page)
 
         # Create the link for preview.
         preview_lms_base = settings.FEATURES.get('PREVIEW_LMS_BASE')

cms/djangoapps/contentstore/views/course.py

@@ -239,6 +239,7 @@ def _course_outline_json(request, course_key):
     return create_xblock_info(
         course_module,
         include_child_info=True,
+        course_outline=True,
         include_children_predicate=lambda xblock: not xblock.category == 'vertical'
     )
 

cms/djangoapps/contentstore/views/item.py

@@ -286,6 +286,7 @@ def xblock_outline_handler(request, usage_key_string):
         return JsonResponse(create_xblock_info(
             root_xblock,
             include_child_info=True,
+            course_outline=True,
             include_children_predicate=lambda xblock: not xblock.category == 'vertical'
         ))
     else:
@@ -587,16 +588,18 @@ def _get_module_info(xblock, rewrite_static_links=True):
 
 
 def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=False, include_child_info=False,
-                       include_edited_by=False, include_published_by=False, include_children_predicate=NEVER):
+                       course_outline=False, include_children_predicate=NEVER):
     """
     Creates the information needed for client-side XBlockInfo.
 
     If data or metadata are not specified, their information will not be added
     (regardless of whether or not the xblock actually has data or metadata).
 
-    There are two optional boolean parameters:
+    There are three optional boolean parameters:
       include_ancestor_info - if true, ancestor info is added to the response
       include_child_info - if true, direct child info is included in the response
+      course_outline - if true, the xblock is being rendered on behalf of the course outline.
+        There are certain expensive computations that do not need to be included in this case.
 
     In addition, an optional include_children_predicate argument can be provided to define whether or
     not a particular xblock should have its children included.
@@ -622,7 +625,9 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F
     # Compute the child info first so it can be included in aggregate information for the parent
     if include_child_info and xblock.has_children:
         child_info = _create_xblock_child_info(
-            xblock, include_children_predicate=include_children_predicate
+            xblock,
+            course_outline,
+            include_children_predicate=include_children_predicate
         )
     else:
         child_info = None
@@ -630,7 +635,6 @@ 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)
-    currently_visible_to_students = is_currently_visible_to_students(xblock)
 
     is_xblock_unit = is_unit(xblock)
     is_unit_with_changes = is_xblock_unit and modulestore().has_changes(xblock.location)
@@ -645,8 +649,6 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F
         'studio_url': xblock_studio_url(xblock),
         "released_to_students": datetime.now(UTC) > xblock.start,
         "release_date": release_date,
-        "release_date_from": _get_release_date_from(xblock) if release_date else None,
-        "currently_visible_to_students": currently_visible_to_students,
         "visibility_state": _compute_visibility_state(xblock, child_info, is_unit_with_changes) if not xblock.category == 'course' else None
     }
     if data is not None:
@@ -654,19 +656,19 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F
     if metadata is not None:
         xblock_info["metadata"] = metadata
     if include_ancestor_info:
-        xblock_info['ancestor_info'] = _create_xblock_ancestor_info(xblock)
+        xblock_info['ancestor_info'] = _create_xblock_ancestor_info(xblock, course_outline)
     if child_info:
         xblock_info['child_info'] = child_info
-    # Currently, 'edited_by' and 'published_by' are only used by the container page.  Only compute them when asked to do
-    # so, since safe_get_username() is expensive.
-    if include_edited_by:
-        xblock_info['edited_by'] = safe_get_username(xblock.subtree_edited_by)
-    if include_published_by:
-        xblock_info['published_by'] = safe_get_username(xblock.published_by)
-    # On the unit page only, add 'has_changes' to indicate when there are changes that can be discarded.
-    # We don't add it in general because it is an expensive operation.
-    if is_xblock_unit:
+    # Currently, 'edited_by', 'published_by', and 'release_date_from', and 'has_changes' are only used by the
+    # container page when rendering a unit. Since they are expensive to compute, only include them for units
+    # that are not being rendered on the course outline.
+    if is_xblock_unit and not course_outline:
+        xblock_info["edited_by"] = safe_get_username(xblock.subtree_edited_by)
+        xblock_info["published_by"] = safe_get_username(xblock.published_by)
+        xblock_info["currently_visible_to_students"] = is_currently_visible_to_students(xblock)
         xblock_info['has_changes'] = is_unit_with_changes
+        if release_date:
+            xblock_info["release_date_from"] = _get_release_date_from(xblock)
 
     return xblock_info
 
@@ -740,7 +742,7 @@ def _compute_visibility_state(xblock, child_info, is_unit_with_changes):
         return VisibilityState.ready
 
 
-def _create_xblock_ancestor_info(xblock):
+def _create_xblock_ancestor_info(xblock, course_outline):
     """
     Returns information about the ancestors of an xblock. Note that the direct parent will also return
     information about all of its children.
@@ -756,6 +758,7 @@ def _create_xblock_ancestor_info(xblock):
             ancestors.append(create_xblock_info(
                 ancestor,
                 include_child_info=include_child_info,
+                course_outline=course_outline,
                 include_children_predicate=direct_children_only
             ))
             collect_ancestor_info(get_parent_xblock(ancestor))
@@ -765,7 +768,7 @@ def _create_xblock_ancestor_info(xblock):
     }
 
 
-def _create_xblock_child_info(xblock, include_children_predicate=NEVER):
+def _create_xblock_child_info(xblock, course_outline, include_children_predicate=NEVER):
     """
     Returns information about the children of an xblock, as well as about the primary category
     of xblock expected as children.
@@ -780,7 +783,8 @@ def _create_xblock_child_info(xblock, include_children_predicate=NEVER):
     if xblock.has_children and include_children_predicate(xblock):
         child_info['children'] = [
             create_xblock_info(
-                child, include_child_info=True, include_children_predicate=include_children_predicate
+                child, include_child_info=True, course_outline=course_outline,
+                include_children_predicate=include_children_predicate
             ) for child in xblock.get_children()
         ]
     return child_info

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

@@ -1108,7 +1108,7 @@ class TestXBlockInfo(ItemTest):
         outline_url = reverse_usage_url('xblock_outline_handler', self.usage_key)
         resp = self.client.get(outline_url, HTTP_ACCEPT='application/json')
         json_response = json.loads(resp.content)
-        self.validate_course_xblock_info(json_response)
+        self.validate_course_xblock_info(json_response, course_outline=True)
 
     def test_chapter_xblock_info(self):
         chapter = modulestore().get_item(self.chapter.location)
@@ -1133,7 +1133,6 @@ class TestXBlockInfo(ItemTest):
         xblock_info = create_xblock_info(
             vertical,
             include_child_info=True,
-            include_edited_by=True,
             include_children_predicate=ALWAYS,
             include_ancestor_info=True
         )
@@ -1148,7 +1147,7 @@ class TestXBlockInfo(ItemTest):
         )
         self.validate_component_xblock_info(xblock_info)
 
-    def validate_course_xblock_info(self, xblock_info, has_child_info=True):
+    def validate_course_xblock_info(self, xblock_info, has_child_info=True, course_outline=False):
         """
         Validate that the xblock info is correct for the test course.
         """
@@ -1158,7 +1157,7 @@ class TestXBlockInfo(ItemTest):
         self.assertTrue(xblock_info['published'])
 
         # Finally, validate the entire response for consistency
-        self.validate_xblock_info_consistency(xblock_info, has_child_info=has_child_info)
+        self.validate_xblock_info_consistency(xblock_info, has_child_info=has_child_info, course_outline=course_outline)
 
     def validate_chapter_xblock_info(self, xblock_info, has_child_info=True):
         """
@@ -1168,18 +1167,20 @@ class TestXBlockInfo(ItemTest):
         self.assertEqual(xblock_info['id'], 'i4x://MITx/999/chapter/Week_1')
         self.assertEqual(xblock_info['display_name'], 'Week 1')
         self.assertTrue(xblock_info['published'])
+        self.assertIsNone(xblock_info.get('edited_by', None))
 
         # Finally, validate the entire response for consistency
         self.validate_xblock_info_consistency(xblock_info, has_child_info=has_child_info)
 
     def validate_sequential_xblock_info(self, xblock_info, has_child_info=True):
         """
-        Validate that the xblock info is correct for the test chapter.
+        Validate that the xblock info is correct for the test sequential.
         """
         self.assertEqual(xblock_info['category'], 'sequential')
         self.assertEqual(xblock_info['id'], 'i4x://MITx/999/sequential/Lesson_1')
         self.assertEqual(xblock_info['display_name'], 'Lesson 1')
         self.assertTrue(xblock_info['published'])
+        self.assertIsNone(xblock_info.get('edited_by', None))
 
         # Finally, validate the entire response for consistency
         self.validate_xblock_info_consistency(xblock_info, has_child_info=has_child_info)
@@ -1204,7 +1205,7 @@ class TestXBlockInfo(ItemTest):
         self.validate_course_xblock_info(ancestors[2], has_child_info=False)
 
         # Finally, validate the entire response for consistency
-        self.validate_xblock_info_consistency(xblock_info, has_child_info=True, has_ancestor_info=True, has_edited_by=True)
+        self.validate_xblock_info_consistency(xblock_info, has_child_info=True, has_ancestor_info=True)
 
     def validate_component_xblock_info(self, xblock_info):
         """
@@ -1214,11 +1215,13 @@ class TestXBlockInfo(ItemTest):
         self.assertEqual(xblock_info['id'], 'i4x://MITx/999/video/My_Video')
         self.assertEqual(xblock_info['display_name'], 'My Video')
         self.assertTrue(xblock_info['published'])
+        self.assertIsNone(xblock_info.get('edited_by', None))
 
         # Finally, validate the entire response for consistency
         self.validate_xblock_info_consistency(xblock_info)
 
-    def validate_xblock_info_consistency(self, xblock_info, has_ancestor_info=False, has_child_info=False, has_edited_by=False):
+    def validate_xblock_info_consistency(self, xblock_info, has_ancestor_info=False, has_child_info=False,
+                                         course_outline=False):
         """
         Validate that the xblock info is internally consistent.
         """
@@ -1232,7 +1235,8 @@ class TestXBlockInfo(ItemTest):
             for ancestor in xblock_info['ancestor_info']['ancestors']:
                 self.validate_xblock_info_consistency(
                     ancestor,
-                    has_child_info=(ancestor == ancestors[0])    # Only the direct ancestor includes children
+                    has_child_info=(ancestor == ancestors[0]),    # Only the direct ancestor includes children
+                    course_outline=course_outline
                 )
         else:
             self.assertIsNone(xblock_info.get('ancestor_info', None))
@@ -1242,11 +1246,12 @@ class TestXBlockInfo(ItemTest):
                 for child_response in xblock_info['child_info']['children']:
                     self.validate_xblock_info_consistency(
                         child_response,
-                        has_child_info=(not child_response.get('child_info', None) is None)
+                        has_child_info=(not child_response.get('child_info', None) is None),
+                        course_outline=course_outline
                     )
         else:
             self.assertIsNone(xblock_info.get('child_info', None))
-        if has_edited_by:
+        if xblock_info['category'] == 'vertical' and not course_outline:
             self.assertEqual(xblock_info['edited_by'], 'testuser')
         else:
             self.assertIsNone(xblock_info.get('edited_by', None))