-
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
feat: provisionally support V2 libraries in LibraryContentBlock (randomized only) #33263
Conversation
Rather than implementing V2-library and static-library-reference support in a new block, we will be enhancing the existing `LibraryContentBlock` in-place. Relevant ADR PR: #33231
Co-Authored-By: Connor Haugh <[email protected]> Co-Authored-By: Eugene Dyudyunov <[email protected]>
02bd84a
to
9eee194
Compare
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.
Carrying over all outstanding comments from #33057
xmodule/assets/library_content/public/js/library_content_edit_helpers.js
Show resolved
Hide resolved
xmodule/assets/library_content/public/js/library_content_edit_helpers.js
Show resolved
Hide resolved
...per comment in cms/djangoapps/contentstore/tasks.py. This fixes an exception that was raised in the background whenever the update_children_task was started: Traceback (most recent call last): File "/openedx/venv/lib/python3.8/site-packages/celery/utils/dispatch/signal.py", line 276, in send response = receiver(signal=self, sender=sender, **named) File "/openedx/venv/lib/python3.8/site-packages/user_tasks/signals.py", line 215, in start_user_task sender.status.start() File "/openedx/venv/lib/python3.8/site-packages/user_tasks/tasks.py", line 84, in status name = self.generate_name(arguments_dict) File "/openedx/edx-platform/openedx/core/djangoapps/content_libraries/tasks.py", line 274, in generate_name key = arguments_dict['dest_block_key'] KeyError: 'dest_block_key'
Just like ./common/ and ./openedx/, the unit tests in ./xmodule/ validate behavior in both LMS and CMS. In order to fully test ./xmodule/, we need to run its tests in both contexts. Also in this commit: * refactor: Rename the shards to be clear whether they're running under LMS or CMS. * docs: correct comments regarding conditions under which codejail's test_cant_do_something_forbidden is skipped. * test: update a unit test which was using the now-deleted library_sourced block to use library_content block instead.
Sandbox deployment started. |
Sandbox deployment failed. Please check the settings and requirements and retry deployment. |
@connorhaugh Unit tests are all set now. The only failing check is a random pylint thing that I can fix Monday. I'll be manually testing this more Monday morning. Assuming the recent commits look good to you and we don't find bugs, let's merge in the afternoon? |
@kdmccormick I'm also giving it one final once over. |
Pull request was closed
Sandbox deployment started. |
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
Sandbox update request received. Deployment will start soon. |
Sandbox deployment failed. Please check the settings and requirements and retry deployment. |
Sandbox update request received. Deployment will start soon. |
2 similar comments
Sandbox update request received. Deployment will start soon. |
Sandbox update request received. Deployment will start soon. |
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. |
Co-author credit to @connorhaugh and RaccoonGang.