From a40d2b65d23d8b44f6b0a592cfbddb2231a1819d Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Wed, 28 Oct 2015 19:05:40 -0400 Subject: [PATCH] Course Blocks App MA-1556 --- lms/djangoapps/course_blocks/__init__.py | 27 ++ lms/djangoapps/course_blocks/api.py | 97 +++++ lms/djangoapps/course_blocks/signals.py | 27 ++ .../course_blocks/transformers/__init__.py | 3 + .../transformers/tests/test_helpers.py | 330 ++++++++++++++++++ .../course_blocks/transformers/utils.py | 16 + lms/djangoapps/course_blocks/usage_info.py | 36 ++ lms/envs/common.py | 4 + lms/envs/dev.py | 5 + lms/envs/test.py | 4 + openedx/core/lib/block_cache/__init__.py | 5 +- openedx/core/lib/block_cache/block_cache.py | 6 + .../core/lib/block_cache/block_structure.py | 17 +- setup.py | 6 + 14 files changed, 575 insertions(+), 8 deletions(-) create mode 100644 lms/djangoapps/course_blocks/__init__.py create mode 100644 lms/djangoapps/course_blocks/api.py create mode 100644 lms/djangoapps/course_blocks/signals.py create mode 100644 lms/djangoapps/course_blocks/transformers/__init__.py create mode 100644 lms/djangoapps/course_blocks/transformers/tests/test_helpers.py create mode 100644 lms/djangoapps/course_blocks/transformers/utils.py create mode 100644 lms/djangoapps/course_blocks/usage_info.py diff --git a/lms/djangoapps/course_blocks/__init__.py b/lms/djangoapps/course_blocks/__init__.py new file mode 100644 index 0000000000..6357a46361 --- /dev/null +++ b/lms/djangoapps/course_blocks/__init__.py @@ -0,0 +1,27 @@ +""" +The Course Blocks app, built upon the Block Cache framework in +openedx.core.lib.block_cache, is a higher layer django app in LMS that +provides additional context of Courses and Users (via usage_info.py) with +implementations for Block Structure Transformers that are related to +block structure course access. + +As described in the Block Cache framework's __init__ module, this +framework provides faster access to course blocks for performance +sensitive features, by caching all transformer-required data so no +modulestore access is necessary during block access. + +It is expected that only Block Access related transformers reside in +this django app, as they are cross-cutting authorization transformers +required across other features. Other higher-level and feature-specific +transformers should be implemented in their own separate apps. + +Note: Currently, some of the implementation is redundant with the +has_access code in courseware/access.py. However, we do have short-term +plans for refactoring the current has_access code to use Course Blocks +instead (https://openedx.atlassian.net/browse/MA-1019). We have +introduced this redundancy in the short-term as an incremental +implementation approach, reducing risk with initial release of this app. +""" + +# Importing signals is necessary to activate the course publish/delete signal handlers. +from . import signals # pylint: disable=unused-import diff --git a/lms/djangoapps/course_blocks/api.py b/lms/djangoapps/course_blocks/api.py new file mode 100644 index 0000000000..995a251b67 --- /dev/null +++ b/lms/djangoapps/course_blocks/api.py @@ -0,0 +1,97 @@ +""" +API entry point to the course_blocks app with top-level +get_course_blocks and clear_course_from_cache functions. +""" +from django.core.cache import cache + +from openedx.core.lib.block_cache.block_cache import get_blocks, clear_block_cache +from xmodule.modulestore.django import modulestore + +from .transformers import ( + library_content, + start_date, + user_partitions, + visibility, +) +from .usage_info import CourseUsageInfo + + +# Default list of transformers for manipulating course block structures +# based on the user's access to the course blocks. +COURSE_BLOCK_ACCESS_TRANSFORMERS = [ + library_content.ContentLibraryTransformer(), + start_date.StartDateTransformer(), + user_partitions.UserPartitionTransformer(), + visibility.VisibilityTransformer(), +] + + +def get_course_blocks( + user, + root_block_usage_key, + transformers=None +): + """ + A higher order function implemented on top of the + block_cache.get_blocks function returning a transformed block + structure for the given user starting at root_block_usage_key. + + Note: The current implementation requires the root_block_usage_key + to be the root block of its corresponding course. However, this + is a short-term limitation, which will be addressed in a coming + ticket (https://openedx.atlassian.net/browse/MA-1604). Once that + ticket is implemented, callers will be able to get course blocks + starting at any arbitrary location within a block structure. + + Arguments: + user (django.contrib.auth.models.User) - User object for + which the block structure is to be transformed. + + root_block_usage_key (UsageKey) - The usage_key for the root + of the block structure that is being accessed. + + transformers ([BlockStructureTransformer]) - The list of + transformers whose transform methods are to be called. + If None, COURSE_BLOCK_ACCESS_TRANSFORMERS is used. + + Returns: + BlockStructureBlockData - A transformed block structure, + starting at root_block_usage_key, that has undergone the + transform methods for the given user and the course + associated with the block structure. If using the default + transformers, the transformed block structure will be + exactly equivalent to the blocks that the given user has + access. + """ + store = modulestore() + if root_block_usage_key != store.make_course_usage_key(root_block_usage_key.course_key): + # Enforce this check for now until MA-1604 is implemented. + # Otherwise, callers will get incorrect block data after a + # new version of the course is published, since + # clear_course_from_cache only clears the cached block + # structures starting at the root block of the course. + raise NotImplementedError + + return get_blocks( + cache, + store, + CourseUsageInfo(root_block_usage_key.course_key, user), + root_block_usage_key, + COURSE_BLOCK_ACCESS_TRANSFORMERS if transformers is None else transformers, + ) + + +def clear_course_from_cache(course_key): + """ + A higher order function implemented on top of the + block_cache.clear_block_cache function that clears the block + structure from the cache for the block structure starting at the + root block of the course for the given course_key. + + Note: See Note in get_course_blocks. Even after MA-1604 is + implemented, this implementation should still be valid since the + entire block structure of the course is cached, even though + arbitrary access to an intermediate block will be supported. + """ + course_usage_key = modulestore().make_course_usage_key(course_key) + return clear_block_cache(cache, course_usage_key) diff --git a/lms/djangoapps/course_blocks/signals.py b/lms/djangoapps/course_blocks/signals.py new file mode 100644 index 0000000000..2767b56a1a --- /dev/null +++ b/lms/djangoapps/course_blocks/signals.py @@ -0,0 +1,27 @@ +""" +Signal handlers for invalidating cached data. +""" +from django.dispatch.dispatcher import receiver + +from xmodule.modulestore.django import SignalHandler + +from .api import clear_course_from_cache + + +@receiver(SignalHandler.course_published) +def _listen_for_course_publish(sender, course_key, **kwargs): # pylint: disable=unused-argument + """ + Catches the signal that a course has been published in the module + store and invalidates the corresponding cache entry if one exists. + """ + clear_course_from_cache(course_key) + + +@receiver(SignalHandler.course_deleted) +def _listen_for_course_delete(sender, course_key, **kwargs): # pylint: disable=unused-argument + """ + Catches the signal that a course has been deleted from the + module store and invalidates the corresponding cache entry if one + exists. + """ + clear_course_from_cache(course_key) diff --git a/lms/djangoapps/course_blocks/transformers/__init__.py b/lms/djangoapps/course_blocks/transformers/__init__.py new file mode 100644 index 0000000000..9ab8802aec --- /dev/null +++ b/lms/djangoapps/course_blocks/transformers/__init__.py @@ -0,0 +1,3 @@ +""" +Module container for all Course Block Access Transformers. +""" diff --git a/lms/djangoapps/course_blocks/transformers/tests/test_helpers.py b/lms/djangoapps/course_blocks/transformers/tests/test_helpers.py new file mode 100644 index 0000000000..6cc5072b60 --- /dev/null +++ b/lms/djangoapps/course_blocks/transformers/tests/test_helpers.py @@ -0,0 +1,330 @@ +""" +Test helpers for testing course block transformers. +""" +from student.tests.factories import CourseEnrollmentFactory, UserFactory +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from lms.djangoapps.courseware.access import has_access + +from ...api import get_course_blocks + + +class CourseStructureTestCase(ModuleStoreTestCase): + """ + Helper for test cases that need to build course structures. + """ + def setUp(self): + """ + Create users. + """ + super(CourseStructureTestCase, self).setUp() + # Set up users. + self.password = 'test' + self.user = UserFactory.create(password=self.password) + self.staff = UserFactory.create(password=self.password, is_staff=True) + + def create_block_id(self, block_type, block_ref): + """ + Returns the block id (display name) that is used in the test + course structures for the given block type and block reference + string. + """ + return '{}_{}'.format(block_type, block_ref) + + def build_xblock(self, block_hierarchy, block_map, parent): + """ + Build an XBlock, add it to block_map, and call build_xblock on + the children defined in block_dict. + + Arguments: + block_hierarchy (BlockStructureDict): Definition of + hierarchy, from this block down. + block_map (dict[str: XBlock]): Mapping from '#ref' values to + their XBlocks. + parent (XBlock): Parent block for this xBlock. + """ + block_type = block_hierarchy['#type'] + block_ref = block_hierarchy['#ref'] + factory = (CourseFactory if block_type == 'course' else ItemFactory) + kwargs = {key: value for key, value in block_hierarchy.iteritems() if key[0] != '#'} + + if block_type != 'course': + kwargs['category'] = block_type + if parent: + kwargs['parent'] = parent + + xblock = factory.create( + display_name=self.create_block_id(block_type, block_ref), + publish_item=True, + **kwargs + ) + block_map[block_ref] = xblock + + for child_hierarchy in block_hierarchy.get('#children', []): + self.build_xblock(child_hierarchy, block_map, xblock) + + def add_parents(self, block_hierarchy, block_map): + """ + Recursively traverse the block_hierarchy and add additional + parents. This method is expected to be called only after all + blocks have been created. + + The additional parents are obtained from the '#parents' field + and is expected to be a list of '#ref' values of the parents. + + Note: if a '#parents' field is found, the block is removed from + the course block since it is expected to not belong to the root. + If the block is meant to be a direct child of the course as + well, the course should be explicitly listed in '#parents'. + + Arguments: + block_hierarchy (BlockStructureDict): + Definition of block hierarchy. + block_map (dict[str: XBlock]): + Mapping from '#ref' values to their XBlocks. + + """ + parents = block_hierarchy.get('#parents', []) + if parents: + block_key = block_map[block_hierarchy['#ref']].location + + # First remove the block from the course. + # It would be re-added to the course if the course was + # explicitly listed in parents. + course = modulestore().get_item(block_map['course'].location) + course.children.remove(block_key) + block_map['course'] = update_block(course) + + # Add this to block to each listed parent. + for parent_ref in parents: + parent_block = modulestore().get_item(block_map[parent_ref].location) + parent_block.children.append(block_key) + block_map[parent_ref] = update_block(parent_block) + + # recursively call the children + for child_hierarchy in block_hierarchy.get('#children', []): + self.add_parents(child_hierarchy, block_map) + + def build_course(self, course_hierarchy): + """ + Build a hierarchy of XBlocks. + + Arguments: + course_hierarchy (BlockStructureDict): Definition of course + hierarchy. + + where a BlockStructureDict is a list of dicts in the form { + 'key1': 'value1', + ... + 'keyN': 'valueN', + '#type': block_type, + '#ref': short_string_for_referencing_block, + '#children': list[BlockStructureDict], + '#parents': list['#ref' values] + } + + Special keys start with '#'; the rest just get passed as + kwargs to Factory.create. + + Note: the caller has a choice of whether to create + (1) a nested block structure with children blocks embedded + within their parents, or + (2) a flat block structure with children blocks defined + alongside their parents and attached via the #parents + field, or + (3) a combination of both #1 and #2 used for whichever + blocks. + + Note 2: When the #parents field is used in addition to the + nested pattern for a block, it specifies additional parents + that aren't already implied by having the block exist within + another block's #children field. + + Returns: + dict[str: XBlock]: + Mapping from '#ref' values to their XBlocks. + """ + block_map = {} + + # build the course tree + for block_hierarchy in course_hierarchy: + self.build_xblock(block_hierarchy, block_map, parent=None) + + # add additional parents if the course is a DAG or built + # linearly (without specifying '#children' values) + for block_hierarchy in course_hierarchy: + self.add_parents(block_hierarchy, block_map) + + return block_map + + def get_block_key_set(self, blocks, *refs): + """ + Gets the set of usage keys that correspond to the list of + #ref values as defined on blocks. + + Returns: set[UsageKey] + """ + xblocks = (blocks[ref] for ref in refs) + return set([xblock.location for xblock in xblocks]) + + +class BlockParentsMapTestCase(ModuleStoreTestCase): + """ + Test helper class for creating a test course of + a graph of vertical blocks based on a parents_map. + """ + + # Tree formed by parent_map: + # 0 + # / \ + # 1 2 + # / \ / \ + # 3 4 / 5 + # \ / + # 6 + # Note the parents must always have lower indices than their + # children. + parents_map = [[], [0], [0], [1], [1], [2], [2, 4]] + + def setUp(self, **kwargs): + super(BlockParentsMapTestCase, self).setUp(**kwargs) + + # create the course + self.course = CourseFactory.create() + + # an ordered list of block locations, where the index + # corresponds to the block's index in the parents_map. + self.xblock_keys = [self.course.location] + + # create all other blocks in the course + for i, parents_index in enumerate(self.parents_map): + if i == 0: + continue # course already created + + # create the block as a vertical + self.xblock_keys.append( + ItemFactory.create( + parent=self.get_block(parents_index[0]), + category="vertical", + ).location + ) + + # add additional parents + if len(parents_index) > 1: + for index in range(1, len(parents_index)): + parent_index = parents_index[index] + parent_block = self.get_block(parent_index) + parent_block.children.append(self.xblock_keys[i]) + update_block(parent_block) + + self.password = 'test' + self.student = UserFactory.create(is_staff=False, username='test_student', password=self.password) + self.staff = UserFactory.create(is_staff=True, username='test_staff', password=self.password) + CourseEnrollmentFactory.create(is_active=True, mode='honor', user=self.student, course_id=self.course.id) + + def assert_transform_results( + self, + test_user, + expected_user_accessible_blocks, + blocks_with_differing_access, + transformers=None, + ): + """ + Verifies the results of transforming the blocks in the course. + + Arguments: + test_user (User): The non-staff user that is being tested. + For example, self.student. + + expected_user_accessible_blocks (set(int)): Set of blocks + (indices) that a student user is expected to have access + to after the transformers are executed. + + blocks_with_differing_access (set(int)): Set of + blocks (indices) whose access will differ from the + transformers result and the current implementation of + has_access. + + transformers (BlockStructureTransformer): An optional list + of transformer that are to be executed. If not + provided, the default value used by get_course_blocks + is used. + """ + def check_results(user, expected_accessible_blocks, blocks_with_differing_access): + """ + Verifies the results of transforming the blocks in the + course for the given user. + """ + + self.client.login(username=user.username, password=self.password) + block_structure = get_course_blocks(user, self.course.location, transformers=transformers) + + # Enumerate through all the blocks that were created in the + # course + for i, xblock_key in enumerate(self.xblock_keys): + + # verify existence of the block + block_structure_result = block_structure.has_block(xblock_key) + has_access_result = bool(has_access(user, 'load', self.get_block(i), course_key=self.course.id)) + + # compare with expected value + self.assertEquals( + block_structure_result, + i in expected_accessible_blocks, + "block_structure return value {0} not equal to expected value for block {1} for user {2}".format( + block_structure_result, i, user.username + ) + ) + + # compare with has_access result + if i in blocks_with_differing_access: + self.assertNotEqual( + block_structure_result, + has_access_result, + "block structure ({0}) & has_access ({1}) results are equal for block {2} for user {3}".format( + block_structure_result, has_access_result, i, user.username + ) + ) + else: + self.assertEquals( + block_structure_result, + has_access_result, + "block structure ({0}) & has_access ({1}) results not equal for block {2} for user {3}".format( + block_structure_result, has_access_result, i, user.username + ) + ) + + self.client.logout() + + # verify given test user has access to expected blocks + check_results( + test_user, + expected_user_accessible_blocks, + blocks_with_differing_access + ) + + # verify staff has access to all blocks + check_results(self.staff, set(range(len(self.parents_map))), {}) + + def get_block(self, block_index): + """ + Helper method to retrieve the requested block (index) from the + modulestore + """ + return modulestore().get_item(self.xblock_keys[block_index]) + + +def update_block(block): + """ + Helper method to update the block in the modulestore + """ + return modulestore().update_item(block, 'test_user') + + +def create_location(org, course, run, block_type, block_id): + """ + Returns the usage key for the given key parameters using the + default modulestore + """ + return modulestore().make_course_key(org, course, run).make_usage_key(block_type, block_id) diff --git a/lms/djangoapps/course_blocks/transformers/utils.py b/lms/djangoapps/course_blocks/transformers/utils.py new file mode 100644 index 0000000000..9884951251 --- /dev/null +++ b/lms/djangoapps/course_blocks/transformers/utils.py @@ -0,0 +1,16 @@ +""" +Common Helper utilities for transformers +""" + + +def get_field_on_block(block, field_name, default_value=None): + """ + Get the field value that is directly set on the xblock. + Do not get the inherited value since field inheritance + returns value from only a single parent chain + (e.g., doesn't take a union in DAGs). + """ + if block.fields[field_name].is_set_on(block): + return getattr(block, field_name) + else: + return default_value diff --git a/lms/djangoapps/course_blocks/usage_info.py b/lms/djangoapps/course_blocks/usage_info.py new file mode 100644 index 0000000000..f07d3d62c3 --- /dev/null +++ b/lms/djangoapps/course_blocks/usage_info.py @@ -0,0 +1,36 @@ +""" +Declares CourseUsageInfo class to be used by the transform method in +Transformers. +""" +from lms.djangoapps.courseware.access import _has_access_to_course + + +class CourseUsageInfo(object): + ''' + A class object that encapsulates the course and user context to be + used as currency across block structure transformers, by passing + an instance of it in calls to BlockStructureTransformer.transform + methods. + ''' + def __init__(self, course_key, user): + # Course identifier (opaque_keys.edx.keys.CourseKey) + self.course_key = course_key + + # User object (django.contrib.auth.models.User) + self.user = user + + # Cached value of whether the user has staff access (bool/None) + self._has_staff_access = None + + @property + def has_staff_access(self): + ''' + Returns whether the user has staff access to the course + associated with this CourseUsageInfo instance. + + For performance reasons (minimizing multiple SQL calls), the + value is cached within this instance. + ''' + if self._has_staff_access is None: + self._has_staff_access = _has_access_to_course(self.user, 'staff', self.course_key) + return self._has_staff_access diff --git a/lms/envs/common.py b/lms/envs/common.py index 1aa953a7a7..6072e7087e 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -1954,8 +1954,12 @@ INSTALLED_APPS = ( 'lms.djangoapps.lms_xblock', + # Course data caching 'openedx.core.djangoapps.content.course_overviews', 'openedx.core.djangoapps.content.course_structures', + 'lms.djangoapps.course_blocks', + + # Old course structure API 'course_structure_api', # Mailchimp Syncing diff --git a/lms/envs/dev.py b/lms/envs/dev.py index 0dc306b5c1..f8b00de05e 100644 --- a/lms/envs/dev.py +++ b/lms/envs/dev.py @@ -97,6 +97,11 @@ CACHES = { 'BACKEND': 'django.core.cache.backends.locmem.LocMemCache', 'LOCATION': 'edx_course_structure_mem_cache', }, + 'lms.course_blocks': { + 'BACKEND': 'django.core.cache.backends.locmem.LocMemCache', + 'KEY_FUNCTION': 'util.memcache.safe_key', + 'LOCATION': 'lms_course_blocks_cache', + }, } diff --git a/lms/envs/test.py b/lms/envs/test.py index ba943ab5e6..91fe133dd9 100644 --- a/lms/envs/test.py +++ b/lms/envs/test.py @@ -223,6 +223,10 @@ CACHES = { 'BACKEND': 'django.core.cache.backends.locmem.LocMemCache', 'LOCATION': 'edx_location_block_cache', }, + 'lms.course_blocks': { + 'BACKEND': 'django.core.cache.backends.locmem.LocMemCache', + 'LOCATION': 'edx_location_course_blocks', + }, } # Dummy secret key for dev diff --git a/openedx/core/lib/block_cache/__init__.py b/openedx/core/lib/block_cache/__init__.py index 28ba7eaae0..4cc1cb613e 100644 --- a/openedx/core/lib/block_cache/__init__.py +++ b/openedx/core/lib/block_cache/__init__.py @@ -51,7 +51,10 @@ Registry. Transformers are registered using the platform's PluginManager (e.g., Stevedore). This is currently done by updating setup.py. Only registered transformers are called during the Collect Phase. And only registered transformers can be used during the -Transform phase. +Transform phase. Exceptions to this rule are any nested transformers +that are contained within higher-order transformers - as long as the +higher-order transformers are registered and appropriately call the +contained transformers within them. Note: A partial subset (as an ordered list) of the registered transformers can be requested during the Transform phase, allowing diff --git a/openedx/core/lib/block_cache/block_cache.py b/openedx/core/lib/block_cache/block_cache.py index dba7adc608..7914cec3e3 100644 --- a/openedx/core/lib/block_cache/block_cache.py +++ b/openedx/core/lib/block_cache/block_cache.py @@ -36,6 +36,12 @@ def get_blocks(cache, modulestore, usage_info, root_block_usage_key, transformer transformers whose transform methods are to be called. This list should be a subset of the list of registered transformers in the Transformer Registry. + + Returns: + BlockStructureBlockData - A transformed block structure, + starting at root_block_usage_key, that has undergone the + transform methods in the given transformers with the + given usage_info. """ # Verify that all requested transformers are registered in the diff --git a/openedx/core/lib/block_cache/block_structure.py b/openedx/core/lib/block_cache/block_structure.py index f6a0817daa..a8db0497e7 100644 --- a/openedx/core/lib/block_cache/block_structure.py +++ b/openedx/core/lib/block_cache/block_structure.py @@ -118,6 +118,16 @@ class BlockStructure(object): """ return usage_key in self._block_relations + def get_block_keys(self): + """ + Returns the block keys in the block structure. + + Returns: + iterator(UsageKey) - An iterator of the usage + keys of all the blocks in the block structure. + """ + return self._block_relations.iterkeys() + #--- Block structure traversal methods ---# def topological_traversal( @@ -198,13 +208,6 @@ class BlockStructure(object): # Replace this structure's relations with the newly pruned one. self._block_relations = pruned_block_relations - def _get_block_keys(self): - """ - Returns an iterator of all the block keys in the block - structure. - """ - return self._block_relations.iterkeys() - def _add_relation(self, parent_key, child_key): """ Adds a parent to child relationship in this block structure. diff --git a/setup.py b/setup.py index 6bae27c051..b59b5fd156 100644 --- a/setup.py +++ b/setup.py @@ -48,5 +48,11 @@ setup( "cohort = openedx.core.djangoapps.course_groups.partition_scheme:CohortPartitionScheme", "verification = openedx.core.djangoapps.credit.partition_schemes:VerificationPartitionScheme", ], + "openedx.block_structure_transformer": [ + "library_content = lms.djangoapps.course_blocks.transformers.library_content:ContentLibraryTransformer", + "start_date = lms.djangoapps.course_blocks.transformers.start_date:StartDateTransformer", + "user_partitions = lms.djangoapps.course_blocks.transformers.user_partitions:UserPartitionTransformer", + "visibility = lms.djangoapps.course_blocks.transformers.visibility:VisibilityTransformer", + ], } )