Merge pull request #9911 from cpennington/fix-plat-858

Delete DIRECT_ONLY_CATEGORIES from both draft and published branches by default

cms/djangoapps/contentstore/tests/test_courseware_index.py

@@ -29,8 +29,10 @@ from xmodule.modulestore.tests.django_utils import (
 )
 from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, LibraryFactory
 from xmodule.modulestore.tests.mongo_connection import MONGO_PORT_NUM, MONGO_HOST
-from xmodule.modulestore.tests.test_cross_modulestore_import_export import MongoContentstoreBuilder
-from xmodule.modulestore.tests.utils import create_modulestore_instance, LocationMixin, MixedSplitTestCase
+from xmodule.modulestore.tests.utils import (
+    create_modulestore_instance, LocationMixin,
+    MixedSplitTestCase, MongoContentstoreBuilder
+)
 from xmodule.tests import DATA_DIR
 from xmodule.x_module import XModuleMixin
 from xmodule.partitions.partitions import UserPartition

common/lib/xmodule/xmodule/modulestore/__init__.py

@@ -315,7 +315,8 @@ class BulkOperationsMixin(object):
         Sends out the signal that items have been published from within this course.
         """
         if self.signal_handler and bulk_ops_record.has_publish_item:
-            self.signal_handler.send("course_published", course_key=course_id)
+            # We remove the branch, because publishing always means copying from draft to published
+            self.signal_handler.send("course_published", course_key=course_id.for_branch(None))
             bulk_ops_record.has_publish_item = False
 
     def send_bulk_library_updated_signal(self, bulk_ops_record, library_id):
@@ -1345,22 +1346,6 @@ class ModuleStoreWriteBase(ModuleStoreReadBase, ModuleStoreWrite):
         parent.children.append(item.location)
         self.update_item(parent, user_id)
 
-    def _flag_publish_event(self, course_key):
-        """
-        Wrapper around calls to fire the course_published signal
-        Unless we're nested in an active bulk operation, this simply fires the signal
-        otherwise a publish will be signalled at the end of the bulk operation
-
-        Arguments:
-            course_key - course_key to which the signal applies
-        """
-        if self.signal_handler:
-            bulk_record = self._get_bulk_ops_record(course_key) if isinstance(self, BulkOperationsMixin) else None
-            if bulk_record and bulk_record.active:
-                bulk_record.has_publish_item = True
-            else:
-                self.signal_handler.send("course_published", course_key=course_key)
-
     def _flag_library_updated_event(self, library_key):
         """
         Wrapper around calls to fire the library_updated signal

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

@@ -5,7 +5,7 @@ This module provides an abstraction for Module Stores that support Draft and Pub
 import threading
 from abc import ABCMeta, abstractmethod
 from contextlib import contextmanager
-from . import ModuleStoreEnum
+from . import ModuleStoreEnum, BulkOperationsMixin
 
 # Things w/ these categories should never be marked as version=DRAFT
 DIRECT_ONLY_CATEGORIES = ['course', 'chapter', 'sequential', 'about', 'static_tab', 'course_info']
@@ -62,7 +62,7 @@ class BranchSettingMixin(object):
             return self.default_branch_setting_func()
 
 
-class ModuleStoreDraftAndPublished(BranchSettingMixin):
+class ModuleStoreDraftAndPublished(BranchSettingMixin, BulkOperationsMixin):
     """
     A mixin for a read-write database backend that supports two branches, Draft and Published, with
     options to prefer Draft and fallback to Published.
@@ -87,6 +87,11 @@ class ModuleStoreDraftAndPublished(BranchSettingMixin):
 
     @abstractmethod
     def unpublish(self, location, user_id):
+        """
+        Turn the published version into a draft, removing the published version.
+
+        Raises: InvalidVersionError if called on a DIRECT_ONLY_CATEGORY
+        """
         raise NotImplementedError
 
     @abstractmethod
@@ -112,6 +117,23 @@ class ModuleStoreDraftAndPublished(BranchSettingMixin):
         """
         raise NotImplementedError
 
