From 1614fb29566881bf2dcaddf028ebab721735841f Mon Sep 17 00:00:00 2001
From: Dillon Dumesnil <ddumesnil@edx.org>
Date: Fri, 26 Mar 2021 11:34:04 -0700
Subject: [PATCH] fix: AA-724: Updating the HiddenContentTransformer
We heard about a bug where learners granted extensions would still
lose access to content if it was marked as "hidden after due date".
This was caused by the HiddenContentTransformer using the due date
from collection time (publish time) rather than the user date returned
from the edx-when DateOverrideTransformer.
A small subtletly of these PRs is that Transformers with the
FilteringTransformerMixin are executed before those without it so
part of the fix was to make the HiddenContentTransformer not use the
FilteringTransformerMixin to ensure the DateOverrideTransformer had
run first.
Part 2/3 - Updating transform method + updating Read version
---
.../transformers/hidden_content.py | 40 +++++-------
.../transformers/tests/test_hidden_content.py | 62 ++++++++++++++++++-
2 files changed, 74 insertions(+), 28 deletions(-)
diff --git a/lms/djangoapps/course_blocks/transformers/hidden_content.py b/lms/djangoapps/course_blocks/transformers/hidden_content.py
index 9870667cec3..2a331323d46 100644
--- a/lms/djangoapps/course_blocks/transformers/hidden_content.py
+++ b/lms/djangoapps/course_blocks/transformers/hidden_content.py
@@ -7,10 +7,7 @@ from datetime import datetime
from pytz import utc
-from openedx.core.djangoapps.content.block_structure.transformer import (
- BlockStructureTransformer,
- FilteringTransformerMixin
-)
+from openedx.core.djangoapps.content.block_structure.transformer import BlockStructureTransformer
from xmodule.seq_module import SequenceBlock
from .utils import collect_merged_boolean_field, collect_merged_date_field
@@ -18,21 +15,24 @@ from .utils import collect_merged_boolean_field, collect_merged_date_field
MAXIMUM_DATE = utc.localize(datetime.max)
-class HiddenContentTransformer(FilteringTransformerMixin, BlockStructureTransformer):
+class HiddenContentTransformer(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
+ which the user does not have access. The hide_after_due
+ field 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.
+
+ IMPORTANT: Must be run _after_ the DateOverrideTransformer from edx-when
+ in case the 'due' date on a block has been shifted for a user.
"""
WRITE_VERSION = 3
- READ_VERSION = 2
+ READ_VERSION = 3
MERGED_DUE_DATE = 'merged_due_date'
MERGED_HIDE_AFTER_DUE = 'merged_hide_after_due'
@@ -55,16 +55,6 @@ class HiddenContentTransformer(FilteringTransformerMixin, BlockStructureTransfor
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):
"""
@@ -90,16 +80,12 @@ class HiddenContentTransformer(FilteringTransformerMixin, BlockStructureTransfor
block_structure.request_xblock_fields('self_paced', 'end', 'due')
- def transform_block_filters(self, usage_info, block_structure):
+ def transform(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),
- ),
- ]
+ block_structure.remove_block_traversal(lambda block_key: self._is_block_hidden(block_structure, block_key))
def _is_block_hidden(self, block_structure, block_key):
"""
@@ -111,5 +97,9 @@ class HiddenContentTransformer(FilteringTransformerMixin, BlockStructureTransfor
if self_paced:
hidden_date = block_structure[block_structure.root_block_usage_key].end
else:
- hidden_date = self._get_merged_due_date(block_structure, block_key)
+ # Important Note:
+ # A small subtlety of grabbing the due date here is that this transformer relies on the
+ # DateOverrideTransformer (located in edx-when repo) to first set any overrides (one
+ # example is a user receiving an extension on an assignment).
+ hidden_date = block_structure.get_xblock_field(block_key, 'due', None) or MAXIMUM_DATE
return not SequenceBlock.verify_current_content_visibility(hidden_date, hide_after_due)
diff --git a/lms/djangoapps/course_blocks/transformers/tests/test_hidden_content.py b/lms/djangoapps/course_blocks/transformers/tests/test_hidden_content.py
index 4a041f7b6db..20812058c2f 100644
--- a/lms/djangoapps/course_blocks/transformers/tests/test_hidden_content.py
+++ b/lms/djangoapps/course_blocks/transformers/tests/test_hidden_content.py
@@ -8,14 +8,20 @@ from datetime import timedelta
import ddt
from django.utils.timezone import now
-from ..hidden_content import HiddenContentTransformer
-from .helpers import BlockParentsMapTestCase, update_block
+from edx_when.api import get_dates_for_course, set_date_for_block
+from edx_when.field_data import DateOverrideTransformer
+
+from common.djangoapps.student.tests.factories import UserFactory
+from lms.djangoapps.course_blocks.transformers.hidden_content import HiddenContentTransformer
+from lms.djangoapps.course_blocks.transformers.tests.helpers import BlockParentsMapTestCase, update_block
+from openedx.core.djangoapps.content.block_structure.tests.helpers import mock_registered_transformers
+from openedx.core.djangoapps.content.block_structure.transformers import BlockStructureTransformers
@ddt.ddt
class HiddenContentTransformerTestCase(BlockParentsMapTestCase):
"""
- VisibilityTransformer Test
+ HiddenContentTransformer Test
"""
TRANSFORMER_CLASS_TO_TEST = HiddenContentTransformer
ALL_BLOCKS = {0, 1, 2, 3, 4, 5, 6}
@@ -70,6 +76,7 @@ class HiddenContentTransformerTestCase(BlockParentsMapTestCase):
hide_due_values,
expected_visible_blocks,
):
+ """ Tests content is hidden if due date is in the past """
for idx, due_date_type in hide_due_values.items():
block = self.get_block(idx)
block.due = self.DueDateType.due(due_date_type)
@@ -82,3 +89,52 @@ class HiddenContentTransformerTestCase(BlockParentsMapTestCase):
blocks_with_differing_access=None,
transformers=self.transformers,
)
+
+ def test_hidden_content_with_transformer_override(self):
+ """
+ Tests content is hidden if the date changes after collection and
+ during the transform phase (for example, by the DateOverrideTransformer).
+ """
+ with mock_registered_transformers([DateOverrideTransformer, self.TRANSFORMER_CLASS_TO_TEST]):
+ transformers = BlockStructureTransformers(
+ [DateOverrideTransformer(self.student), self.TRANSFORMER_CLASS_TO_TEST()]
+ )
+
+ block = self.get_block(1)
+ block.hide_after_due = True
+ update_block(block)
+ set_date_for_block(self.course.id, block.location, 'due', self.DueDateType.PAST_DATE)
+
+ # Due date is in the past so some blocks are hidden
+ self.assert_transform_results(
+ self.student,
+ self.ALL_BLOCKS - {1, 3, 4},
+ blocks_with_differing_access=None,
+ transformers=transformers,
+ )
+
+ # Set an override for the due date to be in the future
+ set_date_for_block(self.course.id, block.location, 'due', self.DueDateType.FUTURE_DATE, user=self.student)
+ # this line is just to bust the cache for the user so it returns the updated date.
+ get_dates_for_course(self.course.id, user=self.student, use_cached=False)
+
+ # Now all blocks are returned for the student
+ self.assert_transform_results(
+ self.student,
+ self.ALL_BLOCKS,
+ blocks_with_differing_access=None,
+ transformers=transformers,
+ )
+
+ # But not for a different user
+ different_user = UserFactory()
+ with mock_registered_transformers([DateOverrideTransformer, self.TRANSFORMER_CLASS_TO_TEST]):
+ transformers = BlockStructureTransformers(
+ [DateOverrideTransformer(different_user), self.TRANSFORMER_CLASS_TO_TEST()]
+ )
+ self.assert_transform_results(
+ different_user,
+ self.ALL_BLOCKS - {1, 3, 4},
+ blocks_with_differing_access=None,
+ transformers=transformers,
+ )
--
GitLab