Correct publish titles, code review feedback.

cms/djangoapps/contentstore/views/component.py

@@ -178,10 +178,6 @@ def container_handler(request, usage_key_string):
         # 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)
 
-        # On the unit page only, add 'has_changes' to indicate when there are changes that can be discarded.
-        if is_unit_page:
-            xblock_info['has_changes'] = modulestore().has_changes(xblock.location)
-
         # Create the link for preview.
         preview_lms_base = settings.FEATURES.get('PREVIEW_LMS_BASE')
         # need to figure out where this item is in the list of children as the

cms/djangoapps/contentstore/views/item.py

@@ -632,6 +632,9 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F
     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)
+
     xblock_info = {
         "id": unicode(xblock.location),
         "display_name": xblock.display_name_with_default,
@@ -646,7 +649,7 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F
         "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) if not xblock.category == 'course' else None
+        "visibility_state": _compute_visibility_state(xblock, child_info, is_unit_with_changes) if not xblock.category == 'course' else None
     }
     if data is not None:
         xblock_info["data"] = data
@@ -656,6 +659,11 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F
         xblock_info['ancestor_info'] = _create_xblock_ancestor_info(xblock)
     if child_info:
         xblock_info['child_info'] = child_info
+    # 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:
+        xblock_info['has_changes'] = is_unit_with_changes
+
     return xblock_info
 
 
@@ -680,18 +688,20 @@ class VisibilityState(object):
     """
     live = 'live'
     ready = 'ready'
-    unscheduled = 'unscheduled'  # unscheduled
+    unscheduled = 'unscheduled'
     needs_attention = 'needs_attention'
     staff_only = 'staff_only'
 
 
-def _compute_visibility_state(xblock, child_info):
+def _compute_visibility_state(xblock, child_info, is_unit_with_changes):
     """
     Returns the current publish state for the specified xblock and its children
     """
     if xblock.visible_to_staff_only:
         return VisibilityState.staff_only
-    elif is_unit(xblock) and modulestore().has_changes(xblock.location):
+    elif is_unit_with_changes:
+        # Note that a unit that has never been published will fall into this category,
+        # as well as previously published units with draft content.
         return VisibilityState.needs_attention
     is_unscheduled = xblock.start == DEFAULT_START_DATE
     is_live = datetime.now(UTC) > xblock.start

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

@@ -231,6 +231,7 @@ class TestCourseOutline(CourseTestCase):
         self.assertEqual(json_response['category'], 'course')
         self.assertEqual(json_response['id'], 'i4x://MITx/999/course/Robot_Super_Course')
         self.assertEqual(json_response['display_name'], 'Robot Super Course')
+        self.assertTrue(json_response['published'])
         self.assertIsNone(json_response['visibility_state'])
 
         # Now verify the first child
@@ -240,6 +241,7 @@ class TestCourseOutline(CourseTestCase):
         self.assertEqual(first_child_response['category'], 'chapter')
         self.assertEqual(first_child_response['id'], 'i4x://MITx/999/chapter/Week_1')
         self.assertEqual(first_child_response['display_name'], 'Week 1')
+        self.assertTrue(json_response['published'])
         self.assertEqual(first_child_response['visibility_state'], VisibilityState.unscheduled)
         self.assertTrue(len(first_child_response['child_info']['children']) > 0)
 
@@ -253,6 +255,7 @@ class TestCourseOutline(CourseTestCase):
         self.assertIsNotNone(json_response['display_name'])
         self.assertIsNotNone(json_response['id'])
         self.assertIsNotNone(json_response['category'])
+        self.assertTrue(json_response['published'])
         if json_response.get('child_info', None):
             for child_response in json_response['child_info']['children']:
                 self.assert_correct_json_response(child_response)

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

@@ -531,8 +531,25 @@ class TestEditItem(ItemTest):
         self.assertEqual(unit2_usage_key, children[1])
 
     def _is_location_published(self, location):
+        """
+        Returns whether or not the item with given location has a published version.
+        """
         return modulestore().has_item(location, revision=ModuleStoreEnum.RevisionOption.published_only)
 
+    def _verify_published_with_no_draft(self, location):
+        """
+        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))
+
+    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))
+
     def test_make_public(self):
         """ Test making a private problem public (publishing it). """
         # When the problem is first created, it is only in draft (because of its category).
@@ -541,7 +558,7 @@ class TestEditItem(ItemTest):
             self.problem_update_url,
             data={'publish': 'make_public'}
         )
-        self.assertTrue(self._is_location_published(self.problem_usage_key))
+        self._verify_published_with_no_draft(self.problem_usage_key)
 
     def test_make_draft(self):
         """ Test creating a draft version of a public problem. """
