From 838060f30c6611524fd56b498c661216b7194236 Mon Sep 17 00:00:00 2001 From: Martyn James Date: Wed, 11 Mar 2015 16:31:14 -0400 Subject: [PATCH] Update course_publish event to: * Only fire on bulk operation if something therein was published * Wrap calls that use the following repeated pattern: if self.signal_handler and not bulk_record.active: self.signal_handler.send() * Ensure consistent firing of signal between split and draft implementations * Updated tests to use typical course nesting structures * Added tests within bulk operations --- .../xmodule/xmodule/modulestore/__init__.py | 27 +++ .../xmodule/xmodule/modulestore/mongo/base.py | 6 +- .../xmodule/modulestore/mongo/draft.py | 25 +- .../xmodule/modulestore/split_mongo/split.py | 4 +- .../modulestore/split_mongo/split_draft.py | 3 + .../tests/test_mixed_modulestore.py | 225 ++++++++++++++++-- 6 files changed, 258 insertions(+), 32 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index 1741f8285c..8bf5972752 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/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): """ @@ -1297,6 +1307,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""" diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index 0509404a53..a3775c2ddc 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -472,8 +472,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 @@ -1307,7 +1307,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 diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index 5c30f5e379..774a11b1a3 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/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): """ diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index e1dc791ae7..803c113281 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/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): """ 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 7d84e56124..5830a64f01 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/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) 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 cbd6c6762e..8b9bd99145 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/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') + @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) + + 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) - 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) + 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)