[BD-04] Create CustomTagTemplateBlock, convert TranslateCustomTagDescriptor to XBlock and remove RawDescriptor by symbolist · Pull Request #26973 · openedx/openedx-platform

symbolist · 2021-03-11T15:47:00Z

Description

Creates CustomTagTemplateBlock for use with CustomTagBlock instead of RawDescriptor.
Converts TranslateCustomTagDescriptor to TranslateCustomTagBlock.
In https://github.com/edx/edx-platform/pull/25955 HiddenDescriptor (which was a subclass of RawDescriptor with a custom student_view()) was converted to an XBlock. It is used as the default_class by the CachingDescriptorSystem classes. However RawDescriptor is still being used by XMLModuleStore. This has been replaced by HiddenDescriptor as well.
Removes RawDescriptor since it is no longer used.

Part of XModule to XBlock Conversion work.

Which edX user roles will this change impact? "Developer"

Testing Instructions

Checkout the master branch.
Create a course and import this archive:
In the course check that there are two units: the Custom Tag Unit with html, custom_tag, image and book XBlocks and the Poll Unit with a poll_question XBlock.
Go to http://localhost:18010/admin/xblock_django/xblockconfiguration/add/ and add an entry for poll_question with it set to NOT enabled.
Visit the Poll Unit and verify that the following message is shown instead of the XBlock: "ERROR: "poll_question" is an unknown component type. This component will be hidden in LMS."
Delete both the units from the Course Outline.
Checkout this branch, do pip -e common/lib/xmodule in make studio-shell and then do make lms-restart and make-studio-restart.
Import the archive again.
Check that both the Units have the same content in Studio. The poll_question XBlock should have been imported in but the error message should show in its place: "ERROR: "poll_question" is an unknown component type. This component will be hidden in LMS."
Check that both the Units are visible in LMS. Though in the Poll Unit the poll_question XBlock will be hidden.
Export the course. Check that the vertical and custom_tags dir content is the same as before. However the poll_question dir should NOT be present (because that XBlock could not be exported).

Screenshots

Custom Tag Unit in Studio

Custom Tag Unit in LMS

Poll Unit in Studio after Poll XBlock has been disabled

Poll Unit in LMS after Poll XBlock has been disabled

MRO Analysis

Descriptor MRO	Module MRO	Block MRO	Notes
<class 'xmodule.backcompat_module.TranslateCustomTagDescriptor'>		<class 'xmodule.template_module.TranslateCustomTagBlock'>
<class 'xmodule.x_module.XModuleDescriptor'>
<class 'xmodule.x_module.XModuleDescriptorToXBlockMixin'>		<class 'xmodule.x_module.XModuleDescriptorToXBlockMixin'>
<class 'xmodule.x_module.XModule'>
<class 'xmodule.x_module.XModuleToXBlockMixin'>	<class 'xmodule.x_module.XModuleToXBlockMixin'>
<class 'xmodule.x_module.HTMLSnippet'>	<class 'xmodule.x_module.HTMLSnippet'>		This is not needed because there are no views.
<class 'xmodule.x_module.ResourceTemplates'>			This is not needed because there are no templates.
<class 'xmodule.x_module.XModuleMixin'>	<class 'xmodule.x_module.XModuleMixin'>	<class 'xmodule.x_module.XModuleMixin'>
<class 'xmodule.x_module.XModuleFields'>	<class 'xmodule.x_module.XModuleFields'>	<class 'xmodule.x_module.XModuleFields'>
<class 'xblock.core.XBlock'>	<class 'xblock.core.XBlock'>	<class 'xblock.core.XBlock'>

openedx-webhooks · 2021-03-11T15:47:05Z

Thanks for the pull request, @symbolist! I've created BLENDED-791 to keep track of it in Jira. More details are on the BD-04 project page.

When this pull request is ready, tag your edX technical lead.

…RawDescriptor.

In https://github.com/edx/edx-platform/pull/25955 `HiddenDescriptor` (which was a subclass of `RawDescriptor` with a custom `student_view()`) was converted to an XBlock. It is used as the `default_class` by the `CachingDescriptorSystem` classes. However `RawDescriptor` is still being used by `XMLModuleStore`. This has been replaced by `HiddenDescriptor` as well.