@@ -554,17 +571,13 @@ class TestEditItem(ItemTest):
             self.problem_update_url,
             data={'publish': 'discard_changes'}
         )
-        self.assertTrue(self._is_location_published(self.problem_usage_key))
+        self._verify_published_with_no_draft(self.problem_usage_key)
         published = modulestore().get_item(self.problem_usage_key, revision=ModuleStoreEnum.RevisionOption.published_only)
         self.assertIsNone(published.due)
 
     def test_republish(self):
         """ Test republishing an item. """
         new_display_name = 'New Display Name'
-        republish_data = {
-            'publish': 'republish',
-            'display_name': new_display_name
-        }
 
         # When the problem is first created, it is only in draft (because of its category).
         self.assertFalse(self._is_location_published(self.problem_usage_key))
@@ -600,7 +613,7 @@ class TestEditItem(ItemTest):
                 }
             }
         )
-        self.assertTrue(self._is_location_published(self.problem_usage_key))
+        self._verify_published_with_no_draft(self.problem_usage_key)
         published = modulestore().get_item(
             self.problem_usage_key,
             revision=ModuleStoreEnum.RevisionOption.published_only
@@ -616,7 +629,7 @@ class TestEditItem(ItemTest):
             self.problem_update_url,
             data={'publish': 'make_public'}
         )
-        self.assertTrue(self._is_location_published(self.problem_usage_key))
+        self._verify_published_with_no_draft(self.problem_usage_key)
         published = modulestore().get_item(self.problem_usage_key, revision=ModuleStoreEnum.RevisionOption.published_only)
 
         # Update the draft version and check that published is different.
@@ -651,7 +664,7 @@ class TestEditItem(ItemTest):
             self.problem_update_url,
             data={'publish': 'make_public'}
         )
-        self.assertTrue(self._is_location_published(self.problem_usage_key))
+        self._verify_published_with_no_draft(self.problem_usage_key)
         published = modulestore().get_item(self.problem_usage_key, revision=ModuleStoreEnum.RevisionOption.published_only)
 
         # Now make a draft
@@ -706,8 +719,8 @@ class TestEditItem(ItemTest):
             data={'publish': 'make_public'}
         )
         self.assertEqual(resp.status_code, 200)
-        self.assertTrue(self._is_location_published(unit_usage_key))
-        self.assertTrue(self._is_location_published(html_usage_key))
+        self._verify_published_with_no_draft(unit_usage_key)
+        self._verify_published_with_no_draft(html_usage_key)
 
         # Make a draft for the unit and verify that the problem also has a draft
         resp = self.client.ajax_post(
@@ -718,10 +731,8 @@ class TestEditItem(ItemTest):
             }
         )
         self.assertEqual(resp.status_code, 200)
-        self.assertTrue(self._is_location_published(unit_usage_key))
-        self.assertTrue(self._is_location_published(html_usage_key))
-        self.assertTrue(modulestore().get_item(unit_usage_key).has_changes())
-        self.assertTrue(modulestore().get_item(html_usage_key).has_changes())
+        self._verify_published_with_draft(unit_usage_key)
+        self._verify_published_with_draft(html_usage_key)
 
 
 class TestEditSplitModule(ItemTest):
@@ -1143,6 +1154,7 @@ class TestXBlockInfo(ItemTest):
         self.assertEqual(xblock_info['category'], 'course')
         self.assertEqual(xblock_info['id'], 'i4x://MITx/999/course/Robot_Super_Course')
         self.assertEqual(xblock_info['display_name'], 'Robot Super Course')
+        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)
@@ -1154,6 +1166,7 @@ class TestXBlockInfo(ItemTest):
         self.assertEqual(xblock_info['category'], 'chapter')
         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.assertEqual(xblock_info['edited_by'], 'testuser')
 
         # Finally, validate the entire response for consistency
@@ -1166,6 +1179,7 @@ class TestXBlockInfo(ItemTest):
         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.assertEqual(xblock_info['edited_by'], 'testuser')
 
         # Finally, validate the entire response for consistency
@@ -1178,6 +1192,7 @@ class TestXBlockInfo(ItemTest):
         self.assertEqual(xblock_info['category'], 'vertical')
         self.assertEqual(xblock_info['id'], 'i4x://MITx/999/vertical/Unit_1')
         self.assertEqual(xblock_info['display_name'], 'Unit 1')
+        self.assertTrue(xblock_info['published'])
         self.assertEqual(xblock_info['edited_by'], 'testuser')
 
         # Validate that the correct ancestor info has been included
@@ -1199,6 +1214,7 @@ class TestXBlockInfo(ItemTest):
         self.assertEqual(xblock_info['category'], 'video')
         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.assertEqual(xblock_info['edited_by'], 'testuser')
 
         # Finally, validate the entire response for consistency
