fix: handle paste of library content blocks correctly

[bradenmacdonald]

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]

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.

[kdmccormick]

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

[kdmccormick]

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)

[kdmccormick]

👍🏻 👍🏻

[kdmccormick]

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

[bradenmacdonald]

From what I've heard, product wants to rip them out.

[bradenmacdonald]

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

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

edx-platform/openedx/core/djangoapps/content_libraries/tasks.py

Lines 85 to 94 in 8ec4b2b

	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]

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

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

[openedx-webhooks]

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

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

[edx-pipeline-bot]

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

[edx-pipeline-bot]

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

[edx-pipeline-bot]

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

[edx-pipeline-bot]

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

[bradenmacdonald]

@kdmccormick Should we backport this to Quince?

[kdmccormick]

@bradenmacdonald Yes, definitely.

[bradenmacdonald]

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

[kdmccormick]

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

[bradenmacdonald]

@kdmccormick Here is the backport PR: #34274