Merge pull request #6935 from edx/clintonb/course-structure

Generating Course Structure Asynchronously on Course Publish

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

@@ -170,7 +170,7 @@ class BulkOperationsMixin(object):
         self._active_bulk_ops = ActiveBulkThread(self._bulk_ops_record_type)
 
     @contextmanager
-    def bulk_operations(self, course_id):
+    def bulk_operations(self, course_id, emit_signals=True):
         """
         A context manager for notifying the store of bulk operations. This affects only the current thread.
 
@@ -181,7 +181,7 @@ class BulkOperationsMixin(object):
             self._begin_bulk_operation(course_id)
             yield
         finally:
-            self._end_bulk_operation(course_id)
+            self._end_bulk_operation(course_id, emit_signals)
 
     # the relevant type of bulk_ops_record for the mixin (overriding classes should override
     # this variable)
@@ -197,12 +197,14 @@ class BulkOperationsMixin(object):
         # Retrieve the bulk record based on matching org/course/run (possibly ignoring case)
         if ignore_case:
             for key, record in self._active_bulk_ops.records.iteritems():
-                if (
+                # Shortcut: check basic equivalence for cases where org/course/run might be None.
+                if key == course_key or (
                     key.org.lower() == course_key.org.lower() and
                     key.course.lower() == course_key.course.lower() and
                     key.run.lower() == course_key.run.lower()
                 ):
                     return record
+
         return self._active_bulk_ops.records[course_key.for_branch(None)]
 
     @property
@@ -242,7 +244,7 @@ class BulkOperationsMixin(object):
         if bulk_ops_record.is_root:
             self._start_outermost_bulk_operation(bulk_ops_record, course_key)
 
-    def _end_outermost_bulk_operation(self, bulk_ops_record, course_key):
+    def _end_outermost_bulk_operation(self, bulk_ops_record, course_key, emit_signals=True):
         """
         The outermost nested bulk_operation call: do the actual end of the bulk operation.
 
@@ -250,7 +252,7 @@ class BulkOperationsMixin(object):
         """
         pass
 
-    def _end_bulk_operation(self, course_key):
+    def _end_bulk_operation(self, course_key, emit_signals=True):
         """
         End the active bulk operation on course_key.
         """
@@ -266,7 +268,7 @@ class BulkOperationsMixin(object):
         if bulk_ops_record.active:
             return
 
-        self._end_outermost_bulk_operation(bulk_ops_record, course_key)
+        self._end_outermost_bulk_operation(bulk_ops_record, course_key, emit_signals)
 
         self._clear_bulk_ops_record(course_key)
 
@@ -900,7 +902,7 @@ class ModuleStoreRead(ModuleStoreAssetBase):
         pass
 
     @contextmanager
-    def bulk_operations(self, course_id):
+    def bulk_operations(self, course_id, emit_signals=True):    # pylint: disable=unused-argument
         """
         A context manager for notifying the store of bulk operations. This affects only the current thread.
         """
@@ -1242,10 +1244,11 @@ class ModuleStoreWriteBase(ModuleStoreReadBase, ModuleStoreWrite):
         This base method just copies the assets. The lower level impls must do the actual cloning of
         content.
         """