@@ -1211,6 +1227,7 @@ class TestXBlockInfo(ItemTest):
         self.assertIsNotNone(xblock_info['display_name'])
         self.assertIsNotNone(xblock_info['id'])
         self.assertIsNotNone(xblock_info['category'])
+        self.assertTrue(xblock_info['published'])
         self.assertEqual(xblock_info['edited_by'], 'testuser')
         if has_ancestor_info:
             self.assertIsNotNone(xblock_info.get('ancestor_info', None))
@@ -1243,14 +1260,17 @@ class TestXBlockPublishingInfo(ItemTest):
     SECOND_UNIT_PATH = [0, 1]
 
     def _create_child(self, parent, category, display_name, publish_item=False, staff_only=False):
+        """
+        Creates a child xblock for the given parent.
+        """
         return ItemFactory.create(
             parent_location=parent.location, category=category, display_name=display_name,
             user_id=self.user.id, publish_item=publish_item, visible_to_staff_only=staff_only
         )
 
-    def _get_child(self, xblock_info, index):
+    def _get_child_xblock_info(self, xblock_info, index):
         """
-        Returns the child at the specified index.
+        Returns the child xblock info at the specified index.
         """
         children = xblock_info['child_info']['children']
         self.assertTrue(len(children) > index)
@@ -1293,12 +1313,12 @@ class TestXBlockPublishingInfo(ItemTest):
     def _verify_visibility_state(self, xblock_info, expected_state, path=None):
         """
         Verify the publish state of an item in the xblock_info. If no path is provided
-        then the root item is verified.
+        then the root item will be verified.
         """
         if path:
-            direct_child = self._get_child(xblock_info, path[0])
+            direct_child_xblock_info = self._get_child_xblock_info(xblock_info, path[0])
             remaining_path = path[1:] if len(path) > 1 else None
-            self._verify_visibility_state(direct_child, expected_state, remaining_path)
+            self._verify_visibility_state(direct_child_xblock_info, expected_state, remaining_path)
         else:
             self.assertEqual(xblock_info['visibility_state'], expected_state)
 
@@ -1311,10 +1331,13 @@ class TestXBlockPublishingInfo(ItemTest):
         chapter = self._create_child(self.course, 'chapter', "Test Chapter")
         self._create_child(chapter, 'sequential', "Empty Sequential")
         xblock_info = self._get_xblock_info(chapter.location)
-        self.assertEqual(xblock_info['visibility_state'], VisibilityState.unscheduled)
-        self.assertEqual(self._get_child(xblock_info, 0)['visibility_state'], VisibilityState.unscheduled)
+        self._verify_visibility_state(xblock_info, VisibilityState.unscheduled)
+        self._verify_visibility_state(xblock_info, VisibilityState.unscheduled, path=self.FIRST_SUBSECTION_PATH)
 
     def test_published_unit(self):
+        """
+        Tests the visibility state of a published unit with release date in the future.
+        """
         chapter = self._create_child(self.course, 'chapter', "Test Chapter")
         sequential = self._create_child(chapter, 'sequential', "Test Sequential")
         self._create_child(sequential, 'vertical', "Published Unit", publish_item=True)
@@ -1327,6 +1350,9 @@ class TestXBlockPublishingInfo(ItemTest):
         self._verify_visibility_state(xblock_info, VisibilityState.staff_only, path=self.SECOND_UNIT_PATH)
 
     def test_released_unit(self):
+        """
+        Tests the visibility state of a published unit with release date in the past.
+        """
         chapter = self._create_child(self.course, 'chapter', "Test Chapter")
         sequential = self._create_child(chapter, 'sequential', "Test Sequential")
         self._create_child(sequential, 'vertical', "Published Unit", publish_item=True)
@@ -1339,10 +1365,14 @@ class TestXBlockPublishingInfo(ItemTest):
         self._verify_visibility_state(xblock_info, VisibilityState.staff_only, path=self.SECOND_UNIT_PATH)
 
     def test_unpublished_changes(self):
+        """
+        Tests the visibility state of a published unit with draft (unpublished) changes.
+        """
         chapter = self._create_child(self.course, 'chapter', "Test Chapter")
         sequential = self._create_child(chapter, 'sequential', "Test Sequential")
         unit = self._create_child(sequential, 'vertical', "Published Unit", publish_item=True)
         self._create_child(sequential, 'vertical', "Staff Only Unit", staff_only=True)
+        # Setting the display name creates a draft version of unit.
         self._set_display_name(unit.location, 'Updated Unit')
         xblock_info = self._get_xblock_info(chapter.location)
         self._verify_visibility_state(xblock_info, VisibilityState.needs_attention)