[BD-13] Deprecate ModuleSystem.render_template by pomegranited · Pull Request #28571 · openedx/openedx-platform

pomegranited · 2021-08-29T10:33:22Z

Description

Builds on the shim added by https://github.com/edx/edx-platform/pull/28440 to deprecate the ModuleSystem.render_template attribute, in favor of using the added MakoService.render_template.

This change is only a refactoring, and should not affect Learners, Course Authors, or anyone else using edx-platform.

Supporting information

Content Core Platform Simplication project
BLENDED-113 (edX internal ticket)
FAL-2304 (OpenCraft internal ticket)

Testing instructions

LMS as learner:

Login as a demo learner (honor@example.com / edx)
Browse around the BD-13 test course and ensure that blocks render as expected, and that you can submit answers to problems. This course has all of the affected XBlocks in it, including an example Video block which shows the CC license wrapper (cf 7fd1467).
Switch to the Legacy Course Experience and browse around in that too.

LMS as course staff:

Login as a demo staff user (staff@example.com / edx)
Navigate to a course and its Instructor Dashboard > Email tab
Ensure that you can see and use the HTML WYSIWYG editor to craft emails to students. (cf 96d9208)

Studio:

Login as a demo staff (staff@example.com / edx)
Navigate to the BD-13 test course and browse through the blocks. Ensure they render and edit as expected.
Check that you can submit answers to problems, and save changes to blocks.
Preview unpublished changes and ensure they render as expected.
Publish changes, and ensure they render in the LMS as expected.

Deadline

None

openedx-webhooks · 2021-08-29T10:33:27Z

Thanks for the pull request, @pomegranited! I've created BLENDED-947 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.

common/lib/xmodule/xmodule/word_cloud_module.py

lms/djangoapps/courseware/module_render.py

common/djangoapps/edxmako/services.py

common/lib/xmodule/xmodule/word_cloud_module.py

symbolist · 2021-09-30T13:51:59Z