-        # copy the assets
-        if self.contentstore:
-            self.contentstore.copy_all_course_assets(source_course_id, dest_course_id)
-        return dest_course_id
+        with self.bulk_operations(dest_course_id):
+            # copy the assets
+            if self.contentstore:
+                self.contentstore.copy_all_course_assets(source_course_id, dest_course_id)
+            return dest_course_id
 
     def delete_course(self, course_key, user_id, **kwargs):
         """

common/lib/xmodule/xmodule/modulestore/django.py

@@ -7,24 +7,30 @@ Passes settings.MODULESTORE as kwargs to MongoModuleStore
 from __future__ import absolute_import
 
 from importlib import import_module
-from django.conf import settings
-if not settings.configured:
-    settings.configure()
-from django.core.cache import get_cache, InvalidCacheBackendError
-import django.utils
+import logging
 
 import re
+from django.conf import settings
 
-from xmodule.util.django import get_current_request_hostname
-import xmodule.modulestore  # pylint: disable=unused-import
-from xmodule.modulestore.mixed import MixedModuleStore
-from xmodule.modulestore.draft_and_published import BranchSettingMixin
+# This configuration must be executed BEFORE any additional Django imports. Otherwise, the imports may fail due to
+# Django not being configured properly. This mostly applies to tests.
+if not settings.configured:
+    settings.configure()
+
+from django.core.cache import get_cache, InvalidCacheBackendError
+import django.dispatch
+import django.utils
 from xmodule.contentstore.django import contentstore
+from xmodule.modulestore.draft_and_published import BranchSettingMixin
+from xmodule.modulestore.mixed import MixedModuleStore
+from xmodule.util.django import get_current_request_hostname
 import xblock.reference.plugins
 
-# We may not always have the request_cache module available
+
 try:
+    # We may not always have the request_cache module available
     from request_cache.middleware import RequestCache
+
     HAS_REQUEST_CACHE = True
 except ImportError:
     HAS_REQUEST_CACHE = False
@@ -33,13 +39,66 @@ except ImportError:
 try:
     from xblock_django.user_service import DjangoXBlockUserService
     from crum import get_current_user
+
     HAS_USER_SERVICE = True
 except ImportError:
     HAS_USER_SERVICE = False
 
+log = logging.getLogger(__name__)
 ASSET_IGNORE_REGEX = getattr(settings, "ASSET_IGNORE_REGEX", r"(^\._.*$)|(^\.DS_Store$)|(^.*~$)")
 
 
+class SignalHandler(object):
+    """
+    This class is to allow the modulestores to emit signals that can be caught
+    by other parts of the Django application. If your app needs to do something
+    every time a course is published (e.g. search indexing), you can listen for
+    that event and kick off a celery task when it happens.
+
+    To listen for a signal, do the following::
+
+        from django.dispatch import receiver
+        from celery.task import task
+        from xmodule.modulestore.django import modulestore, SignalHandler
+
+        @receiver(SignalHandler.course_published)
+        def listen_for_course_publish(sender, course_key, **kwargs):
+            do_my_expensive_update.delay(course_key)
+
+        @task()
+        def do_my_expensive_update(course_key):
+            # ...
+
+    Things to note:
+
+    1. We receive using the Django Signals mechanism.
+    2. The sender is going to be the class of the modulestore sending it.
+    3. Always have **kwargs in your signal handler, as new things may be added.
+    4. The thing that listens for the signal lives in process, but should do
+       almost no work. Its main job is to kick off the celery task that will
+       do the actual work.
+
+    """
+    course_published = django.dispatch.Signal(providing_args=["course_key"])
+
+    _mapping = {
+        "course_published": course_published
+    }
+
+    def __init__(self, modulestore_class):
+        self.modulestore_class = modulestore_class
+
+    def send(self, signal_name, **kwargs):
+        """
+        Send the signal to the receivers.
+        """
+        signal = self._mapping[signal_name]
+        responses = signal.send_robust(sender=self.modulestore_class, **kwargs)
+
+        for receiver, response in responses:
+            log.info('Sent %s signal to %s with kwargs %s. Response was: %s', signal_name, receiver, kwargs, response)
+
+
 def load_function(path):
     """
     Load a function by name.
@@ -59,6 +118,7 @@ def create_modulestore_instance(
         i18n_service=None,
         fs_service=None,
         user_service=None,
+        signal_handler=None,
 ):
     """
     This will return a new instance of a modulestore given an engine and options
@@ -104,6 +164,7 @@ def create_modulestore_instance(
         i18n_service=i18n_service or ModuleI18nService(),
         fs_service=fs_service or xblock.reference.plugins.FSService(),
         user_service=user_service or xb_user_service,
+        signal_handler=signal_handler or SignalHandler(class_),
         **_options
     )
 
@@ -149,6 +210,7 @@ class ModuleI18nService(object):
     i18n service.
 
     """
+
     def __getattr__(self, name):
         return getattr(django.utils.translation, name)
 
@@ -166,6 +228,7 @@ class ModuleI18nService(object):
         # right there.  If you are reading this comment after April 1, 2014,
         # then Cale was a liar.
         from util.date_utils import strftime_localized
+
         return strftime_localized(*args, **kwargs)
 
 
@@ -177,6 +240,7 @@ def _get_modulestore_branch_setting():
 
     The value of the branch setting is cached in a thread-local variable so it is not repeatedly recomputed
     """
+
     def get_branch_setting():
         """
         Finds and returns the branch setting based on the Django request and the configuration settings

common/lib/xmodule/xmodule/modulestore/mixed.py

@@ -108,6 +108,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
             fs_service=None,
             user_service=None,
             create_modulestore_instance=None,
+            signal_handler=None,
             **kwargs
     ):
         """
@@ -142,6 +143,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
                     for course_key, store_key in self.mappings.iteritems()
                     if store_key == key
                 ]
+
             store = create_modulestore_instance(
                 store_settings['ENGINE'],
                 self.contentstore,
@@ -150,6 +152,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
                 i18n_service=i18n_service,
                 fs_service=fs_service,
                 user_service=user_service,
+                signal_handler=signal_handler,
             )
             # replace all named pointers to the store into actual pointers
             for course_key, store_name in self.mappings.iteritems():
@@ -634,6 +637,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
             * copy the assets
             * migrate the courseware
         """
