Skip to content

fix: remove legacy xmodulemixin from xblocks-contrib xblocks#38271

Draft
irtazaakram wants to merge 1 commit intomasterfrom
fix-extracted-xblocks-legacy-mixins
Draft

fix: remove legacy xmodulemixin from xblocks-contrib xblocks#38271
irtazaakram wants to merge 1 commit intomasterfrom
fix-extracted-xblocks-legacy-mixins

Conversation

@irtazaakram
Copy link
Copy Markdown
Member

@irtazaakram irtazaakram commented Apr 2, 2026

Relevant PR in xblocks-contrib: openedx/xblocks-contrib#230

@irtazaakram irtazaakram force-pushed the fix-extracted-xblocks-legacy-mixins branch 3 times, most recently from 4faae90 to 42f9eda Compare April 9, 2026 06:43
Comment on lines 2090 to +2100
@@ -2097,7 +2097,7 @@ def add_optional_apps(optional_apps, installed_apps):
# .. toggle_warning: Not production-ready until relevant subtask https://github.com/openedx/edx-platform/issues/34827 is done.
# .. toggle_creation_date: 2024-11-10
# .. toggle_target_removal_date: 2026-04-10
USE_EXTRACTED_PROBLEM_BLOCK = False
USE_EXTRACTED_PROBLEM_BLOCK = True
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

TODO: Revert these flags before merging

Comment on lines +1200 to +1201
settings_mixins = getattr(settings, 'XBLOCK_MIXINS', ())
self.xblock_mixins = tuple(dict.fromkeys(settings_mixins + xblock_mixins))
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.

You need to add this because XModuleMixin was missing from modulestore-based tests, right?

For modulestore-based app code, the mixins are being added here:

xblock_mixins=getattr(settings, 'XBLOCK_MIXINS', ()),

Do you know why that isn't working for tests? Ideally, we should only add XBLOCK_MIXINS in one place.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It was XMLModuleStore's tests that were failing. I've reverted this change and made this addition to tests so we are only adding it in one place.

@irtazaakram irtazaakram force-pushed the fix-extracted-xblocks-legacy-mixins branch from 76fc611 to 017e505 Compare April 10, 2026 10:46
@irtazaakram irtazaakram force-pushed the fix-extracted-xblocks-legacy-mixins branch from 017e505 to 2c98abc Compare April 10, 2026 11:17
@kdmccormick
Copy link
Copy Markdown
Member

kdmccormick commented Apr 10, 2026

Code is crashing on the following test sceneario, it's working fine on the master/main branch.
Test Scenario: Add Video Component (Block) in the Content Library

Right, this is the scenario I was talking about in sync: The new runtime intentionally does not add XModuleMixin because we are trying to keep the legacy attributes out of new runtime code. Content Libraries uses this runtime. We have two options:

  1. We remove the usage of just enough legacy attributes so that Content Libraries can continue to work without XModuleMixin.
  2. We temporarily add XModuleMixin to the new runtime, with a TODO comment and a link to a followup issue.

Of course (1) is preferred if it's easy. But if (1) would mean that the refactoring would go past next Wednesday, then go with (2).

Let me know if you'd like help debugging or deciding which option to do.

@farhan
Copy link
Copy Markdown
Contributor

farhan commented Apr 10, 2026

Code is crashing on the following test sceneario, it's working fine on the master/main branch.
Test Scenario: Add Video Component (Block) in the Content Library

Right, this is the scenario I was talking about in sync: The new runtime intentionally does not add XModuleMixin because we are trying to keep the legacy attributes out of new runtime code. Content Libraries uses this runtime. We have two options:

  1. We remove the usage of just enough legacy attributes so that Content Libraries can continue to work without XModuleMixin.
  2. We temporarily add XModuleMixin to the new runtime, with a TODO comment and a link to a followup issue.

Of course (1) is preferred if it's easy. But if (1) would mean that the refactoring would go past next Wednesday, then go with (2).

Let me know if you'd like help debugging or deciding which option to do.

I moved the comment in this PR

I think we can spend limited time in exploring 1 or 2 cases if it found extra complex then let's go for point 2 now and create a story with details for point 1

For example: usage of bind_for_student for VideoBlock in content library

Perhaps we have option 3 as well:
3. Move all the methods back to the XBlocks which are needed to run XBlocks in the content-library without attaching XModuleMixin to content library runtime.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants