Skip to content

[BD-13] refactor: Remove unused XModule classes - #31173

Merged
Agrendalath merged 12 commits into
openedx:masterfrom
open-craft:maxim/remove-unused-xmodule-classes
Nov 7, 2022
Merged

[BD-13] refactor: Remove unused XModule classes#31173
Agrendalath merged 12 commits into
openedx:masterfrom
open-craft:maxim/remove-unused-xmodule-classes

Conversation

@Cup0fCoffee

@Cup0fCoffee Cup0fCoffee commented Oct 19, 2022

Copy link
Copy Markdown
Contributor

Description

This PR removes XModule classes that are no longer being used after the conversion of all the XModules to standard XBlocks.

Testing instructions

Test that when editing in studio the following XBlock work that same as before these changes:

  • VideoBlock - especially tabs when editing
  • WordCloudBlock

Other notes

How can we test CustomTagBlock? #31173 (comment)

Private-ref: BB-6732

@openedx-webhooks

openedx-webhooks commented Oct 19, 2022

Copy link
Copy Markdown

Thanks for the pull request, @Cup0fCoffee!

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

@openedx-webhooks openedx-webhooks added the blended PR is managed through 2U's blended developmnt program label Oct 19, 2022
@Cup0fCoffee Cup0fCoffee changed the title [WIP][BD-13] Remove unused XModule classes [WIP][BD-13] refactor: Remove unused XModule classes Oct 19, 2022
@Cup0fCoffee
Cup0fCoffee force-pushed the maxim/remove-unused-xmodule-classes branch 2 times, most recently from 4cef89f to 36a8f22 Compare October 21, 2022 10:31
@Cup0fCoffee Cup0fCoffee changed the title [WIP][BD-13] refactor: Remove unused XModule classes [BD-13] refactor: Remove unused XModule classes Oct 24, 2022
@Agrendalath

Agrendalath commented Oct 27, 2022

Copy link
Copy Markdown
Member

How can we test CustomTagBlock?

This one is a bit tricky. We can import course-xmodules.tar.gz, as it cannot be created directly in Studio (source: #26873).

@Agrendalath Agrendalath left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I just left one small suggestion.

👍

  • I tested this: checked that it's not breaking existing XBlocks (including AnnotatableBlock, CustomTagBlock, etc.)
  • 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

Comment thread xmodule/editing_module.py Outdated
@Agrendalath

Agrendalath commented Oct 27, 2022

Copy link
Copy Markdown
Member

@ormsbee, I just left one small suggestion on the PR, but it's good to go from my end. Would you like to take a look?

Edit: my message got duplicated somehow, so I deleted the other one.

@0x29a

0x29a commented Oct 27, 2022

Copy link
Copy Markdown
Contributor

@Cup0fCoffee, I guess xmodule/tests/test_files/test_tabseditingdescriptor.css and xmodule/tests/test_files/test_tabseditingdescriptor.scss can be removed too, as they're used only in xmodule/tests/test_editing_module.py, which is being removed here.

@ormsbee

ormsbee commented Oct 28, 2022

Copy link
Copy Markdown
Contributor

@Agrendalath: Just one quick sanity check–after these class removals, will we still have the ability to edit the raw OLX of unknown XBlock types in Studio? If the answer to that is yes, I don't have anything to add here.

Is the primary regression risk area going to be Studio authoring?

@Cup0fCoffee

Copy link
Copy Markdown
Contributor Author

@0x29a Nice catch! Removing it.

@Cup0fCoffee
Cup0fCoffee force-pushed the maxim/remove-unused-xmodule-classes branch 3 times, most recently from 5566921 to c6895f4 Compare October 28, 2022 13:22
@Cup0fCoffee
Cup0fCoffee force-pushed the maxim/remove-unused-xmodule-classes branch from c6895f4 to 3e9437c Compare October 28, 2022 13:38
@Agrendalath

Copy link
Copy Markdown
Member

@ormsbee,

Just one quick sanity check–after these class removals, will we still have the ability to edit the raw OLX of unknown XBlock types in Studio? If the answer to that is yes, I don't have anything to add here.

By "unknown XBlock types" do you mean "XBlocks that are not (and were not) installed in the platform but are imported in a course"? If yes, then it is not working for me on the devstack using the current master version (it hasn't been working for me for at least a few months). When load_error_modules is False, the XBlock is just skipped (it doesn't appear in the export too). When load_error_modules is True, I'm getting an ErrorBlock, which has an empty editor in Studio. I can verify this in environments if this is something that is expected to work.
This PR should not alter this behavior in any way, though, as it's removing unused code that was left after converting old built-in XModules to XBlocks.

If you mean editing raw OLX of blocks like CustomTagBlock and AnnotatableBlock (that are using the XMLEditingDescriptor JS class in Studio), then this part works correctly.

Is the primary regression risk area going to be Studio authoring?

That's right.

@ormsbee

ormsbee commented Oct 28, 2022

Copy link
Copy Markdown
Contributor

@Agrendalath: Sounds good, thank you for the detailed explanation.

@Agrendalath
Agrendalath merged commit fd2e95f into openedx:master Nov 7, 2022
@Agrendalath
Agrendalath deleted the maxim/remove-unused-xmodule-classes branch November 7, 2022 16:42
@openedx-webhooks

Copy link
Copy Markdown

@Cup0fCoffee 🎉 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

Copy link
Copy Markdown
Contributor

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

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

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

1 similar comment
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blended PR is managed through 2U's blended developmnt program

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants