[BD-13] Deprecate ModuleSystem.render_template (fixed) #29354
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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. (cherry picked from commit 457f959)
from block code. (cherry picked from commit 8d62d33)
from test code. (cherry picked from commit 26b4346)
The AuthoringMixin is automatically added to all XBlocks (see settings.XBLOCK_MIXINS), and AuthoringMixin.visibility_view expects the "mako" service. This test verifies the bug by testing the PureXBlock, which does not require the "mako" service, and so fails when the visibility_view is rendered.
|
Thanks for the pull request, @pomegranited! I've created BLENDED-1017 to keep track of it in Jira. More details are on the BD-13 project page. When this pull request is ready, tag your edX technical lead. |
openedx-webhooks
added
blended
which fixes the visibility_view for XBlocks which don't explicitly require the mako service. Also removes the unneeded class property _services_requested from AuthoringMixin and StudioEditableBlock. This property is better provided by the XBlockMixin class.
symbolist
approved these changes
Nov 21, 2021
Contributor
symbolist
left a comment
symbolist
left a comment
There was a problem hiding this comment.
Thanks for the fix! That was a sneaky one.
- I tested this as per instructions including the fix for the visibility settings.
- I read through the code
- I checked for accessibility issues. N/A
- Includes documentation
Contributor
|
Hi @jristau1984, my understanding from Dave Ormsbee is that your team is going to continue supporting the work on this epic. Will someone be available to merge this PR at an appropriate time and monitor its deployment? Thanks! 🙂 |
|
Your PR has finished running tests. There were no failures. |
|
@pomegranited 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Contributor
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
Contributor
|
EdX Release Notice: This PR has been deployed to the production environment. |
pomegranited
added a commit
to open-craft/openedx-platform
that referenced
this pull request
Apr 21, 2022
…ring MFE When the XBlock runtime is used in Studio/authoring mode, it needs to use the template path prefix "lms." in order to locate the block templates. (In LMS view, no prefix is required.) Fixes bug introduced by openedx#29354 and removes template workaround added by openedx#29517 (see Author Notes & Concerns)
jawad-khan
pushed a commit
that referenced
this pull request
Jun 14, 2022
…ring MFE When the XBlock runtime is used in Studio/authoring mode, it needs to use the template path prefix "lms." in order to locate the block templates. (In LMS view, no prefix is required.) Fixes bug introduced by #29354 and removes template workaround added by #29517 (see Author Notes & Concerns)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Re-submits the changes from https://github.com/edx/edx-platform/pull/28571 which were reverted by https://github.com/edx/edx-platform/pull/29341, and fixes the bug that caused this revert.
Builds on the shim added by https://github.com/edx/edx-platform/pull/28440 to deprecate the
ModuleSystem.render_templateattribute, in favor of using the addedMakoService.render_template.This change is only a refactoring, and should not affect Learners, Course Authors, or anyone else using edx-platform.
Supporting information
See https://github.com/edx/edx-platform/pull/28571#issuecomment-971893521 for details about why that PR was reverted. 343a182 adds a test for this issue, demonstrating that the new test fails, but is fixed by the subsequent commt 8c874c2.
See also:
Testing instructions
Sandbox provisioning:
LMS as learner:
honor@example.com/edx)LMS as course staff:
staff@example.com/edx)Studio:
staff@example.com/edx).Test the Studio visibility bug:
This bug occurs only in the XBlocks which do not
@need("mako"), like ORA2.For OpenCraft staff:
edx)If verifying the bug exists: note that an error is shown in studio, saying "Error: service 'mako' was not requested"
If verifying that the bug is fixed: ensure that you can edit the block's visibility.
Deadline
None