symbolist · 2021-03-11T17:21:47Z

common/lib/xmodule/xmodule/backcompat_module.py

            return system.process_xml(etree.tostring(xml_object))
-
-
-class TranslateCustomTagDescriptor(XModuleDescriptor):  # lint-amnesty, pylint: disable=abstract-method, missing-class-docstring


Moved it to template_module.py.

symbolist · 2021-03-11T17:23:47Z

common/lib/xmodule/xmodule/template_module.py


-class CustomTagBlock(
+
+class CustomTagTemplateBlock(  # pylint: disable=abstract-method


CustomTagDescriptor inherited from RawDescriptor and custom_tag_template pointed to RawDescriptor as well. So I have made this the base class and CustomTagBlock inherited from it.

symbolist · 2021-03-11T17:25:28Z

openedx/core/lib/xblock_builtin/xblock_discussion/xblock_discussion/__init__.py


    # support for legacy OLX format - consumed by XmlParserMixin.load_metadata
-    metadata_translations = dict(RawDescriptor.metadata_translations)
+    metadata_translations = dict(XmlParserMixin.metadata_translations)


metadata_translations is specified in only two places: XModuleDescriptorToXBlockMixin and XmlParserMixin. Both have the same values.

symbolist · 2021-03-11T17:31:56Z

@ormsbee @kdmccormick Hopefully this is going to be the last one! It is ready for review. 🙂

kdmccormick · 2021-03-11T17:44:44Z

Wohoo! We'll take a look soon 😄

ormsbee · 2021-03-11T17:50:52Z

So way back when the behavior when you tried to import something that was completely unrecognized (like a <foo> tag with a url_name and some basic fields) and then re-exported that, it should preserve the content exactly as is. The RawDescriptor would dump the raw XML into a field and spit it back out again on export. That still works, right?

edx-status-bot · 2021-03-11T17:55:07Z

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

symbolist · 2021-03-11T21:46:11Z

@ormsbee

There are two cases of that and I am afraid both are kind of broken:

If an XBlock is installed but disabled and a course import includes it: it seems the ImportSystem is not loading or receiving the disabled_xblocks setting. This causes the disabled XBlock class to be loaded here and so the definition_from_xml() of the XBlock to be called instead of the one in RawMixin which means the data field does not get set. However on export RawMixin.definition_to_xml() is called which returns from here.
If the XBlock is not installed at all: I do see RawMixin. definition_from_xml () getting called. However, it causes the Studio content views to stop working with the following error (this is a known error as far as I understand):

  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/core/handlers/exception.py", line 34, in inner
    response = get_response(request)
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/core/handlers/base.py", line 115, in _get_response
    response = self.process_exception_by_middleware(e, request)
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/core/handlers/base.py", line 113, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/usr/lib/python3.8/contextlib.py", line 75, in inner
    return func(*args, **kwds)
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/views/decorators/http.py", line 40, in inner
    return func(request, *args, **kwargs)
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/contrib/auth/decorators.py", line 21, in _wrapped_view
    return view_func(request, *args, **kwargs)
  File "/edx/app/edxapp/edx-platform/cms/djangoapps/contentstore/views/component.py", line 171, in container_handler
    xblock_info = create_xblock_info(xblock, include_ancestor_info=is_unit_page)
  File "/edx/app/edxapp/edx-platform/cms/djangoapps/contentstore/views/item.py", line 1222, in create_xblock_info
    'edited_on': get_default_time_display(xblock.subtree_edited_on) if xblock.subtree_edited_on else None,
  File "/edx/app/edxapp/edx-platform/common/lib/xmodule/xmodule/modulestore/edit_info.py", line 41, in subtree_edited_on
    return self.runtime.get_subtree_edited_on(self)
  File "/edx/app/edxapp/edx-platform/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py", line 327, in get_subtree_edited_on
    self._compute_subtree_edited_internal(
  File "/edx/app/edxapp/edx-platform/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py", line 362, in _compute_subtree_edited_internal
    child_data = self.get_module_data(BlockKey(*child), course_key)
  File "/edx/app/edxapp/edx-platform/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py", line 160, in get_module_data
    raise ItemNotFoundError(block_key)
xmodule.modulestore.exceptions.ItemNotFoundError: BlockKey(type='poll_question', id='poll1')

This behavior hasn't changed in this PR and was the same before https://github.com/edx/edx-platform/pull/25955 (multiple people tested the behavior as part of the testing instructions).

ormsbee · 2021-03-11T23:05:23Z

Ugh. Okay. Well, as long as this isn't a regression, I'm definitely not going to hold it up over that.

There's so much we need to fix in the whole import/export stack of things... 🔧

ormsbee · 2021-03-11T23:30:27Z

I'll merge this in the morning.

edx-pipeline-bot · 2021-03-12T15:47:19Z

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

edx-pipeline-bot · 2021-03-12T16:14:31Z

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

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 Mar 11, 2021

symbolist force-pushed the symbolist/convert-to-xblock branch from bf4f6a3 to beb7b92 Compare March 11, 2021 16:20

symbolist changed the title ~~Create CustomTagTemplateBlock, convert TranslateCustomTagDescriptor to XBlock and remove RawDescriptor~~ [BD-04] Create CustomTagTemplateBlock, convert TranslateCustomTagDescriptor to XBlock and remove RawDescriptor Mar 11, 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 Mar 11, 2021

symbolist added 4 commits March 11, 2021 22:20

Created CustomTagTemplateBlock to use with CustomTagBlock instead of …

556ef4c

…RawDescriptor.

Convert TranslateCustomTagDescriptor to TranslateCustomTagBlock.

fb02353

Remove unused RawDescriptor and EmptyDataRawDescriptor.

23d1e5b

symbolist force-pushed the symbolist/convert-to-xblock branch from beb7b92 to 23d1e5b Compare March 11, 2021 17:21

symbolist marked this pull request as ready for review March 11, 2021 17:21

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

symbolist commented Mar 11, 2021

View reviewed changes

openedx-webhooks added awaiting prioritization and removed needs triage labels Mar 11, 2021

ormsbee approved these changes Mar 11, 2021

View reviewed changes

openedx-webhooks added engineering review and removed awaiting prioritization labels Mar 11, 2021

ormsbee merged commit bebcbef into openedx:master Mar 12, 2021

openedx-webhooks removed the engineering review label Mar 12, 2021

openedx-webhooks added the merged label Mar 12, 2021

Agrendalath deleted the symbolist/convert-to-xblock branch September 9, 2021 21:02

Agrendalath mentioned this pull request Sep 9, 2021

fix: do not remove comments from XML during course import #28557

Merged

2 tasks

kdmccormick mentioned this pull request Nov 12, 2024

Improve error for missing (Translate)CustomTagBlock template #35829

Closed

		return system.process_xml(etree.tostring(xml_object))


		class TranslateCustomTagDescriptor(XModuleDescriptor): # lint-amnesty, pylint: disable=abstract-method, missing-class-docstring


		class CustomTagBlock(

		class CustomTagTemplateBlock( # pylint: disable=abstract-method

Conversation

symbolist commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing Instructions

Screenshots

MRO Analysis

Uh oh!

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

Uh oh!

Uh oh!

symbolist Mar 11, 2021

Choose a reason for hiding this comment

Uh oh!

symbolist Mar 11, 2021

Choose a reason for hiding this comment

Uh oh!

symbolist Mar 11, 2021

Choose a reason for hiding this comment

Uh oh!

symbolist commented Mar 11, 2021

Uh oh!

kdmccormick commented Mar 11, 2021

Uh oh!

ormsbee commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edx-status-bot commented Mar 11, 2021

Uh oh!

symbolist commented Mar 11, 2021

Uh oh!

ormsbee commented Mar 11, 2021

Uh oh!

ormsbee commented Mar 11, 2021

Uh oh!

edx-pipeline-bot commented Mar 12, 2021

Uh oh!

edx-pipeline-bot commented Mar 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

symbolist commented Mar 11, 2021 •

edited

Loading

openedx-webhooks commented Mar 11, 2021 •

edited

Loading

ormsbee commented Mar 11, 2021 •

edited

Loading