refactor: deprecates ModuleSystem.render_template

in favor of the added MakoSystem render_template method. Related changes: * Adds the MakoService to the StudioEditModuleRuntime, PreviewModuleSystem, LmsModuleSystem, and XBlockRuntime * MakoService constructor takes a `namespace_prefix` string, so that the CMS PreviewModuleSystem can render to LMS templates, without needing the special render_from_lms helper method. * ModuleSystem.render_template becomes a read-only property, so the constructor calls and test module systems are updated accordingly. * Adds tests for the MakoService and module system shims.
2021-10-05 12:38:42 +10:30
parent 480e8997ec
commit 457f959356
21 changed files with 219 additions and 84 deletions
--- a/common/djangoapps/edxmako/services.py
+++ b/common/djangoapps/edxmako/services.py
@@ -0,0 +1,30 @@
+"""
+Supports rendering an XBlock to HTML using mako templates.
+"""
+
+from xblock.reference.plugins import Service
+
+from common.djangoapps.edxmako.shortcuts import render_to_string
+
+
+class MakoService(Service):
+    """
+    A service for rendering XBlocks to HTML using mako templates.
+
+    Args:
+        namespace_prefix(string): optional prefix to the mako namespace used to find the template file.
+           e.g to access LMS templates from within Studio code, pass namespace_prefix='lms.'
+    """
+    def __init__(
+        self,
+        namespace_prefix='',
+        **kwargs
+    ):
+        super().__init__(**kwargs)
+        self.namespace_prefix = namespace_prefix
+
+    def render_template(self, template_file, dictionary, namespace='main'):
+        """
+        Takes (template_file, dictionary) and returns rendered HTML.
+        """
+        return render_to_string(template_file, dictionary, namespace=self.namespace_prefix + namespace)
--- a/common/djangoapps/edxmako/tests.py
+++ b/common/djangoapps/edxmako/tests.py
@@ -14,6 +14,7 @@ from edx_django_utils.cache import RequestCache

 from common.djangoapps.edxmako import LOOKUP, add_lookup
 from common.djangoapps.edxmako.request_context import get_template_request_context
