Skip to content

Allow theme template block override - #15947

Merged
UmanShahzad merged 1 commit into
openedx:masterfrom
open-craft:allow-theme-template-block-override
Nov 29, 2017
Merged

Allow theme template block override#15947
UmanShahzad merged 1 commit into
openedx:masterfrom
open-craft:allow-theme-template-block-override

Conversation

@cgopalan

@cgopalan cgopalan commented Sep 2, 2017

Copy link
Copy Markdown

Stanford implemented the ability to override only specific named blocks of templates from a comprehensive theme, but they never contributed it upstream.

This is the PR: Stanford-Online#535

Dependencies: None

Sandbox URL: TBD - sandbox is being provisioned.

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

Merge deadline: None

Testing instructions:

  1. 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()}
  1. 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.

Reviewers

Settings
In lms.env.json ensure that

COMPREHENSIVE_THEME_DIRS: ["/edx/app/edxapp/themes"]
ENABLE_COMPREHENSIVE_THEMING: true
DEFAULT_SITE_THEME: "testtheme"

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @cgopalan! 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.

@cgopalan

cgopalan commented Sep 2, 2017

Copy link
Copy Markdown
Author

@mtyaka @bradenmacdonald theres one test thats failing which I am not sure why. If I run that test in my local, it fails for the master branch too. So its not related to this PR.

@cgopalan
cgopalan force-pushed the allow-theme-template-block-override branch from 4782a82 to 9bed181 Compare September 10, 2017 07:21
@cgopalan

Copy link
Copy Markdown
Author

@mtyaka This should be ready for review again. I have added a comment explaining why it works, and also added some tests.

@mtyaka mtyaka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cgopalan This looks great, thank you! I noticed a few typos. Can you fix those and then open an OSPR ticket, please?

Comment thread common/djangoapps/edxmako/paths.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: thats -> that's

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: missing newline at end of file

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing "from": "comes from the..."

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing "from": "comes from the..."

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duh! Thanks for catching those silly errors. Must be because of the late-night work! Will fix ASAP.

@cgopalan
cgopalan force-pushed the allow-theme-template-block-override branch from 9bed181 to 96e704e Compare September 12, 2017 01:42
@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @cgopalan! I've created OSPR-1889 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • edx-code email threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

If you like, you can add yourself to the AUTHORS file for this repo, though that isn't required. Please see the CONTRIBUTING file for more information.

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Sep 12, 2017
@cgopalan

Copy link
Copy Markdown
Author

@mtyaka The same python tests failed as a couple of builds before - I suspect it might be due to some timezone issues, since the test is looking for a certain date (Sep 11) but finds the next day (Sep 12). Also not sure why the bokchoy job failed as I see no test failures.
Can you trigger the failed builds again?

@nedbat

nedbat commented Nov 3, 2017

Copy link
Copy Markdown
Contributor

@edx/learner Who can review this?

@UmanShahzad

Copy link
Copy Markdown
Contributor

@edx/learner this would be a great addition if it can get a review!

@bderusha

Copy link
Copy Markdown
Contributor

This looks good! Nice work! 👍 :shipit:

@UmanShahzad

Copy link
Copy Markdown
Contributor

@bderusha Thanks!!

@UmanShahzad
UmanShahzad merged commit 37844fd into openedx:master Nov 29, 2017
@UmanShahzad
UmanShahzad deleted the allow-theme-template-block-override branch November 29, 2017 17:25
@openedx-webhooks

Copy link
Copy Markdown

@cgopalan 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

@jzoldak

jzoldak commented Nov 29, 2017

Copy link
Copy Markdown
Contributor

@UmanShahzad @bderusha this PR introduced xsslint violations that pushed us over the limit and master is now failing the quality build.

@UmanShahzad
UmanShahzad restored the allow-theme-template-block-override branch November 29, 2017 18:34
@bderusha

Copy link
Copy Markdown
Contributor

@jzoldak Was this some kind of race condition? Shouldn't that have broken the build locally?

@jzoldak

jzoldak commented Nov 29, 2017

Copy link
Copy Markdown
Contributor

@bderusha Yes, I believe it was a race condition and this PR would have failed if rebased before merge. Happens occasionally.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

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

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

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

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

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

@UmanShahzad
UmanShahzad deleted the allow-theme-template-block-override branch December 3, 2017 17:58
@clemente

Copy link
Copy Markdown
Contributor

A newer version of this PR which fixes some issues is at https://github.com/edx/edx-platform/pull/16856

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants