edx-platform
edx-platform copied to clipboard
openedx
[WIP][BD-13][BB-6735] refactor: Unify XBlock naming
Description
Describe what this pull request changes, and why. Include implications for people using this change. Design decisions and their rationales should be documented in the repo (docstring / ADR), per OEP-19, and can be linked here.
Useful information to include:
- Which edX user roles will this change impact? Common user roles are "Learner", "Course Author", "Developer", and "Operator".
- Include screenshots for changes to the UI (ideally, both "before" and "after" screenshots, if applicable).
- Provide links to the description of corresponding configuration changes. Remember to correctly annotate these changes.
Supporting information
Link to other information about the change, such as Jira issues, GitHub issues, or Discourse discussions. Be sure to check they are publicly readable, or if not, repeat the information here.
Testing instructions
Please provide detailed step-by-step instructions for testing this change.
Deadline
"None" if there's no rush, or provide a specific date or event (and reason) if there is one.
Other information
Include anything else that will help reviewers and consumers understand the change.
- Does this change depend on other changes elsewhere?
- Any special concerns or limitations? For example: deprecations, migrations, security, or accessibility.
- If your database migration can't be rolled back easily.
Thanks for the pull request, @0x29a!
When this pull request is ready, tag your edX technical lead.
@Agrendalath, ah, I forgot to mention this in the PR description. You have to run pip install -e . in the root of your container, so the openedx package is reinstalled. If that's not the case, please consider the related configuration PR. You might need to update MODULESTORE value in the lms.yml and studio.yml.
You might need to update MODULESTORE value in the lms.yml and studio.yml.
@0x29a, this one did the trick. Thanks!
@ormsbee, would you like to take a look at this?
Please ignore the first 7 commits, as they are from https://github.com/openedx/edx-platform/pull/31113. We initially wanted to unify the descriptor naming in this PR, so we used that as a base branch. It turned out that there are too many occurrences to rename everything in a single PR.
If you could also check https://github.com/openedx/configuration/pull/6833, then it would be perfect. I don't have write permissions to that repository, so my +1 doesn't count as an official approval there.
@Agrendalath: What currently happens if the configured default_class isn't found (e.g. it's an import error)? Can we make edx-platform code fall back to a hardcoded default in that case (and make that default HiddenBlock), to make sure that old broken config won't blow things up? People do some interesting things with config and I'm not confident we'd catch it all.
@ormsbee, yes, it raises ModuleNotFoundError: No module named 'xmodule.hidden_module'. It's a good idea to fall back to the HiddenBlock.
We can catch this in MongoModuleStore, SplitMongoModuleStore, and XMLModuleStore. Alternatively, setting _options['default_class'] = 'xmodule.hidden_block.HiddenBlock' on exception here should work too.
cc: @0x29a
@Agrendalath, I've added the fall back code and re-deployed the sandbox with "wrong" configuration.
Here is what can be seen in the logs:
Dec 13 11:02:13 edxapp-extpr31113sandboxopencr-appserver-8 [service_variant=lms][xmodule.modulestore.mongo.base][env:sandbox] ERROR [edxapp-extpr31113sandboxopencr-appserver-8 1618] [user None] [ip None] [base.py:529] - Failed to import the default store class. Falling back to xmodu
le.hidden_block.HiddenDescriptor
Traceback (most recent call last):
File "/edx/app/edxapp/edx-platform/xmodule/modulestore/mongo/base.py", line 525, in __init__
class_ = getattr(import_module(module_path), class_name)
File "/usr/lib/python3.8/importlib/__init__.py", line 127, in import_module
return _bootstrap._gcd_import(name[level:], package, level)
File "<frozen importlib._bootstrap>", line 1014, in _gcd_import
File "<frozen importlib._bootstrap>", line 991, in _find_and_load
File "<frozen importlib._bootstrap>", line 973, in _find_and_load_unlocked
ModuleNotFoundError: No module named 'xmodule.hidden_module'
@Agrendalath: I'm okay without the final check. Please just schedule with @jristau1984 as usual.
That being said, if this change is going to break dev environments when people pull the latest in git, please post a notice about it in forums and the #dev channel in Slack, along with whatever error message they'll encounter.
Thank you!
@0x29a 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.
EdX Release Notice: This PR has been deployed to the production environment.
This is awesome. Great work to everyone involved in getting this point 🎸⚡
