Merge pull request #7435 from edx/mjames/SOL-499

Update course_publish event actions - Logged SOL-611 for follow up work 

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

@@ -119,6 +119,7 @@ class BulkOpsRecord(object):
     """
     def __init__(self):
         self._active_count = 0
+        self.has_publish_item = False
 
     @property
     def active(self):
@@ -281,6 +282,15 @@ class BulkOperationsMixin(object):
         """
         return self._get_bulk_ops_record(course_key, ignore_case).active
 
+    def send_bulk_published_signal(self, bulk_ops_record, course_id):
+        """
+        Sends out the signal that items have been published from within this course.
+        """
+        signal_handler = getattr(self, 'signal_handler', None)
+        if signal_handler and bulk_ops_record.has_publish_item:
+            signal_handler.send("course_published", course_key=course_id)
+            bulk_ops_record.has_publish_item = False
+
 
 class EditInfo(object):
     """
@@ -1299,6 +1309,23 @@ 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
+        """
+        signal_handler = getattr(self, 'signal_handler', None)
+        if 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:
+                signal_handler.send("course_published", course_key=course_key)
+
 
 def only_xmodules(identifier, entry_points):
     """Only use entry_points that are supplied by the xmodule package"""

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

@@ -474,8 +474,8 @@ class MongoBulkOpsMixin(BulkOperationsMixin):
         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)
+            if emit_signals:
+                self.send_bulk_published_signal(bulk_ops_record, course_id)
 
             bulk_ops_record.dirty = False  # brand spanking clean now
 
@@ -1328,7 +1328,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo
             else:
                 parent.children.insert(kwargs.get('position'), xblock.location)
 
-            self.update_item(parent, user_id)
+            self.update_item(parent, user_id, child_update=True)
 
         return xblock
 

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

@@ -439,7 +439,15 @@ class DraftModuleStore(MongoModuleStore):
         # convert the subtree using the original item as the root
         self._breadth_first(convert_item, [location])
 
-    def update_item(self, xblock, user_id, allow_not_found=False, force=False, isPublish=False, **kwargs):
+    def update_item(
+            self,
+            xblock,
+            user_id,
+            allow_not_found=False,
+            force=False,
+            isPublish=False,
+            child_update=False,
+            **kwargs):
         """
         See superclass doc.
         In addition to the superclass's behavior, this method converts the unit to draft if it's not
@@ -451,9 +459,8 @@ class DraftModuleStore(MongoModuleStore):
         if draft_loc.revision == MongoRevisionKey.published:
             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)
+            if isPublish or (item.category in DIRECT_ONLY_CATEGORIES and not child_update):
+                self._flag_publish_event(course_key)
             return item
 
         if not super(DraftModuleStore, self).has_item(draft_loc):
@@ -532,7 +539,8 @@ class DraftModuleStore(MongoModuleStore):
             parent_block = super(DraftModuleStore, self).get_item(parent_location)
             parent_block.children.remove(location)
             parent_block.location = parent_location  # ensure the location is with the correct revision
-            self.update_item(parent_block, user_id)
+            self.update_item(parent_block, user_id, child_update=True)
+        self._flag_publish_event(location.course_key)
 
         if is_item_direct_only or revision == ModuleStoreEnum.RevisionOption.all:
             as_functions = [as_draft, as_published]
@@ -728,8 +736,7 @@ class DraftModuleStore(MongoModuleStore):
             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)
+        self._flag_publish_event(course_key)
 
         # Now it's been published, add the object to the courseware search index so that it appears in search results
         CoursewareSearchIndexer.do_publish_index(self, location)
@@ -747,9 +754,7 @@ class DraftModuleStore(MongoModuleStore):
         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)
+        self._flag_publish_event(course_key)
 
     def revert_to_published(self, location, user_id=None):
         """

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

@@ -268,9 +268,7 @@ class SplitBulkWriteMixin(BulkOperationsMixin):
                 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)
