Merge pull request #27153 from edx/ddumesnil/hidden-content-fix-aa-724-part-2

fix: AA-724: Updating the HiddenContentTransformer

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)

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,
+        )