-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: refactor MakoService to allow specifying namespace per template #33061
fix: refactor MakoService to allow specifying namespace per template #33061
Conversation
Thanks for the pull request, @bradenmacdonald! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me.
I tested each block in studio, library and lms with local devstack and do not see any issues.
@bradenmacdonald 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
I don't mind that this got merged, but I think the terminology is further obfuscating the problem. This isn't about CMS vs LMS (in fact, it is only ever relevant to CMS). The namespace isn't being used as a namespace; it's being used as a switch between mako "engines," specifically between regular CMS and CMS "preview" (for authors previewing student views). Basically I don't like the naming of these functions. |
@kenclary The only difference between those "engines" is the So I thought this naming would be most clear to developers, as it reflects the actual difference/goal (set which template paths get searched), and not the mechanism by which that happens or the confusing term used ("namespace"). What would you suggest instead?
Let's say you want to render |
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
…emplate (openedx#33061)" (openedx#33070) This reverts commit d60cdc2.
…y (Take 2) (openedx#33077) * fix: refactor MakoService to allow specifying namespace per template (openedx#33061) * fix: instr. dashboard broken by bulk email reusing HtmlBlock studio_view * fix: lint issue from unused import
Description
Currently, the "Manage Access" menu in Studio is not working consistently (sometimes it does work though), due to a template error:
This is not the first error we've had with the MakoService being unable to find mako templates, and the problem stems from different search paths for "LMS templates" vs "CMS templates", and the fact that the runtime has this internal state that determines which paths to search for any given template. As you can see, the CMS runtime is often using the wrong paths, resulting in this error.
I believe that the best fix for this issue that will keep it from happening in the future, as it has happened at least 3 or 4 times so far, is to change the
MakoService
API to allow specifying the search path along with the template name. You want to render a template that's inlms/templates/...
? Great, now callrender_lms_template()
. Want to render one that's incms/templates/...
? Great, now callrender_cms_template()
.Of course mako templates are terrible and they should all be replaced with django templates or React UI, but that's a battle for another day.
Supporting information
One of the past bugs: #32895
Testing instructions
["annotatable", "poll", "library_content", "lti_consumer"]
.Deadline
ASAP - CAT-2 issue
Other info
Private-ref: MNG-3884