refactor: move xmodule folder to root by mumarkhan999 · Pull Request #30394 · openedx/openedx-platform

mumarkhan999 · 2022-05-16T07:01:03Z

In this PR
We are moving common/lib/xmodule/xmodule/ to / root level.

The following things have been updated in this PR

entry points and other configs in setup.py
Github actions
symlinks
Module imports
Webpack configurations
Linting and coverage related configs
Few docs

Ticket Links

Public: BOM-2584
Internal: BOM-2584

Sandbox URL for this PR https://xmodule.sandbox.edx.org/

This sandbox is operational till Saturday, 2 July 2022

Jawayria · 2022-06-03T10:46:34Z

cms/static/js/spec/views/container_spec.js

Using the correct symlink name worked here but I wonder how previous one is working on master then?

@Jawayria:
Using the correct symlink name worked here but I wonder how previous one is working on master then

This works on master because of this line in webpack.common.config.js: https://github.com/openedx/edx-platform/blob/7f5d8e3511ed70a3097b3114818a0e335eefb867/webpack.common.config.js#L394

That line adds common/lib/xmodule to Webpack's list of directories for JS module resolution. When Webpack sees import ..., it'll look in all of those directories for modules. That's how it finds the file common/lib/xmodule/xmodule/js/src/xmodule.js based on import xmodule/js/src/xmodule.

Your fix does indeed fix the tests, but I am nervous to change import paths in tests, because I also do not understand this mish-mash of JavaScript tooling very well :) if we change import paths in tests but neglect to do so in some untested production file, then it could cause bugs.

So, we instead need to change webpack.common.config.js to search from the repository root (.) instead of common/lib/xmodule. According to webpack's docs, though, adding . to the module resolution list would cause Webpack to search for imports in every single JavaScript directory, not just the repository root... but if we add the absolute path using path.resolve(__dirname), then everything works fine!

Jawayria · 2022-06-03T10:47:39Z

xmodule/js/spec/time_spec.js

Here I had to make correction in path and similar corrections in some other files. Again I don't understand how is it working on master?

@Jawayria
Here I had to made correction in path and similar corrections in some other files. Again I don't understand how is it working on master?

Just like with the other import issue: this works on master because in webpack.common.js, common/lib/xmodule/xmodule/js/src is added to the list of module resolution directories. If you fix the resolution path, though, then the original imports should start working again.

kdmccormick

Hi @Jawayria , I spent some time digging into this and I think I figured out what's going on with those broken JS import paths. The main culprit seems to be some webpack configuration that needs updating, which I explain more in my comments.

I also reviewed your PR because, well, I needed to look through the entire thing to figure out the import bug anyway 😄 If it was not ready for review my apologies.

I also want to ping @ormsbee and @0x29a (from the BD-13 XModule cleanup) about this PR just as an FYI and a heads-up on the upcoming exciting merge conflicts.

scripts/verify-dunder-init.sh

kdmccormick · 2022-06-05T03:34:30Z

common/lib/xmodule/test_files/formularesponse_with_hint.xml

Why did you delete this file, and the other XML files in this directory? Were they unused?

These were 10 years old files. I tried to find their usage but couldn't find any. That's why removed them.

Sounds good to me. Just make a note in your final commit message so that future developers know that it was on purpose.

xmodule/tests/__init__.py

.github/CODEOWNERS

docs/guides/conf.py

kdmccormick · 2022-06-06T01:06:52Z

cms/static/js/spec/views/container_spec.js

@Jawayria:
Using the correct symlink name worked here but I wonder how previous one is working on master then

This works on master because of this line in webpack.common.config.js: https://github.com/openedx/edx-platform/blob/7f5d8e3511ed70a3097b3114818a0e335eefb867/webpack.common.config.js#L394

That line adds common/lib/xmodule to Webpack's list of directories for JS module resolution. When Webpack sees import ..., it'll look in all of those directories for modules. That's how it finds the file common/lib/xmodule/xmodule/js/src/xmodule.js based on import xmodule/js/src/xmodule.

Your fix does indeed fix the tests, but I am nervous to change import paths in tests, because I also do not understand this mish-mash of JavaScript tooling very well :) if we change import paths in tests but neglect to do so in some untested production file, then it could cause bugs.

So, we instead need to change webpack.common.config.js to search from the repository root (.) instead of common/lib/xmodule. According to webpack's docs, though, adding . to the module resolution list would cause Webpack to search for imports in every single JavaScript directory, not just the repository root... but if we add the absolute path using path.resolve(__dirname), then everything works fine!

setup.py

xmodule/assets/vertical/public/js/vertical_student_view.js

setup.py

kdmccormick · 2022-06-06T01:55:47Z

xmodule/js/spec/time_spec.js

@Jawayria
Here I had to made correction in path and similar corrections in some other files. Again I don't understand how is it working on master?

Just like with the other import issue: this works on master because in webpack.common.js, common/lib/xmodule/xmodule/js/src is added to the list of module resolution directories. If you fix the resolution path, though, then the original imports should start working again.

kdmccormick

Thanks for making those fixes. This is so close! Two things:

The 2u-internal ticket link in your commit message is hidden to all of the community except 2U. Why not include the public openedx.atlassian.net version of the ticket too?
Compared to master, the verify unit tests count output dropped by a few hundred tests. This is because ./xmodule tests are not being counted nor run on this branch.

mumarkhan999 · 2022-06-16T14:59:14Z

Thanks for making those fixes. This is so close! Two things:

The 2u-internal ticket link in your commit message is hidden to all of the community except 2U. Why not include the public openedx.atlassian.net version of the ticket too?