+from common.djangoapps.edxmako.services import MakoService
 from common.djangoapps.edxmako.shortcuts import (
    is_any_marketing_link_set,
    is_marketing_link_set,
@@ -208,3 +209,23 @@ class MakoRequestContextTest(TestCase):
        the threadlocal REQUEST_CONTEXT.context. This is meant to run in CMS.
        """
        assert "We're having trouble rendering your component" in render_to_string('html_error.html', None)
+
+
+@ddt.ddt
+class MakoServiceTestCase(TestCase):
+    """
+    Tests for the MakoService
+    """
+    @ddt.data(
+        (MakoService(),
+         '<div id="mako_id" ns="main">Testing the MakoService</div>\n'),
+        (MakoService(namespace_prefix='lms.'),
+         '<div id="mako_id" ns="main">Testing the MakoService</div>\n'),
+    )
+    @ddt.unpack
+    def test_render_template(self, service, expected_html):
+        """
+        Tests MakoService.render_template returns the expected rendered content.
+        """
+        html = service.render_template('templates/edxmako.html', {'element_id': 'mako_id'})
+        assert html == expected_html
--- a/common/lib/xmodule/xmodule/tests/init.py
+++ b/common/lib/xmodule/xmodule/tests/init.py
@@ -11,7 +11,6 @@ Run like this:
 import inspect
 import json
 import os
-import pprint
 import sys
 import traceback
 import unittest
@@ -34,7 +33,7 @@ from xmodule.modulestore import ModuleStoreEnum
 from xmodule.modulestore.draft_and_published import ModuleStoreDraftAndPublished
 from xmodule.modulestore.inheritance import InheritanceMixin
 from xmodule.modulestore.xml import CourseLocationManager
-from xmodule.tests.helpers import StubUserService
+from xmodule.tests.helpers import mock_render_template, StubMakoService, StubUserService
 from xmodule.x_module import ModuleSystem, XModuleDescriptor, XModuleMixin


@@ -93,18 +92,13 @@ def get_test_system(
    course_id=CourseKey.from_string('/'.join(['org', 'course', 'run'])),
    user=None,
    user_is_staff=False,
+    render_template=None,
 ):
    """
    Construct a test ModuleSystem instance.

-    By default, the render_template() method simply returns the repr of the
-    context it is passed.  You can override this behavior by monkey patching::
-
-        system = get_test_system()
-        system.render_template = my_render_func
-
-    where `my_render_func` is a function of the form my_render_func(template, context).
-
+    By default, the descriptor system's render_template() method simply returns the repr of the
+    context it is passed.  You can override this by passing in a different render_template argument.
    """
    if not user:
        user = Mock(name='get_test_system.user', is_staff=False)
@@ -114,6 +108,8 @@ def get_test_system(
        user_is_staff=user_is_staff,
    )

+    mako_service = StubMakoService(render_template=render_template)
+
    descriptor_system = get_test_descriptor_system()

    def get_module(descriptor):
@@ -136,7 +132,6 @@ def get_test_system(
        static_url='/static',
        track_function=Mock(name='get_test_system.track_function'),
        get_module=get_module,
-        render_template=mock_render_template,
        replace_urls=str,
        get_real_user=lambda __: user,
        filestore=Mock(name='get_test_system.filestore', root_path='.'),
@@ -144,6 +139,7 @@ def get_test_system(
        hostname="edx.org",
        services={
            'user': user_service,
+            'mako': mako_service,
        },
        xqueue={
            'interface': None,
@@ -161,7 +157,7 @@ def get_test_system(
    )


-def get_test_descriptor_system():
+def get_test_descriptor_system(render_template=None):
    """
    Construct a test DescriptorSystem instance.
    """
@@ -171,7 +167,7 @@ def get_test_descriptor_system():
        load_item=Mock(name='get_test_descriptor_system.load_item'),
        resources_fs=Mock(name='get_test_descriptor_system.resources_fs'),
        error_tracker=Mock(name='get_test_descriptor_system.error_tracker'),
-        render_template=mock_render_template,
+        render_template=render_template or mock_render_template,
        mixins=(InheritanceMixin, XModuleMixin),
        field_data=field_data,
        services={'field-data': field_data},
@@ -180,16 +176,6 @@ def get_test_descriptor_system():
    return descriptor_system


-def mock_render_template(*args, **kwargs):
-    """
-    Pretty-print the args and kwargs.
-
-    Allows us to not depend on any actual template rendering mechanism,
-    while still returning a unicode object
-    """
-    return pprint.pformat((args, kwargs)).encode().decode()
-
-
 class ModelsTest(unittest.TestCase):  # lint-amnesty, pylint: disable=missing-class-docstring

    def test_load_class(self):
--- a/common/lib/xmodule/xmodule/tests/helpers.py
+++ b/common/lib/xmodule/xmodule/tests/helpers.py
@@ -5,6 +5,7 @@ Utility methods for unit tests.

 import filecmp
 from unittest.mock import Mock
+import pprint

 from path import Path as path
 from xblock.reference.user_service import UserService, XBlockUser
@@ -30,6 +31,31 @@ def directories_equal(directory1, directory2):
    return compare_dirs(path(directory1), path(directory2))


+def mock_render_template(*args, **kwargs):
+    """
+    Pretty-print the args and kwargs.
+
+    Allows us to not depend on any actual template rendering mechanism,
+    while still returning a unicode object
+    """
+    return pprint.pformat((args, kwargs)).encode().decode()
+
+
+class StubMakoService:
+    """
+    Stub MakoService for testing modules that use mako templates.
+    """
+
+    def __init__(self, render_template=None):
+        self._render_template = render_template or mock_render_template
+
+    def render_template(self, *args, **kwargs):
+        """
+        Invokes the configured render_template method.
+        """
+        return self._render_template(*args, **kwargs)
+
+
 class StubUserService(UserService):
    """
    Stub UserService for testing the sequence module.
--- a/common/lib/xmodule/xmodule/tests/test_capa_module.py
+++ b/common/lib/xmodule/xmodule/tests/test_capa_module.py
@@ -79,7 +79,8 @@ class CapaFactory:
                              response_num, input_num))

    @classmethod
