From ee8fc41b628d7d6c406fd7c48c6bcf757a644866 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Tue, 26 Aug 2014 15:45:08 -0400 Subject: [PATCH] compute_publish_state -> has_published_version LMS-11213 --- cms/djangoapps/contentstore/tests/utils.py | 79 ++++--------------- .../contentstore/views/component.py | 7 +- cms/djangoapps/contentstore/views/item.py | 7 +- .../xmodule/xmodule/modulestore/__init__.py | 26 ++---- .../modulestore/draft_and_published.py | 2 +- .../lib/xmodule/xmodule/modulestore/mixed.py | 4 +- .../xmodule/modulestore/mongo/draft.py | 27 +++---- .../modulestore/split_mongo/split_draft.py | 24 +----- .../tests/test_mixed_modulestore.py | 42 +++++----- common/lib/xmodule/xmodule/tests/__init__.py | 2 +- 10 files changed, 61 insertions(+), 159 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/utils.py b/cms/djangoapps/contentstore/tests/utils.py index fca7d8b2a2..cdb0948b20 100644 --- a/cms/djangoapps/contentstore/tests/utils.py +++ b/cms/djangoapps/contentstore/tests/utils.py @@ -9,17 +9,14 @@ from django.test.client import Client from django.contrib.auth.models import User from xmodule.contentstore.django import contentstore -from xmodule.modulestore import PublishState, ModuleStoreEnum +from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.inheritance import own_metadata from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory -from xmodule.modulestore.draft_and_published import DIRECT_ONLY_CATEGORIES from xmodule.modulestore.xml_importer import import_from_xml from student.models import Registration from opaque_keys.edx.locations import SlashSeparatedCourseKey, AssetLocation from contentstore.utils import reverse_url -from xmodule.modulestore.mongo.draft import DraftModuleStore -from xblock.fields import Scope from xmodule.modulestore.split_mongo.split import SplitMongoModuleStore @@ -151,16 +148,16 @@ class CourseTestCase(ModuleStoreTestCase): # create a Draft vertical vertical = self.store.get_item(course_id.make_usage_key('vertical', self.TEST_VERTICAL), depth=1) draft_vertical = self.store.convert_to_draft(vertical.location, self.user.id) - self.assertEqual(self.store.compute_publish_state(draft_vertical), PublishState.draft) + self.assertTrue(self.store.has_published_version(draft_vertical)) # create a Private (draft only) vertical private_vertical = self.store.create_item(self.user.id, course_id, 'vertical', self.PRIVATE_VERTICAL) - self.assertEqual(self.store.compute_publish_state(private_vertical), PublishState.private) + self.assertFalse(self.store.has_published_version(private_vertical)) # create a Published (no draft) vertical public_vertical = self.store.create_item(self.user.id, course_id, 'vertical', self.PUBLISHED_VERTICAL) public_vertical = self.store.publish(public_vertical.location, self.user.id) - self.assertEqual(self.store.compute_publish_state(public_vertical), PublishState.public) + self.assertTrue(self.store.has_published_version(public_vertical)) # add the new private and new public as children of the sequential sequential = self.store.get_item(course_id.make_usage_key('sequential', self.SEQUENTIAL)) @@ -197,11 +194,7 @@ class CourseTestCase(ModuleStoreTestCase): def verify_item_publish_state(item, publish_state): """Verifies the publish state of the item is as expected.""" - if publish_state in (PublishState.private, PublishState.draft): - self.assertTrue(getattr(item, 'is_draft', False)) - else: - self.assertFalse(getattr(item, 'is_draft', False)) - self.assertEqual(self.store.compute_publish_state(item), publish_state) + self.assertEqual(self.store.has_published_version(item), publish_state) def get_and_verify_publish_state(item_type, item_name, publish_state): """Gets the given item from the store and verifies the publish state of the item is as expected.""" @@ -210,18 +203,18 @@ class CourseTestCase(ModuleStoreTestCase): return item # verify that the draft vertical is draft - vertical = get_and_verify_publish_state('vertical', self.TEST_VERTICAL, PublishState.draft) + vertical = get_and_verify_publish_state('vertical', self.TEST_VERTICAL, True) for child in vertical.get_children(): - verify_item_publish_state(child, PublishState.draft) + verify_item_publish_state(child, True) # make sure that we don't have a sequential that is not in draft mode - sequential = get_and_verify_publish_state('sequential', self.SEQUENTIAL, PublishState.public) + sequential = get_and_verify_publish_state('sequential', self.SEQUENTIAL, True) # verify that we have the private vertical - private_vertical = get_and_verify_publish_state('vertical', self.PRIVATE_VERTICAL, PublishState.private) + private_vertical = get_and_verify_publish_state('vertical', self.PRIVATE_VERTICAL, False) # verify that we have the public vertical - public_vertical = get_and_verify_publish_state('vertical', self.PUBLISHED_VERTICAL, PublishState.public) + public_vertical = get_and_verify_publish_state('vertical', self.PUBLISHED_VERTICAL, True) # verify verticals are children of sequential for vert in [vertical, private_vertical, public_vertical]: @@ -266,22 +259,11 @@ class CourseTestCase(ModuleStoreTestCase): course2_item_loc = course2_item_loc.replace(name=new_name) course2_item = self.store.get_item(course2_item_loc) - try: - # compare published state - self.assertEqual( - self.store.compute_publish_state(course1_item), - self.store.compute_publish_state(course2_item) - ) - except AssertionError: - c1_state = self.compute_real_state(course1_item) - c2_state = self.compute_real_state(course2_item) - self.assertEqual( - c1_state, - c2_state, - "Publish states not equal: course item {} in state {} != course item {} in state {}".format( - course1_item_loc, c1_state, course2_item.location, c2_state - ) - ) + # compare published state + self.assertEqual( + self.store.has_published_version(course1_item), + self.store.has_published_version(course2_item) + ) # compare data self.assertEqual(hasattr(course1_item, 'data'), hasattr(course2_item, 'data')) @@ -332,37 +314,6 @@ class CourseTestCase(ModuleStoreTestCase): else: self.assertEqual(value, course2_asset_attrs[key]) - def compute_real_state(self, item): - """ - In draft mongo, compute_published_state can return draft when the draft == published, but in split, - it'll return public in that case - """ - supposed_state = self.store.compute_publish_state(item) - if supposed_state == PublishState.draft and isinstance(item.runtime.modulestore, DraftModuleStore): - # see if the draft differs from the published - published = self.store.get_item(item.location, revision=ModuleStoreEnum.RevisionOption.published_only) - if item.get_explicitly_set_fields_by_scope() != published.get_explicitly_set_fields_by_scope(): - # checking content: if published differs from item, return draft - return supposed_state - if item.get_explicitly_set_fields_by_scope(Scope.settings) != published.get_explicitly_set_fields_by_scope(Scope.settings): - # checking settings: if published differs from item, return draft - return supposed_state - if item.has_children and item.children != published.children: - # checking children: if published differs from item, return draft - return supposed_state - # published == item in all respects, so return public - return PublishState.public - elif supposed_state == PublishState.public and item.location.category in DIRECT_ONLY_CATEGORIES: - if not all([ - self.store.has_item(child_loc, revision=ModuleStoreEnum.RevisionOption.draft_only) - for child_loc in item.children - ]): - return PublishState.draft - else: - return supposed_state - else: - return supposed_state - def get_url(handler_name, key_value, key_name='usage_key_string', kwargs=None): """ diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index b050ecb433..f8e8ae2f4f 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -11,7 +11,6 @@ from django.conf import settings from xmodule.modulestore.exceptions import ItemNotFoundError from edxmako.shortcuts import render_to_response -from xmodule.modulestore import PublishState from xmodule.modulestore.django import modulestore from xblock.core import XBlock @@ -96,11 +95,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().compute_publish_state(unit) != PublishState.private - if has_published: - can_view_live = True - break + can_view_live = any([modulestore().has_published_version(unit) for unit in subsection_units]) return render_to_response( 'edit_subsection.html', diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 2959ba1391..6da18ae47a 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, PublishState, EdxJSONEncoder +from xmodule.modulestore import ModuleStoreEnum, EdxJSONEncoder from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError from xmodule.modulestore.inheritance import own_metadata @@ -387,8 +387,7 @@ def _save_xblock(user, xblock, data=None, children=None, metadata=None, nullout= # then this 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, but only at the unit level. if publish == 'republish' and xblock.category not in DIRECT_ONLY_CATEGORIES: - published = modulestore().compute_publish_state(xblock) != PublishState.private - if published: + if modulestore().has_published_version(xblock): publish = 'make_public' # Make public after updating the xblock, in case the caller asked for both an update and a publish. @@ -659,7 +658,7 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F visibility_state = _compute_visibility_state(xblock, child_info, is_xblock_unit and has_changes) else: visibility_state = None - published = modulestore().compute_publish_state(xblock) != PublishState.private + published = modulestore().has_published_version(xblock) xblock_info = { "id": unicode(xblock.location), diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index df20eafec3..4cb269994a 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -90,16 +90,6 @@ class ModuleStoreEnum(object): test = -3 -class PublishState(object): - """ - The legacy publish state for a given xblock-- either 'draft', 'private', or 'public'. These states - are no longer used in Studio, but they are still referenced in a few places in LMS. - """ - draft = 'draft' - private = 'private' - public = 'public' - - class ModuleStoreRead(object): """ An abstract interface for a database backend that stores XModuleDescriptor @@ -307,15 +297,9 @@ class ModuleStoreRead(object): pass @abstractmethod - def compute_publish_state(self, xblock): + def has_published_version(self, xblock): """ - Returns whether this xblock is draft, public, or private. - - Returns: - PublishState.draft - content is in the process of being edited, but still has a previous - version deployed to LMS - PublishState.public - content is locked and deployed to LMS - PublishState.private - content is editable and not deployed to LMS + Returns true if this xblock exists in the published course regardless of whether it's up to date """ pass @@ -529,11 +513,11 @@ class ModuleStoreReadBase(ModuleStoreRead): None ) - def compute_publish_state(self, xblock): + def has_published_version(self, xblock): """ - Returns PublishState.public since this is a read-only store. + Returns True since this is a read-only store. """ - return PublishState.public + return True def heartbeat(self): """ diff --git a/common/lib/xmodule/xmodule/modulestore/draft_and_published.py b/common/lib/xmodule/xmodule/modulestore/draft_and_published.py index 7db8ec0559..9b521a68a1 100644 --- a/common/lib/xmodule/xmodule/modulestore/draft_and_published.py +++ b/common/lib/xmodule/xmodule/modulestore/draft_and_published.py @@ -96,7 +96,7 @@ class ModuleStoreDraftAndPublished(BranchSettingMixin): raise NotImplementedError @abstractmethod - def compute_publish_state(self, xblock): + def has_published_version(self, xblock): raise NotImplementedError @abstractmethod diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index 6b40be78fb..4d1d4037f8 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/common/lib/xmodule/xmodule/modulestore/mixed.py @@ -540,7 +540,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): ) ) - def compute_publish_state(self, xblock): + def has_published_version(self, xblock): """ Returns whether this xblock is draft, public, or private. @@ -552,7 +552,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): """ course_id = xblock.scope_ids.usage_id.course_key store = self._get_modulestore_for_courseid(course_id) - return store.compute_publish_state(xblock) + return store.has_published_version(xblock) @strip_key def publish(self, location, user_id, **kwargs): diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index 6aaa0e10f1..08f3aa30ad 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py @@ -11,7 +11,7 @@ import logging from opaque_keys.edx.locations import Location from xmodule.exceptions import InvalidVersionError -from xmodule.modulestore import PublishState, ModuleStoreEnum +from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.exceptions import ItemNotFoundError, DuplicateItemError, DuplicateCourseError from xmodule.modulestore.mongo.base import ( MongoModuleStore, MongoRevisionKey, as_draft, as_published, @@ -591,13 +591,13 @@ class DraftModuleStore(MongoModuleStore): def has_changes(self, xblock): """ - Check if the xblock or its children have been changed since the last publish. + Check if the subtree rooted at xblock has any drafts and thus may possibly have changes :param xblock: xblock to check - :return: True if the draft and published versions differ + :return: True if there are any drafts anywhere in the subtree under xblock (a weaker + condition than for other stores) """ - # don't check children if this block has changes (is not public) - if self.compute_publish_state(xblock) != PublishState.public: + if getattr(xblock, 'is_draft', False): return True # if this block doesn't have changes, then check its children elif xblock.has_children: @@ -771,25 +771,18 @@ class DraftModuleStore(MongoModuleStore): return queried_children - def compute_publish_state(self, xblock): + def has_published_version(self, xblock): """ - Returns whether this xblock is draft, public, or private. - - Returns: - PublishState.draft - content is in the process of being edited, but still has a previous - version deployed to LMS - PublishState.public - content is locked and deployed to LMS - PublishState.private - content is editable and not deployed to LMS + Returns True if this xblock has an existing published version regardless of whether the + published version is up to date. """ if getattr(xblock, 'is_draft', False): published_xblock_location = as_published(xblock.location) try: xblock.runtime.lookup_item(published_xblock_location) except ItemNotFoundError: - return PublishState.private - return PublishState.draft - else: - return PublishState.public + return False + return True def _verify_branch_setting(self, expected_branch_setting): """ 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 26714f1e1f..698769e2ca 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py @@ -3,7 +3,7 @@ Module for the dual-branch fall-back Draft->Published Versioning ModuleStore """ from split import SplitMongoModuleStore, EXCLUDE_ALL -from xmodule.modulestore import ModuleStoreEnum, PublishState +from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.exceptions import InsufficientSpecificationError from xmodule.modulestore.draft_and_published import ( ModuleStoreDraftAndPublished, DIRECT_ONLY_CATEGORIES, UnsupportedRevisionError @@ -302,27 +302,11 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS block_locator = self._map_revision_to_branch(block_locator) return super(DraftVersioningModuleStore, self).get_block_generations(block_locator) - def compute_publish_state(self, xblock): + def has_published_version(self, xblock): """ - Returns whether this xblock is draft, public, or private. - - Returns: - PublishState.draft - published exists and is different from draft - PublishState.public - published exists and is the same as draft - PublishState.private - no published version exists + Returns whether this xblock has a published version (whether it's up to date or not). """ - draft_head = self._get_head(xblock, ModuleStoreEnum.BranchName.draft) - published_head = self._get_head(xblock, ModuleStoreEnum.BranchName.published) - - if not published_head: - # published version does not exist - return PublishState.private - elif self._get_version(draft_head) == self._get_version(published_head): - # published and draft versions are equal - return PublishState.public - else: - # published and draft versions differ - return PublishState.draft + return self._get_head(xblock, ModuleStoreEnum.BranchName.published) is not None def convert_to_draft(self, location, user_id): """ 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 5860c0f8ff..394b55652e 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -9,7 +9,7 @@ import datetime from pytz import UTC from xmodule.tests import DATA_DIR -from xmodule.modulestore import ModuleStoreEnum, PublishState +from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.exceptions import InvalidVersionError @@ -988,11 +988,11 @@ class TestMixedModuleStore(unittest.TestCase): ) self.assertIsNotNone(draft_xblock) - @ddt.data(('draft', 1, 0), ('split', 4, 0)) + @ddt.data(('draft', 1, 0), ('split', 2, 0)) @ddt.unpack - def test_compute_publish_state(self, default_ms, max_find, max_send): + def test_has_published_version(self, default_ms, max_find, max_send): """ - Test the compute_publish_state method + Test the has_published_version method """ self.initdb(default_ms) self._create_block_hierarchy() @@ -1002,37 +1002,33 @@ class TestMixedModuleStore(unittest.TestCase): item_location = item.location mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID)) with check_mongo_calls(mongo_store, max_find, max_send): - self.assertEquals(self.store.compute_publish_state(item), PublishState.private) + self.assertFalse(self.store.has_published_version(item)) # Private -> Public self.store.publish(item_location, self.user_id) item = self.store.get_item(item_location) - self.assertEquals(self.store.compute_publish_state(item), PublishState.public) + self.assertTrue(self.store.has_published_version(item)) # Public -> Private self.store.unpublish(item_location, self.user_id) item = self.store.get_item(item_location) - self.assertEquals(self.store.compute_publish_state(item), PublishState.private) + self.assertFalse(self.store.has_published_version(item)) # Private -> Public self.store.publish(item_location, self.user_id) item = self.store.get_item(item_location) - self.assertEquals(self.store.compute_publish_state(item), PublishState.public) + self.assertTrue(self.store.has_published_version(item)) # Public -> Draft with NO changes - # Note: This is where Split and Mongo differ self.store.convert_to_draft(item_location, self.user_id) item = self.store.get_item(item_location) - self.assertEquals( - self.store.compute_publish_state(item), - PublishState.draft if default_ms == 'draft' else PublishState.public - ) + self.assertTrue(self.store.has_published_version(item)) # Draft WITH changes item.display_name = 'new name' item = self.store.update_item(item, self.user_id) self.assertTrue(self.store.has_changes(item)) - self.assertEquals(self.store.compute_publish_state(item), PublishState.draft) + self.assertTrue(self.store.has_published_version(item)) @ddt.data('draft', 'split') def test_auto_publish(self, default_ms): @@ -1047,47 +1043,47 @@ class TestMixedModuleStore(unittest.TestCase): # test create_course to make sure we are autopublishing test_course = self.store.create_course('testx', 'GreekHero', 'test_run', self.user_id) - self.assertEqual(self.store.compute_publish_state(test_course), PublishState.public) + self.assertTrue(self.store.has_published_version(test_course)) test_course_key = test_course.id # test create_item of direct-only category to make sure we are autopublishing chapter = self.store.create_item(self.user_id, test_course_key, 'chapter', 'Overview') - self.assertEqual(self.store.compute_publish_state(chapter), PublishState.public) + self.assertTrue(self.store.has_published_version(chapter)) chapter_location = chapter.location # test create_child of direct-only category to make sure we are autopublishing sequential = self.store.create_child(self.user_id, chapter_location, 'sequential', 'Sequence') - self.assertEqual(self.store.compute_publish_state(sequential), PublishState.public) + self.assertTrue(self.store.has_published_version(sequential)) # test update_item of direct-only category to make sure we are autopublishing sequential.display_name = 'sequential1' sequential = self.store.update_item(sequential, self.user_id) - self.assertEqual(self.store.compute_publish_state(sequential), PublishState.public) + self.assertTrue(self.store.has_published_version(sequential)) # test delete_item of direct-only category to make sure we are autopublishing self.store.delete_item(sequential.location, self.user_id, revision=ModuleStoreEnum.RevisionOption.all) chapter = self.store.get_item(chapter.location.for_branch(None)) - self.assertEqual(self.store.compute_publish_state(chapter), PublishState.public) + self.assertTrue(self.store.has_published_version(chapter)) # test create_child of NOT direct-only category to make sure we aren't autopublishing problem_child = self.store.create_child(self.user_id, chapter_location, 'problem', 'Problem_Child') - self.assertEqual(self.store.compute_publish_state(problem_child), PublishState.private) + self.assertFalse(self.store.has_published_version(problem_child)) # test create_item of NOT direct-only category to make sure we aren't autopublishing problem_item = self.store.create_item(self.user_id, test_course_key, 'problem', 'Problem_Item') - self.assertEqual(self.store.compute_publish_state(problem_item), PublishState.private) + self.assertFalse(self.store.has_published_version(problem_item)) # test update_item of NOT direct-only category to make sure we aren't autopublishing problem_item.display_name = 'Problem_Item1' problem_item = self.store.update_item(problem_item, self.user_id) - self.assertEqual(self.store.compute_publish_state(problem_item), PublishState.private) + self.assertFalse(self.store.has_published_version(problem_item)) # test delete_item of NOT direct-only category to make sure we aren't autopublishing self.store.delete_item(problem_child.location, self.user_id) chapter = self.store.get_item(chapter.location.for_branch(None)) - self.assertEqual(self.store.compute_publish_state(chapter), PublishState.public) + self.assertTrue(self.store.has_published_version(chapter)) @ddt.data('draft', 'split') def test_get_courses_for_wiki_shared(self, default_ms): diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index 1939294d6a..2023dbd941 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -23,7 +23,7 @@ from xmodule.modulestore.inheritance import InheritanceMixin, own_metadata from opaque_keys.edx.locations import SlashSeparatedCourseKey from xmodule.mako_module import MakoDescriptorSystem from xmodule.error_module import ErrorDescriptor -from xmodule.modulestore import PublishState, ModuleStoreEnum +from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.mongo.draft import DraftModuleStore from xmodule.modulestore.draft_and_published import DIRECT_ONLY_CATEGORIES