From 37b8e6360c19e224c960c2e82daab3885aa39273 Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Thu, 14 Jul 2016 01:22:05 -0400 Subject: [PATCH] HiddenContentTransformer to check for hide_after_due --- lms/djangoapps/course_api/blocks/api.py | 3 +- lms/djangoapps/course_blocks/api.py | 3 - .../transformers/hidden_content.py | 102 +++++++++++++++++ .../course_blocks/transformers/start_date.py | 44 ++------ .../transformers/tests/helpers.py | 38 ++++--- .../transformers/tests/test_hidden_content.py | 83 ++++++++++++++ .../course_blocks/transformers/utils.py | 104 +++++++++++++++++- .../course_blocks/transformers/visibility.py | 31 ++---- setup.py | 1 + 9 files changed, 329 insertions(+), 80 deletions(-) create mode 100644 lms/djangoapps/course_blocks/transformers/hidden_content.py create mode 100644 lms/djangoapps/course_blocks/transformers/tests/test_hidden_content.py diff --git a/lms/djangoapps/course_api/blocks/api.py b/lms/djangoapps/course_api/blocks/api.py index ab8fd53bbc..58371b5c3d 100644 --- a/lms/djangoapps/course_api/blocks/api.py +++ b/lms/djangoapps/course_api/blocks/api.py @@ -3,6 +3,7 @@ API function for retrieving course blocks data """ from lms.djangoapps.course_blocks.api import get_course_blocks, COURSE_BLOCK_ACCESS_TRANSFORMERS +from lms.djangoapps.course_blocks.transformers.hidden_content import HiddenContentTransformer from openedx.core.lib.block_structure.transformers import BlockStructureTransformers from .transformers.blocks_api import BlocksAPITransformer @@ -51,7 +52,7 @@ def get_blocks( # create ordered list of transformers, adding BlocksAPITransformer at end. transformers = BlockStructureTransformers() if user is not None: - transformers += COURSE_BLOCK_ACCESS_TRANSFORMERS + [ProctoredExamTransformer()] + transformers += COURSE_BLOCK_ACCESS_TRANSFORMERS + [ProctoredExamTransformer(), HiddenContentTransformer()] transformers += [ BlocksAPITransformer( block_counts, diff --git a/lms/djangoapps/course_blocks/api.py b/lms/djangoapps/course_blocks/api.py index 6de78d78da..57b8db9076 100644 --- a/lms/djangoapps/course_blocks/api.py +++ b/lms/djangoapps/course_blocks/api.py @@ -2,11 +2,8 @@ API entry point to the course_blocks app with top-level get_course_blocks function. """ -from django.core.cache import cache from openedx.core.djangoapps.content.block_structure.api import get_block_structure_manager -from openedx.core.lib.block_structure.manager import BlockStructureManager from openedx.core.lib.block_structure.transformers import BlockStructureTransformers -from xmodule.modulestore.django import modulestore from .transformers import ( library_content, diff --git a/lms/djangoapps/course_blocks/transformers/hidden_content.py b/lms/djangoapps/course_blocks/transformers/hidden_content.py new file mode 100644 index 0000000000..4e1597513e --- /dev/null +++ b/lms/djangoapps/course_blocks/transformers/hidden_content.py @@ -0,0 +1,102 @@ +""" +Visibility Transformer implementation. +""" +from datetime import datetime +from pytz import utc + +from openedx.core.lib.block_structure.transformer import BlockStructureTransformer, FilteringTransformerMixin +from xmodule.seq_module import SequenceModule +from .utils import collect_merged_boolean_field, collect_merged_date_field + + +MAXIMUM_DATE = utc.localize(datetime.max) + + +class HiddenContentTransformer(FilteringTransformerMixin, BlockStructureTransformer): + """ + A transformer that enforces the hide_after_due field on + blocks by removing children blocks from the block structure for + which the user does not have access. The due and hide_after_due + fields on a block is percolated down to its descendants, so that + all blocks enforce the hidden content settings from their ancestors. + + For a block with multiple parents, access is denied only if + access is denied from all its parents. + + Staff users are exempted from hidden content rules. + """ + VERSION = 1 + MERGED_DUE_DATE = 'merged_due_date' + MERGED_HIDE_AFTER_DUE = 'merged_hide_after_due' + + @classmethod + def name(cls): + """ + Unique identifier for the transformer's class; + same identifier used in setup.py. + """ + return "hidden_content" + + @classmethod + def _get_merged_hide_after_due(cls, block_structure, block_key): + """ + Returns whether the block with the given block_key in the + given block_structure should be visible to staff only per + computed value from ancestry chain. + """ + return block_structure.get_transformer_block_field( + block_key, cls, cls.MERGED_HIDE_AFTER_DUE, False + ) + + @classmethod + def _get_merged_due_date(cls, block_structure, block_key): + """ + Returns the merged value for the start date for the block with + the given block_key in the given block_structure. + """ + return block_structure.get_transformer_block_field( + block_key, cls, cls.MERGED_DUE_DATE, False + ) + + @classmethod + def collect(cls, block_structure): + """ + Collects any information that's necessary to execute this + transformer's transform method. + """ + collect_merged_boolean_field( + block_structure, + transformer=cls, + xblock_field_name='hide_after_due', + merged_field_name=cls.MERGED_HIDE_AFTER_DUE, + ) + + collect_merged_date_field( + block_structure, + transformer=cls, + xblock_field_name='due', + merged_field_name=cls.MERGED_DUE_DATE, + default_date=MAXIMUM_DATE, + func_merge_parents=max, + func_merge_ancestors=min, + ) + + def transform_block_filters(self, usage_info, block_structure): + # Users with staff access bypass the Visibility check. + if usage_info.has_staff_access: + return [block_structure.create_universal_filter()] + + return [ + block_structure.create_removal_filter( + lambda block_key: self._is_block_hidden(block_structure, block_key), + ), + ] + + def _is_block_hidden(self, block_structure, block_key): + """ + Returns whether the block with the given block_key should + be hidden, given the current time. + """ + due = self._get_merged_due_date(block_structure, block_key) + hide_after_due = self._get_merged_hide_after_due(block_structure, block_key) + return not SequenceModule.verify_current_content_visibility(due, hide_after_due) diff --git a/lms/djangoapps/course_blocks/transformers/start_date.py b/lms/djangoapps/course_blocks/transformers/start_date.py index 81d1bb3397..5a8ccb09b1 100644 --- a/lms/djangoapps/course_blocks/transformers/start_date.py +++ b/lms/djangoapps/course_blocks/transformers/start_date.py @@ -5,7 +5,7 @@ from openedx.core.lib.block_structure.transformer import BlockStructureTransform from lms.djangoapps.courseware.access_utils import check_start_date from xmodule.course_metadata_utils import DEFAULT_START_DATE -from .utils import get_field_on_block +from .utils import collect_merged_date_field class StartDateTransformer(FilteringTransformerMixin, BlockStructureTransformer): @@ -36,7 +36,7 @@ class StartDateTransformer(FilteringTransformerMixin, BlockStructureTransformer) return "start_date" @classmethod - def get_merged_start_date(cls, block_structure, block_key): + def _get_merged_start_date(cls, block_structure, block_key): """ Returns the merged value for the start date for the block with the given block_key in the given block_structure. @@ -53,35 +53,15 @@ class StartDateTransformer(FilteringTransformerMixin, BlockStructureTransformer) """ block_structure.request_xblock_fields('days_early_for_beta') - for block_key in block_structure.topological_traversal(): - - # compute merged value of start date from all parents - parents = block_structure.get_parents(block_key) - min_all_parents_start_date = min( - cls.get_merged_start_date(block_structure, parent_key) - for parent_key in parents - ) if parents else None - - # set the merged value for this block - block_start = get_field_on_block(block_structure.get_xblock(block_key), 'start') - if min_all_parents_start_date is None: - # no parents so just use value on block or default - merged_start_value = block_start or DEFAULT_START_DATE - - elif not block_start: - # no value on this block so take value from parents - merged_start_value = min_all_parents_start_date - - else: - # max of merged-start-from-all-parents and this block - merged_start_value = max(min_all_parents_start_date, block_start) - - block_structure.set_transformer_block_field( - block_key, - cls, - cls.MERGED_START_DATE, - merged_start_value - ) + collect_merged_date_field( + block_structure, + transformer=cls, + xblock_field_name='start', + merged_field_name=cls.MERGED_START_DATE, + default_date=DEFAULT_START_DATE, + func_merge_parents=min, + func_merge_ancestors=max, + ) def transform_block_filters(self, usage_info, block_structure): # Users with staff access bypass the Start Date check. @@ -91,7 +71,7 @@ class StartDateTransformer(FilteringTransformerMixin, BlockStructureTransformer) removal_condition = lambda block_key: not check_start_date( usage_info.user, block_structure.get_xblock_field(block_key, 'days_early_for_beta'), - self.get_merged_start_date(block_structure, block_key), + self._get_merged_start_date(block_structure, block_key), usage_info.course_key, ) return [block_structure.create_removal_filter(removal_condition)] diff --git a/lms/djangoapps/course_blocks/transformers/tests/helpers.py b/lms/djangoapps/course_blocks/transformers/tests/helpers.py index af8b5afa77..2ce251f6d1 100644 --- a/lms/djangoapps/course_blocks/transformers/tests/helpers.py +++ b/lms/djangoapps/course_blocks/transformers/tests/helpers.py @@ -255,7 +255,7 @@ class BlockParentsMapTestCase(TransformerRegistryTestMixin, ModuleStoreTestCase) self, test_user, expected_user_accessible_blocks, - blocks_with_differing_access, + blocks_with_differing_access=None, transformers=None, ): """ @@ -272,7 +272,8 @@ class BlockParentsMapTestCase(TransformerRegistryTestMixin, ModuleStoreTestCase) 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. + has_access. If not provided, does not compare with + has_access results. transformers (BlockStructureTransformers): An optional collection of transformers that are to be executed. If not @@ -312,7 +313,6 @@ class BlockParentsMapTestCase(TransformerRegistryTestMixin, ModuleStoreTestCase) # compute access results of the block block_structure_result = xblock_key in block_structure - has_access_result = bool(has_access(user, 'load', self.get_block(i), course_key=self.course.id)) # compare with expected value self.assertEquals( @@ -323,23 +323,25 @@ class BlockParentsMapTestCase(TransformerRegistryTestMixin, ModuleStoreTestCase) ) ) - # 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 + if blocks_with_differing_access: + # compare with has_access_result + has_access_result = bool(has_access(user, 'load', self.get_block(i), course_key=self.course.id)) + 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 + 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() diff --git a/lms/djangoapps/course_blocks/transformers/tests/test_hidden_content.py b/lms/djangoapps/course_blocks/transformers/tests/test_hidden_content.py new file mode 100644 index 0000000000..480480e94f --- /dev/null +++ b/lms/djangoapps/course_blocks/transformers/tests/test_hidden_content.py @@ -0,0 +1,83 @@ +""" +Tests for HiddenContentTransformer. +""" +from datetime import timedelta +import ddt +from django.utils.timezone import now +from nose.plugins.attrib import attr + +from ..hidden_content import HiddenContentTransformer +from .helpers import BlockParentsMapTestCase, update_block + + +@attr('shard_3') +@ddt.ddt +class HiddenContentTransformerTestCase(BlockParentsMapTestCase): + """ + VisibilityTransformer Test + """ + TRANSFORMER_CLASS_TO_TEST = HiddenContentTransformer + ALL_BLOCKS = {0, 1, 2, 3, 4, 5, 6} + + class DueDateType(object): + """ + Use constant enum types for deterministic ddt test method names (rather than dynamically generated timestamps) + """ + none = 1, + future = 2, + past = 3 + + TODAY = now() + PAST_DATE = TODAY - timedelta(days=30) + FUTURE_DATE = TODAY + timedelta(days=30) + + @classmethod + def due(cls, enum_value): + """ + Returns a start date for the given enum value + """ + if enum_value == cls.future: + return cls.FUTURE_DATE + elif enum_value == cls.past: + return cls.PAST_DATE + else: + return None + + # Following test cases are based on BlockParentsMapTestCase.parents_map + @ddt.data( + ({}, ALL_BLOCKS), + + ({0: DueDateType.none}, ALL_BLOCKS), + ({0: DueDateType.future}, ALL_BLOCKS), + ({1: DueDateType.none}, ALL_BLOCKS), + ({1: DueDateType.future}, ALL_BLOCKS), + ({4: DueDateType.none}, ALL_BLOCKS), + ({4: DueDateType.future}, ALL_BLOCKS), + + ({0: DueDateType.past}, {}), + ({1: DueDateType.past}, ALL_BLOCKS - {1, 3, 4}), + ({2: DueDateType.past}, ALL_BLOCKS - {2, 5}), + ({4: DueDateType.past}, ALL_BLOCKS - {4}), + + ({1: DueDateType.past, 2: DueDateType.past}, {0}), + ({1: DueDateType.none, 2: DueDateType.past}, ALL_BLOCKS - {2, 5}), + ({1: DueDateType.past, 2: DueDateType.none}, ALL_BLOCKS - {1, 3, 4}), + ) + @ddt.unpack + def test_hidden_content( + self, + hide_due_values, + expected_visible_blocks, + ): + for idx, due_date_type in hide_due_values.iteritems(): + block = self.get_block(idx) + block.due = self.DueDateType.due(due_date_type) + block.hide_after_due = True + update_block(block) + + self.assert_transform_results( + self.student, + expected_visible_blocks, + blocks_with_differing_access=None, + transformers=self.transformers, + ) diff --git a/lms/djangoapps/course_blocks/transformers/utils.py b/lms/djangoapps/course_blocks/transformers/utils.py index 9884951251..5ca498053b 100644 --- a/lms/djangoapps/course_blocks/transformers/utils.py +++ b/lms/djangoapps/course_blocks/transformers/utils.py @@ -10,7 +10,103 @@ def get_field_on_block(block, field_name, default_value=None): 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 + try: + if block.fields[field_name].is_set_on(block): + return getattr(block, field_name) + except KeyError: + pass + return default_value + + +def collect_merged_boolean_field( + block_structure, + transformer, + xblock_field_name, + merged_field_name, +): + """ + Collects a boolean xBlock field of name xblock_field_name + for the given block_structure and transformer. The boolean + value is percolated down the hierarchy of the block_structure + and stored as a value of merged_field_name in the + block_structure. + + Assumes that the boolean field is False, by default. So, + the value is ANDed across all parents for blocks with + multiple parents and ORed across all ancestors down a single + hierarchy chain. + """ + + for block_key in block_structure.topological_traversal(): + # compute merged value of the boolean field from all parents + parents = block_structure.get_parents(block_key) + all_parents_merged_value = all( # pylint: disable=invalid-name + block_structure.get_transformer_block_field( + parent_key, transformer, merged_field_name, False, + ) + for parent_key in parents + ) if parents else False + + # set the merged value for this block + block_structure.set_transformer_block_field( + block_key, + transformer, + merged_field_name, + ( + all_parents_merged_value or + get_field_on_block( + block_structure.get_xblock(block_key), xblock_field_name, + False, + ) + ) + ) + + +def collect_merged_date_field( + block_structure, + transformer, + xblock_field_name, + merged_field_name, + default_date, + func_merge_parents=min, + func_merge_ancestors=max, +): + """ + Collects a date xBlock field of name xblock_field_name + for the given block_structure and transformer. The date + value is percolated down the hierarchy of the block_structure + and stored as a value of merged_field_name in the + block_structure. + """ + + for block_key in block_structure.topological_traversal(): + + parents = block_structure.get_parents(block_key) + block_date = get_field_on_block(block_structure.get_xblock(block_key), xblock_field_name) + if not parents: + # no parents so just use value on block or default + merged_date_value = block_date or default_date + + else: + # compute merged value of date from all parents + merged_all_parents_date = func_merge_parents( + block_structure.get_transformer_block_field( + parent_key, transformer, merged_field_name, default_date, + ) + for parent_key in parents + ) + + if not block_date: + # no value on this block so take value from parents + merged_date_value = merged_all_parents_date + + else: + # compute merged date of the block and the parent + merged_date_value = func_merge_ancestors(merged_all_parents_date, block_date) + + block_structure.set_transformer_block_field( + block_key, + transformer, + merged_field_name, + merged_date_value + ) diff --git a/lms/djangoapps/course_blocks/transformers/visibility.py b/lms/djangoapps/course_blocks/transformers/visibility.py index 98c3036657..1fad7ff57e 100644 --- a/lms/djangoapps/course_blocks/transformers/visibility.py +++ b/lms/djangoapps/course_blocks/transformers/visibility.py @@ -2,6 +2,7 @@ Visibility Transformer implementation. """ from openedx.core.lib.block_structure.transformer import BlockStructureTransformer, FilteringTransformerMixin +from .utils import collect_merged_boolean_field class VisibilityTransformer(FilteringTransformerMixin, BlockStructureTransformer): @@ -30,7 +31,7 @@ class VisibilityTransformer(FilteringTransformerMixin, BlockStructureTransformer return "visibility" @classmethod - def get_visible_to_staff_only(cls, block_structure, block_key): + def _get_visible_to_staff_only(cls, block_structure, block_key): """ Returns whether the block with the given block_key in the given block_structure should be visible to staff only per @@ -46,26 +47,12 @@ class VisibilityTransformer(FilteringTransformerMixin, BlockStructureTransformer Collects any information that's necessary to execute this transformer's transform method. """ - for block_key in block_structure.topological_traversal(): - - # compute merged value of visible_to_staff_only from all parents - parents = block_structure.get_parents(block_key) - all_parents_visible_to_staff_only = all( # pylint: disable=invalid-name - cls.get_visible_to_staff_only(block_structure, parent_key) - for parent_key in parents - ) if parents else False - - # set the merged value for this block - block_structure.set_transformer_block_field( - block_key, - cls, - cls.MERGED_VISIBLE_TO_STAFF_ONLY, - # merge visible_to_staff_only from all parents and this block - ( - all_parents_visible_to_staff_only or - block_structure.get_xblock(block_key).visible_to_staff_only - ) - ) + collect_merged_boolean_field( + block_structure, + transformer=cls, + xblock_field_name='visible_to_staff_only', + merged_field_name=cls.MERGED_VISIBLE_TO_STAFF_ONLY, + ) def transform_block_filters(self, usage_info, block_structure): # Users with staff access bypass the Visibility check. @@ -74,6 +61,6 @@ class VisibilityTransformer(FilteringTransformerMixin, BlockStructureTransformer return [ block_structure.create_removal_filter( - lambda block_key: self.get_visible_to_staff_only(block_structure, block_key), + lambda block_key: self._get_visible_to_staff_only(block_structure, block_key), ) ] diff --git a/setup.py b/setup.py index 0ae21d650b..5e72d514d6 100644 --- a/setup.py +++ b/setup.py @@ -49,6 +49,7 @@ setup( "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", + "hidden_content = lms.djangoapps.course_blocks.transformers.hidden_content:HiddenContentTransformer", "course_blocks_api = lms.djangoapps.course_api.blocks.transformers.blocks_api:BlocksAPITransformer", "proctored_exam = lms.djangoapps.course_api.blocks.transformers.proctored_exam:ProctoredExamTransformer", "grades = lms.djangoapps.courseware.transformers.grades:GradesTransformer",