Merge pull request #15905 from open-craft/bradmerlin/html-xblock-student_view_data-upgrade

Add student_view_data to HTML XBlock

common/lib/xmodule/xmodule/html_module.py

@@ -6,6 +6,7 @@ import sys
 import textwrap
 from datetime import datetime
 
+from django.conf import settings
 from fs.errors import ResourceNotFoundError
 from lxml import etree
 from path import Path as path
@@ -69,6 +70,8 @@ class HtmlBlock(object):
         scope=Scope.settings
     )
 
+    ENABLE_HTML_XBLOCK_STUDENT_VIEW_DATA = 'ENABLE_HTML_XBLOCK_STUDENT_VIEW_DATA'
+
     @XBlock.supports("multi_device")
     def student_view(self, _context):
         """
@@ -76,13 +79,25 @@ class HtmlBlock(object):
         """
         return Fragment(self.get_html())
 
+    def student_view_data(self, context=None):  # pylint: disable=unused-argument
+        """
+        Return a JSON representation of the student_view of this XBlock.
+        """
+        if getattr(settings, 'FEATURES', {}).get(self.ENABLE_HTML_XBLOCK_STUDENT_VIEW_DATA, False):
+            return {'enabled': True, 'html': self.get_html()}
+        else:
+            return {
+                'enabled': False,
+                'message': 'To enable, set FEATURES["{}"]'.format(self.ENABLE_HTML_XBLOCK_STUDENT_VIEW_DATA)
+            }
+
     def get_html(self):
         """ Returns html required for rendering XModule. """
 
         # When we switch this to an XBlock, we can merge this with student_view,
         # but for now the XModule mixin requires that this method be defined.
         # pylint: disable=no-member
-        if self.system.anonymous_student_id:
+        if self.data is not None and getattr(self.system, 'anonymous_student_id', None) is not None:
             return self.data.replace("%%USER_ID%%", self.system.anonymous_student_id)
         return self.data
 

common/lib/xmodule/xmodule/tests/test_html_module.py

 import unittest
-
 from mock import Mock
+import ddt
+
+from django.test.utils import override_settings
+
 from opaque_keys.edx.locator import CourseLocator
 from xblock.field_data import DictFieldData
 from xblock.fields import ScopeIds
@@ -24,6 +27,60 @@ def instantiate_descriptor(**field_data):
     )
 
 
+@ddt.ddt
+class HtmlModuleCourseApiTestCase(unittest.TestCase):
+    """
+    Test the HTML XModule's student_view_data method.
+    """
+
+    @ddt.data(
+        dict(),
+        dict(FEATURES={}),
+        dict(FEATURES=dict(ENABLE_HTML_XBLOCK_STUDENT_VIEW_DATA=False))
+    )
+    def test_disabled(self, settings):
+        """
+        Ensure that student_view_data does not return html if the ENABLE_HTML_XBLOCK_STUDENT_VIEW_DATA feature flag
+        is not set.
+        """
+        descriptor = Mock()
+        field_data = DictFieldData({'data': '<h1>Some HTML</h1>'})
+        module_system = get_test_system()
+        module = HtmlModule(descriptor, module_system, field_data, Mock())
+
+        with override_settings(**settings):
+            self.assertEqual(module.student_view_data(), dict(
+                enabled=False,
+                message='To enable, set FEATURES["ENABLE_HTML_XBLOCK_STUDENT_VIEW_DATA"]',
+            ))
+
+    @ddt.data(
+        '<h1>Some content</h1>',  # Valid HTML
+        '',
+        None,
+        '<h1>Some content</h',  # Invalid HTML
+        '<script>alert()</script>',  # Does not escape tags
+        '<img src="data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7">',  # Images allowed
+        'short string ' * 100,  # May contain long strings
+    )
+    @override_settings(FEATURES=dict(ENABLE_HTML_XBLOCK_STUDENT_VIEW_DATA=True))
+    def test_common_values(self, html):
+        """
+        Ensure that student_view_data will return HTML data when enabled,
+        can handle likely input,
+        and doesn't modify the HTML in any way.
+
+        This means that it does NOT protect against XSS, escape HTML tags, etc.
+
+        Note that the %%USER_ID%% substitution is tested below.
+        """
+        descriptor = Mock()
+        field_data = DictFieldData({'data': html})
+        module_system = get_test_system()
+        module = HtmlModule(descriptor, module_system, field_data, Mock())
+        self.assertEqual(module.student_view_data(), dict(enabled=True, html=html))
+
+
 class HtmlModuleSubstitutionTestCase(unittest.TestCase):
     descriptor = Mock()
 