common/lib/xmodule/xmodule/studio_editable.py

    This class is only intended to be used with an XBlock!
    """
+    _services_requested = {
+        'mako': 'need',


Why do we need this?

StudioEditableBlock needs the mako service now, but since it's not an XBlock, it doesn't have the _services_requested class property itself, so the XBlock.needs decorator causes this error on init:

AttributeError: type object 'StudioEditableBlock' has no attribute '_services_requested'

I borrowed this trick from AuthoringMixin, but an empty dict can perform the same duty, so I'll fix that here.

Added comment about this: https://github.com/open-craft/edx-platform/blob/jill/bd-13-render_template/common/lib/xmodule/xmodule/studio_editable.py#L15-L16

openedx/core/lib/license/wrapper.py

cms/djangoapps/contentstore/views/tests/test_preview.py

common/djangoapps/edxmako/services.py

common/djangoapps/edxmako/tests.py

common/lib/xmodule/xmodule/mako_module.py

cms/lib/xblock/authoring_mixin.py

common/lib/xmodule/xmodule/tests/test_editing_module.py

common/lib/xmodule/xmodule/tests/test_xml_module.py

symbolist · 2021-10-15T10:20:47Z

common/lib/xmodule/xmodule/studio_editable.py

Is this raised in a test because otherwise this mixin should only be used as a mixin by other XBlocks. In that case maybe we can just make it inherit from XBlock instead?

common/lib/xmodule/xmodule/mako_module.py

pomegranited · 2021-10-18T04:49:49Z

@symbolist There's 1 error reported in the JS test, but it's unrelated to this work:

  CMS.Views.Metadata.VideoList
    ✓ Initialize
    Render
      ✓ is rendered in correct way
      ✓ is rendered with opened extra videos bar
      ✗ is rendered without opened extra videos bar
	Expected spy closeExtraVideosBar to have been called.
	@/home/runner/work/edx-platform/edx-platform/cms/static/js/spec/video/transcripts/videolist_spec.js:9:12828
	then/</</<@/home/runner/work/edx-platform/edx-platform/cms/static/common/js/vendor/jquery.js:3366:30
	fire@/home/runner/work/edx-platform/edx-platform/cms/static/common/js/vendor/jquery.js:3187:11
	fireWith@/home/runner/work/edx-platform/edx-platform/cms/static/common/js/vendor/jquery.js:3317:7

Firefox 61.0.0 (Linux 0.0.0) CMS.Views.Metadata.VideoList Render is rendered without opened extra videos bar FAILED
	Expected spy closeExtraVideosBar to have been called.
	@/home/runner/work/edx-platform/edx-platform/cms/static/js/spec/video/transcripts/videolist_spec.js:9:12828
	then/</</<@/home/runner/work/edx-platform/edx-platform/cms/static/common/js/vendor/jquery.js:3366:30
	fire@/home/runner/work/edx-platform/edx-platform/cms/static/common/js/vendor/jquery.js:3187:11
	fireWith@/home/runner/work/edx-platform/edx-platform/cms/static/common/js/vendor/jquery.js:3317:7

Does this need to be resolved before merging?

symbolist · 2021-10-19T13:55:52Z

jenkins run js

symbolist · 2021-10-19T13:56:53Z

Ah, looks like that is being run by Github Actions now.

symbolist

@pomegranited Thanks!

I tested this by viewing and editing all the XBlocks in the test course and confirming they are rendering.
I read through the code
I checked for accessibility issues. There are no UI changes.
Includes documentation

symbolist · 2021-10-21T13:56:42Z

@ormsbee This is ready to go!

ormsbee · 2021-11-08T14:28:30Z

@pomegranited: I realize this is entirely my fault for sitting on this PR too long, but may I please ask for a rebase? I should be able to merge tomorrow morning.

Thank you.

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.

from block code.

from test code.

edx-status-bot · 2021-11-09T04:20:12Z

Your PR has finished running tests. There were no failures.

pomegranited · 2021-11-09T04:46:16Z

@ormsbee No worries at all! Those conflicts are all from my other https://github.com/edx/edx-platform/pull/29190 that you just merged, so rebasing would have been a necessity either way.

Fixed the conflicts and rebased, and the tests look good again, so this should be ready to merge now. Thank you! 😄

openedx-webhooks · 2021-11-09T17:21:06Z

@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.

edx-pipeline-bot · 2021-11-09T18:26:46Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

edx-pipeline-bot · 2021-11-09T19:21:28Z

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

dianakhuang · 2021-11-16T20:48:00Z

@pomegranited We were seeing issues with visibility settings for ORA and LTI blocks and the error "Error: service 'mako' was not requested", so we are going to revert this PR.

pomegranited · 2021-11-17T03:08:36Z

Thanks for the heads up @dianakhuang and @jnlapierre -- if you could provide some details here, I'll address them with a follow-up PR.

dianakhuang · 2021-11-17T19:14:48Z

@pomegranited

The steps to reproduce are this:

Create an LTI or ORA component in Studio
Click the 'gear' icon:
We expect to see visibility settings, but instead we get the error message:

Students will not be able to access this component. Re-edit your component to fix the error.

Error: Service 'mako' was not requested.

I did some digging on our own services, but I wasn't able to find the corresponding stacktrace in our Splunk logs.

pomegranited · 2021-11-18T05:28:38Z

Thanks for the info @dianakhuang ! I can reproduce this on my devstack with ORA2, so will sort that one out.

But I was unable to reproduce the issue with an LTI component, and (other than XBlock) the LTIBlock doesn't seem to share any common base classes with OpenAssessmentBlock. Would it be possible for you to provide a course export which demonstrates this issue for LTI?

CC @jnlapierre

pomegranited · 2021-11-18T08:16:08Z

@dianakhuang CC @jnlapierre I've identified the issue with ORA2, which was caused by a missed change to the AuthoringMixin, and would affect any XBlock which didn't explicitly @need("mako"). The LTIBlock did @need("mako"), so I'm still not clear on why this would cause a problem -- maybe there's a different LTI XBlock out there somewhere in a separate repo?

But I'm re-submitting this PR, and will add a test to show the issue, then add the fix to show the issue has been resolved.

openedx-webhooks added open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Aug 29, 2021

pomegranited changed the title ~~Deprecate ModuleSystem.render_template~~ [BD-13] Deprecate ModuleSystem.render_template Aug 29, 2021

openedx-webhooks added blended PR is managed through 2U's blended developmnt program and removed open-source-contribution PR author is not from Axim or 2U labels Aug 29, 2021

symbolist reviewed Aug 30, 2021

View reviewed changes

common/lib/xmodule/xmodule/word_cloud_module.py Outdated Show resolved Hide resolved

pomegranited force-pushed the jill/bd-13-render_template branch from 1c8b506 to dc141f6 Compare September 1, 2021 05:44

symbolist reviewed Sep 1, 2021

View reviewed changes

lms/djangoapps/courseware/module_render.py Outdated Show resolved Hide resolved

symbolist reviewed Sep 12, 2021

View reviewed changes

common/djangoapps/edxmako/services.py Outdated Show resolved Hide resolved

pomegranited force-pushed the jill/bd-13-render_template branch 3 times, most recently from 5a1698a to 96d9208 Compare September 28, 2021 12:30

pomegranited marked this pull request as ready for review September 29, 2021 05:04

pomegranited requested a review from a team September 29, 2021 05:04

openedx-webhooks added needs triage and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Sep 29, 2021

pomegranited force-pushed the jill/bd-13-render_template branch from d30556f to c3eb358 Compare September 29, 2021 05:47

symbolist suggested changes Sep 30, 2021

View reviewed changes

pomegranited force-pushed the jill/bd-13-render_template branch 2 times, most recently from eed7d32 to a50742f Compare October 5, 2021 02:56

symbolist suggested changes Oct 17, 2021

View reviewed changes

pomegranited force-pushed the jill/bd-13-render_template branch from fd844ee to 924d0d0 Compare October 19, 2021 23:21

symbolist approved these changes Oct 21, 2021

View reviewed changes

openedx-webhooks added engineering review and removed needs triage labels Nov 1, 2021

pomegranited added 3 commits November 9, 2021 14:03

refactor: use MakoService.render_template to remove deprecation warnings

8d62d33

from block code.

refactor: use MakoService.render_template to remove deprecation warnings

26b4346

from test code.

pomegranited force-pushed the jill/bd-13-render_template branch from 924d0d0 to 26b4346 Compare November 9, 2021 03:38

ormsbee merged commit 2d60224 into openedx:master Nov 9, 2021

openedx-webhooks added merged and removed engineering review labels Nov 9, 2021

pomegranited deleted the jill/bd-13-render_template branch November 9, 2021 23:40

jnlapierre mentioned this pull request Nov 16, 2021

Revert "[BD-13] Deprecate ModuleSystem.render_template" #29341

Merged

pomegranited mentioned this pull request Nov 18, 2021

[BD-13] Deprecate ModuleSystem.render_template (fixed) #29354

Merged

collinpreston mentioned this pull request Nov 9, 2022

feat: Course mode admin explicitly defining upgrade deadline. #31246

Closed

Comments

Conversation

pomegranited commented Aug 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting information

Testing instructions

Deadline

Uh oh!

openedx-webhooks commented Aug 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

symbolist Sep 30, 2021

Choose a reason for hiding this comment

Uh oh!

pomegranited Oct 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pomegranited Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

symbolist Oct 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pomegranited commented Oct 18, 2021

Uh oh!

symbolist commented Oct 19, 2021

Uh oh!

symbolist commented Oct 19, 2021

Uh oh!

symbolist left a comment

Choose a reason for hiding this comment

Uh oh!

symbolist commented Oct 21, 2021

Uh oh!

ormsbee commented Nov 8, 2021

Uh oh!

edx-status-bot commented Nov 9, 2021

Uh oh!

pomegranited commented Nov 9, 2021

Uh oh!

openedx-webhooks commented Nov 9, 2021

Uh oh!

edx-pipeline-bot commented Nov 9, 2021

Uh oh!

edx-pipeline-bot commented Nov 9, 2021

Uh oh!

dianakhuang commented Nov 16, 2021

Uh oh!

pomegranited commented Nov 17, 2021

Uh oh!

dianakhuang commented Nov 17, 2021

Uh oh!

pomegranited commented Nov 18, 2021

Uh oh!

pomegranited commented Nov 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pomegranited commented Aug 29, 2021 •

edited

Loading

openedx-webhooks commented Aug 29, 2021 •

edited

Loading

pomegranited Oct 1, 2021 •

edited

Loading

pomegranited Oct 4, 2021 •

edited

Loading