Skip to content

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

Merged
irtazaakram merged 12 commits intomasterfrom
fix-extracted-xblocks-legacy-mixins
Apr 22, 2026
Merged

fix: remove legacy xmodulemixin from xblocks-contrib xblocks#38271
irtazaakram merged 12 commits intomasterfrom
fix-extracted-xblocks-legacy-mixins

Conversation

@irtazaakram
Copy link
Copy Markdown
Member

@irtazaakram irtazaakram commented Apr 2, 2026 •
edited
Loading

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

Settings

XQUEUE_DOCKER_IMAGE: docker.io/overhangio/openedx-xqueue:21.0.0
CODEJAIL_ENABLE_K8S_DAEMONSET: true

Tutor requirements

pip install git+https://github.com/irtazaakram/tutor-xqueue.git@fix-url
pip install git+https://github.com/eduNEXT/tutor-contrib-codejail.git@main
tutor plugins enable codejail xqueue

@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
irtazaakram
irtazaakram commented Apr 9, 2026
View reviewed changes
Comment thread openedx/envs/common.py Outdated
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

@irtazaakram irtazaakram Apr 9, 2026

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

Copy link
Copy Markdown
Contributor

@salman2013 salman2013 Apr 20, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Just a note: USE_EXTRACTED_DISCUSSION_BLOCK has now been enabled.

kdmccormick
kdmccormick reviewed Apr 9, 2026
View reviewed changes
Comment thread xmodule/modulestore/__init__.py Outdated
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

@kdmccormick kdmccormick Apr 9, 2026

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:

openedx-platform/xmodule/modulestore/django.py

Line 321 in dc6e19e

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

@irtazaakram irtazaakram Apr 10, 2026

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 2 times, most recently from 017e505 to 2c98abc Compare April 10, 2026 11:17
@kdmccormick
Copy link
Copy Markdown
Member

kdmccormick commented Apr 10, 2026 •
edited
Loading

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 •
edited
Loading

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.

@irtazaakram irtazaakram mentioned this pull request Apr 14, 2026
Merged
@irtazaakram irtazaakram force-pushed the fix-extracted-xblocks-legacy-mixins branch from 8a71c01 to c394521 Compare April 14, 2026 08:21
@irtazaakram irtazaakram reopened this Apr 14, 2026
@irtazaakram irtazaakram added the create-sandbox open-craft-grove should create a sandbox environment from this PR label Apr 14, 2026
@kaustavb12 kaustavb12 added create-sandbox open-craft-grove should create a sandbox environment from this PR and removed create-sandbox open-craft-grove should create a sandbox environment from this PR labels Apr 15, 2026
@irtazaakram irtazaakram self-assigned this Apr 15, 2026
@kdmccormick kdmccormick self-requested a review April 17, 2026 13:49
@farhan
Copy link
Copy Markdown
Contributor

farhan commented Apr 20, 2026 •
edited
Loading

  • I think after updating this PR to latest XBlock; we should remove the attributes from the XmoduleMixin which has been recently added to XBlock class

@irtazaakram irtazaakram moved this to 👀 In review in Aximprovements Team Apr 20, 2026
irtazaakram and others added 3 commits April 21, 2026 13:16
@irtazaakram
Merge branch 'master' into fix-extracted-xblocks-legacy-mixins
22ce5e0
@github-actions @irtazaakram
feat: Upgrade Python dependency xblocks-contrib (#38399)
512b2c9 
Commit generated by workflow `openedx/openedx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`

Co-authored-by: irtazaakram <51848298+irtazaakram@users.noreply.github.com>
@irtazaakram
fix: revert extracted problem toggle
c6a2a90
@irtazaakram irtazaakram marked this pull request as ready for review April 21, 2026 09:29
@irtazaakram irtazaakram merged commit a82cd98 into master Apr 22, 2026
47 checks passed
@irtazaakram irtazaakram deleted the fix-extracted-xblocks-legacy-mixins branch April 22, 2026 13:06
@github-project-automation github-project-automation Bot moved this from 👀 In review to ✅ Done in Aximprovements Team Apr 22, 2026
@kdmccormick kdmccormick mentioned this pull request Apr 22, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kdmccormick kdmccormick kdmccormick approved these changes

@feanil feanil Awaiting requested review from feanil

@salman2013 salman2013 Awaiting requested review from salman2013 salman2013 is a code owner

@farhan farhan Awaiting requested review from farhan farhan is a code owner

Assignees

@irtazaakram irtazaakram

Labels

create-sandbox open-craft-grove should create a sandbox environment from this PR

Projects

Aximprovements Team
Status: ✅ Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@irtazaakram @kdmccormick @farhan @salman2013 @kaustavb12