-    def create(cls, attempts=None, problem_state=None, correct=False, xml=None, override_get_score=True, **kwargs):
+    def create(cls, attempts=None, problem_state=None, correct=False, xml=None, override_get_score=True,
+               render_template=None, **kwargs):
        """
        All parameters are optional, and are added to the created problem if specified.

@@ -95,6 +96,8 @@ class CapaFactory:
                module.

            attempts: also added to instance state.  Will be converted to an int.
+
+            render_template: pass function or Mock for testing
        """
        location = BlockUsageLocator(
            CourseLocator("edX", "capa_test", "2012_Fall", deprecated=True),
@@ -113,8 +116,11 @@ class CapaFactory:
            # since everything else is a string.
            field_data['attempts'] = int(attempts)

-        system = get_test_system(course_id=location.course_key, user_is_staff=kwargs.get('user_is_staff', False))
-        system.render_template = Mock(return_value="<div>Test Template HTML</div>")
+        system = get_test_system(
+            course_id=location.course_key,
+            user_is_staff=kwargs.get('user_is_staff', False),
+            render_template=render_template or Mock(return_value="<div>Test Template HTML</div>"),
+        )
        module = ProblemBlock(
            system,
            DictFieldData(field_data),
@@ -1520,7 +1526,8 @@ class ProblemBlockTest(unittest.TestCase):  # lint-amnesty, pylint: disable=miss
        # assert that we got here without exploding

    def test_get_problem_html(self):
-        module = CapaFactory.create()
+        render_template = Mock(return_value="<div>Test Template HTML</div>")
+        module = CapaFactory.create(render_template=render_template)

        # We've tested the show/hide button logic in other tests,
        # so here we hard-wire the values
@@ -1532,9 +1539,6 @@ class ProblemBlockTest(unittest.TestCase):  # lint-amnesty, pylint: disable=miss
        module.should_show_reset_button = Mock(return_value=show_reset_button)
        module.should_show_save_button = Mock(return_value=show_save_button)

-        # Mock the system rendering function
-        module.system.render_template = Mock(return_value="<div>Test Template HTML</div>")
-
        # Patch the capa problem's HTML rendering
        with patch('capa.capa_problem.LoncapaProblem.get_html') as mock_html:
            mock_html.return_value = "<div>Test Problem HTML</div>"
@@ -1549,7 +1553,7 @@ class ProblemBlockTest(unittest.TestCase):  # lint-amnesty, pylint: disable=miss
        assert html == '<div>Test Template HTML</div>'

        # Check the rendering context
-        render_args, _ = module.system.render_template.call_args
+        render_args, _ = render_template.call_args
        assert len(render_args) == 2

        template_name = render_args[0]
@@ -1584,9 +1588,10 @@ class ProblemBlockTest(unittest.TestCase):  # lint-amnesty, pylint: disable=miss
    def test_demand_hint(self):
        # HTML generation is mocked out to be meaningless here, so instead we check
        # the context dict passed into HTML generation.
-        module = CapaFactory.create(xml=self.demand_xml)
+        render_template = Mock(return_value="<div>Test Template HTML</div>")
+        module = CapaFactory.create(xml=self.demand_xml, render_template=render_template)
        module.get_problem_html()  # ignoring html result
-        context = module.system.render_template.call_args[0][1]
+        context = render_template.call_args[0][1]
        assert context['demand_hint_possible']
        assert context['should_enable_next_hint']

@@ -1621,9 +1626,10 @@ class ProblemBlockTest(unittest.TestCase):  # lint-amnesty, pylint: disable=miss
              <hint>Only demand hint</hint>
            </demandhint>
            </problem>"""
-        module = CapaFactory.create(xml=test_xml)
+        render_template = Mock(return_value="<div>Test Template HTML</div>")
+        module = CapaFactory.create(xml=test_xml, render_template=render_template)
        module.get_problem_html()  # ignoring html result
-        context = module.system.render_template.call_args[0][1]
+        context = render_template.call_args[0][1]
        assert context['demand_hint_possible']
        assert context['should_enable_next_hint']

@@ -1652,9 +1658,10 @@ class ProblemBlockTest(unittest.TestCase):  # lint-amnesty, pylint: disable=miss
                You can add an optional hint like this. Problems that have a hint include a hint button, and this text appears the first time learners select the button.</hint>
            </demandhint>
            </problem>"""
-        module = CapaFactory.create(xml=test_xml)
+        render_template = Mock(return_value="<div>Test Template HTML</div>")
+        module = CapaFactory.create(xml=test_xml, render_template=render_template)
        module.get_problem_html()  # ignoring html result
-        context = module.system.render_template.call_args[0][1]
+        context = render_template.call_args[0][1]
        assert context['demand_hint_possible']
        assert context['should_enable_next_hint']

@@ -1696,7 +1703,8 @@ class ProblemBlockTest(unittest.TestCase):  # lint-amnesty, pylint: disable=miss
        rendering, a "dummy" problem is created with an error
        message to display to the user.
        """
-        module = CapaFactory.create()
+        render_template = Mock(return_value="<div>Test Template HTML</div>")
+        module = CapaFactory.create(render_template=render_template)

        # Save the original problem so we can compare it later
        original_problem = module.lcp
@@ -1705,9 +1713,6 @@ class ProblemBlockTest(unittest.TestCase):  # lint-amnesty, pylint: disable=miss
        # is asked to render itself as HTML
        module.lcp.get_html = Mock(side_effect=Exception("Test"))

-        # Stub out the get_test_system rendering function
-        module.system.render_template = Mock(return_value="<div>Test Template HTML</div>")
-
        # Turn off DEBUG
        module.system.DEBUG = False

@@ -1717,7 +1722,7 @@ class ProblemBlockTest(unittest.TestCase):  # lint-amnesty, pylint: disable=miss
        assert html is not None

        # Check the rendering context
-        render_args, _ = module.system.render_template.call_args
+        render_args, _ = render_template.call_args
        context = render_args[1]
        assert 'error' in context['problem']['html']

@@ -1728,16 +1733,14 @@ class ProblemBlockTest(unittest.TestCase):  # lint-amnesty, pylint: disable=miss
        """
        Test the html response when an error occurs with DEBUG on
        """
-        module = CapaFactory.create()
+        render_template = Mock(return_value="<div>Test Template HTML</div>")
+        module = CapaFactory.create(render_template=render_template)

        # Simulate throwing an exception when the capa problem
        # is asked to render itself as HTML
        error_msg = "Superterrible error happened: ☠"
        module.lcp.get_html = Mock(side_effect=Exception(error_msg))

-        # Stub out the get_test_system rendering function
-        module.system.render_template = Mock(return_value="<div>Test Template HTML</div>")
-
        # Make sure DEBUG is on
        module.system.DEBUG = True

@@ -1747,7 +1750,7 @@ class ProblemBlockTest(unittest.TestCase):  # lint-amnesty, pylint: disable=miss
        assert html is not None

        # Check the rendering context
-        render_args, _ = module.system.render_template.call_args
+        render_args, _ = render_template.call_args
        context = render_args[1]
        assert error_msg in context['problem']['html']

@@ -2111,9 +2114,10 @@ class ProblemBlockTest(unittest.TestCase):  # lint-amnesty, pylint: disable=miss
        """
        Verify that if problem display name is not provided then a default name is used.
        """
-        module = CapaFactory.create(display_name=display_name)
+        render_template = Mock(return_value="<div>Test Template HTML</div>")
+        module = CapaFactory.create(display_name=display_name, render_template=render_template)
        module.get_problem_html()
-        render_args, _ = module.system.render_template.call_args
+        render_args, _ = render_template.call_args
        context = render_args[1]
        assert context['problem']['name'] == module.location.block_type

--- a/common/lib/xmodule/xmodule/tests/test_delay_between_attempts.py
+++ b/common/lib/xmodule/xmodule/tests/test_delay_between_attempts.py
@@ -103,8 +103,7 @@ class CapaFactoryWithDelay:
            # since everything else is a string.
            field_data['attempts'] = int(attempts)

-        system = get_test_system()
-        system.render_template = Mock(return_value="<div>Test Template HTML</div>")
+        system = get_test_system(render_template=Mock(return_value="<div>Test Template HTML</div>"))
        module = ProblemBlock(
            system,
            DictFieldData(field_data),
--- a/common/lib/xmodule/xmodule/tests/test_editing_module.py
+++ b/common/lib/xmodule/xmodule/tests/test_editing_module.py
@@ -22,8 +22,7 @@ class TabsEditingDescriptorTestCase(unittest.TestCase):

    def setUp(self):
        super().setUp()
-        system = get_test_descriptor_system()
-        system.render_template = Mock(return_value="<div>Test Template HTML</div>")
+        system = get_test_descriptor_system(render_template=Mock())
        self.tabs = [
            {
                'name': "Test_css",
--- a/common/lib/xmodule/xmodule/tests/test_xml_module.py
+++ b/common/lib/xmodule/xmodule/tests/test_xml_module.py
@@ -347,8 +347,7 @@ class EditableMetadataFieldsTest(unittest.TestCase):
                non_editable_fields.append(TestModuleDescriptor.due)
                return non_editable_fields

-        system = get_test_descriptor_system()
-        system.render_template = Mock(return_value="<div>Test Template HTML</div>")
+        system = get_test_descriptor_system(render_template=Mock())
        return system.construct_xblock_from_class(TestModuleDescriptor, field_data=field_data, scope_ids=Mock())

    def assert_field_values(self, editable_fields, name, field, explicitly_set, value, default_value,  # lint-amnesty, pylint: disable=dangerous-default-value
--- a/common/lib/xmodule/xmodule/x_module.py
+++ b/common/lib/xmodule/xmodule/x_module.py
@@ -1748,6 +1748,7 @@ class ModuleSystemShim:
    """

    @property
+<<<<<<< HEAD
    def anonymous_student_id(self):
        """
        Returns the anonymous user ID for the current user and course.
@@ -1809,6 +1810,23 @@ class ModuleSystemShim:
            return self._services['user'].get_current_user().opt_attrs.get(ATTR_KEY_USER_IS_STAFF)
        return None

+    @property
+    def render_template(self):
+        """
+        Returns a function that takes (template_file, context), and returns rendered html.
+
+        Deprecated in favor of the mako service.
+        """
+        warnings.warn(
+            'Use of runtime.render_template is deprecated. '
+            'Use MakoService.render_template or a JavaScript-based template instead.',
+            DeprecationWarning, stacklevel=2,
+        )
+        render_service = self._services.get('mako')
+        if render_service:
+            return render_service.render_template
+        return None
+

 class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemShim, Runtime):
    """
@@ -1824,7 +1842,7 @@ class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemShim,
    """

    def __init__(
-            self, static_url, track_function, get_module, render_template,
+            self, static_url, track_function, get_module,
            replace_urls, descriptor_runtime, filestore=None,
            debug=False, hostname="", xqueue=None, publish=None, node_path="",
            course_id=None,
@@ -1846,9 +1864,6 @@ class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemShim,
                         module instance object.  If the current user does not have
                         access to that location, returns None.

-        render_template - a function that takes (template_file, context), and
-                         returns rendered html.
-
        filestore - A filestore ojbect.  Defaults to an instance of OSFS based
                         at settings.DATA_DIR.

@@ -1904,7 +1919,6 @@ class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemShim,
        self.track_function = track_function
        self.filestore = filestore
        self.get_module = get_module
-        self.render_template = render_template
        self.DEBUG = self.debug = debug
        self.HOSTNAME = self.hostname = hostname
        self.replace_urls = replace_urls
--- a/common/test/templates/edxmako.html
+++ b/common/test/templates/edxmako.html
@@ -0,0 +1 @@
+<%page expression_filter="h"/><div id="${element_id}" ns="main">Testing the MakoService</div>
				`@@ -0,0 +1 @@`
				`<%page expression_filter="h"/><div id="${element_id}" ns="main">Testing the MakoService</div>`