Compared to master, the verify unit tests count output dropped by a few hundred tests. This is because ./xmodule tests are not being counted nor run on this branch.

Fixed.

kdmccormick · 2022-06-16T15:17:56Z

@mumarkhan999 pytest is still not being run in the xmodule/ folder. You can tell because there are no unit-tests / python-3.8-django-pinned,xmodule-* entries in the list of checks on this PR. I think you need to edit the list of shards in unit-tests.yml.

kdmccormick

Thanks for all your hard work on this huge PR. Just two more questions, and then I'm ready to approve, assuming tests pass.

kdmccormick · 2022-06-16T15:32:30Z

.github/workflows/unit-test-shards.json

You had originally added an xmodule-2 shard, which tested the folder using CMS settings. I thought that was a great idea. Any specific reason you decided to remove it?

Actually when the xmodule folder was in common the tests were also being executed by just lms settings... so I just tried to keep it that way.

Fair enough.

kdmccormick · 2022-06-16T15:33:09Z

.github/workflows/unit-tests.yml

👍🏻

This isn't something you need to fix now, but it's unfortunate that we need to list shards both here and in unit-test-shards.json. It would be really easy to add a new shard in the JSON file but forget to add it here. Could you ask your tech lead to add this to your backlog?

Yeah sure... will create a ticket for this.

kdmccormick · 2022-06-16T15:55:38Z

I recommend giving whoever is on call for edX T&L (@connorhaugh ?) a heads up before merging this one.

- Moving xmodule folder to root as we're dissolving sub-projects of common folder in edx-platform - More info: https://openedx.atlassian.net/browse/BOM-2579 - -e common/lib/xmodule has been removed from the requirements as xmodule has itself become the part of edx-platform and not being installed through requirements - The test files common/lib/xmodule/test_files/ have been removed as they are not being used anymore

edx-pipeline-bot · 2022-06-20T12:30:11Z

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

kdmccormick · 2022-06-21T12:44:29Z

🤞🏻

edx-pipeline-bot · 2022-06-21T13:38:45Z

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

mumarkhan999 force-pushed the umar/move-xmdule-folder-to-root branch 9 times, most recently from 2363b2e to 8cc7b23 Compare May 23, 2022 06:59

mumarkhan999 force-pushed the umar/move-xmdule-folder-to-root branch 12 times, most recently from c31dfdf to 3691d4b Compare May 27, 2022 12:04

Jawayria closed this Jun 2, 2022

Jawayria reopened this Jun 2, 2022

Jawayria force-pushed the umar/move-xmdule-folder-to-root branch from 90ab2d7 to 0f02b40 Compare June 2, 2022 11:59

Jawayria reviewed Jun 3, 2022

View reviewed changes

Jawayria force-pushed the umar/move-xmdule-folder-to-root branch from 80fb790 to 62bda91 Compare June 3, 2022 12:06

This was referenced Jun 5, 2022

refactor: move ./common/lib/xmodule/xmodule to ./xmodule #27870

Closed

temp: experimenting with xmodule root folder js tests #30539

Closed

kdmccormick reviewed Jun 6, 2022

View reviewed changes

mumarkhan999 force-pushed the umar/move-xmdule-folder-to-root branch 5 times, most recently from 25bcd0b to bc3fea6 Compare June 15, 2022 07:40

kdmccormick requested changes Jun 15, 2022

View reviewed changes

mumarkhan999 force-pushed the umar/move-xmdule-folder-to-root branch 4 times, most recently from 13a247a to 47323fe Compare June 16, 2022 14:36

mumarkhan999 force-pushed the umar/move-xmdule-folder-to-root branch from 47323fe to d543a9a Compare June 16, 2022 15:27

kdmccormick reviewed Jun 16, 2022

View reviewed changes

kdmccormick approved these changes Jun 16, 2022

View reviewed changes

mumarkhan999 force-pushed the umar/move-xmdule-folder-to-root branch 3 times, most recently from cb1ac69 to aac1bfc Compare June 20, 2022 09:04

mumarkhan999 force-pushed the umar/move-xmdule-folder-to-root branch from aac1bfc to a91df0c Compare June 20, 2022 09:34

mumarkhan999 merged commit 182eb39 into master Jun 20, 2022

mumarkhan999 deleted the umar/move-xmdule-folder-to-root branch June 20, 2022 11:03

mumarkhan999 mentioned this pull request Jun 20, 2022

Revert "refactor: move xmodule folder to root" #30621

Merged

mumarkhan999 mentioned this pull request Jun 20, 2022

Revert "Revert "refactor: move xmodule folder to root"" #30622

Merged

kdmccormick mentioned this pull request Jun 20, 2022

docs: update xmodule README #30623

Merged

angonz mentioned this pull request Dec 28, 2022

Broken 'Pages' view in Studio is back in Tutor 14 / Nutmeg overhangio/tutor#769

Closed

Conversation

mumarkhan999 commented May 16, 2022 • edited by kdmccormick Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jawayria Jun 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kdmccormick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kdmccormick left a comment

Choose a reason for hiding this comment

Uh oh!

mumarkhan999 commented Jun 16, 2022

Uh oh!

kdmccormick commented Jun 16, 2022

Uh oh!

kdmccormick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kdmccormick Jun 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kdmccormick commented Jun 16, 2022

Uh oh!

edx-pipeline-bot commented Jun 20, 2022

Uh oh!

kdmccormick commented Jun 21, 2022

Uh oh!

edx-pipeline-bot commented Jun 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mumarkhan999 commented May 16, 2022 •

edited by kdmccormick

Loading

Jawayria Jun 3, 2022 •

edited

Loading

kdmccormick Jun 16, 2022 •

edited

Loading