+            self.send_bulk_published_signal(bulk_write_record, course_key)
 
     def get_course_index(self, course_key, ignore_case=False):
         """

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

@@ -201,6 +201,7 @@ class DraftVersioningModuleStore(SplitMongoModuleStore, ModuleStoreDraftAndPubli
                     ]
                 )
 
+            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)
@@ -356,6 +357,8 @@ class DraftVersioningModuleStore(SplitMongoModuleStore, ModuleStoreDraftAndPubli
             blacklist=blacklist
         )
 
+        self._flag_publish_event(location.course_key)
+
         # Now it's been published, add the object to the courseware search index so that it appears in search results
         CoursewareSearchIndexer.do_publish_index(self, location)
 

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

@@ -1976,9 +1976,8 @@ class TestMixedModuleStore(CourseComparisonTest):
             dest_store = self.store._get_modulestore_by_type(ModuleStoreEnum.Type.split)
             self.assertCoursesEqual(source_store, source_course_key, dest_store, dest_course_id)
 
-    @skip("PLAT-449 XModule TestMixedModuleStore intermittent test failure")
     @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
-    def test_course_publish_signal_firing(self, default):
+    def test_course_publish_signal_direct_firing(self, default):
         with MongoContentstoreBuilder().build() as contentstore:
             self.store = MixedModuleStore(
                 contentstore=contentstore,
@@ -2016,22 +2015,29 @@ class TestMixedModuleStore(CourseComparisonTest):
                         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)
+    @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
+    def test_course_publish_signal_rerun_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)
 
-                    self.store.update_item(block, self.user_id)
-                    self.assertEqual(receiver.call_count, 1)
+            with self.store.default_store(default):
+                self.assertIsNotNone(self.store.thread_cache.default_store.signal_handler)
 
-                    self.store.publish(block.location, self.user_id)
-                    self.assertEqual(receiver.call_count, 2)
+                with mock_signal_receiver(SignalHandler.course_published) as receiver:
+                    self.assertEqual(receiver.call_count, 0)
 
-                    self.store.unpublish(block.location, self.user_id)
-                    self.assertEqual(receiver.call_count, 3)
+                    # 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)
 
-                    self.store.delete_item(block.location, self.user_id)
-                    self.assertEqual(receiver.call_count, 4)
+                    course_key = course.id
 
                     # Test course re-runs
                     receiver.reset_mock()
@@ -2039,6 +2045,24 @@ class TestMixedModuleStore(CourseComparisonTest):
                     self.store.clone_course(course_key, dest_course_id, self.user_id)
                     self.assertEqual(receiver.call_count, 1)
 
+    @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
+    def test_course_publish_signal_import_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):
+                self.assertIsNotNone(self.store.thread_cache.default_store.signal_handler)
+
+                with mock_signal_receiver(SignalHandler.course_published) as receiver:
+                    self.assertEqual(receiver.call_count, 0)
+
                     # Test course imports
                     # Note: The signal is fired once when the course is created and
                     # a second time after the actual data import.
@@ -2049,3 +2073,172 @@ class TestMixedModuleStore(CourseComparisonTest):
                         create_if_not_present=True,
                     )
                     self.assertEqual(receiver.call_count, 2)
+
+    @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
+    def test_course_publish_signal_publish_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):
+                self.assertIsNotNone(self.store.thread_cache.default_store.signal_handler)
+
+                with mock_signal_receiver(SignalHandler.course_published) as receiver:
+                    self.assertEqual(receiver.call_count, 0)
+
+                    # 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)
+
+                    # Test a draftable block type, which needs to be explicitly published, and nest it within the
+                    # normal structure - this is important because some implementors change the parent when adding a
+                    # non-published child; if parent is in DIRECT_ONLY_CATEGORIES then this should not fire the event
+                    receiver.reset_mock()
+                    section = self.store.create_item(self.user_id, course.id, 'chapter')
+                    self.assertEqual(receiver.call_count, 1)
+
+                    subsection = self.store.create_child(self.user_id, section.location, 'sequential')
+                    self.assertEqual(receiver.call_count, 2)
+
+                    # 'units' and 'blocks' are draftable types
+                    receiver.reset_mock()
+                    unit = self.store.create_child(self.user_id, subsection.location, 'vertical')
+                    self.assertEqual(receiver.call_count, 0)
+
+                    block = self.store.create_child(self.user_id, unit.location, 'problem')
+                    self.assertEqual(receiver.call_count, 0)
+
+                    self.store.update_item(block, self.user_id)
+                    self.assertEqual(receiver.call_count, 0)
+
+                    self.store.publish(unit.location, self.user_id)
+                    self.assertEqual(receiver.call_count, 1)
+
+                    self.store.unpublish(unit.location, self.user_id)
+                    self.assertEqual(receiver.call_count, 2)
+
+                    self.store.delete_item(unit.location, self.user_id)
+                    self.assertEqual(receiver.call_count, 3)
+
+    @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
+    def test_bulk_course_publish_signal_direct_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):
+                self.assertIsNotNone(self.store.thread_cache.default_store.signal_handler)
+
+                with mock_signal_receiver(SignalHandler.course_published) as receiver:
+                    self.assertEqual(receiver.call_count, 0)
+
+                    # 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)
+
+                    course_key = course.id
+
+                    # Test non-draftable block types. No signals should be received until
+                    receiver.reset_mock()
+                    with self.store.bulk_operations(course_key):
+                        categories = DIRECT_ONLY_CATEGORIES
+                        for block_type in categories:
+                            log.debug('Testing with block type %s', block_type)
+                            block = self.store.create_item(self.user_id, course_key, block_type)
+                            self.assertEqual(receiver.call_count, 0)
+
+                            block.display_name = block_type
+                            self.store.update_item(block, self.user_id)
+                            self.assertEqual(receiver.call_count, 0)
+
+                            self.store.publish(block.location, self.user_id)
+                            self.assertEqual(receiver.call_count, 0)
+
+                    self.assertEqual(receiver.call_count, 1)
+
+    @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
+    def test_bulk_course_publish_signal_publish_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):
+                self.assertIsNotNone(self.store.thread_cache.default_store.signal_handler)
+
+                with mock_signal_receiver(SignalHandler.course_published) as receiver:
+                    self.assertEqual(receiver.call_count, 0)
+
+                    # 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)
+
+                    course_key = course.id
+
+                    # Test a draftable block type, which needs to be explicitly published, and nest it within the
+                    # normal structure - this is important because some implementors change the parent when adding a
+                    # non-published child; if parent is in DIRECT_ONLY_CATEGORIES then this should not fire the event
+                    receiver.reset_mock()
+                    with self.store.bulk_operations(course_key):
+                        section = self.store.create_item(self.user_id, course_key, 'chapter')
+                        self.assertEqual(receiver.call_count, 0)
+
+                        subsection = self.store.create_child(self.user_id, section.location, 'sequential')
+                        self.assertEqual(receiver.call_count, 0)
+
+                        # 'units' and 'blocks' are draftable types
+                        unit = self.store.create_child(self.user_id, subsection.location, 'vertical')
+                        self.assertEqual(receiver.call_count, 0)
+
+                        block = self.store.create_child(self.user_id, unit.location, 'problem')
+                        self.assertEqual(receiver.call_count, 0)
+
+                        self.store.update_item(block, self.user_id)
+                        self.assertEqual(receiver.call_count, 0)
+
+                        self.store.publish(unit.location, self.user_id)
+                        self.assertEqual(receiver.call_count, 0)
+
+                        self.store.unpublish(unit.location, self.user_id)
+                        self.assertEqual(receiver.call_count, 0)
+
+                        self.store.delete_item(unit.location, self.user_id)
+                        self.assertEqual(receiver.call_count, 0)
+
+                    self.assertEqual(receiver.call_count, 1)
+
+                    # Test editing draftable block type without publish
+                    receiver.reset_mock()
+                    with self.store.bulk_operations(course_key):
+                        unit = self.store.create_child(self.user_id, subsection.location, 'vertical')
+                        self.assertEqual(receiver.call_count, 0)
+                        block = self.store.create_child(self.user_id, unit.location, 'problem')
+                        self.assertEqual(receiver.call_count, 0)
+                        self.store.publish(unit.location, self.user_id)
+                        self.assertEqual(receiver.call_count, 0)
+                    self.assertEqual(receiver.call_count, 1)
+
+                    receiver.reset_mock()
+                    with self.store.bulk_operations(course_key):
+                        self.assertEqual(receiver.call_count, 0)
+                        unit.display_name = "Change this unit"
+                        self.store.update_item(unit, self.user_id)
+                        self.assertEqual(receiver.call_count, 0)
+                    self.assertEqual(receiver.call_count, 0)