Skip to content

[BD-13][BB-6709] refactor: Delete XModule compatibility code from xml_module.py #31108

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.

Sign up for GitHub

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

Merged
Agrendalath merged 1 commit into from
Jan 26, 2023
Merged

[BD-13][BB-6709] refactor: Delete XModule compatibility code from xml_module.py #31108

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 21 additions & 16 deletions xmodule/xml_block.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,16 +113,15 @@ class XmlMixin:
xml_attributes = Dict(help="Map of unhandled xml attributes, used only for storage between import and export",
default={}, scope=Scope.settings)

# The attributes will be removed from the definition xml passed
# to definition_from_xml, and from the xml returned by definition_to_xml

# Note -- url_name isn't in this list because it's handled specially on
# import and export.

Comment on lines -116 to -121
Copy link
Contributor Author

@0x29a 0x29a Dec 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment was referring to metadata_attributes and no longer needed.

metadata_to_strip = ('data_dir',
'tabs', 'grading_policy',
'discussion_blackouts',
# VS[compat] -- remove the below attrs once everything is in the CMS
# VS[compat]
# These attributes should have been removed from here once all 2012-fall courses imported into
# the CMS and "inline" OLX format deprecated. But, it never got deprecated. Moreover, it's
# widely used to this date. So, we still have to strip them. Also, removing of "filename"
# changes OLX returned by `/api/olx-export/v1/xblock/{block_id}/`, which indicates that some
# places in the platform rely on it.
'course', 'org', 'url_name', 'filename',
# Used for storing xml attributes between import and export, for roundtrips
'xml_attributes')
Expand Down Expand Up @@ -211,8 +210,9 @@ def load_definition(cls, xml_object, system, def_id, id_generator):
id_generator: used to generate the usage_id
"""

# VS[compat] -- the filename attr should go away once everything is
# converted. (note: make sure html files still work once this goes away)
# VS[compat]
# The filename attr should have been removed once all 2012-fall courses imported into the CMS and "inline" OLX
# format deprecated. This never happened, and `filename` is still used, so we have too keep both formats.
filename = xml_object.get('filename')
if filename is None:
definition_xml = copy.deepcopy(xml_object)
Expand All @@ -222,10 +222,9 @@ def load_definition(cls, xml_object, system, def_id, id_generator):
filepath = cls._format_filepath(xml_object.tag, filename)

# VS[compat]
# TODO (cpennington): If the file doesn't exist at the right path,
# give the class a chance to fix it up. The file will be written out
# again in the correct format. This should go away once the CMS is
# online and has imported all current (fall 2012) courses from xml
# If the file doesn't exist at the right path, give the class a chance to fix it up. The file will be
# written out again in the correct format. This should have gone away once the CMS became online and had
# imported all 2012-fall courses from XML.
if not system.resources_fs.exists(filepath) and hasattr(cls, 'backcompat_paths'):
candidates = cls.backcompat_paths(filepath)
for candidate in candidates:
Expand Down Expand Up @@ -311,7 +310,13 @@ def parse_xml(cls, node, runtime, _keys, id_generator):
usage_id = id_generator.create_usage(def_id)
aside_children = []

# VS[compat] -- detect new-style each-in-a-file mode
# VS[compat]
# In 2012, when the platform didn't have CMS, and all courses were handwritten XML files, problem tags
# contained XML problem descriptions withing themselves. Later, when Studio has been created, and "pointer" tags
# became the preferred problem format, edX has to add this compatibility code to 1) support both pre- and
# post-Studio course formats simulteneously, and 2) be able to migrate 2012-fall courses to Studio. Old style
# support supposed to be removed, but the deprecation process have never been initiated, so this
# compatibility must stay, probably forever.
if is_pointer_tag(node):
# new style:
# read the actual definition file--named using url_name.replace(':','/')
Expand All @@ -324,8 +329,8 @@ def parse_xml(cls, node, runtime, _keys, id_generator):
# Note: removes metadata.
definition, children = cls.load_definition(definition_xml, runtime, def_id, id_generator)

# VS[compat] -- make Ike's github preview links work in both old and
# new file layouts
# VS[compat]
# Make Ike's github preview links work in both old and new file layouts.
Copy link
Member

@Agrendalath Agrendalath Dec 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does it mean? Do we still need this?

Copy link
Contributor Author

@0x29a 0x29a Dec 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Agrendalath, I couldn't figure this out for sure. This was added in 932a9be, and perhaps is needed for add_staff_markup function, but I don't have any context on https://github.com/MITx to confidently say whether this is still needed. My wild guess: some courses were stored on GitHub, and this was needed to generate "edit" GitHub links for staff.

if is_pointer_tag(node):
# new style -- contents actually at filepath
definition['filename'] = [filepath, filepath]
Expand Down