-
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: set upstream link for re-copied block from course originally from library #35784
Merged
ChrisChV
merged 3 commits into
openedx:master
from
open-craft:navin/fix-copy-copied-upstream
Nov 7, 2024
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,7 +24,7 @@ | |
from xmodule.xml_block import XmlMixin | ||
|
||
from cms.djangoapps.models.settings.course_grading import CourseGradingModel | ||
from cms.lib.xblock.upstream_sync import UpstreamLink, BadUpstream, BadDownstream, fetch_customizable_fields | ||
from cms.lib.xblock.upstream_sync import UpstreamLink, UpstreamLinkException, fetch_customizable_fields | ||
from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers | ||
import openedx.core.djangoapps.content_staging.api as content_staging_api | ||
import openedx.core.djangoapps.content_tagging.api as content_tagging_api | ||
|
@@ -323,6 +323,56 @@ def import_staged_content_from_user_clipboard(parent_key: UsageKey, request) -> | |
return new_xblock, notices | ||
|
||
|
||
def _fetch_and_set_upstream_link( | ||
copied_from_block: str, | ||
copied_from_version_num: int, | ||
temp_xblock: XBlock, | ||
user: User | ||
): | ||
""" | ||
Fetch and set upstream link for the given xblock. This function handles following cases: | ||
* the xblock is copied from a v2 library; the library block is set as upstream. | ||
* the xblock is copied from a course; no upstream is set, only copied_from_block is set. | ||
* the xblock is copied from a course where the source block was imported from a library; the original libary block | ||
is set as upstream. | ||
""" | ||
# Try to link the pasted block (downstream) to the copied block (upstream). | ||
temp_xblock.upstream = copied_from_block | ||
try: | ||
UpstreamLink.get_for_block(temp_xblock) | ||
except UpstreamLinkException: | ||
# Usually this will fail. For example, if the copied block is a modulestore course block, it can't be an | ||
# upstream. That's fine! Instead, we store a reference to where this block was copied from, in the | ||
# 'copied_from_block' field (from AuthoringMixin). | ||
|
||
# In case if the source block was imported from a library, we need to check its upstream | ||
# and set the same upstream link for the new block. | ||
source_descriptor = modulestore().get_item(UsageKey.from_string(copied_from_block)) | ||
if source_descriptor.upstream: | ||
_fetch_and_set_upstream_link( | ||
source_descriptor.upstream, | ||
source_descriptor.upstream_version, | ||
temp_xblock, | ||
user, | ||
) | ||
else: | ||
# else we store a reference to where this block was copied from, in the 'copied_from_block' | ||
# field (from AuthoringMixin). | ||
temp_xblock.upstream = None | ||
temp_xblock.copied_from_block = copied_from_block | ||
else: | ||
# But if it doesn't fail, then populate the `upstream_version` field based on what was copied. Note that | ||
# this could be the latest published version, or it could be an an even newer draft version. | ||
temp_xblock.upstream_version = copied_from_version_num | ||
# Also, fetch upstream values (`upstream_display_name`, etc.). | ||
# Recall that the copied block could be a draft. So, rather than fetching from the published upstream (which | ||
# could be older), fetch from the copied block itself. That way, if an author customizes a field, but then | ||
# later wants to restore it, it will restore to the value that the field had when the block was pasted. Of | ||
# course, if the author later syncs updates from a *future* published upstream version, then that will fetch | ||
# new values from the published upstream content. | ||
fetch_customizable_fields(upstream=temp_xblock, downstream=temp_xblock, user=user) | ||
|
||
|
||
def _import_xml_node_to_parent( | ||
node, | ||
parent_xblock: XBlock, | ||
|
@@ -404,28 +454,7 @@ def _import_xml_node_to_parent( | |
raise NotImplementedError("We don't yet support pasting XBlocks with children") | ||
temp_xblock.parent = parent_key | ||
if copied_from_block: | ||
# Try to link the pasted block (downstream) to the copied block (upstream). | ||
temp_xblock.upstream = copied_from_block | ||
try: | ||
UpstreamLink.get_for_block(temp_xblock) | ||
except (BadDownstream, BadUpstream): | ||
# Usually this will fail. For example, if the copied block is a modulestore course block, it can't be an | ||
# upstream. That's fine! Instead, we store a reference to where this block was copied from, in the | ||
# 'copied_from_block' field (from AuthoringMixin). | ||
temp_xblock.upstream = None | ||
temp_xblock.copied_from_block = copied_from_block | ||
else: | ||
# But if it doesn't fail, then populate the `upstream_version` field based on what was copied. Note that | ||
# this could be the latest published version, or it could be an an even newer draft version. | ||
temp_xblock.upstream_version = copied_from_version_num | ||
# Also, fetch upstream values (`upstream_display_name`, etc.). | ||
# Recall that the copied block could be a draft. So, rather than fetching from the published upstream (which | ||
# could be older), fetch from the copied block itself. That way, if an author customizes a field, but then | ||
# later wants to restore it, it will restore to the value that the field had when the block was pasted. Of | ||
# course, if the author later syncs updates from a *future* published upstream version, then that will fetch | ||
# new values from the published upstream content. | ||
fetch_customizable_fields(upstream=temp_xblock, downstream=temp_xblock, user=user) | ||
|
||
_fetch_and_set_upstream_link(copied_from_block, copied_from_version_num, temp_xblock, user) | ||
# Save the XBlock into modulestore. We need to save the block and its parent for this to work: | ||
new_xblock = store.update_item(temp_xblock, user.id, allow_not_found=True) | ||
parent_xblock.children.append(new_xblock.location) | ||
|
@@ -436,7 +465,7 @@ def _import_xml_node_to_parent( | |
# Allow an XBlock to do anything fancy it may need to when pasted from the clipboard. | ||
# These blocks may handle their own children or parenting if needed. Let them return booleans to | ||
# let us know if we need to handle these or not. | ||
children_handed = new_xblock.studio_post_paste(store, node) | ||
children_handled = new_xblock.studio_post_paste(store, node) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo in variable causing |
||
|
||
if not children_handled: | ||
for child_node in child_nodes: | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -455,26 +455,6 @@ def setUp(self): | |
self.lib_block_tags = ['tag_1', 'tag_5'] | ||
tagging_api.tag_object(str(self.lib_block_key), taxonomy_all_org, self.lib_block_tags) | ||
|
||
def test_paste_from_library_creates_link(self): | ||
""" | ||
When we copy a v2 lib block into a course, the dest block should be linked up to the lib block. | ||
""" | ||
copy_response = self.client.post(CLIPBOARD_ENDPOINT, {"usage_key": str(self.lib_block_key)}, format="json") | ||
assert copy_response.status_code == 200 | ||
|
||
paste_response = self.client.post(XBLOCK_ENDPOINT, { | ||
"parent_locator": str(self.course.usage_key), | ||
"staged_content": "clipboard", | ||
}, format="json") | ||
assert paste_response.status_code == 200 | ||
|
||
new_block_key = UsageKey.from_string(paste_response.json()["locator"]) | ||
new_block = modulestore().get_item(new_block_key) | ||
assert new_block.upstream == str(self.lib_block_key) | ||
assert new_block.upstream_version == 3 | ||
assert new_block.upstream_display_name == "MCQ-draft" | ||
assert new_block.upstream_max_attempts == 5 | ||
|
||
def test_paste_from_library_read_only_tags(self): | ||
""" | ||
When we copy a v2 lib block into a course, the dest block should have read-only copied tags. | ||
|
@@ -555,6 +535,34 @@ def test_paste_from_library_copies_asset(self): | |
assert image_asset.name == "1px.webp" | ||
assert image_asset.length == len(webp_raw_data) | ||
|
||
def test_paste_from_course_block_imported_from_library_creates_link(self): | ||
""" | ||
When we copy a course xblock which was imported or copied from v2 lib block into a course, | ||
the dest block should be linked up to the original lib block. | ||
""" | ||
def _copy_paste_and_assert_link(key_to_copy): | ||
copy_response = self.client.post(CLIPBOARD_ENDPOINT, {"usage_key": str(key_to_copy)}, format="json") | ||
assert copy_response.status_code == 200 | ||
|
||
paste_response = self.client.post(XBLOCK_ENDPOINT, { | ||
"parent_locator": str(self.course.usage_key), | ||
"staged_content": "clipboard", | ||
}, format="json") | ||
assert paste_response.status_code == 200 | ||
|
||
new_block_key = UsageKey.from_string(paste_response.json()["locator"]) | ||
new_block = modulestore().get_item(new_block_key) | ||
assert new_block.upstream == str(self.lib_block_key) | ||
assert new_block.upstream_version == 3 | ||
assert new_block.upstream_display_name == "MCQ-draft" | ||
assert new_block.upstream_max_attempts == 5 | ||
return new_block_key | ||
|
||
# first verify link for copied block from library | ||
new_block_key = _copy_paste_and_assert_link(self.lib_block_key) | ||
# next verify link for copied block from the pasted block | ||
_copy_paste_and_assert_link(new_block_key) | ||
Comment on lines
+561
to
+564
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oo that's nice! |
||
|
||
|
||
class ClipboardPasteFromV1LibraryTestCase(ModuleStoreTestCase): | ||
""" | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Your documentation here is so good -- it's really helpful to see all the many ways we manage links between blocks. Thank you for taking the time to explain!