Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Waffle flags to roll out the extracted XBlocks #35549

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

farhan
Copy link
Contributor

@farhan farhan commented Sep 26, 2024

Add Waffle flags to roll out the extracted XBlocks
Flags will use to toggle between the old and new block quickly
without putting course content or user state at risk.

Testing notes:

It requires to restart lms/cms to put the waffle flag into effect

Ticket: #35308

Waffle Flags

Following built in blocks wafffle flags are in action in this PR and has been implemented:

  • word_cloud
  • annotatable
  • poll_question
  • lti
  • html
  • discussion
  • problem
  • video

Current Status:

  • Waffle flags word_cloud, annotatable, poll_question, lti, html has been tested on lms and cms
  • Waffle flags discussion, problem, video has been tested on lms and cms
    • They are currently reverted because of excessive amount of test cases were failing.

Next action items:

  • Fix all the test cases related to word_cloud, annotatable, poll_question, lti, html
  • Put back implementation of discussion, problem, video (one by one if required)
  • Fix all the test cases

@farhan farhan force-pushed the farhan/waffle-flag-for-extracted-xblock branch 2 times, most recently from cabc843 to 64cf575 Compare October 2, 2024 09:31
@farhan farhan changed the title Test PR Add Waffle flags to roll out the extracted XBlocks Oct 2, 2024
@farhan farhan marked this pull request as ready for review October 2, 2024 10:02
@farhan farhan force-pushed the farhan/waffle-flag-for-extracted-xblock branch 3 times, most recently from 4ed8c6d to 34d042c Compare October 14, 2024 08:59
@farhan farhan linked an issue Oct 14, 2024 that may be closed by this pull request
@farhan farhan force-pushed the farhan/waffle-flag-for-extracted-xblock branch 2 times, most recently from cca34ec to b2dcc10 Compare October 16, 2024 16:17
@farhan farhan force-pushed the farhan/waffle-flag-for-extracted-xblock branch 2 times, most recently from 833b935 to 9820dd6 Compare October 27, 2024 06:38
@farhan farhan closed this Oct 28, 2024
@farhan farhan reopened this Oct 28, 2024
@farhan farhan force-pushed the farhan/waffle-flag-for-extracted-xblock branch 3 times, most recently from 9eb768c to 84aba33 Compare October 31, 2024 08:38
@farhan farhan force-pushed the farhan/waffle-flag-for-extracted-xblock branch from 84aba33 to 73ddfa0 Compare November 6, 2024 07:07
@farhan farhan added the create-sandbox open-craft-grove should create a sandbox environment from this PR label Nov 6, 2024
@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@farhan farhan force-pushed the farhan/waffle-flag-for-extracted-xblock branch from a2e4584 to 73ddfa0 Compare November 6, 2024 08:15
@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@farhan farhan force-pushed the farhan/waffle-flag-for-extracted-xblock branch 4 times, most recently from 681020d to 6c91c56 Compare November 7, 2024 11:43
@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

xmodule/toggles.py Outdated Show resolved Hide resolved
@farhan farhan force-pushed the farhan/waffle-flag-for-extracted-xblock branch 2 times, most recently from 59bb65f to 9091992 Compare November 20, 2024 07:56
@farhan farhan marked this pull request as ready for review November 20, 2024 10:35
@farhan
Copy link
Contributor Author

farhan commented Nov 20, 2024

@kdmccormick @openedx/axim-aximprovements @ttqureshi PR is available for code review
Things to mention:

  1. Please review the settings files, either I have added the flags in right module
  2. Tests are failing 9b75f1e when turn the extracted block flags, which is fine but should be tested when we will publish the extracted block functionality in the xblocks-contrib pypi package.
    a. We should add it in the acceptance criteria of the extracted xblock story.

@farhan farhan force-pushed the farhan/waffle-flag-for-extracted-xblock branch from 164ab17 to 55dbfb2 Compare November 20, 2024 13:01
Copy link
Contributor

@ttqureshi ttqureshi left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

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

Thanks for converting the flags to toggles. Just a couple more comments from me.

lms/envs/common.py Outdated Show resolved Hide resolved
xmodule/toggles.py Outdated Show resolved Hide resolved
xmodule/tests/test_vertical.py Outdated Show resolved Hide resolved
@farhan farhan requested a review from feanil November 22, 2024 06:33
@farhan
Copy link
Contributor Author

farhan commented Nov 22, 2024

@kdmccormick @feanil
PR is available for code review again
CMS requires to add the settings flags into its cms/envs/common.py file, so I have imported there as well
Tests were failing 2b57c29

Please squash & merge in time slot, if all seems ok

Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

