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

0x29a · 2022-10-05T18:34:41Z

Description

Expands VS[compat] comments in the xml_module.py file.

openedx-webhooks · 2022-10-05T18:34:44Z

Thanks for the pull request, @0x29a!

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

0x29a · 2022-11-23T14:17:14Z

Posting my findings here for now, will add the most important parts to the PR description later.

Initial goal of this PR was to remove the compatibility code marked with VS[compat] comments from xml_module.py file. See the diff.

What exactly was this code for?

As far as I could understand, initially the platform didn't have Studio, and all courses consisted of a handwritten OLX. Back then, OLX wasn't standardized, so when Studio was created, edX has to add this compatibility code to 1) support both pre- and post-Studio course formats, and 2) be able to migrate these courses to Studio.

Specifically, the compatibility code adds the possibility to embed XML problem description right inside its tag, which should have been deprecated (example of before and after). Without the compatibility code (this, specifically), all problems have to be "pointer tags" and point at separate files.

What's the problem with just removing it?

It seems to be used to this day.

Removing this code breaks hundreds of tests. Most of them, including each from test_import.py, rely on course fixtures in the old format. I've tried importing toy course into the modern platform, exporting it and substituting the fixture OLX with the exported one. This made almost all tests from test_import.py passing, which suggests that they cover valid cases.

Another example of a failed test. It's using the problem with inline XML description, and likely tests something important, but without the compatibility code it can't be parsed, as poll_question doesn't have url_name attribute and doesn't point at a separate file. All such tests must be reworked.

Finally, cc2olx tool generates OLX in the old format, so if we remove this compatibility code we'll need to update it too.

Questions

Is there a timeline for removing this code? I see that DataDog counters were added to track what code is using the compatibility code, but then they were removed. Also, tests with the "inline" OLX or importing cc2olx-generated courses don't seem to emit any deprecation warnings. So, probably it was just marked as deprecated? @ormsbee, probably you can shed some light on this?

cc @Agrendalath

ormsbee · 2022-12-05T05:05:36Z

😞 Yeah, we can't break OLX as it's being used in the wild, and inline definitions like this are too pervasive at this point to deprecate. So that ability needs to stay, and probably will stay forever.

0x29a · 2022-12-20T13:12:01Z

xmodule/xml_block.py

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


This comment was referring to metadata_attributes and no longer needed.

0x29a · 2022-12-20T13:14:29Z

@ormsbee, thank you for confirming.

@Agrendalath, as you suggested, I expanded VS[compat] comments. Please take a look.

Agrendalath

@0x29a, this looks good to me.

@ormsbee, as we cannot remove some compatibility code, we have updated the comments to reflect the current state. Would you like to take a look?

Agrendalath · 2022-12-21T16:29:34Z

xmodule/xml_block.py

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


What does it mean? Do we still need this?

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

Agrendalath

👍

I tested this: this changes only the comments
I read through the code
I checked for accessibility issues: n/a
Includes documentation
I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository: n/a

Agrendalath · 2023-01-10T18:17:35Z

@ormsbee, I realized I didn't give a "formal" +1. Would you like to check if we missed anything in the expanded explanations about the compatibility?

openedx-webhooks · 2023-01-26T14:28:20Z

@0x29a 🎉 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 · 2023-01-26T15:30:51Z

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

edx-pipeline-bot · 2023-01-26T15:56:04Z

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

openedx-webhooks added the blended PR is managed through 2U's blended developmnt program label Oct 5, 2022

0x29a force-pushed the 0x29a/bb6709/delete_xmodule_compatibility_code branch 3 times, most recently from f39c6cb to 50ded66 Compare October 16, 2022 16:41

0x29a force-pushed the 0x29a/bb6709/delete_xmodule_compatibility_code branch from 50ded66 to cb5e0d0 Compare October 24, 2022 02:38

docs: update VS[compat] comments

060a5b0

0x29a force-pushed the 0x29a/bb6709/delete_xmodule_compatibility_code branch from 7ac3736 to 060a5b0 Compare December 20, 2022 13:07

0x29a commented Dec 20, 2022

View reviewed changes

0x29a changed the title ~~[WIP][BD-13][BB-6709] refactor: Delete XModule compatibility code from xml_module.py~~ [BD-13][BB-6709] refactor: Delete XModule compatibility code from xml_module.py Dec 20, 2022

Agrendalath reviewed Dec 21, 2022

View reviewed changes

Agrendalath mentioned this pull request Jan 4, 2023

reafactor: simplify XModule serialization/deserialization layer [BD-13] #30072

Merged

Agrendalath approved these changes Jan 10, 2023

View reviewed changes

Agrendalath merged commit d624114 into openedx:master Jan 26, 2023

Agrendalath deleted the 0x29a/bb6709/delete_xmodule_compatibility_code branch January 26, 2023 14:28

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

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

Uh oh!

Conversation

0x29a commented Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

openedx-webhooks commented Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0x29a commented Nov 23, 2022

What exactly was this code for?

What's the problem with just removing it?

Questions

Uh oh!

ormsbee commented Dec 5, 2022

Uh oh!

0x29a Dec 20, 2022

Choose a reason for hiding this comment

Uh oh!

0x29a commented Dec 20, 2022

Uh oh!

Agrendalath left a comment

Choose a reason for hiding this comment

Uh oh!

Agrendalath Dec 21, 2022

Choose a reason for hiding this comment

Uh oh!

0x29a Dec 22, 2022

Choose a reason for hiding this comment

Uh oh!

Agrendalath left a comment

Choose a reason for hiding this comment

Uh oh!

Agrendalath commented Jan 10, 2023

Uh oh!

openedx-webhooks commented Jan 26, 2023

Uh oh!

edx-pipeline-bot commented Jan 26, 2023

Uh oh!

edx-pipeline-bot commented Jan 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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

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

0x29a commented Oct 5, 2022 •

edited

Loading

openedx-webhooks commented Oct 5, 2022 •

edited

Loading