PLAT-1419 Make edxmako a proper template backend by jmbowman · Pull Request #16710 · openedx/openedx-platform

jmbowman · 2017-11-29T19:05:42Z

This refactoring wasn't strictly necessary to achieve Django 1.10 compatibility, but it makes it much easier to figure out which recent changes to the Django template system impact us. It also makes it easier to understand our template engine and gets us more in line with modern Django practice in this regard. Notable changes:

edxmako now adds support for Mako templates as a new backend in the TEMPLATES setting.
LMS preview from Studio is just another backend with appropriate name and directory search path.
Context processors are run in a manner much closer to how the default Django template backend does it.
Consolidated the rendering logic from the render_to_response shortcut and Template.render into a single implementation.
We had a lot of code to support providing both a context dictionary and a Django Context object to rendering shortcuts, but never actually used the latter. Django is moving away from having users interact directly with Context objects, so I removed it from the allowed parameters to simplify things.
We were populating most Mako rendering contexts with an unused django_context variable; removed it.
We had an empty management commands directory and an unused templatetag_helpers module; removed them both.
The derived settings implementation was updated to support derivation of values in nested dictionaries and/or lists.

It should now be feasible to deprecate the rendering functions from edxmako.shortcuts in favor of the standard Django rendering shortcuts, but we use these in dozens of modules. Leaving that as an exercise for another time (if it's even worth pursuing).

bmedx

Looks ok, just some nits and questions

bmedx · 2017-11-30T21:33:24Z

+        Collect and cache the active context processors.
+        """
+        context_processors = _builtin_context_processors
+        context_processors += tuple(self.context_processors)


It's so weird and seemingly unnecessary that they made _builtin_context_processors a tuple, but the conf option a list.

Interesting; the original TEMPLATE_CONTEXT_PROCESSORS setting was a tuple, but the new context_processors option under TEMPLATES is a list. Maybe that's just because everything else under TEMPLATES was already a list for ease of customization, so it made sense to have them consistent? I definitely think using a tuple internally is the right call, to avoid accidental modifications.

bmedx · 2017-11-30T21:36:05Z

@@ -17,7 +17,7 @@ class MakoLoader(object):
    """
    This is a Django loader object which will load the template as a
    Mako template if the first line is "## mako". It is based off BaseLoader


Nit: This class looks like it's just "Loader" now

Fixed the package move but not the rename; fixing.

bmedx · 2017-11-30T22:06:55Z

-        context_dictionary['django_context'] = context_instance
        context_dictionary['marketing_link'] = marketing_link
+        context_dictionary['is_any_marketing_link_set'] = is_any_marketing_link_set
+        context_dictionary['is_marketing_link_set'] = is_marketing_link_set


I understand what the comment is saying here, but I'm not sure I get where/why we turn off context processors in tests. Digging through the code hasn't been enlightening. These seem weird here, just trying to understand.

It's not that we deliberately disable them for tests, but that context processors take an HttpRequest as their only argument...and sometimes we don't have an actual web request and the associated machinery, because we're testing something smaller than a full view rendering. This can also be a problem in Celery tasks, etc.

doctoryes · 2017-11-30T22:44:59Z

-        template_locations = settings.MAKO_TEMPLATES
-        for namespace, directories in template_locations.items():
+        for backend in settings.TEMPLATES:
+            if 'Mako' not in backend['BACKEND']:


Nit: Maybe use edxmako here instead?

Sure, that's a little more specific.

doctoryes · 2017-12-01T18:53:50Z

+            raise TemplateDoesNotExist(template_name)
+
+    @cached_property
+    def template_context_processors(self):


What uses this property? I couldn't find any usages.

It's used by Django's RequestContext class in bind_template: https://github.com/django/django/blob/stable/1.8.x/django/template/context.py#L237

doctoryes · 2017-12-01T18:58:37Z

@@ -53,7 +53,8 @@ def load_template(self, template_name, template_dirs=None):
                                output_encoding='utf-8',


Since load_template has been deprecated in the base Loader class since Django 1.9, should we also add a deprecation warning (perhaps shimmed) at the beginning of this method?

We'll need to refactor this to switch to the new methods before we upgrade to 2.0 or above, but we have the whole Python 3 upgrade to finish before that. I created PLAT-1820 to track this, but don't think we want to spam Splunk with lots of deprecation warnings about it.

Agreed - ticket over spam.

doctoryes · 2017-12-01T19:21:07Z

+        if len(args) and isinstance(args[0], MakoTemplate):
+            self.mako_template = args[0]
+        else:
+            kwargs['lookup'] = LOOKUP['main']


Is LOOKUP['main'] guaranteed to exist here?

In practice, yes. It doesn't feel like throwing an exception from __init__ or passing None into the MakoTemplate constructor would be any better at debugging future failures around this than just bubbling up the KeyError; do you have any better suggestions?

Nope - I support just letting the KeyError bubble up if it ever happens. We'd see it in the logs.

doctoryes · 2017-12-01T19:28:39Z

+        """
+        Add to the given dictionary context variables which should always be
+        present, even when context processors aren't run during tests.  Using
+        a context processor should almost always be preferred to adding more


Nit: Perhaps stronger language here? Such as: "Never add to these variables."?

Not sure if that's appropriate; I think the most accurate guidance is "only add things here needed to render the base templates", so this already feels a little on the scare words side to me.

doctoryes · 2017-12-01T19:30:37Z

+        """
+        Render the specified template with the given dictionary of context data.
+        """
+        return self.lookup.get_template(template_filename).render(**dictionary)


That's better.

doctoryes · 2017-12-01T19:45:41Z

        self.module.DICT_VALUE['test_key'] = lambda settings: settings.DERIVED_VALUE * 3
-        derived(('DICT_VALUE', 'test_key'))
+        derived_collection_entry('DICT_VALUE', 'test_key')
+        self.module.DICT_VALUE['list_key'] = ['first', lambda settings: settings.DERIVED_VALUE]


Nit: first -> zeroeth?

I think I'll just change it to something like "not derived".

edx-pipeline-bot · 2017-12-04T20:38:24Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Tuesday, December 05, 2017.

edx-pipeline-bot · 2017-12-05T19:27:46Z

EdX Release Notice: This PR has been deployed to the production environment.

jmbowman force-pushed the jmbowman/PLAT-1419 branch 3 times, most recently from 8781a8c to e4ffab2 Compare November 30, 2017 19:38

jmbowman requested review from bmedx, doctoryes, macdiesel and pwnage101 November 30, 2017 20:35

bmedx approved these changes Nov 30, 2017

View reviewed changes

doctoryes reviewed Dec 1, 2017

View reviewed changes

PLAT-1419 Make edxmako a proper template backend

fdc50c3

jmbowman force-pushed the jmbowman/PLAT-1419 branch from e4ffab2 to fdc50c3 Compare December 1, 2017 22:02

doctoryes approved these changes Dec 4, 2017

View reviewed changes

jmbowman merged commit 067785d into master Dec 4, 2017

jmbowman deleted the jmbowman/PLAT-1419 branch December 4, 2017 16:07

		@@ -53,7 +53,8 @@ def load_template(self, template_name, template_dirs=None):
		output_encoding='utf-8',

Uh oh!

Conversation

jmbowman commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmedx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edx-pipeline-bot commented Dec 4, 2017

Uh oh!

edx-pipeline-bot commented Dec 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jmbowman commented Nov 29, 2017 •

edited

Loading