From e4ea28f7f687339457a0da5ddeb66a1c0f78e943 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 8 Jan 2015 11:59:02 -0800 Subject: [PATCH 1/8] Minor pylint fixes --- cms/djangoapps/contentstore/features/courses.py | 3 +-- common/djangoapps/terrain/ui_helpers.py | 13 ++++++------- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/cms/djangoapps/contentstore/features/courses.py b/cms/djangoapps/contentstore/features/courses.py index cb7b85f076..218faded58 100644 --- a/cms/djangoapps/contentstore/features/courses.py +++ b/cms/djangoapps/contentstore/features/courses.py @@ -34,9 +34,8 @@ def i_create_a_course(step): create_a_course() -# pylint: disable=invalid-name @step('I click the course link in Studio Home$') -def i_click_the_course_link_in_studio_home(step): +def i_click_the_course_link_in_studio_home(step): # pylint: disable=invalid-name course_css = 'a.course-link' world.css_click(course_css) diff --git a/common/djangoapps/terrain/ui_helpers.py b/common/djangoapps/terrain/ui_helpers.py index 2b5248a1ab..cdd9e2fbd6 100644 --- a/common/djangoapps/terrain/ui_helpers.py +++ b/common/djangoapps/terrain/ui_helpers.py @@ -28,28 +28,27 @@ GLOBAL_WAIT_FOR_TIMEOUT = 60 REQUIREJS_WAIT = { # Settings - Schedule & Details - re.compile('^Schedule & Details Settings \|'): [ + re.compile(r'^Schedule & Details Settings \|'): [ "jquery", "js/base", "js/models/course", "js/models/settings/course_details", "js/views/settings/main"], # Settings - Advanced Settings - re.compile('^Advanced Settings \|'): [ + re.compile(r'^Advanced Settings \|'): [ "jquery", "js/base", "js/models/course", "js/models/settings/advanced", "js/views/settings/advanced", "codemirror"], # Unit page - re.compile('^Unit \|'): [ + re.compile(r'^Unit \|'): [ "jquery", "js/base", "js/models/xblock_info", "js/views/pages/container", "js/collections/component_template", "xmodule", "coffee/src/main", "xblock/cms.runtime.v1"], # Content - Outline # Note that calling your org, course number, or display name, 'course' will mess this up - re.compile('^Course Outline \|'): [ + re.compile(r'^Course Outline \|'): [ "js/base", "js/models/course", "js/models/location", "js/models/section"], # Dashboard - # pylint: disable=anomalous-backslash-in-string - re.compile('^Studio Home \|'): [ + re.compile(r'^Studio Home \|'): [ "js/sock", "gettext", "js/base", "jquery.ui", "coffee/src/main", "underscore"], @@ -60,7 +59,7 @@ REQUIREJS_WAIT = { ], # Pages - re.compile('^Pages \|'): [ + re.compile(r'^Pages \|'): [ 'js/models/explicit_url', 'coffee/src/views/tabs', 'xmodule', 'coffee/src/main', 'xblock/cms.runtime.v1' ], From 91cb371de056f310ef8639f7a5c9ea1118aaa443 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 19 Dec 2014 14:16:24 -0800 Subject: [PATCH 2/8] Emit per-student library content events to the tracking log for analytics --- .../xmodule/xmodule/library_content_module.py | 34 ++++++++++++++++--- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/common/lib/xmodule/xmodule/library_content_module.py b/common/lib/xmodule/xmodule/library_content_module.py index e4c3252b37..78a8e0eecf 100644 --- a/common/lib/xmodule/xmodule/library_content_module.py +++ b/common/lib/xmodule/xmodule/library_content_module.py @@ -221,24 +221,50 @@ class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule): # Already done: return self._selected_set # pylint: disable=access-member-before-definition # Determine which of our children we will show: + jsonify_block_keys = lambda keys: [unicode(self.location.course_key.make_usage_key(*key)) for key in keys] selected = set(tuple(k) for k in self.selected) # set of (block_type, block_id) tuples valid_block_keys = set([(c.block_type, c.block_id) for c in self.children]) # pylint: disable=no-member # Remove any selected blocks that are no longer valid: - selected -= (selected - valid_block_keys) + invalid_block_keys = (selected - valid_block_keys) + if invalid_block_keys: + selected -= invalid_block_keys + # Publish an event for analytics purposes: + self.runtime.publish(self, "edx.librarycontentblock.content.removed", { + "location": unicode(self.location), + "blocks": jsonify_block_keys(invalid_block_keys), + "reason": "invalid", # Deleted from library or library being used has changed + }) # If max_count has been decreased, we may have to drop some previously selected blocks: + overlimit_block_keys = set() while len(selected) > self.max_count: - selected.pop() + overlimit_block_keys.add(selected.pop()) + if overlimit_block_keys: + # Publish an event for analytics purposes: + self.runtime.publish(self, "edx.librarycontentblock.content.removed", { + "location": unicode(self.location), + "blocks": jsonify_block_keys(overlimit_block_keys), + "reason": "overlimit", + }) # Do we have enough blocks now? num_to_add = self.max_count - len(selected) if num_to_add > 0: + added_block_keys = None # We need to select [more] blocks to display to this user: + pool = valid_block_keys - selected if self.mode == "random": - pool = valid_block_keys - selected num_to_add = min(len(pool), num_to_add) - selected |= set(random.sample(pool, num_to_add)) + added_block_keys = set(random.sample(pool, num_to_add)) # We now have the correct n random children to show for this user. else: raise NotImplementedError("Unsupported mode.") + selected |= added_block_keys + if added_block_keys: + # Publish an event for analytics purposes: + self.runtime.publish(self, "edx.librarycontentblock.content.assigned", { + "location": unicode(self.location), + "added": jsonify_block_keys(added_block_keys), + "result": jsonify_block_keys(selected), + }) # Save our selections to the user state, to ensure consistency: self.selected = list(selected) # TODO: this doesn't save from the LMS "Progress" page. # Cache the results From 407265bc45a2bb8fe65ca5955ee6b12d0c9d671d Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 19 Dec 2014 23:22:30 -0800 Subject: [PATCH 3/8] Store the usage locator of library blocks in split when they are inherited into a course --- .../lib/xmodule/xmodule/modulestore/mixed.py | 12 ++++++++++ .../xmodule/modulestore/split_mongo/split.py | 22 +++++++++++++++++++ .../modulestore/split_mongo/split_draft.py | 9 ++++++++ 3 files changed, 43 insertions(+) diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index 6175427659..85ba62d82e 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/common/lib/xmodule/xmodule/modulestore/mixed.py @@ -509,6 +509,18 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): store = self._get_modulestore_for_courseid(location.course_key) return store.get_parent_location(location, **kwargs) + def get_block_original_usage(self, usage_key): + """ + If a block was inherited into another structure using copy_from_template, + this will return the original block usage locator from which the + copy was inherited. + """ + try: + store = self._verify_modulestore_support(usage_key.course_key, 'get_block_original_usage') + return store.get_block_original_usage(usage_key) + except NotImplementedError: + return None, None + def get_modulestore_type(self, course_id): """ Returns a type which identifies which modulestore is servicing the given course_id. diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index c4d5d638e8..0d7a861b2b 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -454,12 +454,17 @@ class SplitBulkWriteMixin(BulkOperationsMixin): if block_info['edit_info'].get('update_version') == update_version: return + original_usage = block_info['edit_info'].get('original_usage') + original_usage_version = block_info['edit_info'].get('original_usage_version') block_info['edit_info'] = { 'edited_on': datetime.datetime.now(UTC), 'edited_by': user_id, 'previous_version': block_info['edit_info']['update_version'], 'update_version': update_version, } + if original_usage: + block_info['edit_info']['original_usage'] = original_usage + block_info['edit_info']['original_usage_version'] = original_usage_version def find_matching_course_indexes(self, branch=None, search_targets=None): """ @@ -1254,6 +1259,21 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): # TODO implement pass + def get_block_original_usage(self, usage_key): + """ + If a block was inherited into another structure using copy_from_template, + this will return the original block usage locator and version from + which the copy was inherited. + + Returns usage_key, version if the data is available, otherwise returns (None, None) + """ + blocks = self._lookup_course(usage_key.course_key).structure['blocks'] + block = blocks.get(BlockKey.from_usage_key(usage_key)) + if block and 'original_usage' in block['edit_info']: + usage_key = BlockUsageLocator.from_string(block['edit_info']['original_usage']) + return usage_key, block['edit_info'].get('original_usage_version') + return None, None + def create_definition_from_data(self, course_key, new_def_data, category, user_id): """ Pull the definition fields out of descriptor and save to the db as a new definition @@ -2214,6 +2234,8 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): # Setting it to the source_block_info structure version here breaks split_draft's has_changes() method. new_block_info['edit_info']['edited_by'] = user_id new_block_info['edit_info']['edited_on'] = datetime.datetime.now(UTC) + new_block_info['edit_info']['original_usage'] = unicode(usage_key.replace(branch=None, version_guid=None)) + new_block_info['edit_info']['original_usage_version'] = source_block_info['edit_info'].get('update_version') dest_structure['blocks'][new_block_key] = new_block_info children = source_block_info['fields'].get('children') 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 30fe251823..d58be235dc 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py @@ -268,6 +268,15 @@ class DraftVersioningModuleStore(SplitMongoModuleStore, ModuleStoreDraftAndPubli location = self._map_revision_to_branch(location, revision=revision) return super(DraftVersioningModuleStore, self).get_parent_location(location, **kwargs) + def get_block_original_usage(self, usage_key): + """ + If a block was inherited into another structure using copy_from_template, + this will return the original block usage locator from which the + copy was inherited. + """ + usage_key = self._map_revision_to_branch(usage_key) + return super(DraftVersioningModuleStore, self).get_block_original_usage(usage_key) + def get_orphans(self, course_key, **kwargs): course_key = self._map_revision_to_branch(course_key) return super(DraftVersioningModuleStore, self).get_orphans(course_key, **kwargs) From 3973317aa90718f6c2f407bbf9cdb7b58135609c Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 9 Jan 2015 11:01:40 -0800 Subject: [PATCH 4/8] Report the original LibraryUsageLocator in LMS analytics --- .../xmodule/xmodule/library_content_module.py | 15 +++++--- common/lib/xmodule/xmodule/library_tools.py | 38 +++++++++++++++++++ lms/djangoapps/lms_xblock/runtime.py | 2 + 3 files changed, 50 insertions(+), 5 deletions(-) diff --git a/common/lib/xmodule/xmodule/library_content_module.py b/common/lib/xmodule/xmodule/library_content_module.py index 78a8e0eecf..e3384efefc 100644 --- a/common/lib/xmodule/xmodule/library_content_module.py +++ b/common/lib/xmodule/xmodule/library_content_module.py @@ -220,8 +220,11 @@ class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule): if hasattr(self, "_selected_set"): # Already done: return self._selected_set # pylint: disable=access-member-before-definition + + lib_tools = self.runtime.service(self, 'library_tools') + format_block_keys = lambda block_keys: lib_tools.create_block_analytics_summary(self.location.course_key, block_keys) + # Determine which of our children we will show: - jsonify_block_keys = lambda keys: [unicode(self.location.course_key.make_usage_key(*key)) for key in keys] selected = set(tuple(k) for k in self.selected) # set of (block_type, block_id) tuples valid_block_keys = set([(c.block_type, c.block_id) for c in self.children]) # pylint: disable=no-member # Remove any selected blocks that are no longer valid: @@ -231,8 +234,9 @@ class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule): # Publish an event for analytics purposes: self.runtime.publish(self, "edx.librarycontentblock.content.removed", { "location": unicode(self.location), - "blocks": jsonify_block_keys(invalid_block_keys), + "removed": format_block_keys(invalid_block_keys), "reason": "invalid", # Deleted from library or library being used has changed + "result": format_block_keys(selected), }) # If max_count has been decreased, we may have to drop some previously selected blocks: overlimit_block_keys = set() @@ -242,8 +246,9 @@ class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule): # Publish an event for analytics purposes: self.runtime.publish(self, "edx.librarycontentblock.content.removed", { "location": unicode(self.location), - "blocks": jsonify_block_keys(overlimit_block_keys), + "removed": format_block_keys(overlimit_block_keys), "reason": "overlimit", + "result": format_block_keys(selected), }) # Do we have enough blocks now? num_to_add = self.max_count - len(selected) @@ -262,8 +267,8 @@ class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule): # Publish an event for analytics purposes: self.runtime.publish(self, "edx.librarycontentblock.content.assigned", { "location": unicode(self.location), - "added": jsonify_block_keys(added_block_keys), - "result": jsonify_block_keys(selected), + "added": format_block_keys(added_block_keys), + "result": format_block_keys(selected), }) # Save our selections to the user state, to ensure consistency: self.selected = list(selected) # TODO: this doesn't save from the LMS "Progress" page. diff --git a/common/lib/xmodule/xmodule/library_tools.py b/common/lib/xmodule/xmodule/library_tools.py index 8f3c6d0cee..aebe2aef98 100644 --- a/common/lib/xmodule/xmodule/library_tools.py +++ b/common/lib/xmodule/xmodule/library_tools.py @@ -44,6 +44,44 @@ class LibraryToolsService(object): return library.location.library_key.version_guid return None + def create_block_analytics_summary(self, course_key, block_keys): + """ + Given a CourseKey and a list of (block_type, block_id) pairs, + prepare the JSON-ready metadata needed for analytics logging. + + This is [ + {"usage_key": x, "original_usage_key": y, "original_usage_version": z, "descendants": [...]} + ] + where the main list contains all top-level blocks, and descendants contains a *flat* list of all + descendants of the top level blocks, if any. + """ + def summarize_block(usage_key): + """ Basic information about the given block """ + orig_key, orig_version = self.store.get_block_original_usage(usage_key) + return { + "usage_key": unicode(usage_key), + "original_usage_key": unicode(orig_key) if orig_key else None, + "original_usage_version": unicode(orig_version) if orig_version else None, + } + + result_json = [] + for block_key in block_keys: + key = course_key.make_usage_key(*block_key) + info = summarize_block(key) + info['descendants'] = [] + try: + block = self.store.get_item(key, depth=None) # Load the item and all descendants + children = list(getattr(block, "children", [])) + while children: + child_key = children.pop() + child = self.store.get_item(child_key) + info['descendants'].append(summarize_block(child_key)) + children.extend(getattr(child, "children", [])) + except ItemNotFoundError: + pass # The block has been deleted + result_json.append(info) + return result_json + def _filter_child(self, usage_key, capa_type): """ Filters children by CAPA problem type, if configured diff --git a/lms/djangoapps/lms_xblock/runtime.py b/lms/djangoapps/lms_xblock/runtime.py index 49d0abbf38..7b5c3fe7b9 100644 --- a/lms/djangoapps/lms_xblock/runtime.py +++ b/lms/djangoapps/lms_xblock/runtime.py @@ -10,6 +10,7 @@ from django.conf import settings from lms.djangoapps.lms_xblock.models import XBlockAsidesConfig from openedx.core.djangoapps.user_api.api import course_tag as user_course_tag_api from xmodule.modulestore.django import modulestore +from xmodule.library_tools import LibraryToolsService from xmodule.x_module import ModuleSystem from xmodule.partitions.partitions_service import PartitionService @@ -199,6 +200,7 @@ class LmsModuleSystem(LmsHandlerUrls, ModuleSystem): # pylint: disable=abstract course_id=kwargs.get('course_id'), track_function=kwargs.get('track_function', None), ) + services['library_tools'] = LibraryToolsService(modulestore()) services['fs'] = xblock.reference.plugins.FSService() self.request_token = kwargs.pop('request_token', None) super(LmsModuleSystem, self).__init__(**kwargs) From 3a973d4274355aafb3e896ce9bfd5318d0487355 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 9 Jan 2015 11:02:17 -0800 Subject: [PATCH 5/8] Unit tests for library content analytics --- .../xmodule/modulestore/tests/utils.py | 1 + .../xmodule/tests/test_library_content.py | 237 ++++++++++++++---- 2 files changed, 186 insertions(+), 52 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/utils.py b/common/lib/xmodule/xmodule/modulestore/tests/utils.py index 986a54df9f..10d3dd1742 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/utils.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/utils.py @@ -119,6 +119,7 @@ class MixedSplitTestCase(TestCase): extra.update(kwargs) return ItemFactory.create( category=category, + parent=parent_block, parent_location=parent_block.location, modulestore=self.store, **extra diff --git a/common/lib/xmodule/xmodule/tests/test_library_content.py b/common/lib/xmodule/xmodule/tests/test_library_content.py index 0a60895f0a..7eb1c81a18 100644 --- a/common/lib/xmodule/xmodule/tests/test_library_content.py +++ b/common/lib/xmodule/xmodule/tests/test_library_content.py @@ -5,22 +5,22 @@ Basic unit tests for LibraryContentModule Higher-level tests are in `cms/djangoapps/contentstore/tests/test_libraries.py`. """ from bson.objectid import ObjectId -from mock import patch +from mock import Mock, patch from opaque_keys.edx.locator import LibraryLocator from unittest import TestCase from xblock.fragment import Fragment from xblock.runtime import Runtime as VanillaRuntime -from xmodule.x_module import AUTHOR_VIEW from xmodule.library_content_module import ( LibraryVersionReference, LibraryList, ANY_CAPA_TYPE_VALUE, LibraryContentDescriptor ) -from xmodule.modulestore.tests.factories import LibraryFactory, CourseFactory, ItemFactory +from xmodule.library_tools import LibraryToolsService +from xmodule.modulestore.tests.factories import LibraryFactory, CourseFactory from xmodule.modulestore.tests.utils import MixedSplitTestCase from xmodule.tests import get_test_system from xmodule.validation import StudioValidationMessage - +from xmodule.x_module import AUTHOR_VIEW dummy_render = lambda block, _: Fragment(block.data) # pylint: disable=invalid-name @@ -32,46 +32,21 @@ class LibraryContentTest(MixedSplitTestCase): def setUp(self): super(LibraryContentTest, self).setUp() + self.tools = LibraryToolsService(self.store) self.library = LibraryFactory.create(modulestore=self.store) self.lib_blocks = [ - ItemFactory.create( - category="html", - parent_location=self.library.location, - user_id=self.user_id, - publish_item=False, - metadata={"data": "Hello world from block {}".format(i), }, - modulestore=self.store, - ) + self.make_block("html", self.library, data="Hello world from block {}".format(i)) for i in range(1, 5) ] self.course = CourseFactory.create(modulestore=self.store) - self.chapter = ItemFactory.create( - category="chapter", - parent_location=self.course.location, - user_id=self.user_id, - modulestore=self.store, - ) - self.sequential = ItemFactory.create( - category="sequential", - parent_location=self.chapter.location, - user_id=self.user_id, - modulestore=self.store, - ) - self.vertical = ItemFactory.create( - category="vertical", - parent_location=self.sequential.location, - user_id=self.user_id, - modulestore=self.store, - ) - self.lc_block = ItemFactory.create( - category="library_content", - parent_location=self.vertical.location, - user_id=self.user_id, - modulestore=self.store, - metadata={ - 'max_count': 1, - 'source_libraries': [LibraryVersionReference(self.library.location.library_key)] - } + self.chapter = self.make_block("chapter", self.course) + self.sequential = self.make_block("sequential", self.chapter) + self.vertical = self.make_block("vertical", self.sequential) + self.lc_block = self.make_block( + "library_content", + self.vertical, + max_count=1, + source_libraries=[LibraryVersionReference(self.library.location.library_key)] ) def _bind_course_module(self, module): @@ -80,6 +55,7 @@ class LibraryContentTest(MixedSplitTestCase): """ module_system = get_test_system(course_id=self.course.location.course_key) module_system.descriptor_runtime = module.runtime + module_system._services['library_tools'] = self.tools # pylint: disable=protected-access def get_module(descriptor): """Mocks module_system get_module function""" @@ -92,6 +68,11 @@ class LibraryContentTest(MixedSplitTestCase): module_system.get_module = get_module module.xmodule_runtime = module_system + +class TestLibraryContentModule(LibraryContentTest): + """ + Basic unit tests for LibraryContentModule + """ def _get_capa_problem_type_xml(self, *args): """ Helper function to create empty CAPA problem definition """ problem = "" @@ -111,20 +92,8 @@ class LibraryContentTest(MixedSplitTestCase): ["coderesponse", "optionresponse"] ] for problem_type in problem_types: - ItemFactory.create( - category="problem", - parent_location=self.library.location, - user_id=self.user_id, - publish_item=False, - data=self._get_capa_problem_type_xml(*problem_type), - modulestore=self.store, - ) + self.make_block("problem", self.library, data=self._get_capa_problem_type_xml(*problem_type)) - -class TestLibraryContentModule(LibraryContentTest): - """ - Basic unit tests for LibraryContentModule - """ def test_lib_content_block(self): """ Test that blocks from a library are copied and added as children @@ -338,3 +307,167 @@ class TestLibraryList(TestCase): lib_list = LibraryList() with self.assertRaises(ValueError): lib_list.from_json(["Not-a-library-key,whatever"]) + + +class TestLibraryContentAnalytics(LibraryContentTest): + """ + Test analytics features of LibraryContentModule + """ + def setUp(self): + super(TestLibraryContentAnalytics, self).setUp() + self.publisher = Mock() + self.lc_block.refresh_children() + self.lc_block = self.store.get_item(self.lc_block.location) + self._bind_course_module(self.lc_block) + self.lc_block.xmodule_runtime.publish = self.publisher + + def _assert_event_was_published(self, event_type): + """ + Check that a LibraryContentModule analytics event was published by self.lc_block. + """ + self.assertTrue(self.publisher.called) + self.assertTrue(len(self.publisher.call_args[0]), 3) + _, event_name, event_data = self.publisher.call_args[0] + self.assertEqual(event_name, "edx.librarycontentblock.content.{}".format(event_type)) + self.assertEqual(event_data["location"], unicode(self.lc_block.location)) + return event_data + + def test_assigned_event(self): + """ + Test the "assigned" event emitted when a student is assigned specific blocks. + """ + # In the beginning was the lc_block and it assigned one child to the student: + child = self.lc_block.get_child_descriptors()[0] + child_lib_location, child_lib_version = self.store.get_block_original_usage(child.location) + self.assertIsInstance(child_lib_version, ObjectId) + event_data = self._assert_event_was_published("assigned") + block_info = { + "usage_key": unicode(child.location), + "original_usage_key": unicode(child_lib_location), + "original_usage_version": unicode(child_lib_version), + "descendants": [], + } + self.assertEqual(event_data, { + "location": unicode(self.lc_block.location), + "added": [block_info], + "result": [block_info], + }) + self.publisher.reset_mock() + + # Now increase max_count so that one more child will be added: + self.lc_block.max_count = 2 + del self.lc_block._xmodule._selected_set # Clear the cache (only needed because we skip saving/re-loading the block) pylint: disable=protected-access + children = self.lc_block.get_child_descriptors() + self.assertEqual(len(children), 2) + child, new_child = children if children[0].location == child.location else reversed(children) + event_data = self._assert_event_was_published("assigned") + self.assertEqual(event_data["added"][0]["usage_key"], unicode(new_child.location)) + self.assertEqual(len(event_data["result"]), 2) + + def test_assigned_descendants(self): + """ + Test the "assigned" event emitted includes descendant block information. + """ + # Replace the blocks in the library with a block that has descendants: + with self.store.bulk_operations(self.library.location.library_key): + self.library.children = [] + main_vertical = self.make_block("vertical", self.library) + inner_vertical = self.make_block("vertical", main_vertical) + html_block = self.make_block("html", inner_vertical) + problem_block = self.make_block("problem", inner_vertical) + self.lc_block.refresh_children() + + # Reload lc_block and set it up for a student: + self.lc_block = self.store.get_item(self.lc_block.location) + self._bind_course_module(self.lc_block) + self.lc_block.xmodule_runtime.publish = self.publisher + + # Get the keys of each of our blocks, as they appear in the course: + course_usage_main_vertical = self.lc_block.children[0] + course_usage_inner_vertical = self.store.get_item(course_usage_main_vertical).children[0] + inner_vertical_in_course = self.store.get_item(course_usage_inner_vertical) + course_usage_html = inner_vertical_in_course.children[0] + course_usage_problem = inner_vertical_in_course.children[1] + + # Trigger a publish event: + self.lc_block.get_child_descriptors() + event_data = self._assert_event_was_published("assigned") + + for block_list in (event_data["added"], event_data["result"]): + self.assertEqual(len(block_list), 1) # The main_vertical is the only root block added, and is the only result. + self.assertEqual(block_list[0]["usage_key"], unicode(course_usage_main_vertical)) + + # Check that "descendants" is a flat, unordered list of all of main_vertical's descendants: + descendants_expected = {} + for lib_key, course_usage_key in ( + (inner_vertical.location, course_usage_inner_vertical), + (html_block.location, course_usage_html), + (problem_block.location, course_usage_problem), + ): + descendants_expected[unicode(course_usage_key)] = { + "usage_key": unicode(course_usage_key), + "original_usage_key": unicode(lib_key), + "original_usage_version": unicode(self.store.get_block_original_usage(course_usage_key)[1]), + } + self.assertEqual(len(block_list[0]["descendants"]), len(descendants_expected)) + for descendant in block_list[0]["descendants"]: + self.assertEqual(descendant, descendants_expected.get(descendant["usage_key"])) + + def test_removed_overlimit(self): + """ + Test the "removed" event emitted when we un-assign blocks previously assigned to a student. + We go from one blocks assigned to none because max_count has been decreased. + """ + # Decrease max_count to 1, causing the block to be overlimit: + self.lc_block.get_child_descriptors() # We must call an XModule method before we can change max_count - otherwise the change has no effect + self.publisher.reset_mock() # Clear the "assigned" event that was just published. + self.lc_block.max_count = 0 + del self.lc_block._xmodule._selected_set # Clear the cache (only needed because we skip saving/re-loading the block) pylint: disable=protected-access + + # Check that the event says that one block was removed, leaving no blocks left: + children = self.lc_block.get_child_descriptors() + self.assertEqual(len(children), 0) + event_data = self._assert_event_was_published("removed") + self.assertEqual(len(event_data["removed"]), 1) + self.assertEqual(event_data["result"], []) + self.assertEqual(event_data["reason"], "overlimit") + + def test_removed_invalid(self): + """ + Test the "removed" event emitted when we un-assign blocks previously assigned to a student. + We go from two blocks assigned, to one because the others have been deleted from the library. + """ + # Start by assigning two blocks to the student: + self.lc_block.get_child_descriptors() # We must call an XModule method before we can change max_count - otherwise the change has no effect + self.lc_block.max_count = 2 + del self.lc_block._xmodule._selected_set # Clear the cache (only needed because we skip saving/re-loading the block) pylint: disable=protected-access + initial_blocks_assigned = self.lc_block.get_child_descriptors() + self.assertEqual(len(initial_blocks_assigned), 2) + self.publisher.reset_mock() # Clear the "assigned" event that was just published. + # Now make sure that one of the assigned blocks will have to be un-assigned. + # To cause an "invalid" event, we delete all blocks from the content library except for one of the two already assigned to the student: + keep_block_key = initial_blocks_assigned[0].location + keep_block_lib_usage_key, keep_block_lib_version = self.store.get_block_original_usage(keep_block_key) + deleted_block_key = initial_blocks_assigned[1].location + self.library.children = [keep_block_lib_usage_key] + self.store.update_item(self.library, self.user_id) + self.lc_block.refresh_children() + del self.lc_block._xmodule._selected_set # Clear the cache (only needed because we skip saving/re-loading the block) pylint: disable=protected-access + + # Check that the event says that one block was removed, leaving one block left: + children = self.lc_block.get_child_descriptors() + self.assertEqual(len(children), 1) + event_data = self._assert_event_was_published("removed") + self.assertEqual(event_data["removed"], [{ + "usage_key": unicode(deleted_block_key), + "original_usage_key": None, # Note: original_usage_key info is sadly unavailable because the block has been deleted so that info can no longer be retrieved + "original_usage_version": None, + "descendants": [], + }]) + self.assertEqual(event_data["result"], [{ + "usage_key": unicode(keep_block_key), + "original_usage_key": unicode(keep_block_lib_usage_key), + "original_usage_version": unicode(keep_block_lib_version), + "descendants": [], + }]) + self.assertEqual(event_data["reason"], "invalid") From 9a116529cecfc3221646caf47625f2b346d3d2de Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 9 Jan 2015 11:02:49 -0800 Subject: [PATCH 6/8] Add module key and library source info to problem events --- lms/djangoapps/courseware/module_render.py | 6 ++++ .../courseware/tests/test_module_render.py | 30 +++++++++++++++---- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 07c4d15d38..2f84fcb155 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -755,6 +755,7 @@ def _invoke_xblock_handler(request, course_id, usage_id, handler, suffix, user): try: descriptor = modulestore().get_item(usage_key) + descriptor_orig_usage_key, descriptor_orig_version = modulestore().get_block_original_usage(usage_key) except ItemNotFoundError: log.warn( "Invalid location for course id {course_id}: {usage_key}".format( @@ -768,8 +769,13 @@ def _invoke_xblock_handler(request, course_id, usage_id, handler, suffix, user): tracking_context = { 'module': { 'display_name': descriptor.display_name_with_default, + 'usage_key': unicode(descriptor.location), } } + # For blocks that are inherited from a content library, we add some additional metadata: + if descriptor_orig_usage_key is not None: + tracking_context['module']['original_usage_key'] = unicode(descriptor_orig_usage_key) + tracking_context['module']['original_usage_version'] = unicode(descriptor_orig_version) field_data_cache = FieldDataCache.cache_for_descriptor_descendents( course_id, diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index 7e17441142..ca7c07f8a4 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -5,6 +5,7 @@ Test for lms courseware app, module render unit from functools import partial import json +from bson import ObjectId import ddt from django.http import Http404, HttpResponse from django.core.urlresolvers import reverse @@ -13,6 +14,7 @@ from django.test.client import RequestFactory from django.test.utils import override_settings from django.contrib.auth.models import AnonymousUser from mock import MagicMock, patch, Mock +from opaque_keys.edx.keys import UsageKey from opaque_keys.edx.locations import SlashSeparatedCourseKey from xblock.field_data import FieldData from xblock.runtime import Runtime @@ -971,12 +973,13 @@ class TestModuleTrackingContext(ModuleStoreTestCase): def test_context_contains_display_name(self, mock_tracker): problem_display_name = u'Option Response Problem' - actual_display_name = self.handle_callback_and_get_display_name_from_event(mock_tracker, problem_display_name) - self.assertEquals(problem_display_name, actual_display_name) + module_info = self.handle_callback_and_get_module_info_from_event(mock_tracker, problem_display_name) + self.assertEquals(problem_display_name, module_info['display_name']) - def handle_callback_and_get_display_name_from_event(self, mock_tracker, problem_display_name=None): + def handle_callback_and_get_module_info_from_event(self, mock_tracker, problem_display_name=None): """ - Creates a fake module, invokes the callback and extracts the display name from the emitted problem_check event. + Creates a fake module, invokes the callback and extracts the 'module' + metadata from the emitted problem_check event. """ descriptor_kwargs = { 'category': 'problem', @@ -1000,12 +1003,27 @@ class TestModuleTrackingContext(ModuleStoreTestCase): event = mock_call[1][0] self.assertEquals(event['event_type'], 'problem_check') - return event['context']['module']['display_name'] + return event['context']['module'] def test_missing_display_name(self, mock_tracker): - actual_display_name = self.handle_callback_and_get_display_name_from_event(mock_tracker) + actual_display_name = self.handle_callback_and_get_module_info_from_event(mock_tracker)['display_name'] self.assertTrue(actual_display_name.startswith('problem')) + def test_library_source_information(self, mock_tracker): + """ + Check that XBlocks that are inherited from a library include the + information about their library block source in events. + We patch the modulestore to avoid having to create a library. + """ + original_usage_key = UsageKey.from_string(u'block-v1:A+B+C+type@problem+block@abcd1234') + original_usage_version = ObjectId() + with patch('xmodule.modulestore.mixed.MixedModuleStore.get_block_original_usage', lambda _, key: (original_usage_key, original_usage_version)): + module_info = self.handle_callback_and_get_module_info_from_event(mock_tracker) + self.assertIn('original_usage_key', module_info) + self.assertEqual(module_info['original_usage_key'], unicode(original_usage_key)) + self.assertIn('original_usage_version', module_info) + self.assertEqual(module_info['original_usage_version'], unicode(original_usage_version)) + class TestXmoduleRuntimeEvent(TestSubmittingProblems): """ From 10fe9c01802a8b8d73a2169142fc66ef89b811ff Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Mon, 12 Jan 2015 11:00:51 -0800 Subject: [PATCH 7/8] pylint fixes --- .../xmodule/xmodule/library_content_module.py | 2 +- .../xmodule/tests/test_library_content.py | 37 +++++++++++-------- .../courseware/tests/test_module_render.py | 11 +++--- 3 files changed, 29 insertions(+), 21 deletions(-) diff --git a/common/lib/xmodule/xmodule/library_content_module.py b/common/lib/xmodule/xmodule/library_content_module.py index e3384efefc..c1d5dc9fb5 100644 --- a/common/lib/xmodule/xmodule/library_content_module.py +++ b/common/lib/xmodule/xmodule/library_content_module.py @@ -222,7 +222,7 @@ class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule): return self._selected_set # pylint: disable=access-member-before-definition lib_tools = self.runtime.service(self, 'library_tools') - format_block_keys = lambda block_keys: lib_tools.create_block_analytics_summary(self.location.course_key, block_keys) + format_block_keys = lambda keys: lib_tools.create_block_analytics_summary(self.location.course_key, keys) # Determine which of our children we will show: selected = set(tuple(k) for k in self.selected) # set of (block_type, block_id) tuples diff --git a/common/lib/xmodule/xmodule/tests/test_library_content.py b/common/lib/xmodule/xmodule/tests/test_library_content.py index 7eb1c81a18..c3bb661337 100644 --- a/common/lib/xmodule/xmodule/tests/test_library_content.py +++ b/common/lib/xmodule/xmodule/tests/test_library_content.py @@ -356,7 +356,8 @@ class TestLibraryContentAnalytics(LibraryContentTest): # Now increase max_count so that one more child will be added: self.lc_block.max_count = 2 - del self.lc_block._xmodule._selected_set # Clear the cache (only needed because we skip saving/re-loading the block) pylint: disable=protected-access + # Clear the cache (only needed because we skip saving/re-loading the block) pylint: disable=protected-access + del self.lc_block._xmodule._selected_set children = self.lc_block.get_child_descriptors() self.assertEqual(len(children), 2) child, new_child = children if children[0].location == child.location else reversed(children) @@ -394,24 +395,25 @@ class TestLibraryContentAnalytics(LibraryContentTest): event_data = self._assert_event_was_published("assigned") for block_list in (event_data["added"], event_data["result"]): - self.assertEqual(len(block_list), 1) # The main_vertical is the only root block added, and is the only result. + self.assertEqual(len(block_list), 1) # main_vertical is the only root block added, and is the only result. self.assertEqual(block_list[0]["usage_key"], unicode(course_usage_main_vertical)) # Check that "descendants" is a flat, unordered list of all of main_vertical's descendants: - descendants_expected = {} - for lib_key, course_usage_key in ( + descendants_expected = ( (inner_vertical.location, course_usage_inner_vertical), (html_block.location, course_usage_html), (problem_block.location, course_usage_problem), - ): - descendants_expected[unicode(course_usage_key)] = { + ) + descendant_data_expected = {} + for lib_key, course_usage_key in descendants_expected: + descendant_data_expected[unicode(course_usage_key)] = { "usage_key": unicode(course_usage_key), "original_usage_key": unicode(lib_key), "original_usage_version": unicode(self.store.get_block_original_usage(course_usage_key)[1]), } - self.assertEqual(len(block_list[0]["descendants"]), len(descendants_expected)) + self.assertEqual(len(block_list[0]["descendants"]), len(descendant_data_expected)) for descendant in block_list[0]["descendants"]: - self.assertEqual(descendant, descendants_expected.get(descendant["usage_key"])) + self.assertEqual(descendant, descendant_data_expected.get(descendant["usage_key"])) def test_removed_overlimit(self): """ @@ -419,10 +421,11 @@ class TestLibraryContentAnalytics(LibraryContentTest): We go from one blocks assigned to none because max_count has been decreased. """ # Decrease max_count to 1, causing the block to be overlimit: - self.lc_block.get_child_descriptors() # We must call an XModule method before we can change max_count - otherwise the change has no effect + self.lc_block.get_child_descriptors() # This line is needed in the test environment or the change has no effect self.publisher.reset_mock() # Clear the "assigned" event that was just published. self.lc_block.max_count = 0 - del self.lc_block._xmodule._selected_set # Clear the cache (only needed because we skip saving/re-loading the block) pylint: disable=protected-access + # Clear the cache (only needed because we skip saving/re-loading the block) pylint: disable=protected-access + del self.lc_block._xmodule._selected_set # Check that the event says that one block was removed, leaving no blocks left: children = self.lc_block.get_child_descriptors() @@ -438,21 +441,24 @@ class TestLibraryContentAnalytics(LibraryContentTest): We go from two blocks assigned, to one because the others have been deleted from the library. """ # Start by assigning two blocks to the student: - self.lc_block.get_child_descriptors() # We must call an XModule method before we can change max_count - otherwise the change has no effect + self.lc_block.get_child_descriptors() # This line is needed in the test environment or the change has no effect self.lc_block.max_count = 2 - del self.lc_block._xmodule._selected_set # Clear the cache (only needed because we skip saving/re-loading the block) pylint: disable=protected-access + # Clear the cache (only needed because we skip saving/re-loading the block) pylint: disable=protected-access + del self.lc_block._xmodule._selected_set initial_blocks_assigned = self.lc_block.get_child_descriptors() self.assertEqual(len(initial_blocks_assigned), 2) self.publisher.reset_mock() # Clear the "assigned" event that was just published. # Now make sure that one of the assigned blocks will have to be un-assigned. - # To cause an "invalid" event, we delete all blocks from the content library except for one of the two already assigned to the student: + # To cause an "invalid" event, we delete all blocks from the content library + # except for one of the two already assigned to the student: keep_block_key = initial_blocks_assigned[0].location keep_block_lib_usage_key, keep_block_lib_version = self.store.get_block_original_usage(keep_block_key) deleted_block_key = initial_blocks_assigned[1].location self.library.children = [keep_block_lib_usage_key] self.store.update_item(self.library, self.user_id) self.lc_block.refresh_children() - del self.lc_block._xmodule._selected_set # Clear the cache (only needed because we skip saving/re-loading the block) pylint: disable=protected-access + # Clear the cache (only needed because we skip saving/re-loading the block) pylint: disable=protected-access + del self.lc_block._xmodule._selected_set # Check that the event says that one block was removed, leaving one block left: children = self.lc_block.get_child_descriptors() @@ -460,7 +466,8 @@ class TestLibraryContentAnalytics(LibraryContentTest): event_data = self._assert_event_was_published("removed") self.assertEqual(event_data["removed"], [{ "usage_key": unicode(deleted_block_key), - "original_usage_key": None, # Note: original_usage_key info is sadly unavailable because the block has been deleted so that info can no longer be retrieved + "original_usage_key": None, # Note: original_usage_key info is sadly unavailable because the block has been + # deleted so that info can no longer be retrieved "original_usage_version": None, "descendants": [], }]) diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index ca7c07f8a4..d6ce93563e 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -973,10 +973,10 @@ class TestModuleTrackingContext(ModuleStoreTestCase): def test_context_contains_display_name(self, mock_tracker): problem_display_name = u'Option Response Problem' - module_info = self.handle_callback_and_get_module_info_from_event(mock_tracker, problem_display_name) + module_info = self.handle_callback_and_get_module_info(mock_tracker, problem_display_name) self.assertEquals(problem_display_name, module_info['display_name']) - def handle_callback_and_get_module_info_from_event(self, mock_tracker, problem_display_name=None): + def handle_callback_and_get_module_info(self, mock_tracker, problem_display_name=None): """ Creates a fake module, invokes the callback and extracts the 'module' metadata from the emitted problem_check event. @@ -1006,7 +1006,7 @@ class TestModuleTrackingContext(ModuleStoreTestCase): return event['context']['module'] def test_missing_display_name(self, mock_tracker): - actual_display_name = self.handle_callback_and_get_module_info_from_event(mock_tracker)['display_name'] + actual_display_name = self.handle_callback_and_get_module_info(mock_tracker)['display_name'] self.assertTrue(actual_display_name.startswith('problem')) def test_library_source_information(self, mock_tracker): @@ -1017,8 +1017,9 @@ class TestModuleTrackingContext(ModuleStoreTestCase): """ original_usage_key = UsageKey.from_string(u'block-v1:A+B+C+type@problem+block@abcd1234') original_usage_version = ObjectId() - with patch('xmodule.modulestore.mixed.MixedModuleStore.get_block_original_usage', lambda _, key: (original_usage_key, original_usage_version)): - module_info = self.handle_callback_and_get_module_info_from_event(mock_tracker) + mock_get_original_usage = lambda _, key: (original_usage_key, original_usage_version) + with patch('xmodule.modulestore.mixed.MixedModuleStore.get_block_original_usage', mock_get_original_usage): + module_info = self.handle_callback_and_get_module_info(mock_tracker) self.assertIn('original_usage_key', module_info) self.assertEqual(module_info['original_usage_key'], unicode(original_usage_key)) self.assertIn('original_usage_version', module_info) From 05fc6738f6a47ebf73940b7a81221b6ee5521d24 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Mon, 12 Jan 2015 11:14:35 -0800 Subject: [PATCH 8/8] Add in "previous_count" and "max_count" --- .../xmodule/xmodule/library_content_module.py | 36 +++++++++---------- .../xmodule/tests/test_library_content.py | 4 +++ 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/common/lib/xmodule/xmodule/library_content_module.py b/common/lib/xmodule/xmodule/library_content_module.py index c1d5dc9fb5..4b21f66252 100644 --- a/common/lib/xmodule/xmodule/library_content_module.py +++ b/common/lib/xmodule/xmodule/library_content_module.py @@ -221,35 +221,39 @@ class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule): # Already done: return self._selected_set # pylint: disable=access-member-before-definition + selected = set(tuple(k) for k in self.selected) # set of (block_type, block_id) tuples assigned to this student + previous_count = len(selected) + lib_tools = self.runtime.service(self, 'library_tools') format_block_keys = lambda keys: lib_tools.create_block_analytics_summary(self.location.course_key, keys) + def publish_event(event_name, **kwargs): + """ Publish an event for analytics purposes """ + event_data = { + "location": unicode(self.location), + "result": format_block_keys(selected), + "previous_count": previous_count, + "max_count": self.max_count, + } + event_data.update(kwargs) + self.runtime.publish(self, "edx.librarycontentblock.content.{}".format(event_name), event_data) + # Determine which of our children we will show: - selected = set(tuple(k) for k in self.selected) # set of (block_type, block_id) tuples valid_block_keys = set([(c.block_type, c.block_id) for c in self.children]) # pylint: disable=no-member # Remove any selected blocks that are no longer valid: invalid_block_keys = (selected - valid_block_keys) if invalid_block_keys: selected -= invalid_block_keys # Publish an event for analytics purposes: - self.runtime.publish(self, "edx.librarycontentblock.content.removed", { - "location": unicode(self.location), - "removed": format_block_keys(invalid_block_keys), - "reason": "invalid", # Deleted from library or library being used has changed - "result": format_block_keys(selected), - }) + # reason "invalid" means deleted from library or a different library is now being used. + publish_event("removed", removed=format_block_keys(invalid_block_keys), reason="invalid") # If max_count has been decreased, we may have to drop some previously selected blocks: overlimit_block_keys = set() while len(selected) > self.max_count: overlimit_block_keys.add(selected.pop()) if overlimit_block_keys: # Publish an event for analytics purposes: - self.runtime.publish(self, "edx.librarycontentblock.content.removed", { - "location": unicode(self.location), - "removed": format_block_keys(overlimit_block_keys), - "reason": "overlimit", - "result": format_block_keys(selected), - }) + publish_event("removed", removed=format_block_keys(overlimit_block_keys), reason="overlimit") # Do we have enough blocks now? num_to_add = self.max_count - len(selected) if num_to_add > 0: @@ -265,11 +269,7 @@ class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule): selected |= added_block_keys if added_block_keys: # Publish an event for analytics purposes: - self.runtime.publish(self, "edx.librarycontentblock.content.assigned", { - "location": unicode(self.location), - "added": format_block_keys(added_block_keys), - "result": format_block_keys(selected), - }) + publish_event("assigned", added=format_block_keys(added_block_keys)) # Save our selections to the user state, to ensure consistency: self.selected = list(selected) # TODO: this doesn't save from the LMS "Progress" page. # Cache the results diff --git a/common/lib/xmodule/xmodule/tests/test_library_content.py b/common/lib/xmodule/xmodule/tests/test_library_content.py index c3bb661337..77835c04b1 100644 --- a/common/lib/xmodule/xmodule/tests/test_library_content.py +++ b/common/lib/xmodule/xmodule/tests/test_library_content.py @@ -351,6 +351,8 @@ class TestLibraryContentAnalytics(LibraryContentTest): "location": unicode(self.lc_block.location), "added": [block_info], "result": [block_info], + "previous_count": 0, + "max_count": 1, }) self.publisher.reset_mock() @@ -364,6 +366,8 @@ class TestLibraryContentAnalytics(LibraryContentTest): event_data = self._assert_event_was_published("assigned") self.assertEqual(event_data["added"][0]["usage_key"], unicode(new_child.location)) self.assertEqual(len(event_data["result"]), 2) + self.assertEqual(event_data["previous_count"], 1) + self.assertEqual(event_data["max_count"], 2) def test_assigned_descendants(self): """