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

feat: provisionally support V2 libraries in LibraryContentBlock (randomized only) #33263

Merged
merged 78 commits into from
Nov 20, 2023

Conversation

kdmccormick
Copy link
Member

@kdmccormick kdmccormick commented Sep 14, 2023

Co-author credit to @connorhaugh and RaccoonGang.

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 <chaugh@2u.com>
Co-Authored-By: Eugene Dyudyunov <evgen.dyudyunov@raccoongang.com>
Copy link
Member Author

@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.

Carrying over all outstanding comments from #33057

lms/envs/common.py Show resolved Hide resolved
cms/djangoapps/contentstore/tests/test_libraries.py Outdated Show resolved Hide resolved
xmodule/library_content_block.py Outdated Show resolved Hide resolved
xmodule/library_tools.py Outdated Show resolved Hide resolved
xmodule/tasks.py Outdated Show resolved Hide resolved
xmodule/tasks.py Outdated Show resolved Hide resolved
xmodule/tasks.py Outdated Show resolved Hide resolved
xmodule/tasks.py Outdated Show resolved Hide resolved
@connorhaugh connorhaugh marked this pull request as ready for review September 19, 2023 13:35
connorhaugh and others added 6 commits September 19, 2023 14:14
...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.
@open-craft-grove
Copy link

Sandbox deployment started.

@open-craft-grove
Copy link

Sandbox deployment failed.

Please check the settings and requirements and retry deployment.

@kdmccormick
Copy link
Member Author

@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?

@connorhaugh
Copy link
Contributor

@kdmccormick I'm also giving it one final once over.

@kdmccormick kdmccormick changed the title feat: support V2 libraries in LibraryContentBlock (randomized only) feat: provisionally support V2 libraries in LibraryContentBlock (randomized only) Nov 20, 2023
@kdmccormick kdmccormick enabled auto-merge (squash) November 20, 2023 14:59
@kdmccormick kdmccormick enabled auto-merge (squash) November 20, 2023 15:09
auto-merge was automatically disabled November 20, 2023 15:09

Pull request was closed

@kdmccormick kdmccormick reopened this Nov 20, 2023
@kdmccormick kdmccormick enabled auto-merge (squash) November 20, 2023 15:51
@kdmccormick kdmccormick merged commit e800ae7 into master Nov 20, 2023
121 of 126 checks passed
@kdmccormick kdmccormick deleted the kdmccormick/library-content-v2 branch November 20, 2023 15:58
kdmccormick added a commit that referenced this pull request Nov 20, 2023
@open-craft-grove
Copy link

Sandbox deployment started.

@edx-pipeline-bot
Copy link
Contributor

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

@open-craft-grove
Copy link

Sandbox update request received. Deployment will start soon.

@open-craft-grove
Copy link

Sandbox deployment failed.

Please check the settings and requirements and retry deployment.

@open-craft-grove
Copy link

Sandbox update request received. Deployment will start soon.

2 similar comments
@open-craft-grove
Copy link

Sandbox update request received. Deployment will start soon.

@open-craft-grove
Copy link

Sandbox update request received. Deployment will start soon.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

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.

5 participants