+    def _flag_publish_event(self, course_key):
+        """
+        Wrapper around calls to fire the course_published signal
+        Unless we're nested in an active bulk operation, this simply fires the signal
+        otherwise a publish will be signalled at the end of the bulk operation
+
+        Arguments:
+            course_key - course_key to which the signal applies
+        """
+        if self.signal_handler:
+            bulk_record = self._get_bulk_ops_record(course_key) if isinstance(self, BulkOperationsMixin) else None
+            if bulk_record and bulk_record.active:
+                bulk_record.has_publish_item = True
+            else:
+                # We remove the branch, because publishing always means copying from draft to published
+                self.signal_handler.send("course_published", course_key=course_key.for_branch(None))
+
 
 class UnsupportedRevisionError(ValueError):
     """

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

@@ -754,6 +754,10 @@ class DraftModuleStore(MongoModuleStore):
         NOTE: unlike publish, this gives an error if called above the draftable level as it's intended
         to remove things from the published version
         """
+        # ensure we are not creating a DRAFT of an item that is direct-only
+        if location.category in DIRECT_ONLY_CATEGORIES:
+            raise InvalidVersionError(location)
+
         self._verify_branch_setting(ModuleStoreEnum.Branch.draft_preferred)
         self._convert_to_draft(location, user_id, delete_published=True)
 

common/lib/xmodule/xmodule/modulestore/perf_tests/test_asset_import_export.py

@@ -16,7 +16,7 @@ from xmodule.assetstore import AssetMetadata
 from xmodule.modulestore import ModuleStoreEnum
 from xmodule.modulestore.xml_importer import import_course_from_xml
 from xmodule.modulestore.xml_exporter import export_course_to_xml