It looks like you had to correct various tests because the __name__ of the class changed. Is the name of the class also going to show up in exports or other serializations? If so, we should not update the name here or update the __name__ back to preserve the name in serializations.

@farhan
Copy link
Contributor Author

farhan commented Nov 26, 2024

It looks like you had to correct various tests because the __name__ of the class changed. Is the name of the class also going to show up in exports or other serializations? If so, we should not update the name here or update the __name__ back to preserve the name in serializations.

Thanks for sharing your extensive thoughts @feanil

On my recent testing experience with some Problem XBlocks, their UI is disturbed, it's fine on master

screencapture-apps-local-openedx-io-2000-learning-course-course-v1-edx-2-2-block-v1-edx-2-2-type-sequential-block-182eadafb52f4a99b203cf8429730fe7-block-v1-edx-2-2-type-vertical-block-fa5e73a74e5d44d49e6323da495acd1d-2024-11-26-18_45_01

Let's figure out the ripples and have its testing
cc: @irtazaakram @ttqureshi

@kdmccormick
Copy link
Member

It looks like you had to correct various tests because the name of the class changed. Is the name of the class also going to show up in exports or other serializations? If so, we should not update the name here or update the name back to preserve the name in serializations.

This is a good thing to keep in mind when changing the names of any classes which are persisted to the DB. In this particular case, though, I think we are OK. As far as I know, there are no features that depend on the literal class name of the block. The names that are used in de/serialization are the tag name(s) (problem, video, etc.) that each block is mapped to in setup.py.

Looking through the test cases which were updated, it seems that they were checking the class name rather than the class itself only because the class itself (BlahBlockWithMixins) is generated on-the-fly by mixologist.

@kdmccormick
Copy link
Member

@farhan Taking a look at your screenshot, it seems that either some CSS or JS (or both) is missing. And if you are able to still submit the problem's answer successfully, then that would imply that the JS is present, and that only the CSS is missing.

If you haven't already, try re-building static assets (npm run build-dev within tutor dev run lms bash). If that doesn't work, then I imagine that we have broke something which maps the builtin block classes to their corresponding static assets.

@kdmccormick
Copy link
Member

One fix you could try, related to @feanil 's comment earlier, would be setting __name__ on the built-in class after its definition:

_BuiltInProblemBlock.__name__ = "ProblemBlock"

This essentially tells Python "From the perspective of the module, the class goes by one name (_BuiltInProblemBlock), but I want it to appear to other objects as if it has a different name (ProblemBlock)".

@farhan
Copy link
Contributor Author

farhan commented Nov 26, 2024

@kdmccormick Thanks for sharing thoughts

  1. Related to exporting of course, xblocks are exporting fine as they are using tags rather than xblock class names
  2. I have checked again even rebuilding assets via npm run build is not fixing UI issue (Problems, Video)
  3. Running ./manage.py lms collectstatic is not fixing the ui either
  4. CSS files are generated and present but they are not linking correctly or reading css from the required files
  5. Problem is submitting and checked status exist even on reloading page
  6. When I run the same assets on master branch, UI/styling fixes.

Next:

  • So my as per my analysis, we needs to look into why css files are not linking correctly after changing of class names.
  • Check the effectiveness of following solution:
    _BuiltInProblemBlock.__name__ = "ProblemBlock"

@kdmccormick
Copy link
Member

kdmccormick commented Nov 26, 2024

Sounds good @farhan . You may already know this, but just in case: if you are running on tutor dev, then you will want to use npm run build-dev rather than npm run build. If you are running tutor local, then npm run build is appropriate.

@farhan
Copy link
Contributor Author

farhan commented Nov 26, 2024

Sounds good @farhan . You may already know this, but just in case: if you are running on tutor dev, then you will want to use npm run build-dev rather than npm run build. If you are running tutor local, then npm run build is appropriate.

I had tried npm run build-dev as well.

@farhan farhan force-pushed the farhan/waffle-flag-for-extracted-xblock branch 2 times, most recently from 37f3a0d to ee8a459 Compare November 26, 2024 17:39
@farhan farhan force-pushed the farhan/waffle-flag-for-extracted-xblock branch from ee8a459 to 8d99d1b Compare November 26, 2024 17:45
@farhan
Copy link
Contributor Author

farhan commented Nov 27, 2024

PR is available for code review.
Problem and Video Blocks are rendering normal after following change.
https://github.com/openedx/edx-platform/pull/35549/files#diff-2adcf1a5ddc155e832aeab0472da97156e8487a683da9f786198332367eacc20R2519

I have reverted the test case fixing commit.

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.

Add Django Settings to roll out extracted XBlocks
6 participants