+
         source_modulestore = self._get_modulestore_for_courselike(source_course_id)
         # for a temporary period of time, we may want to hardcode dest_modulestore as split if there's a split
         # to have only course re-runs go to split. This code, however, uses the config'd priority
@@ -643,9 +647,9 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
 
         if dest_modulestore.get_modulestore_type() == ModuleStoreEnum.Type.split:
             split_migrator = SplitMigrator(dest_modulestore, source_modulestore)
-            split_migrator.migrate_mongo_course(
-                source_course_id, user_id, dest_course_id.org, dest_course_id.course, dest_course_id.run, fields, **kwargs
-            )
+            split_migrator.migrate_mongo_course(source_course_id, user_id, dest_course_id.org,
+                                                dest_course_id.course, dest_course_id.run, fields, **kwargs)
+
             # the super handles assets and any other necessities
             super(MixedModuleStore, self).clone_course(source_course_id, dest_course_id, user_id, fields, **kwargs)
         else:
@@ -915,13 +919,13 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
             yield
 
     @contextmanager
-    def bulk_operations(self, course_id):
+    def bulk_operations(self, course_id, emit_signals=True):
         """
         A context manager for notifying the store of bulk operations.
         If course_id is None, the default store is used.
         """
         store = self._get_modulestore_for_courselike(course_id)
-        with store.bulk_operations(course_id):
+        with store.bulk_operations(course_id, emit_signals):
             yield
 
     def ensure_indexes(self):

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

@@ -448,13 +448,17 @@ class MongoBulkOpsMixin(BulkOperationsMixin):
         # ensure it starts clean
         bulk_ops_record.dirty = False
 
-    def _end_outermost_bulk_operation(self, bulk_ops_record, course_id):
+    def _end_outermost_bulk_operation(self, bulk_ops_record, course_id, emit_signals=True):
         """
         Restart updating the meta-data inheritance cache for the given course.
         Refresh the meta-data inheritance cache now since it was temporarily disabled.
         """
         if bulk_ops_record.dirty:
             self.refresh_cached_metadata_inheritance_tree(course_id)
+
+            if emit_signals and self.signal_handler:
+                self.signal_handler.send("course_published", course_key=course_id)
+
             bulk_ops_record.dirty = False  # brand spanking clean now
 
     def _is_in_bulk_operation(self, course_id, ignore_case=False):
@@ -504,6 +508,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo
                  i18n_service=None,
                  fs_service=None,
                  user_service=None,
+                 signal_handler=None,
                  retry_wait_time=0.1,
                  **kwargs):
         """
@@ -560,6 +565,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo
         self.user_service = user_service
 
         self._course_run_cache = {}
+        self.signal_handler = signal_handler
 
     def close_connections(self):
         """
@@ -1117,14 +1123,15 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo
         if courses.count() > 0:
             raise DuplicateCourseError(course_id, courses[0]['_id'])
 
-        xblock = self.create_item(user_id, course_id, 'course', course_id.run, fields=fields, **kwargs)
+        with self.bulk_operations(course_id):
+            xblock = self.create_item(user_id, course_id, 'course', course_id.run, fields=fields, **kwargs)
 
-        # create any other necessary things as a side effect
-        super(MongoModuleStore, self).create_course(
-            org, course, run, user_id, runtime=xblock.runtime, **kwargs
-        )
+            # create any other necessary things as a side effect
+            super(MongoModuleStore, self).create_course(
+                org, course, run, user_id, runtime=xblock.runtime, **kwargs
+            )
 
