[BD-13] [BB-5519] refactor: deletes XModuleDescriptorToXBlockMixin #29927

kaustavb12 · 2022-02-15T14:40:44Z

Description

The following changes are incorporated in this PR:

Delete XModuleDescriptorToXBlockMixin
From XmlParserMixin deletes attributes metadata_traslations and _translate() and all related implementations.
Remove the translations of the following pairs of deprecated OLX attributes during course imports:
<old_attr> : <new_attr>

'slug' : 'url_name'
'name' : 'display_name'
'id' : 'discussion_id' (discussion xblock)
'for' : 'discussion_target' (discussion xblock)
'attempts' : 'max_attempts' (capa)

Supporting information

OpenCraft internal ticket BB-5519

Testing instructions

LMS: https://pr29927.sandbox.opencraft.hosting/
Studio: https://studio.pr29927.sandbox.opencraft.hosting/

Studio:

Login as a demo staff (staff@example.com / edx)
Open any sample course or create a new course
Go to Tools > Import and upload file good.tar.gz
After the file has been imported verify all xblocks have loaded correctly
Go to Tools > Export and download file.
Verify the course is correct
Again go to Tools > Import and upload file bad.tar.gz
In this file, the CAPA module is malformed. Verify Error block is imported in place of the CAPA Multiple choice block

openedx-webhooks · 2022-02-15T14:40:50Z

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

…orToXBlockMixin

Agrendalath

Good to go once the tests pass.

👍

I tested this: tested that the blocks are working within LMS, Studio and Preview; verified that the ErrorBlock is rendering correctly
I read through the code
I checked for accessibility issues: n/a
Includes documentation: n/a
I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository: n/a

Agrendalath · 2022-02-21T20:29:02Z

common/lib/xmodule/xmodule/xml_module.py

Note: it's fine to keep the code repetition here, as we'll be removing this in a follow-up ticket.

Agrendalath · 2022-03-15T22:41:19Z

@ormsbee, would you like to review this before we merge it?

ormsbee · 2022-03-15T23:02:10Z

@Agrendalath, @kaustavb12: Could you please explain in more detail the changes you made to the OLX data files and what that implies about breaking changes to the serialization format?

kaustavb12 · 2022-03-16T13:46:52Z

Hi @ormsbee ! Thanks for reviewing the changes !

As part of this PR, we have removed the translation of legacy attributes which used to happen during course import.

I noticed a few OLX data files also contained these legacy attributes, some of which caused existing test cases to fail, after the current changes.

I have thus changed those legacy attributes to the correct ones in the OLX data files.

This would imply that import of course xmls with legacy format would not be supported after merging of this PR.

cc. @Agrendalath

ormsbee · 2022-03-16T14:45:59Z

@kaustavb12, @Agrendalath: Please spell out exactly what attribute translations will stop working after this commit, and put that information in the PR description and later on in the commit message. If there isn't a DEPR for it, we may have to make one. (It's possible it's on the DEPR board somewhere, in which case great.) We've generally been pretty cautious about breaking backwards compatibility in OLX. This is old and obscure enough where I don't think we will run into trouble, but I do think we need to inform very loudly.

FYI @pdpinch, @dianakhuang, @jristau1984, @doctoryes, @connorhaugh

Agrendalath · 2022-03-16T20:22:31Z

@ormsbee,

If there isn't a DEPR for it, we may have to make one. (It's possible it's on the DEPR board somewhere, in which case great.)

We only have a generic DEPR for XModule to XBlock conversion: openedx/public-engineering#70. Should we put this information as a comment there or create a separate DEPR issue?

ormsbee · 2022-03-16T21:34:47Z

Separate issue please. Thank you.

kaustavb12 · 2022-03-17T17:32:46Z

@ormsbee The following pairs of translations have been removed:
<old_attr> : <new_attr>

'slug' : 'url_name'
'name' : 'display_name'
'id' : 'discussion_id' (discussion xblock)
'for' : 'discussion_target' (discussion xblock)
'attempts' : 'max_attempts' (capa)

cc. @Agrendalath

Agrendalath · 2022-03-21T19:27:46Z

@ormsbee, I've created openedx/public-engineering#74. I've put dates from the BD-13 discovery, though we can adjust them if needed.
cc: @kaustavb12

ormsbee · 2022-03-22T02:12:00Z

Please set the acceptance date to two weeks from now and announce it in the DEPR section of the forums. I'm sorry if this delays this ticket or hampers other refactoring work, but we have to loudly broadcast anything that may break backwards compatibility with our import/export format, even if it's using really old 2012-compatibility features that we don't think anybody even remembers existing.

Agrendalath · 2022-04-01T12:43:02Z

@ormsbee, oh, I've just noticed your comment, as it didn't get through my inbox filters without an explicit mention. I've created the announcement.

I'm sorry if this delays this ticket or hampers other refactoring work, but we have to loudly broadcast anything that may break backwards compatibility with our import/export format, even if it's using really old 2012-compatibility features that we don't think anybody even remembers existing.

