Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
805a2a8
feat: Implement paste button
bradenmacdonald Mar 17, 2023
6e11e05
chore: improve docs and add tests for python API
bradenmacdonald Apr 20, 2023
79dd95d
fix: drive-by fix to use a better API for comparing XML
bradenmacdonald Apr 20, 2023
0bccadf
feat: track which XBlock something was copied from
bradenmacdonald Apr 20, 2023
c85e189
feat: add tests
bradenmacdonald Apr 20, 2023
fe539fe
feat: enable import linter so content_staging's public API is respected
bradenmacdonald Apr 20, 2023
8ce5ea8
fix: error seen when trying to paste drag-and-drop-v2 blocks
bradenmacdonald Apr 24, 2023
2a9350c
fix: use strip_text=True consistently for XML comparisons
bradenmacdonald Apr 24, 2023
a72fdd0
refactor: rename get_user_clipboard_status to get_user_clipboard
bradenmacdonald Apr 25, 2023
db4eb97
feat: Better error reporting when pasting in Studio
bradenmacdonald Apr 25, 2023
9f0dded
chore: convert new test suite to pytest assertions
bradenmacdonald Apr 25, 2023
1bf9f06
refactor: push READY status check into the API per review suggestion
bradenmacdonald Apr 25, 2023
4dd11cd
fix: use strip_text=True consistently for XML comparisons
bradenmacdonald Apr 25, 2023
2bba746
fix: store "copied_from_block" as a string to avoid Reference field i…
bradenmacdonald Apr 25, 2023
d5429b1
fix: minor lint error
bradenmacdonald Apr 25, 2023
5e06a2c
refactor: move data types to data.py per OEP-49
bradenmacdonald Apr 26, 2023
c4129ce
chore: Update with latest master
bradenmacdonald Apr 26, 2023
aaee378
chore: Update with latest master
bradenmacdonald Apr 27, 2023
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
16 changes: 16 additions & 0 deletions cms/djangoapps/contentstore/views/block.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@
from .helpers import (
create_xblock,
get_parent_xblock,
import_staged_content_from_user_clipboard,
is_unit,
usage_key_with_run,
xblock_primary_child_category,
Expand Down Expand Up @@ -169,6 +170,8 @@ def xblock_handler(request, usage_key_string=None):
:display_name: name for new xblock, optional
:boilerplate: template name for populating fields, optional and only used
if duplicate_source_locator is not present
:staged_content: use "clipboard" to paste from the OLX user's clipboard. (Incompatible with all other
fields except parent_locator)
The locator (unicode representation of a UsageKey) for the created xblock (minus children) is returned.
"""
if usage_key_string:
Expand Down Expand Up @@ -701,6 +704,19 @@ def _create_block(request):
if not has_studio_write_access(request.user, usage_key.course_key):
raise PermissionDenied()

if request.json.get('staged_content') == "clipboard":
# Paste from the user's clipboard (content_staging app clipboard, not browser clipboard) into 'usage_key':
try:
created_xblock = import_staged_content_from_user_clipboard(parent_key=usage_key, request=request)
except Exception: # pylint: disable=broad-except
log.exception("Could not paste component into location {}".format(usage_key))
return JsonResponse({"error": _('There was a problem pasting your component.')}, status=400)
if created_xblock is None:
return JsonResponse({"error": _('Your clipboard is empty or invalid.')}, status=400)
return JsonResponse(
{'locator': str(created_xblock.location), 'courseKey': str(created_xblock.location.course_key)}
)

category = request.json['category']
if isinstance(usage_key, LibraryUsageLocator):
# Only these categories are supported at this time.
Expand Down
15 changes: 14 additions & 1 deletion cms/djangoapps/contentstore/views/component.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,11 @@
from cms.djangoapps.contentstore.toggles import use_new_problem_editor
from openedx.core.lib.xblock_utils import get_aside_from_xblock, is_xblock_aside
from openedx.core.djangoapps.discussions.models import DiscussionsConfiguration
try:
# Technically this is a django app plugin, so we should not error if it's not installed:
import openedx.core.djangoapps.content_staging.api as content_staging_api
except ImportError:
content_staging_api = None
Comment thread
Agrendalath marked this conversation as resolved.
Outdated
from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order
from xmodule.modulestore.exceptions import ItemNotFoundError # lint-amnesty, pylint: disable=wrong-import-order

Expand Down Expand Up @@ -185,6 +190,12 @@ def container_handler(request, usage_key_string):
break
index += 1

# Get the status of the user's clipboard so they can paste components if they have something to paste
if content_staging_api:
user_clipboard = content_staging_api.get_user_clipboard_json(request.user.id, request)
else:
user_clipboard = {"content": None}

return render_to_response('container.html', {
'language_code': request.LANGUAGE_CODE,
'context_course': course, # Needed only for display of menus at top of page.
Expand All @@ -205,7 +216,9 @@ def container_handler(request, usage_key_string):
'xblock_info': xblock_info,
'draft_preview_link': preview_lms_link,
'published_preview_link': lms_link,
'templates': CONTAINER_TEMPLATES
'templates': CONTAINER_TEMPLATES,
# Status of the user's clipboard, exactly as would be returned from the "GET clipboard" REST API.
'user_clipboard': user_clipboard,
})
else:
return HttpResponseBadRequest("Only supports HTML requests")
Expand Down
81 changes: 81 additions & 0 deletions cms/djangoapps/contentstore/views/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,31 @@
"""

