-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
[1/3] refactor: remove XModule JS from Django Pipeline #32530
[1/3] refactor: remove XModule JS from Django Pipeline #32530
Conversation
ca78b05
to
3b89711
Compare
8e3296d
to
0ffebad
Compare
0ffebad
to
17faca5
Compare
@andrey-canon Now that the Sass is simplified, I'm working to simplify the XModule JS. This PR is ready if you have the time to review. |
17faca5
to
e0c73a4
Compare
Hi @kdmccormick, I was testing the bulk email part and I couldn't replicate your results This is what I got with master vs your branch I don't know if something is missing in the instructions or if that is an error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kdmccormick my mistake since my volume didn't have the last xmodule changes, I ran the |
Nice, thanks guys. Blocked on merging this currently due to some possible regressions from my last PR. |
e0c73a4
to
323dab1
Compare
`module-js` and `module-descriptor-js` are old JavaScript group indicators, left over from when we managed XModule assets via Django Pipeline. We would like to get rid of them in order to make it easier to build XModule JS without using Python. There is one single usage of `module-js` in the entire platform (the rest have been replaced with Webpack references, which is the less-outdated way of managing XModule assets :). The lone `module-js` reference was added in 2013 [1] so that circuit diagrams would display in the course wiki. However, the ability to render circuits in the wiki was removed in 2015 [2], so it is safe to remove the reference. There is also one single usage of `module-descriptor-js`. It's in the legacy bulk email editor, which hackily cribs from the old HtmlBlock editor. Fortunately, we are able to simply replace the Django Pipeline reference with the equivalent XModule JS Webpack bundle. (Note: The old email editor is currently still supported, but is currently being replaced by frontend-app-communications, so this hack will be gone eventually). Finally, this commit also sneaks in one styling fix: it adds the HtmlBlockEditor CSS back to the aforementioned legacy bulk email page. The missing CSS was causing a read-only 1-line codemirror editor to appear below the HTML editor [3]. This bug was introduced during the original XModule SCSS decoupling [4], which removed builtin block CSS from the LMS-wide bundle, thus removing the HTML editor CSS from the bulk email page. We imagine that nobody noticed because the bug only exists in master (not Palm) and frontend-app-communications seems to be globally enabled on edx.org. As a simple fix, we add the new CSS link to the legacy bulk email page, and it renders fine again [5]. References: 1. openedx@3fc59b3 2. openedx#10324 3. Before fix: https://github.com/openedx/edx-platform/assets/3628148/25fc41b2-403d-4339-8c49-0b04664dfa02 4. openedx#32018 5. After fix: https://github.com/openedx/edx-platform/assets/3628148/9a5d74f1-cc83-4ebe-8f0c-ee270f7721b8 Part of: openedx#32481
323dab1
to
41e7ae4
Compare
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
2U Release Notice: This PR has been deployed to the edX production environment. |
This reverts commit 4aedeb8.
Part of a series of PRs:
The next one is:
Description & Supporting Information
module-js
andmodule-descriptor-js
are old JavaScript group indicators, left over from when we managed XModule assets via Django Pipeline. We would like to get rid of them in order to make it easier to build XModule JS without using Python.There is one single usage of
module-js
in the entire platform (the rest have been replaced with Webpack references, which is the less-outdated way of managing XModule assets :). The lonemodule-js
reference was added in 2013 [1] so that circuit diagrams would display in the course wiki. However, the ability to render circuits in the wiki was removed in 2015 [2], so it is safe to remove the reference.There is also one single usage of
module-descriptor-js
. It's in the legacy bulk email editor, which hackily cribs from the old HtmlBlock editor. Fortunately, we are able to simply replace the Django Pipeline reference with the equivalent XModule JS Webpack bundle. (Note: The old email editor is currently still supported, but is currently being replaced by frontend-app-communications, so this hack will be gone eventually).Finally, this commit also sneaks in one styling fix: it adds the HtmlBlockEditor CSS back to the aforementioned legacy bulk email page. The missing CSS was causing a read-only 1-line codemirror editor to appear below the HTML editor [3]. This bug was introduced during the original XModule SCSS decoupling [4], which removed builtin block CSS from the LMS-wide bundle, thus removing the HTML editor CSS from the bulk email page. We imagine that nobody noticed because the bug only exists in master (not Palm) and frontend-app-communications seems to be globally enabled on edx.org. As a simple fix, we add the new CSS link to the legacy bulk email page, and it renders fine again [5].
References:
3fc59b3
Remove circuit djangoapp from LMS #10324
Before fix:
![image](https://private-user-images.githubusercontent.com/3628148/251806271-25fc41b2-403d-4339-8c49-0b04664dfa02.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzg4Nzk0MzIsIm5iZiI6MTczODg3OTEzMiwicGF0aCI6Ii8zNjI4MTQ4LzI1MTgwNjI3MS0yNWZjNDFiMi00MDNkLTQzMzktOGM0OS0wYjA0NjY0ZGZhMDIucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIwNiUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMDZUMjE1ODUyWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9ZDI2NDIxMDlkMTM3MTIyZjFkM2Y2OTcyNjk0ZTdhNzU3NGYzODJkYjg1NTFjODczNzgwMjQzNTFjYmY3NWRhNiZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.2MVEAvtvAtfkU1o6t0KFGVMTmkoGDDBeCEXco2KOw5o)
Decouple XModule styles from LMS/Studio styles #32018
After fix:
![image](https://private-user-images.githubusercontent.com/3628148/251804871-9a5d74f1-cc83-4ebe-8f0c-ee270f7721b8.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzg4Nzk0MzIsIm5iZiI6MTczODg3OTEzMiwicGF0aCI6Ii8zNjI4MTQ4LzI1MTgwNDg3MS05YTVkNzRmMS1jYzgzLTRlYmUtOGYwYy1lZTI3MGY3NzIxYjgucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIwNiUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMDZUMjE1ODUyWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9ZTZlYzgxODQ1MTA5ZTdiNmViZWVlZGVlODA3MmRlZjI4OTY1NzVhOWQ1ZmYyNDg0MjU1ZjI4ZGY3MmJiNTZlMyZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.aEnoZIom9J8EaleZ2ARLS83wQkdNKSCm2t6xpUPzALY)
Part of: #32481
Testing
Here's how I tested:
Prereqs
Tutor Nighly provisioned, demo course imported, and two users created (one admin, one not):
Write a plugin to disable the new bulk email experience. Save this to
"$(tutor plugins printroot)/legacybulkemail.py"
:Build & run this branch, plus your plugin:
Log out of LMS if you're logged in.
Set up a student to receive bulk email
Enable bulk email
Smoke test legacy bulk email
http://local.overhang.io/course/....
. If it tries to send you to the "New Experience", then stop, something is wrong.Enable the course wiki:
Smoke-test the wiki, which no longer has unnecessary built-in XBlock JS: