From 8813a61da2dc6882d8f1c31038214055dba614ff Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Mon, 15 Mar 2021 09:04:23 -0400 Subject: [PATCH] feat!: remove SemanticSectionDescriptor, the final XModule (#26990) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A "section" tag in an OLX upload used to map to the SemanticSectionDescriptor, which translated it into a Sequence ("sequential" tag). This is both obscure and confusing, since it uses language that predates Studio. Back in the LMS prototype days, "section" was inconsistently used to be interchangeable with "sequence" and "sequential", and what Studio today calls a "section" was called a "chapter". Bits of this legacy terminology are still around in the courseware rendering code. The upshot is that if you make an OLX tag "section" before this commit, it would not map to what we call a "Section" in all our documentation, but to a "Subsection"; furthermore, if that
only had one child element, the node would be replaced with its singular child, removing the
node from the course tree entirely. The fact that you can make a "section" OLX tag at all is nowhere in our documentation because courses haven't been written that way since late 2011 or early 2012. SemanticSectionDescriptor came up as part of the XModule -> XBlock conversion efforts as a legacy XModule that isn't worth converting. With the removal of this class, all XBlocks in edx-platform are "pure" XBlocks, ending our reliance on the XModule-to-XBlock shimming infrastructure. This commit also removes the process_includes decorator, which was only used for "section" tags. This does NOT delete the ProblemBlock-specific tag, which is still supported (if obscure). There is a chance that through tribal knowledge or copy-paste, some section tags survive in the wild of old edX courses. It's difficult for us to assess because by its nature, this tag doesn't just say "section", but instead actually does the mutation on import so it's stored as "sequential" in the modulestore–therefore things like CourseGraph can't detect it. The fix for any such XML-authored courses is: * For instances of
that wrap a single child node, replace the
node in favor of its child node. * For instances of
that wrap a sequence of children, substitute
with Note that "
" is a valid HTML tag type and so may show up in any component that can contain HTML and is unrelated to the course structure OLX tag alias "
" that this commit removes. DEPR-124 --- common/lib/xmodule/setup.py | 6 +- .../lib/xmodule/xmodule/backcompat_module.py | 89 ------------------- common/test/data/simple/course.xml | 18 ++-- common/test/data/simple_with_draft/course.xml | 18 ++-- 4 files changed, 15 insertions(+), 116 deletions(-) delete mode 100644 common/lib/xmodule/xmodule/backcompat_module.py diff --git a/common/lib/xmodule/setup.py b/common/lib/xmodule/setup.py index b314a10e17..281f8a1ec9 100644 --- a/common/lib/xmodule/setup.py +++ b/common/lib/xmodule/setup.py @@ -2,9 +2,6 @@ from setuptools import find_packages, setup -XMODULES = [ - "section = xmodule.backcompat_module:SemanticSectionDescriptor", -] XBLOCKS = [ "about = xmodule.html_module:AboutBlock", "book = xmodule.template_module:TranslateCustomTagBlock", @@ -65,8 +62,7 @@ setup( # See https://setuptools.readthedocs.io/en/latest/setuptools.html#dynamic-discovery-of-services-and-plugins # for a description of entry_points entry_points={ - 'xblock.v1': XMODULES + XBLOCKS, - 'xmodule.v1': XMODULES, + 'xblock.v1': XBLOCKS, 'xblock_asides.v1': XBLOCKS_ASIDES, 'console_scripts': [ 'xmodule_assets = xmodule.static_content:main', diff --git a/common/lib/xmodule/xmodule/backcompat_module.py b/common/lib/xmodule/xmodule/backcompat_module.py deleted file mode 100644 index e07911a8b5..0000000000 --- a/common/lib/xmodule/xmodule/backcompat_module.py +++ /dev/null @@ -1,89 +0,0 @@ -""" -These modules exist to translate old format XML into newer, semantic forms -""" - - -import logging -import traceback -from functools import wraps - -from lxml import etree - -from openedx.core.djangolib.markup import Text - -from .x_module import XModuleDescriptor - -log = logging.getLogger(__name__) - - -def process_includes(fn): - """ - Wraps a XModuleDescriptor.from_xml method, and modifies xml_data to replace - any immediate child items with the contents of the file that they - are supposed to include - """ - @wraps(fn) - def from_xml(cls, xml_data, system, id_generator): - xml_object = etree.fromstring(xml_data) - next_include = xml_object.find('include') - while next_include is not None: - system.error_tracker("WARNING: the tag is deprecated, and will go away.") - file = next_include.get('file').decode('utf-8') - parent = next_include.getparent() - - if file is None: - continue - - try: - ifp = system.resources_fs.open(file) - # read in and convert to XML - incxml = etree.XML(ifp.read()) - - # insert new XML into tree in place of include - parent.insert(parent.index(next_include), incxml) - except Exception: # lint-amnesty, pylint: disable=broad-except - # Log error - msg = "Error in problem xml include: %s" % ( - etree.tostring(next_include, pretty_print=True)) - # tell the tracker - system.error_tracker(msg) - - # work around - parent = next_include.getparent() - errorxml = etree.Element('error') - messagexml = etree.SubElement(errorxml, 'message') - messagexml.text = msg - stackxml = etree.SubElement(errorxml, 'stacktrace') - stackxml.text = traceback.format_exc() - # insert error XML in place of include - parent.insert(parent.index(next_include), errorxml) - - parent.remove(next_include) - - next_include = xml_object.find('include') - return fn(cls, etree.tostring(xml_object), system, id_generator) - return from_xml - - -class SemanticSectionDescriptor(XModuleDescriptor): # lint-amnesty, pylint: disable=abstract-method, missing-class-docstring - resources_dir = None - - @classmethod - @process_includes - def from_xml(cls, xml_data, system, id_generator): - """ - Removes sections with single child elements in favor of just embedding - the child element - """ - xml_object = etree.fromstring(xml_data) - system.error_tracker(Text("WARNING: the <{tag}> tag is deprecated. Please do not use in new content.") - .format(tag=xml_object.tag)) - - if len(xml_object) == 1: - for (key, val) in xml_object.items(): - xml_object[0].set(key, val) - - return system.process_xml(etree.tostring(xml_object[0])) - else: - xml_object.tag = 'sequential' - return system.process_xml(etree.tostring(xml_object)) diff --git a/common/test/data/simple/course.xml b/common/test/data/simple/course.xml index c1490c7092..424d0f4a6c 100644 --- a/common/test/data/simple/course.xml +++ b/common/test/data/simple/course.xml @@ -5,19 +5,15 @@