Allow theme template block overrides. by mtyaka · Pull Request #16856 · openedx/openedx-platform

mtyaka · 2017-12-11T09:19:06Z

This allows an overridding template from a theme to inherit from the same corresponding standard template.

This is useful when you only want to override one or more named blocks, but otherwise make no modifications to the standard template.

This functionality was already implemented in https://github.com/edx/edx-platform/pull/15947, but then reverted in https://github.com/edx/edx-platform/pull/16729 after some issues were discovered.

This PR fixes the issue and adds tests for it.

Dependencies: None

Sandbox URL:

Deployment targets: edx.org and edge.edx.org

Merge deadline: None

Testing instructions:

Create a test theme:
- make a directory /edx/app/edxapp/themes/testtheme
- create a template at /edx/app/edxapp/themes/testtheme/lms/templates/dashboard.html, and put this content into it:

<%inherit file="dashboard.html" />
<%block name="pagetitle">Overridden Title!</%block>
${parent.body()}

After you restart the LMS, you should be able to see "Overriddden Title!" used as the page title on the student dashboard, if the patch is working correctly.

Author notes and concerns:
Note: Some templates are cached, so if you don't see the changes, you can try wiping the cache by doing rm -rf /tmp/mako_* and sudo service memcached restart.

openedx-webhooks · 2017-12-11T09:19:14Z

Thanks for the pull request, @mtyaka! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

Create an OSPR issue for this pull request.

clemente

@mtyaka Thanks! Great work! And yes, this is more complex than the initial PR.