-from xmodule.modulestore.tests.test_cross_modulestore_import_export import (
+from xmodule.modulestore.tests.utils import (
     MODULESTORE_SETUPS,
     SHORT_NAME_MAP,
     TEST_DATA_DIR,

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

@@ -13,7 +13,13 @@ from time import time
 
 # Import this just to export it
 from pymongo.errors import DuplicateKeyError  # pylint: disable=unused-import
-from django.core.cache import get_cache, InvalidCacheBackendError
+
+try:
+    from django.core.cache import get_cache, InvalidCacheBackendError
+    DJANGO_AVAILABLE = True
+except ImportError:
+    DJANGO_AVAILABLE = False
+
 import dogstats_wrapper as dog_stats_api
 
 from contracts import check, new_contract
@@ -216,15 +222,16 @@ class CourseStructureCache(object):
     for set and get.
     """
     def __init__(self):
-        self.no_cache_found = False
-        try:
-            self.cache = get_cache('course_structure_cache')
-        except InvalidCacheBackendError:
-            self.no_cache_found = True
+        self.cache = None
+        if DJANGO_AVAILABLE:
+            try:
+                self.cache = get_cache('course_structure_cache')
+            except InvalidCacheBackendError:
+                pass
 
     def get(self, key, course_context=None):
         """Pull the compressed, pickled struct data from cache and deserialize."""
-        if self.no_cache_found:
+        if self.cache is None:
             return None
 
         with TIMER.timer("CourseStructureCache.get", course_context) as tagger:
@@ -245,7 +252,7 @@ class CourseStructureCache(object):
 
     def set(self, key, structure, course_context=None):
         """Given a structure, will pickle, compress, and write to cache."""
-        if self.no_cache_found:
+        if self.cache is None:
             return None
 
         with TIMER.timer("CourseStructureCache.set", course_context) as tagger:

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

@@ -2383,7 +2383,10 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
             if original_structure['root'] == block_key:
                 raise ValueError("Cannot delete the root of a course")
             if block_key not in original_structure['blocks']:
-                raise ValueError("Cannot delete a block that does not exist")
+                raise ValueError("Cannot delete block_key {} from course {}, because that block does not exist.".format(
+                    block_key,
+                    usage_locator,
+                ))
             index_entry = self._get_index_if_valid(usage_locator.course_key, force)
             new_structure = self.version_structure(usage_locator.course_key, original_structure, user_id)
             new_blocks = new_structure['blocks']
@@ -3034,9 +3037,9 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         Delete the orphan and any of its descendants which no longer have parents.
         """
         if len(self._get_parents_from_structure(orphan, structure)) == 0:
-            for child in structure['blocks'][orphan].fields.get('children', []):
+            orphan_data = structure['blocks'].pop(orphan)
+            for child in orphan_data.fields.get('children', []):
                 self._delete_if_true_orphan(BlockKey(*child), structure)
-            del structure['blocks'][orphan]
 
     @contract(returns=BlockData)
     def _new_block(self, user_id, category, block_fields, definition_id, new_id, raw=False, block_defaults=None):

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

@@ -189,39 +189,41 @@ class DraftVersioningModuleStore(SplitMongoModuleStore, ModuleStoreDraftAndPubli
                     currently only provided by contentstore.views.item.orphan_handler
                 Otherwise, raises a ValueError.
         """
+        allowed_revisions = [
+            None,
+            ModuleStoreEnum.RevisionOption.published_only,
+            ModuleStoreEnum.RevisionOption.all
+        ]
+        if revision not in allowed_revisions:
+            raise UnsupportedRevisionError(allowed_revisions)
+
+        autopublish_parent = False
         with self.bulk_operations(location.course_key):
             if isinstance(location, LibraryUsageLocator):
                 branches_to_delete = [ModuleStoreEnum.BranchName.library]  # Libraries don't yet have draft/publish support
-            elif revision == ModuleStoreEnum.RevisionOption.published_only:
-                branches_to_delete = [ModuleStoreEnum.BranchName.published]
+            elif location.category in DIRECT_ONLY_CATEGORIES:
+                branches_to_delete = [ModuleStoreEnum.BranchName.published, ModuleStoreEnum.BranchName.draft]
             elif revision == ModuleStoreEnum.RevisionOption.all:
                 branches_to_delete = [ModuleStoreEnum.BranchName.published, ModuleStoreEnum.BranchName.draft]
-            elif revision is None:
-                branches_to_delete = [ModuleStoreEnum.BranchName.draft]
             else:
-                raise UnsupportedRevisionError(
-                    [
-                        None,
-                        ModuleStoreEnum.RevisionOption.published_only,
-                        ModuleStoreEnum.RevisionOption.all
-                    ]
-                )
+                if revision == ModuleStoreEnum.RevisionOption.published_only:
+                    branches_to_delete = [ModuleStoreEnum.BranchName.published]
+                elif revision is None:
+                    branches_to_delete = [ModuleStoreEnum.BranchName.draft]
+                    parent_loc = self.get_parent_location(location.for_branch(ModuleStoreEnum.BranchName.draft))
+                    autopublish_parent = (
+                        not skip_auto_publish and
+                        parent_loc is not None and
+                        parent_loc.block_type in DIRECT_ONLY_CATEGORIES
+                    )
 
             self._flag_publish_event(location.course_key)
             for branch in branches_to_delete:
                 branched_location = location.for_branch(branch)
-                parent_loc = self.get_parent_location(branched_location)
-                SplitMongoModuleStore.delete_item(self, branched_location, user_id)
-                # publish parent w/o child if deleted element is direct only (not based on type of parent)
-                # publish vertical to behave more like the old mongo/draft modulestore - TNL-2593
-                if (
-                        branch == ModuleStoreEnum.BranchName.draft and
-                        branched_location.block_type in (DIRECT_ONLY_CATEGORIES + ['vertical']) and
-                        parent_loc and
-                        not skip_auto_publish
-                ):
-                    # will publish if its not an orphan
-                    self.publish(parent_loc.version_agnostic(), user_id, blacklist=EXCLUDE_ALL, **kwargs)
+                super(DraftVersioningModuleStore, self).delete_item(branched_location, user_id)
+
+            if autopublish_parent:
+                self.publish(parent_loc.version_agnostic(), user_id, blacklist=EXCLUDE_ALL, **kwargs)
 
     def _map_revision_to_branch(self, key, revision=None):
         """
@@ -375,6 +377,9 @@ class DraftVersioningModuleStore(SplitMongoModuleStore, ModuleStoreDraftAndPubli
         Deletes the published version of the item.
         Returns the newly unpublished item.
         """
+        if location.block_type in DIRECT_ONLY_CATEGORIES:
+            raise InvalidVersionError(location)
+
         with self.bulk_operations(location.course_key):
             self.delete_item(location, user_id, revision=ModuleStoreEnum.RevisionOption.published_only)
             return self.get_item(location.for_branch(ModuleStoreEnum.BranchName.draft), **kwargs)

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

@@ -5,7 +5,7 @@ from xblock.core import XBlockAside
 from xblock.fields import Scope, String
 from xblock.fragment import Fragment
 from unittest import TestCase
-from xmodule.modulestore.tests.test_cross_modulestore_import_export import XmlModulestoreBuilder
+from xmodule.modulestore.tests.utils import XmlModulestoreBuilder
 from mock import patch
 
 

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

@@ -14,7 +14,7 @@ from xmodule.assetstore import AssetMetadata
 from xmodule.modulestore import ModuleStoreEnum, SortedAssetList, IncorrectlySortedList
 from xmodule.modulestore.exceptions import ItemNotFoundError
 from xmodule.modulestore.tests.factories import CourseFactory
-from xmodule.modulestore.tests.test_cross_modulestore_import_export import (
+from xmodule.modulestore.tests.utils import (
     MIXED_MODULESTORE_BOTH_SETUP, MODULESTORE_SETUPS,
     XmlModulestoreBuilder, MixedModulestoreBuilder
 )

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

@@ -11,7 +11,7 @@ import ddt
 from xmodule.modulestore.xml_importer import import_course_from_xml
 from xmodule.modulestore.xml_exporter import export_course_to_xml
 from xmodule.modulestore.tests.factories import check_mongo_calls
-from xmodule.modulestore.tests.test_cross_modulestore_import_export import (
+from xmodule.modulestore.tests.utils import (
     MixedModulestoreBuilder, VersioningModulestoreBuilder,
     MongoModulestoreBuilder, TEST_DATA_DIR
 )

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

@@ -19,7 +19,7 @@ from xmodule.modulestore.exceptions import ItemNotFoundError
 from xmodule.modulestore.xml_exporter import export_course_to_xml
 from xmodule.modulestore.tests.test_split_w_old_mongo import SplitWMongoCourseBootstrapper
 from xmodule.modulestore.tests.factories import check_mongo_calls, mongo_uses_error_check, CourseFactory, ItemFactory
-from xmodule.modulestore.tests.test_cross_modulestore_import_export import (
+from xmodule.modulestore.tests.utils import (
     MongoContentstoreBuilder, MODULESTORE_SETUPS,
     DRAFT_MODULESTORE_SETUP, SPLIT_MODULESTORE_SETUP, MongoModulestoreBuilder,
 )
@@ -925,49 +925,16 @@ class ElementalUnpublishingTests(DraftPublishedOpBaseTestSetup):
             self.assertOLXIsDraftOnly(block_list_unpublished_children)
             self.assertOLXIsDraftOnly(block_list_untouched)
 
-    @ddt.data(DRAFT_MODULESTORE_SETUP, MongoModulestoreBuilder())
-    def test_unpublish_old_mongo_draft_sequential(self, modulestore_builder):
+    @ddt.data(SPLIT_MODULESTORE_SETUP, DRAFT_MODULESTORE_SETUP, MongoModulestoreBuilder())
+    def test_unpublish_draft_sequential(self, modulestore_builder):
         with self._setup_test(modulestore_builder):
 
-            # MODULESTORE_DIFFERENCE:
-            # In old Mongo, you cannot successfully unpublish an autopublished sequential.
-            # An exception is thrown.
             block_list_to_unpublish = (
                 ('sequential', 'sequential03'),
             )
             with self.assertRaises(InvalidVersionError):
                 self.unpublish(block_list_to_unpublish)
 
-    @ddt.data(SPLIT_MODULESTORE_SETUP)
-    def test_unpublish_split_draft_sequential(self, modulestore_builder):
-        with self._setup_test(modulestore_builder):
-
-            # MODULESTORE_DIFFERENCE:
-            # In Split, the sequential is deleted.
-            # The sequential's children are orphaned - but they stay in
-            # the same draft state they were before.
-            block_list_to_unpublish = (
-                ('sequential', 'sequential03'),
-            )
-            block_list_unpublished_children = (
-                ('vertical', 'vertical06'),
-                ('vertical', 'vertical07'),
-                ('html', 'html12'),
-                ('html', 'html13'),
-                ('html', 'html14'),
-                ('html', 'html15'),
-            )
-            # The autopublished sequential is published - its children are draft.
-            self.assertOLXIsPublishedOnly(block_list_to_unpublish)
-            self.assertOLXIsDraftOnly(block_list_unpublished_children)
-            # Unpublish the sequential.
-            self.unpublish(block_list_to_unpublish)
-            # Since the sequential was autopublished, a draft version of the sequential never existed.
-            # So unpublishing the sequential doesn't make it a draft - it deletes it!
-            self.assertOLXIsDeleted(block_list_to_unpublish)
-            # Its children are orphaned and remain as drafts.
-            self.assertOLXIsDraftOnly(block_list_unpublished_children)
-
 
 @ddt.ddt
 class ElementalDeleteItemTests(DraftPublishedOpBaseTestSetup):
@@ -1122,20 +1089,7 @@ class ElementalDeleteItemTests(DraftPublishedOpBaseTestSetup):
             self.assertOLXIsPublishedOnly(block_list_to_delete)
             self.delete_item(block_list_to_delete, revision=revision)
             self._check_for_item_deletion(block_list_to_delete, result)
-            # MODULESTORE_DIFFERENCE
-            if self.is_split_modulestore:
-                # Split:
-                if revision == ModuleStoreEnum.RevisionOption.published_only:
-                    # If deleting published_only items, the children that are drafts remain.
-                    self.assertOLXIsDraftOnly(block_list_children)
-                else:
-                    self.assertOLXIsDeleted(block_list_children)
-            elif self.is_old_mongo_modulestore:
-                # Old Mongo:
-                # If deleting draft_only or both items, the drafts will be deleted.
-                self.assertOLXIsDeleted(block_list_children)
-            else:
-                raise Exception("Must test either Old Mongo or Split modulestore!")
+            self.assertOLXIsDeleted(block_list_children)
 
     @ddt.data(*itertools.product(
         MODULESTORE_SETUPS,
@@ -1176,20 +1130,7 @@ class ElementalDeleteItemTests(DraftPublishedOpBaseTestSetup):
             self.delete_item(block_list_to_delete, revision=revision)
             self._check_for_item_deletion(block_list_to_delete, result)
             self.assertOLXIsDeleted(autopublished_children)
-            # MODULESTORE_DIFFERENCE
-            if self.is_split_modulestore:
-                # Split:
-                if revision == ModuleStoreEnum.RevisionOption.published_only:
-                    # If deleting published_only items, the children that are drafts remain.
-                    self.assertOLXIsDraftOnly(block_list_draft_children)
-                else:
-                    self.assertOLXIsDeleted(block_list_draft_children)
-            elif self.is_old_mongo_modulestore:
-                # Old Mongo:
-                # If deleting draft_only or both items, the drafts will be deleted.
-                self.assertOLXIsDeleted(block_list_draft_children)
-            else:
-                raise Exception("Must test either Old Mongo or Split modulestore!")
+            self.assertOLXIsDeleted(block_list_draft_children)
 
 
 @ddt.ddt

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

@@ -13,7 +13,7 @@ from xmodule.modulestore.inheritance import InheritanceMixin
 from xmodule.modulestore.mongo import DraftMongoModuleStore
 from xmodule.modulestore.split_mongo.split import SplitMongoModuleStore
 from xmodule.modulestore.tests.mongo_connection import MONGO_PORT_NUM, MONGO_HOST
-from xmodule.modulestore.tests.test_cross_modulestore_import_export import MemoryCache
+from xmodule.modulestore.tests.utils import MemoryCache
 
 
 @attr('mongo')

common/lib/xmodule/xmodule/tests/test_course_metadata_utils.py

@@ -22,7 +22,7 @@ from xmodule.course_metadata_utils import (
     may_certify_for_course,
 )
 from xmodule.fields import Date
-from xmodule.modulestore.tests.test_cross_modulestore_import_export import (
+from xmodule.modulestore.tests.utils import (
     MongoModulestoreBuilder,
     VersioningModulestoreBuilder,
     MixedModulestoreBuilder

lms/djangoapps/ccx/tests/test_ccx_modulestore.py

@@ -45,7 +45,7 @@ class TestCCXModulestoreWrapper(SharedModuleStoreTestCase):
             ) for _ in xrange(2) for s in sequentials
         ]
         cls.blocks = [
-            ItemFactory.create(parent=v) for _ in xrange(2) for v in verticals
+            ItemFactory.create(parent=v, category='html') for _ in xrange(2) for v in verticals
         ]
 
     def setUp(self):