import urllib
from lxml import etree
from uuid import uuid4

from django.http import HttpResponse
from django.utils.translation import gettext as _
from opaque_keys.edx.keys import UsageKey
from opaque_keys.edx.locator import DefinitionLocator, LocalId
from xblock.core import XBlock
from xblock.fields import ScopeIds
from xblock.runtime import IdGenerator
from xmodule.modulestore.django import modulestore
from xmodule.tabs import StaticTab

from cms.djangoapps.contentstore.views.preview import _load_preview_block
from cms.djangoapps.models.settings.course_grading import CourseGradingModel
from common.djangoapps.student import auth
from common.djangoapps.student.roles import CourseCreatorRole, OrgContentCreatorRole
from openedx.core.toggles import ENTRANCE_EXAMS

try:
# Technically this is a django app plugin, so we should not error if it's not installed:
import openedx.core.djangoapps.content_staging.api as content_staging_api
except ImportError:
content_staging_api = None

from ..utils import reverse_course_url, reverse_library_url, reverse_usage_url

__all__ = ['event']
Expand Down Expand Up @@ -271,6 +282,76 @@ def create_xblock(parent_locator, user, category, display_name, boilerplate=None
return created_block


class ImportIdGenerator(IdGenerator):
"""
Modulestore's IdGenerator doesn't work for importing single blocks as OLX,
so we implement our own
"""
def __init__(self, context_key):
super().__init__()
self.context_key = context_key

def create_aside(self, definition_id, usage_id, aside_type):
""" Generate a new aside key """
raise NotImplementedError()

def create_usage(self, def_id) -> UsageKey:
""" Generate a new UsageKey for an XBlock """
# Note: Split modulestore will detect this temporary ID and create a new block ID when the XBlock is saved.
return self.context_key.make_usage_key(def_id.block_type, LocalId())

def create_definition(self, block_type, slug=None) -> DefinitionLocator:
""" Generate a new definition_id for an XBlock """
# Note: Split modulestore will detect this temporary ID and create a new definition ID when the XBlock is saved.
return DefinitionLocator(block_type, LocalId(block_type))


def import_staged_content_from_user_clipboard(parent_key: UsageKey, request):
"""
Import a block (and any children it has) from "staged" OLX.
Does not deal with permissions or REST stuff - do that before calling this.

Returns the newly created block on success or None if the clipboard is
empty.
"""
if not content_staging_api:
raise RuntimeError("The required content_staging app is not installed")
user_clipboard = content_staging_api.get_user_clipboard(request.user.id)
if not user_clipboard:
# Clipboard is empty or expired/error/loading
return None
block_type = user_clipboard.content.block_type
olx_str = content_staging_api.get_staged_content_olx(user_clipboard.content.id)
node = etree.fromstring(olx_str)
store = modulestore()
with store.bulk_operations(parent_key.course_key):
parent_descriptor = store.get_item(parent_key)

@ormsbee ormsbee Apr 26, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A very big descriptor -> block renaming refactor is dropping shortly. Please make check that your naming aligns:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm, not sure. What I found was that this thing is not a proper XBlock, and it doesn't have a full XBlock runtime, just a CachingDescriptorSystem. So that's why I've called it a descriptor here, and then the line below uses _load_preview_block to convert it from a descriptor to a proper XBlock. I had thought we didn't have to worry about those things anymore but I guess we still do.

So in this case I think the language is clear and necessary but I am not sure it aligns with the changes in that PR. @Agrendalath can you advise?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ormsbee, @bradenmacdonald, we still use the "descriptor" term in some places. In this case, we are retrieving an "unbound" XBlock from the Modulestore. _load_preview_block handles adding the services and binding student data. Therefore, it is fair to call it a "descriptor", as the only goal of this variable is to pass it to a function that initializes the full runtime.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Great, thanks for the quick reply. That aligns with my understanding here :)