Of course, that's understandable. Thank you for your guidance on this.

openedx-webhooks · 2022-04-21T15:15:06Z

@kaustavb12 🎉 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 · 2022-04-21T17:53:07Z

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

edx-pipeline-bot · 2022-04-21T18:15:42Z

EdX Release Notice: This PR may have caused e2e tests to fail on Stage. If you're a member of the edX org, please visit #e2e-troubleshooting on Slack to help diagnose the cause of these failures. Otherwise, it is the reviewer's responsibility. E2E tests have failed. https://gocd.tools.edx.org/go/tab/pipeline/history/deploy_to_stage

edx-pipeline-bot · 2022-04-21T22:29:16Z

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

This: 1. Deletes XModuleDescriptorToXBlockMixin 2. Deletes `metadata_traslations` and `_translate()` from `XmlParserMixin`, and all related implementations. 3. Removes translations of deprecated OLX attributes during import/export: <old_attr> : <new_attr> - 'slug' : 'url_name' - 'name' : 'display_name' - 'id' : 'discussion_id' (Discussion XBlock) - 'for' : 'discussion_target' (Discussion XBlock) - 'attempts' : 'max_attempts' (CAPA)

openedx-webhooks added blended PR is managed through 2U's blended developmnt program needs triage labels Feb 15, 2022

kaustavb12 marked this pull request as draft February 15, 2022 14:41

kaustavb12 force-pushed the kaustav/delete_serialization_code branch 3 times, most recently from d49e998 to 48b6c91 Compare February 16, 2022 12:01

kaustavb12 marked this pull request as ready for review February 16, 2022 12:02

kaustavb12 added 2 commits February 21, 2022 21:34

feat: delete add_xml_to_delete and export_to_xml from XModuleDescript…

1b5182f

…orToXBlockMixin

feat: delete parse_xml, parse_xml_new_runtime, from_xml

7d8f4b0

Agrendalath force-pushed the kaustav/delete_serialization_code branch from 48b6c91 to d201b22 Compare February 21, 2022 20:34

Agrendalath approved these changes Feb 21, 2022

View reviewed changes

kaustavb12 force-pushed the kaustav/delete_serialization_code branch from d201b22 to b678466 Compare February 26, 2022 09:44

kaustavb12 added 3 commits February 27, 2022 15:27

feat: delete metadata_translations dict and _translate()

09fa2bb

feat: delete XModuleDescriptorToXBlockMixin

283e43d

test: remove and replace deprecated attributes

e5116f0

kaustavb12 force-pushed the kaustav/delete_serialization_code branch from 2a2682e to e5116f0 Compare February 27, 2022 10:01

test: replace deprecated xml attributes from test data

5fe8ce5

Agrendalath approved these changes Mar 15, 2022

View reviewed changes

Agrendalath mentioned this pull request Mar 21, 2022

[DEPR]: Delete translations of deprecated OLX attributes during course import openedx/public-engineering#74

Closed

Agrendalath merged commit 702866b into openedx:master Apr 21, 2022

Agrendalath deleted the kaustav/delete_serialization_code branch April 21, 2022 15:15

openedx-webhooks added merged and removed needs triage labels Apr 21, 2022

Agrendalath mentioned this pull request Sep 14, 2022

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

Merged

kdmccormick mentioned this pull request Nov 12, 2024

Improve error for missing (Translate)CustomTagBlock template #35829

Closed

[BD-13] [BB-5519] refactor: deletes XModuleDescriptorToXBlockMixin #29927

[BD-13] [BB-5519] refactor: deletes XModuleDescriptorToXBlockMixin #29927

Uh oh!

Conversation

kaustavb12 commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting information

Testing instructions

Uh oh!

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

Uh oh!

Uh oh!

Agrendalath left a comment • 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!

Agrendalath Feb 21, 2022

Choose a reason for hiding this comment

Uh oh!

Agrendalath commented Mar 15, 2022

Uh oh!

ormsbee commented Mar 15, 2022

Uh oh!

kaustavb12 commented Mar 16, 2022

Uh oh!

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

Uh oh!

Uh oh!

Agrendalath commented Mar 16, 2022

Uh oh!

ormsbee commented Mar 16, 2022

Uh oh!

kaustavb12 commented Mar 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Agrendalath commented Mar 21, 2022

Uh oh!

ormsbee commented Mar 22, 2022

Uh oh!

Agrendalath commented Apr 1, 2022

Uh oh!

openedx-webhooks commented Apr 21, 2022

Uh oh!

edx-pipeline-bot commented Apr 21, 2022

Uh oh!

edx-pipeline-bot commented Apr 21, 2022

Uh oh!

edx-pipeline-bot commented Apr 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kaustavb12 commented Feb 15, 2022 •

edited

Loading

openedx-webhooks commented Feb 15, 2022 •

edited

Loading

Agrendalath left a comment •

edited

Loading

ormsbee commented Mar 16, 2022 •

edited

Loading

kaustavb12 commented Mar 17, 2022 •

edited

Loading