lms/djangoapps/course_api/blocks/tests/test_views.py

@@ -22,7 +22,7 @@ class TestBlocksView(SharedModuleStoreTestCase):
     Test class for BlocksView
     """
     requested_fields = ['graded', 'format', 'student_view_multi_device', 'children', 'not_a_field', 'due']
-    BLOCK_TYPES_WITH_STUDENT_VIEW_DATA = ['video', 'discussion']
+    BLOCK_TYPES_WITH_STUDENT_VIEW_DATA = ['video', 'discussion', 'html']
 
     @classmethod
     def setUpClass(cls):

lms/djangoapps/course_api/blocks/transformers/tests/test_student_view.py

 """
 Tests for StudentViewTransformer.
 """
+import ddt
 
 # pylint: disable=protected-access
-
 from openedx.core.djangoapps.content.block_structure.factory import BlockStructureFactory
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.tests.factories import ToyCourseFactory
@@ -11,6 +11,7 @@ from xmodule.modulestore.tests.factories import ToyCourseFactory
 from ..student_view import StudentViewTransformer
 
 
+@ddt.ddt
 class TestStudentViewTransformer(ModuleStoreTestCase):
     """
     Test proper behavior for StudentViewTransformer
@@ -21,20 +22,27 @@ class TestStudentViewTransformer(ModuleStoreTestCase):
         self.course_usage_key = self.store.make_course_usage_key(self.course_key)
         self.block_structure = BlockStructureFactory.create_from_modulestore(self.course_usage_key, self.store)
 
-    def test_transform(self):
+    @ddt.data(
+        'video', 'html', ['video', 'html'], [],
+    )
+    def test_transform(self, requested_student_view_data):
         # collect phase
         StudentViewTransformer.collect(self.block_structure)
         self.block_structure._collect_requested_xblock_fields()
 
         # transform phase
-        StudentViewTransformer('video').transform(usage_info=None, block_structure=self.block_structure)
+        StudentViewTransformer(requested_student_view_data).transform(
+            usage_info=None,
+            block_structure=self.block_structure,
+        )
 
-        # verify video data
+        # verify video data returned iff requested
         video_block_key = self.course_key.make_usage_key('video', 'sample_video')
-        self.assertIsNotNone(
+        self.assertEqual(
             self.block_structure.get_transformer_block_field(
                 video_block_key, StudentViewTransformer, StudentViewTransformer.STUDENT_VIEW_DATA,
-            )
+            ) is not None,
+            'video' in requested_student_view_data
         )
         self.assertFalse(
             self.block_structure.get_transformer_block_field(
@@ -42,12 +50,13 @@ class TestStudentViewTransformer(ModuleStoreTestCase):
             )
         )
 
-        # verify html data
+        # verify html data returned iff requested
         html_block_key = self.course_key.make_usage_key('html', 'toyhtml')
-        self.assertIsNone(
+        self.assertEqual(
             self.block_structure.get_transformer_block_field(
                 html_block_key, StudentViewTransformer, StudentViewTransformer.STUDENT_VIEW_DATA,
-            )
+            ) is not None,
+            'html' in requested_student_view_data
         )
         self.assertTrue(
             self.block_structure.get_transformer_block_field(

lms/envs/common.py

@@ -413,6 +413,9 @@ FEATURES = {
 
     # Set to enable Enterprise integration
     'ENABLE_ENTERPRISE_INTEGRATION': False,
+
+    # Whether HTML XBlocks/XModules return HTML content with the Course Blocks API student_view_data
+    'ENABLE_HTML_XBLOCK_STUDENT_VIEW_DATA': False,
 }
 
 # Settings for the course reviews tool template and identification key, set either to None to disable course reviews

openedx/core/djangoapps/content/block_structure/block_structure.py

@@ -312,7 +312,7 @@ class FieldData(object):
         if self._is_own_field(field_name):
             return super(FieldData, self).__delattr__(field_name)
         else:
-            delattr(self.fields, field_name)
+            del self.fields[field_name]
 
     def _is_own_field(self, field_name):
         """