I tested this in many ways (I'll write the list later) and it works.
In the strange cases it fails with the same errors as master (e.g. maximum recursion depth exceeded). I don't think we need tests for that. These include: invalid templates, inheriting from the wrong one, …

My comments are mainly to try to make the code easier to understand and you can do any other changes that help with that.

It would be much better if everything could be done from get_template, including knowing where to find the file (whether in theme or in standard theme), but I suppose you tried and couldn''t find it and therefore did the „marker class“. Maybe it's useful to add why it can't be done from get_template.

I didn't run the tests yet but I see they passed.

I'll test this more when I use it in some theme adaptation, but I hope I won't find major changes.

clemente · 2017-12-13T05:38:33Z

dasboard → dashboard

Fixed, thanks.

clemente · 2017-12-13T05:45:47Z

In comments there are words „inherit“, „call“, „import“, though I think they refer to the same and it's confusing; would it improve by e.g. „importing“ → „inheriting from“? Or any other simplification.
Maybe the comments can mention a single example? Only if it helps. If adding more text makes it more confusing, then not.

This method is called when a mako template either <%inherit>s or <%include>s another template.
I can see why the comments are confusing, I'll try to make them clearer.

clemente · 2017-12-13T05:48:32Z

This part is also computed in line 109. Although now the flow is clear, I think the repeated part (and the comment in 108) can be computed earlier into a variable, to avoid duplication.

Good idea - I put the repeated parts into a separate private method.

clemente · 2017-12-13T05:57:18Z

At first I didn't understand self-inheritance; maybe it helps to mention that it's the same as before. 1-word fix: just saying „When this self-inheritance […]“ already links it to the description before.

clemente · 2017-12-13T06:00:19Z

Why is HTML(…) needed, when we already have "h"?

HTML is the reverse of h. <%page expression_filter="h"/> at the top of the template means all interpolated strings are HTML escaped by default, so you have to use HTML to explicitly disable that behavior.

EdX quality scripts enforce that all mako templates have <%page expression_filter="h"/> enabled, which is why I used it here even though it's technically unnecessary.

clemente · 2017-12-13T06:04:39Z

I think you should also self.assertContains(resp, "Explore courses") or similar, to check that we're getting the full dashboard with our modifications on top, not just a blank template with the content of the overridden blocks.
Same in the other „dashboard“ test. Or this could be in its own test.

I think the test_parent_content_in_self_inherited_template test covers this case. They are testing the same theme with one test testing the overwritten fields and one testing the non overwritten fields.

Yes, the test_parent_content_in_self_inherited_template covers this case, but this test did not yet exist when @clemente made that comment, I added it later.

mtyaka

Thanks for the review @clemente!

It would be much better if everything could be done from get_template, including knowing where to find the file (whether in theme or in standard theme), but I suppose you tried and couldn''t find it and therefore did the „marker class“.

We cannot do it from get_template because we can't get access to the calling template uri in that method. Make does not pass the calling uri when calling get_template on the template loader.

mtyaka · 2017-12-14T09:12:02Z

Fixed, thanks.

mtyaka · 2017-12-14T09:12:23Z

mtyaka · 2017-12-14T09:14:40Z

This method is called when a mako template either <%inherit>s or <%include>s another template.
I can see why the comments are confusing, I'll try to make them clearer.

mtyaka · 2017-12-14T09:20:03Z

Good idea - I put the repeated parts into a separate private method.

mtyaka · 2017-12-14T09:22:43Z

HTML is the reverse of h. <%page expression_filter="h"/> at the top of the template means all interpolated strings are HTML escaped by default, so you have to use HTML to explicitly disable that behavior.

EdX quality scripts enforce that all mako templates have <%page expression_filter="h"/> enabled, which is why I used it here even though it's technically unnecessary.

clemente

👍

I tested this:
- expectedly fails when defining two times the same block
- expectedly fails when including non-existing file
- expectedly fails when inheriting from two templates at the same time
- when defining unexisting block, its content appears
- I tried to copy/paste the original courses.html to a theme courses.html, still works
- I tried an include inside a block
- complex things and invalid values, like inheriting from a totally different template, fail the same as in the original code
- tests run
- I used this in one file of a client theme
I read through the code
I checked for accessibility issues: none
Includes documentation, as comments in the code and functions/classes

@mtyaka everything works, thanks

mtyaka · 2017-12-15T08:00:24Z

@bderusha @tuchfarber This reimplements ability to override specific named block overrides from a theme template without having to copy the entire template, which was first introduced in https://github.com/edx/edx-platform/pull/15947, but later reverted in https://github.com/edx/edx-platform/pull/16729 after some issues were discovered.

This PR fixes the issues and adds extra tests to cover some edge cases.

BTW, the OSPR form currently doesn't work - I get a 500 error when I try to create a new OSPR issues for this PR.

tuchfarber · 2017-12-21T15:30:04Z

I've tested with the themes it previously broke on and them seem to be working now. Excellent work! Still need to go through the code though.

tuchfarber

Looks good to me

tuchfarber · 2017-12-21T15:39:13Z

I think the test_parent_content_in_self_inherited_template test covers this case. They are testing the same theme with one test testing the overwritten fields and one testing the non overwritten fields.

This allows an overridding template from a theme to inherit from the same corresponding standard template. This is useful when you only want to override one or more named blocks, but otherwise make no modifications to the standard template.

mtyaka · 2017-12-22T08:42:05Z

Squash from a70a4755ed28c95898d29b3e18c7b50f7ee19319.

mtyaka · 2017-12-22T08:42:42Z

Thanks @tuchfarber! I don't have permission to merge this, any chance you could do that after the build is green?

edx-pipeline-bot · 2017-12-27T18:43:38Z

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

edx-pipeline-bot · 2018-01-03T17:32:56Z

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

Rewritten most of the course_about.html template through blocks. This depends on the new template block inheritance system (merged at https://github.com/edx/edx-platform/pull/16856), and it requires the new blocks to be defined in edx-platform too.

edx-pipeline-bot · 2018-01-04T14:50:13Z

EdX Release Notice: This PR has been rolled back from the production environment.

clemente suggested changes Dec 13, 2017

View reviewed changes

clemente mentioned this pull request Dec 13, 2017

Allow theme template block override #15947

Merged

2 tasks

mtyaka force-pushed the mtyaka/template-block-overrides branch from 31e4917 to a70a475 Compare December 14, 2017 10:12

mtyaka commented Dec 14, 2017

View reviewed changes

clemente approved these changes Dec 15, 2017

View reviewed changes

tuchfarber approved these changes Dec 21, 2017

View reviewed changes

Allow theme template block overrides.

f6f29ca

This allows an overridding template from a theme to inherit from the same corresponding standard template. This is useful when you only want to override one or more named blocks, but otherwise make no modifications to the standard template.

mtyaka force-pushed the mtyaka/template-block-overrides branch from a70a475 to f6f29ca Compare December 22, 2017 08:42

tuchfarber merged commit 486f957 into openedx:master Dec 22, 2017

bradenmacdonald deleted the mtyaka/template-block-overrides branch December 22, 2017 17:09

This was referenced Dec 27, 2017

Add some blocks to course_about.html open-craft/openedx-platform#96

Merged

Rewrite course_about.html to use new blocks open-craft/edx-theme#28

Merged

This was referenced Jan 4, 2018

Allow theme template block overrides. open-craft/openedx-platform#101

Merged

Add some blocks to course_about.html #17091

Merged

Uh oh!

Conversation

mtyaka commented Dec 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openedx-webhooks commented Dec 11, 2017

Uh oh!

clemente 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!

mtyaka 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!

clemente left a comment

Choose a reason for hiding this comment

Uh oh!

mtyaka commented Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tuchfarber commented Dec 21, 2017

Uh oh!

tuchfarber left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtyaka commented Dec 22, 2017

Uh oh!

mtyaka commented Dec 22, 2017

Uh oh!

edx-pipeline-bot commented Dec 27, 2017

Uh oh!

edx-pipeline-bot commented Jan 3, 2018

Uh oh!

edx-pipeline-bot commented Jan 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mtyaka commented Dec 11, 2017 •

edited

Loading

mtyaka commented Dec 15, 2017 •

edited

Loading