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
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions cms/djangoapps/contentstore/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -336,8 +336,14 @@ def _import_xml_node_to_parent(
new_xblock = store.update_item(temp_xblock, user_id, allow_not_found=True)
parent_xblock.children.append(new_xblock.location)
store.update_item(parent_xblock, user_id)
for child_node in child_nodes:
_import_xml_node_to_parent(child_node, new_xblock, store, user_id=user_id)
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)

else:
for child_node in child_nodes:
_import_xml_node_to_parent(child_node, new_xblock, store, user_id=user_id)
return new_xblock


Expand Down
105 changes: 103 additions & 2 deletions cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,19 @@
APIs.
"""
import ddt
from django.test import LiveServerTestCase
from opaque_keys.edx.keys import UsageKey
from rest_framework.test import APIClient
from xmodule.modulestore.django import contentstore
from organizations.models import Organization
from xmodule.modulestore.django import contentstore, modulestore
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, upload_file_to_course
from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory, ToyCourseFactory

from cms.djangoapps.contentstore.utils import reverse_usage_url
from openedx.core.lib.blockstore_api.tests.base import BlockstoreAppTestMixin
from openedx.core.djangoapps.content_libraries import api as library_api
from blockstore.apps import api as blockstore_api

CLIPBOARD_ENDPOINT = "/api/content-staging/v1/clipboard/"
XBLOCK_ENDPOINT = "/xblock/"

Expand Down Expand Up @@ -110,7 +117,7 @@ def test_copy_and_paste_component(self, block_args):
"""
Test copying a component (XBlock) from one course into another
"""
source_course = CourseFactory.create(display_name='Destination Course')
source_course = CourseFactory.create(display_name='Source Course')
source_block = BlockFactory.create(parent_location=source_course.location, **block_args)

dest_course = CourseFactory.create(display_name='Destination Course')
Expand Down Expand Up @@ -205,3 +212,97 @@ def test_paste_with_assets(self):
source_pic2_hash = contentstore().find(source_course.id.make_asset_key("asset", "picture2.jpg")).content_digest
dest_pic2_hash = contentstore().find(dest_course_key.make_asset_key("asset", "picture2.jpg")).content_digest
assert source_pic2_hash != dest_pic2_hash # Because there was a conflict, this file was unchanged.


class ClipboardLibraryContentPasteTestCase(BlockstoreAppTestMixin, LiveServerTestCase, ModuleStoreTestCase):
"""
Test Clipboard Paste functionality with library content
"""

def setUp(self):
"""
Set up a v2 Content Library and a library content block
"""
super().setUp()
self.client = APIClient()
self.client.login(username=self.user.username, password=self.user_password)
self.store = modulestore()
# Create a content library:
library = library_api.create_library(
collection_uuid=blockstore_api.create_collection("Collection").uuid,
library_type=library_api.COMPLEX,
org=Organization.objects.create(name="Test Org", short_name="CL-TEST"),
slug="lib",
title="Library",
)
# Populate it with a problem:
problem_key = library_api.create_library_block(library.key, "problem", "p1").usage_key
library_api.set_library_block_olx(problem_key, """
<problem display_name="MCQ" max_attempts="1">
<multiplechoiceresponse>
<label>Q</label>
<choicegroup type="MultipleChoice">
<choice correct="false">Wrong</choice>
<choice correct="true">Right</choice>
</choicegroup>
</multiplechoiceresponse>
</problem>
""")
library_api.publish_changes(library.key)

# Create a library content block (lc), point it out our library, and sync it.
self.course = CourseFactory.create(display_name='Course')
self.orig_lc_block = BlockFactory.create(
parent=self.course,
category="library_content",
source_library_id=str(library.key),
display_name="LC Block",
publish_item=False,
)
self.dest_lc_block = None

self._sync_lc_block_from_library('orig_lc_block')
orig_child = self.store.get_item(self.orig_lc_block.children[0])
assert orig_child.display_name == "MCQ"

def test_paste_library_content_block(self):
"""
Test the special handling of copying and pasting library content
"""
# Copy a library content block that has children:
copy_response = self.client.post(CLIPBOARD_ENDPOINT, {
"usage_key": str(self.orig_lc_block.location)
}, format="json")
assert copy_response.status_code == 200

# Paste the Library content block:
paste_response = self.client.post(XBLOCK_ENDPOINT, {
"parent_locator": str(self.course.location),
"staged_content": "clipboard",
}, format="json")
assert paste_response.status_code == 200
dest_lc_block_key = UsageKey.from_string(paste_response.json()["locator"])

# Get the ID of the new child:
self.dest_lc_block = self.store.get_item(dest_lc_block_key)
dest_child = self.store.get_item(self.dest_lc_block.children[0])
assert dest_child.display_name == "MCQ"

# Importantly, the ID of the child must not changed when the library content is synced.
# Otherwise, user state saved against this child will be lost when it syncs.
self._sync_lc_block_from_library('dest_lc_block')
updated_dest_child = self.store.get_item(self.dest_lc_block.children[0])
assert dest_child.location == updated_dest_child.location

def _sync_lc_block_from_library(self, attr_name):
"""
Helper method to "sync" a Library Content Block by [re-]fetching its
children from the library.
"""
usage_key = getattr(self, attr_name).location
# It's easiest to do this via the REST API:
handler_url = reverse_usage_url('preview_handler', usage_key, kwargs={'handler': 'upgrade_and_sync'})
response = self.client.post(handler_url)
assert response.status_code == 200
# Now reload the block and make sure the child is in place
setattr(self, attr_name, self.store.get_item(usage_key)) # we must reload after upgrade_and_sync
23 changes: 20 additions & 3 deletions openedx/core/djangoapps/content_libraries/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,15 @@
from edx_rest_api_client.client import OAuthAPIClient

from openedx.core.djangoapps.content_libraries import permissions
from openedx.core.djangoapps.content_libraries.constants import DRAFT_NAME, COMPLEX
# pylint: disable=unused-import
from openedx.core.djangoapps.content_libraries.constants import (
ALL_RIGHTS_RESERVED,
CC_4_BY,
COMPLEX,
DRAFT_NAME,
PROBLEM,
VIDEO,
)
from openedx.core.djangoapps.content_libraries.library_bundle import LibraryBundle
from openedx.core.djangoapps.content_libraries.models import (
ContentLibrary,
Expand Down Expand Up @@ -387,8 +395,15 @@ def get_library(library_key):


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

Choose a reason for hiding this comment

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

👍🏻 👍🏻

"""
Create a new content library.
Expand All @@ -405,6 +420,8 @@ def create_library(

allow_public_read: Allow anyone to view blocks (including source) in Studio?

library_type: Deprecated parameter, not really used. Set to COMPLEX.

Comment on lines +423 to +424
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.

Returns a ContentLibraryMetadata instance.
"""
assert isinstance(collection_uuid, UUID)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
from openedx.core.djangoapps.content_libraries.api import (
AccessLevel,
create_library,
COMPLEX,
set_library_user_permissions,
)
from openedx.core.djangoapps.content_tagging.models import TaxonomyOrg
Expand Down Expand Up @@ -104,12 +103,8 @@ def _setUp_library(self):
collection_uuid=self.collection.uuid,
org=self.orgA,
slug="lib_a",
library_type=COMPLEX,
title="Library Org A",
description="This is a library from Org A",
allow_public_learning=False,
allow_public_read=False,
library_license="",
)

def _setUp_users(self):
Expand Down
Loading