# Some blocks like drag-and-drop only work here with the full XBlock runtime loaded:
parent_xblock = _load_preview_block(request, parent_descriptor)
runtime = parent_xblock.runtime
# Generate the new ID:
id_generator = ImportIdGenerator(parent_key.context_key)
def_id = id_generator.create_definition(block_type, user_clipboard.source_usage_key.block_id)
usage_id = id_generator.create_usage(def_id)
keys = ScopeIds(None, block_type, def_id, usage_id)
# parse_xml is a really messy API. We pass both 'keys' and 'id_generator' and, depending on the XBlock, either
# one may be used to determine the new XBlock's usage key, and the other will be ignored. e.g. video ignores
# 'keys' and uses 'id_generator', but the default XBlock parse_xml ignores 'id_generator' and uses 'keys'.
# For children of this block, obviously only id_generator is used.
xblock_class = runtime.load_block_type(block_type)
temp_xblock = xblock_class.parse_xml(node, runtime, keys, id_generator)
Comment thread
Agrendalath marked this conversation as resolved.
Outdated
if xblock_class.has_children and temp_xblock.children:
Comment thread
Agrendalath marked this conversation as resolved.
Outdated
raise NotImplementedError("We don't yet support pasting XBlocks with children")
temp_xblock.parent = parent_key
# Store a reference to where this block was copied from, in the 'copied_from_block' field (AuthoringMixin)
temp_xblock.copied_from_block = str(user_clipboard.source_usage_key)
# 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, request.user.id, allow_not_found=True)
parent_xblock.children.append(new_xblock.location)
store.update_item(parent_xblock, request.user.id)
return new_xblock