-        return xblock
+            return xblock
 
     def create_xblock(
         self, runtime, course_key, block_type, block_id=None, fields=None,
@@ -1305,6 +1312,8 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo
         is_publish_root: when publishing, this indicates whether xblock is the root of the publish and should
           therefore propagate subtree edit info up the tree
         """
+        course_key = xblock.location.course_key
+
         try:
             definition_data = self._serialize_scope(xblock, Scope.content)
             now = datetime.now(UTC)
@@ -1356,8 +1365,8 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo
         except ItemNotFoundError:
             if not allow_not_found:
                 raise
-            elif not self.has_course(xblock.location.course_key):
-                raise ItemNotFoundError(xblock.location.course_key)
+            elif not self.has_course(course_key):
+                raise ItemNotFoundError(course_key)
 
         return xblock
 

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

@@ -167,44 +167,46 @@ class DraftModuleStore(MongoModuleStore):
         if not self.has_course(source_course_id):
             raise ItemNotFoundError("Cannot find a course at {0}. Aborting".format(source_course_id))
 
-        # verify that the dest_location really is an empty course
-        # b/c we don't want the payload, I'm copying the guts of get_items here
-        query = self._course_key_to_son(dest_course_id)
-        query['_id.category'] = {'$nin': ['course', 'about']}
-        if self.collection.find(query).limit(1).count() > 0:
-            raise DuplicateCourseError(
-                dest_course_id,
-                "Course at destination {0} is not an empty course. You can only clone into an empty course. Aborting...".format(
-                    dest_course_id
+        with self.bulk_operations(dest_course_id):
+            # verify that the dest_location really is an empty course
+            # b/c we don't want the payload, I'm copying the guts of get_items here
+            query = self._course_key_to_son(dest_course_id)
+            query['_id.category'] = {'$nin': ['course', 'about']}
+            if self.collection.find(query).limit(1).count() > 0:
+                raise DuplicateCourseError(
+                    dest_course_id,
+                    "Course at destination {0} is not an empty course. "
+                    "You can only clone into an empty course. Aborting...".format(
+                        dest_course_id
+                    )
                 )
-            )
 
-        # clone the assets
-        super(DraftModuleStore, self).clone_course(source_course_id, dest_course_id, user_id, fields)
+            # clone the assets
+            super(DraftModuleStore, self).clone_course(source_course_id, dest_course_id, user_id, fields)
 
-        # get the whole old course
-        new_course = self.get_course(dest_course_id)
-        if new_course is None:
-            # create_course creates the about overview
-            new_course = self.create_course(
-                dest_course_id.org, dest_course_id.course, dest_course_id.run, user_id, fields=fields
-            )
-        else:
-            # update fields on existing course
-            for key, value in fields.iteritems():
-                setattr(new_course, key, value)
-            self.update_item(new_course, user_id)
+            # get the whole old course
+            new_course = self.get_course(dest_course_id)
+            if new_course is None:
+                # create_course creates the about overview
+                new_course = self.create_course(
+                    dest_course_id.org, dest_course_id.course, dest_course_id.run, user_id, fields=fields
+                )
+            else:
+                # update fields on existing course
+                for key, value in fields.iteritems():
+                    setattr(new_course, key, value)
+                self.update_item(new_course, user_id)
 
-        # Get all modules under this namespace which is (tag, org, course) tuple
-        modules = self.get_items(source_course_id, revision=ModuleStoreEnum.RevisionOption.published_only)
-        self._clone_modules(modules, dest_course_id, user_id)
-        course_location = dest_course_id.make_usage_key('course', dest_course_id.run)
-        self.publish(course_location, user_id)
+            # Get all modules under this namespace which is (tag, org, course) tuple
+            modules = self.get_items(source_course_id, revision=ModuleStoreEnum.RevisionOption.published_only)
+            self._clone_modules(modules, dest_course_id, user_id)
+            course_location = dest_course_id.make_usage_key('course', dest_course_id.run)
+            self.publish(course_location, user_id)
 
-        modules = self.get_items(source_course_id, revision=ModuleStoreEnum.RevisionOption.draft_only)
-        self._clone_modules(modules, dest_course_id, user_id)
+            modules = self.get_items(source_course_id, revision=ModuleStoreEnum.RevisionOption.draft_only)
+            self._clone_modules(modules, dest_course_id, user_id)
 
-        return True
+            return True
 
     def _clone_modules(self, modules, dest_course_id, user_id):
         """Clones each module into the given course"""
@@ -447,7 +449,12 @@ class DraftModuleStore(MongoModuleStore):
 
         # if the revision is published, defer to base
         if draft_loc.revision == MongoRevisionKey.published:
-            return super(DraftModuleStore, self).update_item(xblock, user_id, allow_not_found)
+            item = super(DraftModuleStore, self).update_item(xblock, user_id, allow_not_found)
+            course_key = xblock.location.course_key
+            bulk_record = self._get_bulk_ops_record(course_key)
+            if self.signal_handler and not bulk_record.active:
+                self.signal_handler.send("course_published", course_key=course_key)
+            return item
 
         if not super(DraftModuleStore, self).has_item(draft_loc):
             try:
@@ -715,11 +722,15 @@ class DraftModuleStore(MongoModuleStore):
         _verify_revision_is_published(location)
 
         _internal_depth_first(location, True)
+        course_key = location.course_key
+        bulk_record = self._get_bulk_ops_record(course_key)
         if len(to_be_deleted) > 0:
-            bulk_record = self._get_bulk_ops_record(location.course_key)
             bulk_record.dirty = True
             self.collection.remove({'_id': {'$in': to_be_deleted}})
 
+        if self.signal_handler and not bulk_record.active:
+            self.signal_handler.send("course_published", course_key=course_key)
+
         # Now it's been published, add the object to the courseware search index so that it appears in search results
         CoursewareSearchIndexer.add_to_search_index(self, location)
 
@@ -735,6 +746,11 @@ class DraftModuleStore(MongoModuleStore):
         self._verify_branch_setting(ModuleStoreEnum.Branch.draft_preferred)
         self._convert_to_draft(location, user_id, delete_published=True)
 
+        course_key = location.course_key
+        bulk_record = self._get_bulk_ops_record(course_key)
+        if self.signal_handler and not bulk_record.active:
+            self.signal_handler.send("course_published", course_key=course_key)
+
     def revert_to_published(self, location, user_id=None):
         """
         Reverts an item to its last published version (recursively traversing all of its descendants).

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

@@ -229,12 +229,17 @@ class SplitBulkWriteMixin(BulkOperationsMixin):
         # Ensure that any edits to the index don't pollute the initial_index
         bulk_write_record.index = copy.deepcopy(bulk_write_record.initial_index)
 
-    def _end_outermost_bulk_operation(self, bulk_write_record, course_key):
+    def _end_outermost_bulk_operation(self, bulk_write_record, course_key, emit_signals=True):
         """
         End the active bulk write operation on course_key.
         """
+
+        dirty = False
+
         # If the content is dirty, then update the database
         for _id in bulk_write_record.structures.viewkeys() - bulk_write_record.structures_in_db:
+            dirty = True
+
             try:
                 self.db_connection.insert_structure(bulk_write_record.structures[_id])
             except DuplicateKeyError:
@@ -244,6 +249,8 @@ class SplitBulkWriteMixin(BulkOperationsMixin):
                 log.debug("Attempted to insert duplicate structure %s", _id)
 
         for _id in bulk_write_record.definitions.viewkeys() - bulk_write_record.definitions_in_db:
+            dirty = True
+
             try:
                 self.db_connection.insert_definition(bulk_write_record.definitions[_id])
             except DuplicateKeyError:
@@ -253,11 +260,18 @@ class SplitBulkWriteMixin(BulkOperationsMixin):
                 log.debug("Attempted to insert duplicate definition %s", _id)
 
         if bulk_write_record.index is not None and bulk_write_record.index != bulk_write_record.initial_index:
+            dirty = True
+
             if bulk_write_record.initial_index is None:
                 self.db_connection.insert_course_index(bulk_write_record.index)
             else:
                 self.db_connection.update_course_index(bulk_write_record.index, from_index=bulk_write_record.initial_index)
 
+        if dirty and emit_signals:
+            signal_handler = getattr(self, 'signal_handler', None)
+            if signal_handler:
+                signal_handler.send("course_published", course_key=course_key)
+
     def get_course_index(self, course_key, ignore_case=False):
         """
         Return the index for course_key.
@@ -613,7 +627,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
                  default_class=None,
                  error_tracker=null_error_tracker,
                  i18n_service=None, fs_service=None, user_service=None,
-                 services=None, **kwargs):
+                 services=None, signal_handler=None, **kwargs):
         """
         :param doc_store_config: must have a host, db, and collection entries. Other common entries: port, tz_aware.
         """
@@ -642,6 +656,8 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         if user_service is not None:
             self.services["user"] = user_service
 
+        self.signal_handler = signal_handler
+
     def close_connections(self):
         """
         Closes any open connections to the underlying databases
@@ -678,7 +694,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
             depth: how deep below these to prefetch
             lazy: whether to fetch definitions or use placeholders
         '''
-        with self.bulk_operations(course_key):
+        with self.bulk_operations(course_key, emit_signals=False):
             new_module_data = {}
             for block_id in base_block_ids:
                 new_module_data = self.descendants(
@@ -1572,18 +1588,20 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         source_index = self.get_course_index_info(source_course_id)
         if source_index is None:
             raise ItemNotFoundError("Cannot find a course at {0}. Aborting".format(source_course_id))
-        new_course = self.create_course(
-            dest_course_id.org, dest_course_id.course, dest_course_id.run,
-            user_id,
-            fields=fields,
-            versions_dict=source_index['versions'],
-            search_targets=source_index['search_targets'],
-            skip_auto_publish=True,
-            **kwargs
-        )
-        # don't copy assets until we create the course in case something's awry
-        super(SplitMongoModuleStore, self).clone_course(source_course_id, dest_course_id, user_id, fields, **kwargs)
-        return new_course
+
+        with self.bulk_operations(dest_course_id):
+            new_course = self.create_course(
+                dest_course_id.org, dest_course_id.course, dest_course_id.run,
+                user_id,
+                fields=fields,
+                versions_dict=source_index['versions'],
+                search_targets=source_index['search_targets'],
+                skip_auto_publish=True,
+                **kwargs
+            )
+            # don't copy assets until we create the course in case something's awry
+            super(SplitMongoModuleStore, self).clone_course(source_course_id, dest_course_id, user_id, fields, **kwargs)
+            return new_course
 
     DEFAULT_ROOT_BLOCK_ID = 'course'
 

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

@@ -118,7 +118,10 @@ class DraftVersioningModuleStore(SplitMongoModuleStore, ModuleStoreDraftAndPubli
     def update_item(self, descriptor, user_id, allow_not_found=False, force=False, **kwargs):
         old_descriptor_locn = descriptor.location
         descriptor.location = self._map_revision_to_branch(old_descriptor_locn)
-        with self.bulk_operations(descriptor.location.course_key):
+        emit_signals = descriptor.location.branch == ModuleStoreEnum.BranchName.published \
+            or descriptor.location.block_type in DIRECT_ONLY_CATEGORIES
+
+        with self.bulk_operations(descriptor.location.course_key, emit_signals=emit_signals):
             item = super(DraftVersioningModuleStore, self).update_item(
                 descriptor,
                 user_id,
@@ -139,7 +142,9 @@ class DraftVersioningModuleStore(SplitMongoModuleStore, ModuleStoreDraftAndPubli
         See :py:meth `ModuleStoreDraftAndPublished.create_item`
         """
         course_key = self._map_revision_to_branch(course_key)
-        with self.bulk_operations(course_key):
+        emit_signals = course_key.branch == ModuleStoreEnum.BranchName.published \
+            or block_type in DIRECT_ONLY_CATEGORIES
+        with self.bulk_operations(course_key, emit_signals=emit_signals):
             item = super(DraftVersioningModuleStore, self).create_item(
                 user_id, course_key, block_type, block_id=block_id,
                 definition_locator=definition_locator, fields=fields,

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

@@ -4,6 +4,7 @@ Unit tests for the Mixed Modulestore, with DDT for the various stores (Split, Dr
 """
 from collections import namedtuple
 import datetime
+import logging
 import ddt
 import itertools
 import mimetypes
@@ -13,6 +14,7 @@ from uuid import uuid4
 # before importing the module
 # TODO remove this import and the configuration -- xmodule should not depend on django!
 from django.conf import settings
+from mock_django import mock_signal_receiver
 from nose.plugins.attrib import attr
 import pymongo
 from pytz import UTC
@@ -23,7 +25,7 @@ from xmodule.modulestore.tests.test_cross_modulestore_import_export import Mongo
 from xmodule.contentstore.content import StaticContent
 from opaque_keys.edx.keys import CourseKey
 from xmodule.modulestore.xml_importer import import_from_xml
-from nose import SkipTest
+from xmodule.modulestore.django import SignalHandler
 
 if not settings.configured:
     settings.configure()
@@ -32,7 +34,7 @@ from opaque_keys.edx.locations import SlashSeparatedCourseKey
 from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator, LibraryLocator
 from xmodule.exceptions import InvalidVersionError
 from xmodule.modulestore import ModuleStoreEnum
-from xmodule.modulestore.draft_and_published import UnsupportedRevisionError
+from xmodule.modulestore.draft_and_published import UnsupportedRevisionError, DIRECT_ONLY_CATEGORIES
 from xmodule.modulestore.exceptions import ItemNotFoundError, DuplicateCourseError, ReferentialIntegrityError, NoPathToItem
 from xmodule.modulestore.mixed import MixedModuleStore
 from xmodule.modulestore.search import path_to_location
@@ -42,6 +44,8 @@ from xmodule.modulestore.tests.utils import create_modulestore_instance, Locatio
 from xmodule.modulestore.tests.mongo_connection import MONGO_PORT_NUM, MONGO_HOST
 from xmodule.tests import DATA_DIR, CourseComparisonTest
 
+log = logging.getLogger(__name__)
+
 
 @ddt.ddt
 @attr('mongo')
@@ -1960,134 +1964,74 @@ class TestMixedModuleStore(CourseComparisonTest):
             self.assertCoursesEqual(source_store, source_course_key, dest_store, dest_course_id)
 
     @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
-    def test_import_delete_import(self, default):
-        """
-        Test that deleting an element after import and then re-importing restores that element in draft
-        as well as published branches (PLAT_297)
-        """
-        # set the default modulestore
+    def test_course_publish_signal_firing(self, default):
         with MongoContentstoreBuilder().build() as contentstore:
             self.store = MixedModuleStore(
                 contentstore=contentstore,
                 create_modulestore_instance=create_modulestore_instance,
                 mappings={},
+                signal_handler=SignalHandler(MixedModuleStore),
                 **self.OPTIONS
             )
             self.addCleanup(self.store.close_all_connections)
+
             with self.store.default_store(default):
-                dest_course_key = self.store.make_course_key('a', 'course', 'course')
-                courses = import_from_xml(
-                    self.store, self.user_id, DATA_DIR, ['toy'], load_error_modules=False,
-                    static_content_store=contentstore,
-                    target_course_id=dest_course_key,
-                    create_course_if_not_present=True,
-                )
-                course_id = courses[0].id
-                # no need to verify course content here as test_cross_modulestore_import_export does that
-                # delete the vertical
-                vertical_loc = course_id.make_usage_key('vertical', 'vertical_test')
-                self.assertTrue(self.store.has_item(vertical_loc))
-                with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_id):
-                    self.store.delete_item(vertical_loc, self.user_id)
-                # verify it's in the published still
-                with self.store.branch_setting(ModuleStoreEnum.Branch.published_only, course_id):
-                    self.assertTrue(self.store.has_item(vertical_loc))
+                self.assertIsNotNone(self.store.thread_cache.default_store.signal_handler)
 
-                # now re-import
-                import_from_xml(
-                    self.store, self.user_id, DATA_DIR, ['toy'], load_error_modules=False,
-                    static_content_store=contentstore,
-                    target_course_id=dest_course_key,
-                )
-                # verify it's in both published and draft
-                with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_id):
-                    self.assertTrue(self.store.has_item(vertical_loc))
-                with self.store.branch_setting(ModuleStoreEnum.Branch.published_only, course_id):
-                    self.assertTrue(self.store.has_item(vertical_loc))
+                with mock_signal_receiver(SignalHandler.course_published) as receiver:
+                    self.assertEqual(receiver.call_count, 0)
 
-    @ddt.data(ModuleStoreEnum.Type.split)  # Need to fix and add ModuleStoreEnum.Type.mongo,
-    def test_delete_dag(self, default):
-        """
-        Test that deleting an element with more than one parent fully removes it from the course.
-        """
-        # set the default modulestore
-        with MongoContentstoreBuilder().build() as contentstore:
-            self.store = MixedModuleStore(
-                contentstore=contentstore,
-                create_modulestore_instance=create_modulestore_instance,
-                mappings={},
-                **self.OPTIONS
-            )
-            self.addCleanup(self.store.close_all_connections)
-            with self.store.default_store(default):
-                dest_course_key = self.store.make_course_key('a', 'course', 'course')
-                courses = import_from_xml(
-                    self.store, self.user_id, DATA_DIR, ['xml_dag'], load_error_modules=False,
-                    static_content_store=contentstore,
-                    target_course_id=dest_course_key,
-                    create_course_if_not_present=True,
-                )
-                course_id = courses[0].id
-                # ensure both parents point to the dag item
-                dag_item = course_id.make_usage_key('html', 'toyhtml')
-                one_parent = course_id.make_usage_key('vertical', 'vertical_test')
-                other_parent = course_id.make_usage_key('vertical', 'zeta')
-                with self.store.bulk_operations(course_id):
-                    # actually should test get_parent but it's not alphabetized yet
-                    self.assertEqual(self.store.get_parent_location(dag_item), one_parent)
-                    for parent_loc in [one_parent, other_parent]:
-                        parent = self.store.get_item(parent_loc)
-                        self.assertIn(dag_item, parent.children)
-                    # just testing draft branch assuming it doesn't matter which branch
-                    with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_id):
-                        self.store.delete_item(dag_item, self.user_id)
-                        for parent_loc in [one_parent, other_parent]:
-                            parent = self.store.get_item(parent_loc)
-                            self.assertNotIn(dag_item, parent.children)
+                    # Course creation and publication should fire the signal
+                    course = self.store.create_course('org_x', 'course_y', 'run_z', self.user_id)
+                    self.assertEqual(receiver.call_count, 1)
 
-    @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
-    def test_import_edit_import(self, default):
-        """
-        Test that editing an element after import and then re-importing resets the draft and pub'd
-        to the imported pub'd value (PLAT-299)
-        """
-        if default == ModuleStoreEnum.Type.mongo:
-            raise SkipTest
-        # set the default modulestore
-        with MongoContentstoreBuilder().build() as contentstore:
-            self.store = MixedModuleStore(
-                contentstore=contentstore,
-                create_modulestore_instance=create_modulestore_instance,
-                mappings={},
-                **self.OPTIONS
-            )
-            self.addCleanup(self.store.close_all_connections)
-            with self.store.default_store(default):
-                dest_course_key = self.store.make_course_key('a', 'course', 'course')
-                courses = import_from_xml(
-                    self.store, self.user_id, DATA_DIR, ['toy'], load_error_modules=False,
-                    static_content_store=contentstore,
-                    target_course_id=dest_course_key,
-                    create_course_if_not_present=True,
-                )
-                course_id = courses[0].id
-                # no need to verify course content here as test_cross_modulestore_import_export does that
-                # delete the vertical
-                vertical_loc = course_id.make_usage_key('vertical', 'vertical_test')
-                with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_id):
-                    vertical = self.store.get_item(vertical_loc)
-                    vertical.display_name = "4"
-                    self.store.update_item(vertical, self.user_id)
+                    course_key = course.id
 
-                # now re-import
-                import_from_xml(
-                    self.store, self.user_id, DATA_DIR, ['toy'], load_error_modules=False,
-                    static_content_store=contentstore,
-                    target_course_id=dest_course_key,
-                )
-                # verify it's the same in both published and draft (toy has no drafts)
-                with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_id):
-                    draft_vertical = self.store.get_item(vertical_loc)
-                with self.store.branch_setting(ModuleStoreEnum.Branch.published_only, course_id):
-                    published_vertical = self.store.get_item(vertical_loc)
-                self.assertEqual(draft_vertical.display_name, published_vertical.display_name)
+                    # Test non-draftable block types. The block should be published with every change.
+                    categories = DIRECT_ONLY_CATEGORIES
+                    for block_type in categories:
+                        log.debug('Testing with block type %s', block_type)
+                        receiver.reset_mock()
+                        block = self.store.create_item(self.user_id, course_key, block_type)
+                        self.assertEqual(receiver.call_count, 1)
+
+                        block.display_name = block_type
+                        self.store.update_item(block, self.user_id)
+                        self.assertEqual(receiver.call_count, 2)
+
+                        self.store.publish(block.location, self.user_id)
+                        self.assertEqual(receiver.call_count, 3)
+
+                    # Test a draftable block type, which needs to be explicitly published.
+                    receiver.reset_mock()
+                    block = self.store.create_child(self.user_id, course.location, 'problem')
+                    self.assertEqual(receiver.call_count, 1)
+
+                    self.store.update_item(block, self.user_id)
+                    self.assertEqual(receiver.call_count, 1)
+
+                    self.store.publish(block.location, self.user_id)
+                    self.assertEqual(receiver.call_count, 2)
+
+                    self.store.unpublish(block.location, self.user_id)
+                    self.assertEqual(receiver.call_count, 3)
+
+                    self.store.delete_item(block.location, self.user_id)
+                    self.assertEqual(receiver.call_count, 4)
+
+                    # Test course re-runs
+                    receiver.reset_mock()
+                    dest_course_id = self.store.make_course_key("org.other", "course.other", "run.other")
+                    self.store.clone_course(course_key, dest_course_id, self.user_id)
+                    self.assertEqual(receiver.call_count, 1)
+
+                    # Test course imports
+                    # Note: The signal is fired once when the course is created and
+                    # a second time after the actual data import.
+                    receiver.reset_mock()
+                    import_from_xml(
+                        self.store, self.user_id, DATA_DIR, ['toy'], load_error_modules=False,
+                        static_content_store=contentstore,
+                        create_course_if_not_present=True,
+                    )
+                    self.assertEqual(receiver.call_count, 2)

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

@@ -22,7 +22,7 @@ class TestPublish(SplitWMongoCourseBoostrapper):
         # There are 12 created items and 7 parent updates
         # create course: finds: 1 to verify uniqueness, 1 to find parents
         # sends: 1 to create course, 1 to create overview
-        with check_mongo_calls(5, 2):
+        with check_mongo_calls(4, 2):
             super(TestPublish, self)._create_course(split=False)  # 2 inserts (course and overview)
 
         # with bulk will delay all inheritance computations which won't be added into the mongo_calls

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

@@ -34,7 +34,8 @@ def create_modulestore_instance(
         options,
         i18n_service=None,
         fs_service=None,
-        user_service=None
+        user_service=None,
+        signal_handler=None,
 ):
     """
     This will return a new instance of a modulestore given an engine and options
@@ -47,6 +48,7 @@ def create_modulestore_instance(
     return class_(
         doc_store_config=doc_store_config,
         contentstore=contentstore,
+        signal_handler=signal_handler,
         **options
     )
 

common/lib/xmodule/xmodule/modulestore/xml.py

@@ -294,7 +294,8 @@ class XMLModuleStore(ModuleStoreReadBase):
     """
     def __init__(
             self, data_dir, default_class=None, course_dirs=None, course_ids=None,
-            load_error_modules=True, i18n_service=None, fs_service=None, user_service=None, **kwargs
+            load_error_modules=True, i18n_service=None, fs_service=None, user_service=None,
+            signal_handler=None, **kwargs   # pylint: disable=unused-argument
     ):
         """
         Initialize an XMLModuleStore from data_dir