-
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
fix: handle paste of library content blocks correctly #33926
fix: handle paste of library content blocks correctly #33926
Conversation
Thanks for the pull request, @bradenmacdonald! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
b23dc41
to
5fbbaba
Compare
5fbbaba
to
7d54add
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.
Thanks @bradenmacdonald . Worked on tutor dev
for me. Just one optional nit.
I'm glad you went with the approach of just special-casing library_content here instead of the studio_post_paste method, while I'm afraid that only library_content would ever override.
Somewhat tangential, but @ormsbee and I are talking about a more general extension point (something like "map_children_to_upstream") that could I'm hoping we can use to generically generate the correct child ids anytime a node is created with reference to an upstream. The library_content block would just override that one method, and it'd replace the special library_content cases from across the board: duplication, paste, and import. 🤞🏻
if new_xblock.scope_ids.block_type == "library_content": | ||
# Special case handling for library content. If we need this for other blocks in the future, it can be made into | ||
# an API, and we'd call new_block.studio_post_paste() instead of this code. | ||
# In this case, we want to pull the children from the library and let library_tools assign their IDs. | ||
new_xblock.sync_from_library(upgrade_to_latest=False) |
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.
Nit/Optional: using isinstance would make this bit pass type-checking, whenever we get around to enabling that for this module.
But, it also might create an import cycle. Up to you.
if new_xblock.scope_ids.block_type == "library_content": | |
# Special case handling for library content. If we need this for other blocks in the future, it can be made into | |
# an API, and we'd call new_block.studio_post_paste() instead of this code. | |
# In this case, we want to pull the children from the library and let library_tools assign their IDs. | |
new_xblock.sync_from_library(upgrade_to_latest=False) | |
if isinstance(new_xblock, LibraryContentBlock): | |
# Special case handling for library content. If we need this for other blocks in the future, it can be made into | |
# an API, and we'd call new_block.studio_post_paste() instead of this code. | |
# In this case, we want to pull the children from the library and let library_tools assign their IDs. | |
new_xblock.sync_from_library(upgrade_to_latest=False) |
def create_library( | ||
collection_uuid, library_type, org, slug, title, description, allow_public_learning, allow_public_read, | ||
library_license, | ||
collection_uuid, | ||
org, | ||
slug, | ||
title, | ||
description="", | ||
allow_public_learning=False, | ||
allow_public_read=False, | ||
library_license=ALL_RIGHTS_RESERVED, | ||
library_type=COMPLEX, | ||
): |
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.
👍🏻 👍🏻
library_type: Deprecated parameter, not really used. Set to COMPLEX. | ||
|
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.
Thanks for pointing this out. We should either do something library types or rip them out. Let's talk to product about this before the next phase of work. I added an item to #33640
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.
From what I've heard, product wants to rip them out.
@kdmccormick FYI, when testing this I noticed another issue. When you try to paste a library content block that's out of date (changes were made to the library, then it was copied without first being synced), you get a Studio error and the logs say this:
|
@kdmccormick Unfortunately I found a bigger bug here:
The problem is that here: edx-platform/openedx/core/djangoapps/content_libraries/tasks.py Lines 85 to 94 in 8ec4b2b
only the first 10 digits of the LCB's Since this function is only used for blockstore libraries and I think they're not used in prod yet, is it still possible to change I would suggest we use block_id = hashlib.sha1((dest_parent_key.block_id + '-' + str(source_key)).encode('utf-8')).hexdigest()[:20] From what I can tell, the algorithm used for v1 libraries doesn't have this bug. |
@bradenmacdonald Right, those are both known issues with Libraries V2. They shouldn't block merging this fix for Libraries V1. The first issue is expounded upon in this doc. It's talked about in context of export/import...
...but the underlying issue is the same: we need a way to get the library's default settings after duplicating, pasting, or importing. For V1 libraries, the solution was to sync with the old library version. For V2 libraries, we'd like to avoid doing that, because it overrides local content edits, and it doesn't play well with Learning Core's data model. Instead, we're hoping to add the idea of "default settings" to OLX. That'll take some time to implement correctly, which is why your fix here for V1 library copy/paste is still important. Regarding block key generation: yes, the current |
@kdmccormick Ok, got it. I'll merge this PR first thing Monday then as I think it's getting a bit late to merge now. |
@bradenmacdonald 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
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. |
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. |
@kdmccormick Should we backport this to Quince? |
@bradenmacdonald Yes, definitely. |
@kdmccormick Did this get backported or do I need to do something here to make that happen? |
@bradenmacdonald It did not. Would you mind backporting it? |
@kdmccormick Here is the backport PR: #34274 |
Description
In Studio, if you copy and then paste a Library Content Block, it will appear to work fine. But when you later "sync" the block with the latest changes from the library, all of the child blocks will be deleted and re-created with new IDs. This could result in a loss of learner state (answers) for any child blocks of the pasted Library Content block.
Supporting information
Slack Discussion
Testing instructions
Deadline
None, but we're hoping to include this fix into Quince.
Other information
Includes a test case that fails without this fix in place. Writing the test case was by far the hardest part. It seems most of the Library Content Block tests aren't yet tested with v2 libraries, so there wasn't a lot to go on. I wrote this test using V2 though.