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

fix: handle paste of library content blocks correctly #33926

Merged

Conversation

bradenmacdonald
Copy link
Contributor

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

  1. Set up a content library and import its content into a course using a library content block.
  2. Copy and paste the library content block
  3. Note the IDs of its children
  4. Publish a change to the library then update the pasted library content block from step 2.
  5. Note the IDs of its children - should be the same as step 3 if the fix worked.

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.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Dec 13, 2023
@openedx-webhooks
Copy link

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:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

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.

Copy link
Member

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

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. 🤞🏻

Comment on lines 339 to 343
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)
Copy link
Member

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.

Suggested change
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)

Comment on lines 397 to 407
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,
):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻 👍🏻

Comment on lines +423 to +424
library_type: Deprecated parameter, not really used. Set to COMPLEX.

Copy link
Member

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

Copy link
Contributor Author

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.

@bradenmacdonald
Copy link
Contributor Author

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

NotImplementedError: Tried to load version 3 of blockstore-based library lib:foo:bar. Currently, only the latest version (4) may be loaded. This is a known issue. It will be fixed before the production release of blockstore-based (V2) content libraries.

@bradenmacdonald
Copy link
Contributor Author

@kdmccormick Unfortunately I found a bigger bug here:

  1. Create a library content block, linked to a blockstore/v2 library and sync it so it has children.
  2. Using this PR's code, copy and paste the block. So far so good.
  3. Paste a second copy of the block into the same course. It pastes but has no children, and you see this error in the logs:
  File "/openedx/edx-platform/openedx/core/djangoapps/content_libraries/tasks.py", line 199, in _import_from_blockstore
    new_block_id = _import_block(store, user_id, block, dest_key)
  File "/openedx/edx-platform/openedx/core/djangoapps/content_libraries/tasks.py", line 101, in _import_block
    raise ValueError(
ValueError: Expected existing block
block-v1:BradenX+TS100+23+type@html+block@library_co-522bc4b3c0
to be a child of
block-v1:BradenX+TS100+23+type@library_content+block@library_content3
but instead it's a child of
block-v1:BradenX+TS100+23+type@library_content+block@library_content2

The problem is that here:

def generate_block_key(source_key, dest_parent_key):
"""
Deterministically generate an ID for the new block and return the key
"""
block_id = (
dest_parent_key.block_id[:10] +
'-' +
hashlib.sha1(str(source_key).encode('utf-8')).hexdigest()[:10]
)
return dest_parent_key.context_key.make_usage_key(source_key.block_type, block_id)

only the first 10 digits of the LCB's block_id are used, and in that case both library_content2 and library_content3 result in the same shortened dest_parent_key.block_id[:10] component.

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 generate_block_key to use the full dest_parent_key.block_id ?

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.

@kdmccormick
Copy link
Member

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

How do we want to work around this for V2 libs?
..
Well, the current V2 content_libraries API does not support loading blocks for old library versions. We could add support for that, but it wouldn’t really fit efficiently or elegantly with Learning Core’s data model, which optimizes for loading versions of individual components rather than content libraries.
...
Instead, we’d rather add defaults to the OLX export, so that the export comes back with defaults intact, and removes the need to re-load library blocks, thus preserving library edits

...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 generate_block_key function is broken, both because it assumes the length of the block key, but also because it would fail to preserve child keys across a V1->V2 library migration. The fix we're about to merge will turn generate_block_key into a wrapper around modulestore's derive_key utility, which is what V1 libraries use.

@bradenmacdonald
Copy link
Contributor Author

@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 bradenmacdonald merged commit 5ab6238 into openedx:master Dec 18, 2023
64 checks passed
@bradenmacdonald bradenmacdonald deleted the braden/fix-library-content-paste branch December 18, 2023 17:48
@openedx-webhooks
Copy link

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

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

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

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

@edx-pipeline-bot
Copy link
Contributor

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

@bradenmacdonald
Copy link
Contributor Author

@kdmccormick Should we backport this to Quince?

@kdmccormick
Copy link
Member

@bradenmacdonald Yes, definitely.

@bradenmacdonald
Copy link
Contributor Author

@kdmccormick Did this get backported or do I need to do something here to make that happen?

@kdmccormick
Copy link
Member

@bradenmacdonald It did not. Would you mind backporting it?

@bradenmacdonald
Copy link
Contributor Author

@kdmccormick Here is the backport PR: #34274

andrey-canon added a commit to nelc/edx-platform that referenced this pull request May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants