diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index 7ca319fe13..b050ecb433 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/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 diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 7b8bc0bf0f..b4b14a64b6 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/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: diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 3540971e54..f96f7e9632 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/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), diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index 7d0b41887c..9645ccbbf0 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/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). """ diff --git a/common/lib/xmodule/xmodule/modulestore/draft_and_published.py b/common/lib/xmodule/xmodule/modulestore/draft_and_published.py index 27dbaf2e7b..e3f46d2e02 100644 --- a/common/lib/xmodule/xmodule/modulestore/draft_and_published.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index e847ea9131..eca62de852 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/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): """ diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index 6ca71dcd31..aad4c29a00 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/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'): diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index cd7e0bc4c2..d853516c05 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py index c41644ee14..85344f6478 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index c9ca1d821a..07166e1739 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/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') diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py index 0a5a509d7f..15ba65aa9e 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py +++ b/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): """