def is_item_in_course_tree(item):
"""
Check that the item is in the course tree.
Expand Down
62 changes: 62 additions & 0 deletions cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
"""
Test the import_staged_content_from_user_clipboard() method, which is used to
allow users to paste XBlocks that were copied using the staged_content/clipboard
APIs.
"""
from opaque_keys.edx.keys import UsageKey
from rest_framework.test import APIClient
from xmodule.modulestore.django import modulestore
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
from xmodule.modulestore.tests.factories import ToyCourseFactory

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


class ClipboardPasteTestCase(ModuleStoreTestCase):
"""
Test Clipboard Paste functionality
"""

def _setup_course(self):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: did you intentionally decide not to use def setUp(self): (to set self.course_key + self.client there)? I see you're using self.assertEqual instead of a standard assert, so it's a bit odd mix of unittest.TestCase and pytest styles.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I copied this from the other TestCase, where only some of the tests need this setup, which is why it's not in a setUp method. Here I could put it in a setUp method, if you think that's cleaner. Though if we add more tests here, some may not need the course.

As for mixing assert styles, is there a preferred style for the codebase now? I honestly don't know which I "should" be using.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I copied this from the other TestCase, where only some of the tests need this setup, which is why it's not in a setUp method. Here I could put it in a setUp method, if you think that's cleaner. Though if we add more tests here, some may not need the course.

No strong preference here. If you think we can have tests that won't need the course setup, then we can keep it as is.

As for mixing assert styles, is there a preferred style for the codebase now? I honestly don't know which I "should" be using.

I don't see any coding guideline for this, but there were 36 PRs like #26576 that replaced these asserts, so I believe that the pytest style is preferred.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK, I converted this test suite to pytest style.

""" Set up the "Toy Course" and an APIClient for testing clipboard functionality. """
# Setup:
course_key = ToyCourseFactory.create().id # See xmodule/modulestore/tests/sample_courses.py
client = APIClient()
client.login(username=self.user.username, password=self.user_password)
return (course_key, client)

def test_copy_and_paste_video(self):
"""
Test copying a video from the course, and pasting it into the same unit
"""
course_key, client = self._setup_course()

# Check how many blocks are in the vertical currently
parent_key = course_key.make_usage_key("vertical", "vertical_test") # This is the vertical that holds the video
orig_vertical = modulestore().get_item(parent_key)
assert len(orig_vertical.children) == 4

# Copy the video
video_key = course_key.make_usage_key("video", "sample_video")
copy_response = client.post(CLIPBOARD_ENDPOINT, {"usage_key": str(video_key)}, format="json")
assert copy_response.status_code == 200

# Paste the video
paste_response = client.post(XBLOCK_ENDPOINT, {
"parent_locator": str(parent_key),
"staged_content": "clipboard",
}, format="json")
assert paste_response.status_code == 200
new_block_key = UsageKey.from_string(paste_response.json()["locator"])

# Now there should be an extra block in the vertical:
updated_vertical = modulestore().get_item(parent_key)
assert len(updated_vertical.children) == 5
assert updated_vertical.children[-1] == new_block_key
# And it should match the original:
orig_video = modulestore().get_item(video_key)
new_video = modulestore().get_item(new_block_key)
assert new_video.youtube_id_1_0 == orig_video.youtube_id_1_0
# The new block should store a reference to where it was copied from
assert new_video.copied_from_block == str(video_key)
8 changes: 8 additions & 0 deletions cms/lib/xblock/authoring_mixin.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from django.conf import settings
from web_fragments.fragment import Fragment
from xblock.core import XBlock, XBlockMixin
from xblock.fields import String, Scope

log = logging.getLogger(__name__)

Expand Down Expand Up @@ -43,3 +44,10 @@ def visibility_view(self, _context=None):
fragment.add_javascript_url(self._get_studio_resource_url('/js/xblock/authoring.js'))
fragment.initialize_js('VisibilityEditorInit')
return fragment

copied_from_block = String(
# Note: used by the content_staging app. This field is not needed in the LMS.
Comment thread
bradenmacdonald marked this conversation as resolved.
help="ID of the block that this one was copied from, if any. Used when copying and pasting blocks in Studio.",
scope=Scope.settings,
enforce_type=True,
)
69 changes: 67 additions & 2 deletions cms/static/js/views/pages/container.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,14 @@ function($, _, Backbone, gettext, BasePage, ViewUtils, ContainerView, XBlockView
'click .move-button': 'showMoveXBlockModal',
'click .delete-button': 'deleteXBlock',
'click .show-actions-menu-button': 'showXBlockActionsMenu',
'click .new-component-button': 'scrollToNewComponentButtons'
'click .new-component-button': 'scrollToNewComponentButtons',
'click .paste-component-button': 'pasteComponent',
},

options: {
collapsedClass: 'is-collapsed',
canEdit: true // If not specified, assume user has permission to make changes
canEdit: true, // If not specified, assume user has permission to make changes
clipboardData: { content: null },
},

view: 'container_preview',
Expand Down Expand Up @@ -100,6 +102,7 @@ function($, _, Backbone, gettext, BasePage, ViewUtils, ContainerView, XBlockView
}

this.listenTo(Backbone, 'move:onXBlockMoved', this.onXBlockMoved);
this.clipboardBroadcastChannel = new BroadcastChannel("studio_clipboard_channel");
},

getViewParameters: function() {
Expand Down Expand Up @@ -147,6 +150,11 @@ function($, _, Backbone, gettext, BasePage, ViewUtils, ContainerView, XBlockView

// Re-enable Backbone events for any updated DOM elements
self.delegateEvents();

// Show/hide the paste button
if (!self.isLibraryPage) {
self.initializePasteButton();
}
},
block_added: options && options.block_added
});
Expand Down Expand Up @@ -182,6 +190,59 @@ function($, _, Backbone, gettext, BasePage, ViewUtils, ContainerView, XBlockView
}
},

initializePasteButton() {
if (this.options.canEdit) {
// We should have the user's clipboard status.
const data = this.options.clipboardData;
this.refreshPasteButton(data);
// Refresh the status when something is copied on another tab:
this.clipboardBroadcastChannel.onmessage = (event) => { this.refreshPasteButton(event.data); };
} else {
this.$(".paste-component").hide();
}
},

/**
* Given the latest information about the user's clipboard, hide or show the Paste button as appropriate.
*/
refreshPasteButton(data) {
// 'data' is the same data returned by the "get clipboard status" API endpoint
// i.e. /api/content-staging/v1/clipboard/
if (this.options.canEdit && data.content) {
// TODO: check if this is suitable for pasting into a unit
this.$(".paste-component").show();
} else {
this.$(".paste-component").hide();
}
},

/** The user has clicked on the "Paste Component button" */
pasteComponent(event) {
event.preventDefault();
// Get the ID of the container (usually a unit/vertical) that we're pasting into:
const parentElement = this.findXBlockElement(event.target);
const parentLocator = parentElement.data('locator');
// Create a placeholder XBlock while we're pasting:
const $placeholderEl = $(this.createPlaceholderElement());
const addComponentsPanel = $(event.target).closest('.paste-component').prev();
const listPanel = addComponentsPanel.prev();
const scrollOffset = ViewUtils.getScrollOffset(addComponentsPanel);
const placeholderElement = $placeholderEl.appendTo(listPanel);

// Start showing a "Pasting" notification:
ViewUtils.runOperationShowingMessage(gettext('Pasting'), () => {
return $.postJSON(this.getURLRoot() + '/', {
parent_locator: parentLocator,
staged_content: "clipboard",
}).then((data) => {
this.onNewXBlock(placeholderElement, scrollOffset, false, data);
}).fail(() => {
// Remove the placeholder if the paste failed
placeholderElement.remove();
});
});
},

editXBlock: function(event, options) {
event.preventDefault();

Expand Down Expand Up @@ -307,6 +368,8 @@ function($, _, Backbone, gettext, BasePage, ViewUtils, ContainerView, XBlockView
const status = data.content?.status;
if (status === "ready") {
// The XBlock has been copied and is ready to use.
this.refreshPasteButton(data); // Update our UI
this.clipboardBroadcastChannel.postMessage(data); // And notify any other open tabs
return data;
} else if (status === "loading") {
// The clipboard is being loaded asynchonously.
Expand All @@ -316,6 +379,8 @@ function($, _, Backbone, gettext, BasePage, ViewUtils, ContainerView, XBlockView
$.getJSON(clipboardEndpoint, (pollData) => {
const newStatus = pollData.content?.status;
if (newStatus === "ready") {
this.refreshPasteButton(data);
this.clipboardBroadcastChannel.postMessage(pollData);
deferred.resolve(pollData);
} else if (newStatus === "loading") {
setTimeout(checkStatus, 1_000);
Expand Down
Loading