From d81d774d4d1ff3315c7d380f9bce6697d0829489 Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Thu, 14 Sep 2023 13:08:40 -0400 Subject: [PATCH 01/87] feat!: remove LibrarySourcedBlock Rather than implementing V2-library and static-library-reference support in a new block, we will be enhancing the existing `LibraryContentBlock` in-place. Relevant ADR PR: https://github.com/openedx/edx-platform/pull/33231 --- .../contentstore/views/tests/test_block.py | 5 +- .../0013-library-reference-content-block.rst | 8 +- openedx/core/lib/xblock_utils/__init__.py | 2 +- setup.py | 1 - webpack.common.config.js | 1 - xmodule/assets/README.rst | 9 +- .../LibrarySourcedBlockPicker.jsx | 236 ------------------ .../public/js/library_source_block.js | 58 ----- xmodule/assets/library_source_block/style.css | 58 ----- xmodule/library_sourced_block.py | 159 ------------ xmodule/library_tools.py | 4 +- .../library-sourced-block-studio-view.html | 19 -- xmodule/tests/test_library_sourced_block.py | 71 ------ 13 files changed, 13 insertions(+), 618 deletions(-) delete mode 100644 xmodule/assets/library_source_block/LibrarySourcedBlockPicker.jsx delete mode 100644 xmodule/assets/library_source_block/public/js/library_source_block.js delete mode 100644 xmodule/assets/library_source_block/style.css delete mode 100644 xmodule/library_sourced_block.py delete mode 100644 xmodule/templates/library-sourced-block-studio-view.html delete mode 100644 xmodule/tests/test_library_sourced_block.py diff --git a/cms/djangoapps/contentstore/views/tests/test_block.py b/cms/djangoapps/contentstore/views/tests/test_block.py index 49e08b6a9ca1..26b3f91a0bd7 100644 --- a/cms/djangoapps/contentstore/views/tests/test_block.py +++ b/cms/djangoapps/contentstore/views/tests/test_block.py @@ -2675,10 +2675,7 @@ def setUp(self): XBlockStudioConfiguration.objects.create( name="openassessment", enabled=True, support_level="us" ) - # Library Sourced Block and Library Content block has it's own category. - XBlockStudioConfiguration.objects.create( - name="library_sourced", enabled=True, support_level="fs" - ) + # Library Content block has its own category. XBlockStudioConfiguration.objects.create( name="library_content", enabled=True, support_level="fs" ) diff --git a/docs/decisions/0013-library-reference-content-block.rst b/docs/decisions/0013-library-reference-content-block.rst index cd842d8b0f79..3d8dcb2153a2 100644 --- a/docs/decisions/0013-library-reference-content-block.rst +++ b/docs/decisions/0013-library-reference-content-block.rst @@ -3,7 +3,13 @@ Referencing Content Blocks in Library V2 Status ======= -Pending + +**Deferred** as of September 2023. + +The goals described in the ADR are still relevant to future development, +but for the intial launch of Blockstore-backed content libraries, +the existing ``library_content`` block will be used instead, +and it will continue to copy blocks from Blockstore into Modulestore as necessary. Context ======= diff --git a/openedx/core/lib/xblock_utils/__init__.py b/openedx/core/lib/xblock_utils/__init__.py index 0910018f6046..26127dbfb3b8 100644 --- a/openedx/core/lib/xblock_utils/__init__.py +++ b/openedx/core/lib/xblock_utils/__init__.py @@ -452,7 +452,7 @@ def xblock_resource_pkg(block): ProblemBlock, and most other built-in blocks currently. Handling for these assets does not interact with this function. 2. The (preferred) standard XBlock runtime resource loading system, used by - LibrarySourcedBlock. Handling for these assets *does* interact with this + LibraryContentBlock. Handling for these assets *does* interact with this function. We hope to migrate to (2) eventually, tracked by: diff --git a/setup.py b/setup.py index 17d0f07b01e1..f405f92a95b4 100644 --- a/setup.py +++ b/setup.py @@ -22,7 +22,6 @@ "image = xmodule.template_block:TranslateCustomTagBlock", "library = xmodule.library_root_xblock:LibraryRoot", "library_content = xmodule.library_content_block:LibraryContentBlock", - "library_sourced = xmodule.library_sourced_block:LibrarySourcedBlock", "lti = xmodule.lti_block:LTIBlock", "poll_question = xmodule.poll_block:PollBlock", "problem = xmodule.capa_block:ProblemBlock", diff --git a/webpack.common.config.js b/webpack.common.config.js index 4a7bb7366bba..13d368c559bb 100644 --- a/webpack.common.config.js +++ b/webpack.common.config.js @@ -77,7 +77,6 @@ module.exports = Merge.smart({ // Studio Import: './cms/static/js/features/import/factories/import.js', CourseOrLibraryListing: './cms/static/js/features_jsx/studio/CourseOrLibraryListing.jsx', - LibrarySourcedBlockPicker: './xmodule/assets/library_source_block/LibrarySourcedBlockPicker.jsx', // eslint-disable-line max-len 'js/factories/textbooks': './cms/static/js/factories/textbooks.js', 'js/factories/container': './cms/static/js/factories/container.js', 'js/factories/context_course': './cms/static/js/factories/context_course.js', diff --git a/xmodule/assets/README.rst b/xmodule/assets/README.rst index a39b389a357a..a697915db835 100644 --- a/xmodule/assets/README.rst +++ b/xmodule/assets/README.rst @@ -46,11 +46,6 @@ It is collected into the static root, and then linked to from XBlock fragments b .. _annotatable/_display.scss: https://github.com/openedx/edx-platform/tree/master/xmodule/assets/annotatable/_display.scss .. _simplify things: https://github.com/openedx/edx-platform/issues/32621 -Static CSS (.css) -***************** - -Non-themable, ready-to-seve CSS may also be added to the any block type's -subdirectory. For example, see `library_source_block/style.css`_. JavaScript (.js) **************** @@ -72,7 +67,7 @@ Currently, edx-platform XBlock JS is defined both here in `xmodule/assets`_ and * For other "purer" blocks, the JS is used as a standard XBlock fragment. Example blocks: * `VerticalBlock`_ - * `LibrarySourcedBlock`_ + * `LibraryContentBlock`_ As part of an `active build refactoring`_, we will soon consolidate all edx-platform XBlock JS here in `xmodule/assets`_. @@ -82,7 +77,7 @@ As part of an `active build refactoring`_, we will soon consolidate all edx-plat .. _HtmlBlock: https://github.com/openedx/edx-platform/blob/master/xmodule/html_block.py .. _AnnotatableBlock: https://github.com/openedx/edx-platform/blob/master/xmodule/annotatable_block.py .. _VerticalBlock: https://github.com/openedx/edx-platform/blob/master/xmodule/vertical_block.py -.. _LibrarySourcedBlock: https://github.com/openedx/edx-platform/blob/master/xmodule/library_sourced_block.py +.. _LibraryContentBlock: https://github.com/openedx/edx-platform/blob/master/xmodule/library_content_block.py .. _active build refactoring: https://github.com/openedx/edx-platform/issues/31624 .. _builtin_assets.py: https://github.com/openedx/edx-platform/tree/master/xmodule/util/builtin_assets.py .. _static_content.py: https://github.com/openedx/edx-platform/blob/master/xmodule/static_content.py diff --git a/xmodule/assets/library_source_block/LibrarySourcedBlockPicker.jsx b/xmodule/assets/library_source_block/LibrarySourcedBlockPicker.jsx deleted file mode 100644 index 0b6363893a76..000000000000 --- a/xmodule/assets/library_source_block/LibrarySourcedBlockPicker.jsx +++ /dev/null @@ -1,236 +0,0 @@ -/* globals gettext */ - -import 'whatwg-fetch'; -import PropTypes from 'prop-types'; -import React from 'react'; -import _ from 'underscore'; -import styles from './style.css'; - -class LibrarySourcedBlockPicker extends React.Component { - constructor(props) { - super(props); - this.state = { - libraries: [], - xblocks: [], - // eslint-disable-next-line react/no-unused-state - searchedLibrary: '', - libraryLoading: false, - xblocksLoading: false, - selectedLibrary: undefined, - selectedXblocks: new Set(this.props.selectedXblocks), - }; - this.onLibrarySearchInput = this.onLibrarySearchInput.bind(this); - this.onXBlockSearchInput = this.onXBlockSearchInput.bind(this); - this.onLibrarySelected = this.onLibrarySelected.bind(this); - this.onXblockSelected = this.onXblockSelected.bind(this); - this.onDeleteClick = this.onDeleteClick.bind(this); - } - - componentDidMount() { - this.fetchLibraries(); - } - - // eslint-disable-next-line react/sort-comp - fetchLibraries(textSearch = '', page = 1, append = false) { - this.setState({ - // eslint-disable-next-line react/no-access-state-in-setstate - libraries: append ? this.state.libraries : [], - libraryLoading: true, - }, async function() { - try { - let res = await fetch(`/api/libraries/v2/?pagination=true&page=${page}&text_search=${textSearch}`); - res = await res.json(); - this.setState({ - // eslint-disable-next-line react/no-access-state-in-setstate - libraries: this.state.libraries.concat(res.results), - libraryLoading: false, - }, () => { - if (res.next) { - this.fetchLibraries(textSearch, page + 1, true); - } - }); - } catch (error) { - $('#library-sourced-block-picker').trigger('error', { - title: 'Could not fetch library', - message: error, - }); - this.setState({ - libraries: [], - libraryLoading: false, - }); - } - }); - } - - fetchXblocks(library, textSearch = '', page = 1, append = false) { - this.setState({ - // eslint-disable-next-line react/no-access-state-in-setstate - xblocks: append ? this.state.xblocks : [], - xblocksLoading: true, - }, async function() { - try { - let res = await fetch(`/api/libraries/v2/${library}/blocks/?pagination=true&page=${page}&text_search=${textSearch}`); - res = await res.json(); - this.setState({ - // eslint-disable-next-line react/no-access-state-in-setstate - xblocks: this.state.xblocks.concat(res.results), - xblocksLoading: false, - }, () => { - if (res.next) { - this.fetchXblocks(library, textSearch, page + 1, true); - } - }); - } catch (error) { - $('#library-sourced-block-picker').trigger('error', { - title: 'Could not fetch xblocks', - message: error, - }); - this.setState({ - xblocks: [], - xblocksLoading: false, - }); - } - }); - } - - onLibrarySearchInput(event) { - event.persist(); - this.setState({ - // eslint-disable-next-line react/no-unused-state - searchedLibrary: event.target.value, - }); - if (!this.debouncedFetchLibraries) { - this.debouncedFetchLibraries = _.debounce(value => { - this.fetchLibraries(value); - }, 300); - } - this.debouncedFetchLibraries(event.target.value); - } - - onXBlockSearchInput(event) { - event.persist(); - if (!this.debouncedFetchXblocks) { - this.debouncedFetchXblocks = _.debounce(value => { - this.fetchXblocks(this.state.selectedLibrary, value); - }, 300); - } - this.debouncedFetchXblocks(event.target.value); - } - - onLibrarySelected(event) { - this.setState({ - selectedLibrary: event.target.value, - }); - this.fetchXblocks(event.target.value); - } - - onXblockSelected(event) { - // eslint-disable-next-line prefer-const, react/no-access-state-in-setstate - let state = new Set(this.state.selectedXblocks); - if (event.target.checked) { - state.add(event.target.value); - } else { - state.delete(event.target.value); - } - this.setState({ - selectedXblocks: state, - }, this.updateList); - } - - onDeleteClick(event) { - let value; - if (event.target.tagName == 'SPAN') { - value = event.target.parentElement.dataset.value; - } else { - value = event.target.dataset.value; - } - // eslint-disable-next-line prefer-const, react/no-access-state-in-setstate - let state = new Set(this.state.selectedXblocks); - state.delete(value); - this.setState({ - selectedXblocks: state, - }, this.updateList); - } - - updateList(list) { - $('#library-sourced-block-picker').trigger('selected-xblocks', { - sourceBlockIds: Array.from(this.state.selectedXblocks), - }); - } - - render() { - return ( -
-
-
-

-

-
-
-
-
- -
- { - this.state.libraries.map(lib => ( -
- - -
- )) - } - { this.state.libraryLoading && {gettext('Loading...')} } -
-
-
- -
- { - this.state.xblocks.map(block => ( -
- - -
- )) - } - { this.state.xblocksLoading && {gettext('Loading...')} } -
-
-
-

{gettext('Selected blocks')}

-
    - { - Array.from(this.state.selectedXblocks).map(block => ( -
  • - {/* eslint-disable-next-line jsx-a11y/label-has-associated-control */} - - {/* eslint-disable-next-line react/button-has-type */} - -
  • - )) - } -
-
-
-
- ); - } -} - -LibrarySourcedBlockPicker.propTypes = { - // eslint-disable-next-line react/forbid-prop-types - selectedXblocks: PropTypes.array, -}; - -LibrarySourcedBlockPicker.defaultProps = { - selectedXblocks: [], -}; - -export {LibrarySourcedBlockPicker}; // eslint-disable-line import/prefer-default-export diff --git a/xmodule/assets/library_source_block/public/js/library_source_block.js b/xmodule/assets/library_source_block/public/js/library_source_block.js deleted file mode 100644 index 9674080d59dc..000000000000 --- a/xmodule/assets/library_source_block/public/js/library_source_block.js +++ /dev/null @@ -1,58 +0,0 @@ -/* JavaScript for allowing editing options on LibrarySourceBlock's studio view */ -window.LibrarySourceBlockStudioView = function(runtime, element) { - 'use strict'; - var self = this; - - $('#library-sourced-block-picker', element).on('selected-xblocks', function(e, params) { - self.sourceBlockIds = params.sourceBlockIds; - }); - - $('#library-sourced-block-picker', element).on('error', function(e, params) { - runtime.notify('error', {title: gettext(params.title), message: params.message}); - }); - - $('.save-button', element).on('click', function(e) { - e.preventDefault(); - var url = $(e.target).data('submit-url'); - var data = { - values: { - source_block_ids: self.sourceBlockIds - }, - defaults: ['display_name'] - }; - - runtime.notify('save', { - state: 'start', - message: gettext('Saving'), - element: element - }); - $.ajax({ - type: 'POST', - url: url, - data: JSON.stringify(data), - global: false // Disable error handling that conflicts with studio's notify('save') and notify('cancel') - }).done(function() { - runtime.notify('save', { - state: 'end', - element: element - }); - }).fail(function(jqXHR) { - var message = gettext('This may be happening because of an error with our server or your internet connection. Try refreshing the page or making sure you are online.'); // eslint-disable-line max-len - if (jqXHR.responseText) { // Is there a more specific error message we can show? - try { - message = JSON.parse(jqXHR.responseText).error; - if (typeof message === 'object' && message.messages) { - // e.g. {"error": {"messages": [{"text": "Unknown user 'bob'!", "type": "error"}, ...]}} etc. - message = $.map(message.messages, function(msg) { return msg.text; }).join(', '); - } - } catch (error) { message = jqXHR.responseText.substr(0, 300); } - } - runtime.notify('error', {title: gettext('Unable to update settings'), message: message}); - }); - }); - - $('.cancel-button', element).on('click', function(e) { - e.preventDefault(); - runtime.notify('cancel', {}); - }); -}; diff --git a/xmodule/assets/library_source_block/style.css b/xmodule/assets/library_source_block/style.css deleted file mode 100644 index 4892f20405c0..000000000000 --- a/xmodule/assets/library_source_block/style.css +++ /dev/null @@ -1,58 +0,0 @@ -.column { - display: flex; - flex-direction: column; - margin: 10px; - max-width: 300px; - flex-grow: 1; -} -.elementList { - margin-top: 10px; -} -input.search { - width: 100% !important; - height: auto !important; -} -.element > input[type='checkbox'], -.element > input[type='radio'] { - position: absolute; - width: 0 !important; - height: 0 !important; - top: -9999px; -} -.element > .elementItem { - display: flex; - flex-grow: 1; - padding: 0.625rem 1.25rem; - border: 1px solid rgba(0, 0, 0, 0.25); -} -.element + .element > label { - border-top: 0; -} -.element > input[type='checkbox']:focus + label, -.element > input[type='radio']:focus + label, -.element > input:hover + label { - background: #f6f6f7; - cursor: pointer; -} -.element > input:checked + label { - background: #23419f; - color: #fff; -} -.element > input[type='checkbox']:checked:focus + label, -.element > input[type='radio']:checked:focus + label, -.element > input:checked:hover + label { - background: #193787; - cursor: pointer; -} -.selectedBlocks { - padding: 12px 8px 20px; -} -button.remove { - background: #e00; - color: #fff; - border: solid rgba(0,0,0,0.25) 1px; -} -button.remove:focus, -button.remove:hover { - background: #d00; -} diff --git a/xmodule/library_sourced_block.py b/xmodule/library_sourced_block.py deleted file mode 100644 index d96741d6e447..000000000000 --- a/xmodule/library_sourced_block.py +++ /dev/null @@ -1,159 +0,0 @@ -""" -Library Sourced Content XBlock -""" -import logging - -from copy import copy -from mako.template import Template as MakoTemplate -from xblock.core import XBlock -from xblock.fields import Scope, String, List -from xblock.validation import ValidationMessage -from xblockutils.resources import ResourceLoader -from xblockutils.studio_editable import StudioEditableXBlockMixin -from webob import Response -from web_fragments.fragment import Fragment - -from xmodule.studio_editable import StudioEditableBlock as EditableChildrenMixin -from xmodule.validation import StudioValidation, StudioValidationMessage - -log = logging.getLogger(__name__) -loader = ResourceLoader(__name__) - -# Make '_' a no-op so we can scrape strings. Using lambda instead of -# `django.utils.translation.ugettext_noop` because Django cannot be imported in this file -_ = lambda text: text - - -@XBlock.wants('library_tools') # Only needed in studio -class LibrarySourcedBlock(StudioEditableXBlockMixin, EditableChildrenMixin, XBlock): - """ - Library Sourced Content XBlock - - Allows copying specific XBlocks from a Blockstore-based content library into - a modulestore-based course. The selected blocks are copied and become - children of this block. - - When we implement support for Blockstore-based courses, it's expected we'll - use a different mechanism for importing library content into a course. - """ - display_name = String( - help=_("The display name for this component."), - default="Library Sourced Content", - display_name=_("Display Name"), - scope=Scope.content, - ) - source_block_ids = List( - display_name=_("Library Blocks List"), - help=_("Enter the IDs of the library XBlocks that you wish to use."), - scope=Scope.content, - ) - editable_fields = ("display_name", "source_block_ids") - has_children = True - has_author_view = True - resources_dir = 'assets/library_source_block' - MAX_BLOCKS_ALLOWED = 10 - - def __str__(self): - return f"LibrarySourcedBlock: {self.display_name}" - - def __init__(self, *args, **kwargs): - super().__init__(*args, **kwargs) - if not self.source_block_ids: - self.has_children = False - - def studio_view(self, context): - """ - Render a form for editing this XBlock - """ - fragment = Fragment() - static_content = ResourceLoader('common.djangoapps.pipeline_mako').load_unicode('templates/static_content.html') - render_react = MakoTemplate(static_content, default_filters=[]).get_def('renderReact') - react_content = render_react.render( - component="LibrarySourcedBlockPicker", - id="library-sourced-block-picker", - props={ - 'selectedXblocks': self.source_block_ids, - } - ) - fragment.content = loader.render_django_template('templates/library-sourced-block-studio-view.html', { - 'react_content': react_content, - 'save_url': self.runtime.handler_url(self, 'submit_studio_edits'), - }) - - fragment.add_javascript_url(self.runtime.local_resource_url(self, 'public/js/library_source_block.js')) - fragment.initialize_js('LibrarySourceBlockStudioView') - - return fragment - - def author_view(self, context): - """ - Renders the Studio preview view. - """ - fragment = Fragment() - context = {} if not context else copy(context) # Isolate context - without this there are weird bugs in Studio - # EditableChildrenMixin.render_children will render HTML that allows instructors to make edits to the children - context['can_move'] = False - self.render_children(context, fragment, can_reorder=False, can_add=False) - return fragment - - def student_view(self, context): - """ - Renders the view that learners see. - """ - result = Fragment() - child_frags = self.runtime.render_children(self, context=context) - result.add_resources(child_frags) - result.add_content('
') - for frag in child_frags: - result.add_content(frag.content) - result.add_content('
') - return result - - def validate_field_data(self, validation, data): - """ - Validate this block's field data. Instead of checking fields like self.name, check the - fields set on data, e.g. data.name. This allows the same validation method to be re-used - for the studio editor. - """ - if len(data.source_block_ids) > self.MAX_BLOCKS_ALLOWED: - # Because importing library blocks is an expensive operation - validation.add( - ValidationMessage( - ValidationMessage.ERROR, - _("A maximum of {0} components may be added.").format(self.MAX_BLOCKS_ALLOWED) - ) - ) - - def validate(self): - """ - Validates the state of this library_sourced_xblock Instance. This is the override of the general XBlock method, - and it will also ask its superclass to validate. - """ - validation = super().validate() - validation = StudioValidation.copy(validation) - - if not self.source_block_ids: - validation.set_summary( - StudioValidationMessage( - StudioValidationMessage.NOT_CONFIGURED, - _("No XBlock has been configured for this component. Use the editor to select the target blocks."), - action_class='edit-button', - action_label=_("Open Editor") - ) - ) - return validation - - @XBlock.handler - def submit_studio_edits(self, data, suffix=''): - """ - Save changes to this block, applying edits made in Studio. - """ - response = super().submit_studio_edits(data, suffix) - # Replace our current children with the latest ones from the libraries. - lib_tools = self.runtime.service(self, 'library_tools') - try: - lib_tools.import_from_blockstore(self, self.source_block_ids) - except Exception as err: # pylint: disable=broad-except - log.exception(err) - return Response(_("Importing Library Block failed - are the IDs valid and readable?"), status=400) - return response diff --git a/xmodule/library_tools.py b/xmodule/library_tools.py index 58cd8212416f..748f1b58ac23 100644 --- a/xmodule/library_tools.py +++ b/xmodule/library_tools.py @@ -196,7 +196,7 @@ def import_from_blockstore(self, dest_block, blockstore_block_ids): content library) into modulestore, as a new child of dest_block. Any existing children of dest_block are replaced. - This is only used by LibrarySourcedBlock. It should verify first that + This is only used by LibraryContentBlock. It should verify first that the number of block IDs is reasonable. """ dest_key = dest_block.scope_ids.usage_id @@ -216,7 +216,7 @@ def import_from_blockstore(self, dest_block, blockstore_block_ids): raise PermissionDenied() # Read the source block; this will also confirm that user has permission to read it. - # (This could be slow and use lots of memory, except for the fact that LibrarySourcedBlock which calls this + # (This could be slow and use lots of memory, except for the fact that LibraryContentBlock which calls this # should be limiting the number of blocks to a reasonable limit. We load them all now instead of one at a # time in order to raise any errors before we start actually copying blocks over.) orig_blocks = [load_block(UsageKey.from_string(key), user) for key in blockstore_block_ids] diff --git a/xmodule/templates/library-sourced-block-studio-view.html b/xmodule/templates/library-sourced-block-studio-view.html deleted file mode 100644 index 08a2882b51c5..000000000000 --- a/xmodule/templates/library-sourced-block-studio-view.html +++ /dev/null @@ -1,19 +0,0 @@ -{% load i18n %} -
-
-
- {{ react_content|safe }} -
-
- -
diff --git a/xmodule/tests/test_library_sourced_block.py b/xmodule/tests/test_library_sourced_block.py deleted file mode 100644 index 5ec6291542cd..000000000000 --- a/xmodule/tests/test_library_sourced_block.py +++ /dev/null @@ -1,71 +0,0 @@ -""" -Tests for Source from Library XBlock -""" - -from openedx.core.djangoapps.content_libraries.tests.base import ContentLibrariesRestApiTest -from common.djangoapps.student.roles import CourseInstructorRole -from xmodule.modulestore.django import modulestore -from xmodule.modulestore.tests.factories import CourseFactory, BlockFactory -from xmodule.tests import prepare_block_runtime -from xmodule.x_module import STUDENT_VIEW # lint-amnesty, pylint: disable=unused-import - - -class LibrarySourcedBlockTestCase(ContentLibrariesRestApiTest): - """ - Tests for LibraryToolsService which interact with blockstore-based content libraries - """ - def setUp(self): - super().setUp() - self.store = modulestore() - # Create a modulestore course - course = CourseFactory.create(modulestore=self.store, user_id=self.user.id) - CourseInstructorRole(course.id).add_users(self.user) - # Add a "Source from Library" block to the course - self.source_block = BlockFactory.create( - category="library_sourced", - parent=course, - parent_location=course.location, - user_id=self.user.id, - modulestore=self.store - ) - self.submit_url = f'/xblock/{self.source_block.scope_ids.usage_id}/handler/submit_studio_edits' - - def test_block_views(self): - # Create a blockstore content library - library = self._create_library(slug="testlib1_preview", title="Test Library 1", description="Testing XBlocks") - # Add content to the library - html_block_1 = self._add_block_to_library(library["id"], "html", "html_student_preview_1")["id"] - self._set_library_block_olx(html_block_1, 'Student Preview Test 1') - html_block_2 = self._add_block_to_library(library["id"], "html", "html_student_preview_2")["id"] - self._set_library_block_olx(html_block_2, 'Student Preview Test 2') - - # Import the html blocks from the library to the course - post_data = {"values": {"source_block_ids": [html_block_1, html_block_2]}, "defaults": ["display_name"]} - res = self.client.post(self.submit_url, data=post_data, format='json') - - # Check if student_view renders the children correctly - res = self.get_block_view(self.source_block, STUDENT_VIEW) - assert 'Student Preview Test 1' in res - assert 'Student Preview Test 2' in res - - def test_block_limits(self): - # Create a blockstore content library - library = self._create_library(slug="testlib2_preview", title="Test Library 2", description="Testing XBlocks") - # Add content to the library - blocks = [self._add_block_to_library(library["id"], "html", f"block_{i}")["id"] for i in range(11)] - - # Import the html blocks from the library to the course - post_data = {"values": {"source_block_ids": blocks}, "defaults": ["display_name"]} - res = self.client.post(self.submit_url, data=post_data, format='json') - assert res.status_code == 400 - assert res.json()['error']['messages'][0]['text'] == 'A maximum of 10 components may be added.' - - def get_block_view(self, block, view, context=None): - """ - Renders the specified view for a given XBlock - """ - context = context or {} - block = self.store.get_item(block.location) - prepare_block_runtime(block.runtime) - block.bind_for_student(self.user.id) - return block.runtime.render(block, view, context).content From 9eee194edb797d6fcd9ddf1c33e2b0ef0ffcc42b Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Thu, 14 Sep 2023 13:18:42 -0400 Subject: [PATCH 02/87] feat: support V2 libraries in LibraryContentBlock (randomized only) Co-Authored-By: Connor Haugh Co-Authored-By: Eugene Dyudyunov --- .../contentstore/tests/test_libraries.py | 18 +- .../contentstore/views/component.py | 5 + cms/djangoapps/contentstore/views/preview.py | 6 +- .../contentstore/views/tests/test_block.py | 4 +- .../xblock_storage_handlers/view_handlers.py | 22 +- cms/envs/common.py | 2 - cms/lib/xblock/tagging/test.py | 7 +- cms/static/js/views/pages/container.js | 142 +++++++- cms/static/sass/elements/_layout.scss | 4 + cms/static/sass/elements/_xblocks.scss | 25 +- cms/templates/container.html | 11 +- cms/templates/studio_xblock_wrapper.html | 19 +- docs/docs_settings.py | 1 - lms/envs/common.py | 3 + openedx/core/lib/blockstore_api/methods.py | 9 +- webpack.common.config.js | 12 - .../public/js/library_content_edit.js | 18 + .../public/js/library_content_edit_helpers.js | 54 +++ xmodule/library_content_block.py | 79 +++-- xmodule/library_tools.py | 154 ++++++--- .../split_mongo/caching_descriptor_system.py | 7 +- xmodule/tasks.py | 319 ++++++++++++++++++ xmodule/tests/test_library_content.py | 94 +++++- xmodule/tests/test_library_tools.py | 168 +++++++-- 24 files changed, 1018 insertions(+), 165 deletions(-) create mode 100644 xmodule/assets/library_content/public/js/library_content_edit_helpers.js create mode 100644 xmodule/tasks.py diff --git a/cms/djangoapps/contentstore/tests/test_libraries.py b/cms/djangoapps/contentstore/tests/test_libraries.py index bb692c016033..ec683046bff2 100644 --- a/cms/djangoapps/contentstore/tests/test_libraries.py +++ b/cms/djangoapps/contentstore/tests/test_libraries.py @@ -106,7 +106,7 @@ def _refresh_children(self, lib_content_block, status_code_expected=200): lib_content_block.runtime._services['user'] = user_service # pylint: disable=protected-access handler_url = reverse_usage_url( - 'component_handler', + 'preview_handler', lib_content_block.location, kwargs={'handler': 'refresh_children'} ) @@ -359,8 +359,6 @@ def test_change_after_first_sync(self): self.assertEqual(resp.status_code, 200) lc_block = modulestore().get_item(lc_block.location) self.assertEqual(len(lc_block.children), 1) # Children should not be deleted due to a bad setting. - html_block = modulestore().get_item(lc_block.children[0]) - self.assertEqual(html_block.data, data_value) def test_refreshes_children_if_libraries_change(self): """ Tests that children are automatically refreshed if libraries list changes """ @@ -406,7 +404,7 @@ def test_refreshes_children_if_libraries_change(self): html_block = modulestore().get_item(lc_block.children[0]) self.assertEqual(html_block.data, data2) - @patch("xmodule.library_tools.SearchEngine.get_search_engine", Mock(return_value=None, autospec=True)) + @patch("xmodule.tasks.SearchEngine.get_search_engine", Mock(return_value=None, autospec=True)) def test_refreshes_children_if_capa_type_change(self): """ Tests that children are automatically refreshed if capa type field changes """ name1, name2 = "Option Problem", "Multiple Choice Problem" @@ -993,24 +991,22 @@ def test_duplicated_version(self): self.library = store.get_library(self.lib_key) # Refresh our reference to the block - self.lc_block = store.get_item(self.lc_block.location) + self.lc_block = self._refresh_children(self.lc_block) self.problem_in_course = store.get_item(self.problem_in_course.location) # The library has changed... self.assertEqual(len(self.library.children), 2) - # But the block hasn't. - self.assertEqual(len(self.lc_block.children), 1) - self.assertEqual(self.problem_in_course.location, self.lc_block.children[0]) - self.assertEqual(self.problem_in_course.display_name, self.original_display_name) + # and the block has changed too. + self.assertEqual(len(self.lc_block.children), 2) # Duplicate self.lc_block: duplicate = store.get_item( duplicate_block(self.course.location, self.lc_block.location, self.user) ) # The duplicate should have identical children to the original: - self.assertEqual(len(duplicate.children), 1) + self.assertEqual(len(duplicate.children), 2) self.assertTrue(self.lc_block.source_library_version) self.assertEqual(self.lc_block.source_library_version, duplicate.source_library_version) problem2_in_course = store.get_item(duplicate.children[0]) - self.assertEqual(problem2_in_course.display_name, self.original_display_name) + self.assertEqual(problem2_in_course.display_name, self.problem.display_name) diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index 3c31637d4c78..b815a14ed171 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -195,6 +195,9 @@ def container_handler(request, usage_key_string): # Get the status of the user's clipboard so they can paste components if they have something to paste user_clipboard = content_staging_api.get_user_clipboard_json(request.user.id, request) + library_block_types = [problem_type['component'] for problem_type in LIBRARY_BLOCK_TYPES] + is_library_xblock = xblock.location.block_type in library_block_types + return render_to_response('container.html', { 'language_code': request.LANGUAGE_CODE, 'context_course': course, # Needed only for display of menus at top of page. @@ -203,6 +206,7 @@ def container_handler(request, usage_key_string): 'xblock_locator': xblock.location, 'unit': unit, 'is_unit_page': is_unit_page, + 'is_collapsible': is_library_xblock, 'subsection': subsection, 'section': section, 'position': index, @@ -218,6 +222,7 @@ def container_handler(request, usage_key_string): 'templates': CONTAINER_TEMPLATES, # Status of the user's clipboard, exactly as would be returned from the "GET clipboard" REST API. 'user_clipboard': user_clipboard, + 'is_fullwidth_content': is_library_xblock, }) else: return HttpResponseBadRequest("Only supports HTML requests") diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 5ee2740abe78..3729b8e627b9 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -45,7 +45,7 @@ wrap_xblock_aside ) -from ..utils import get_visibility_partition_info +from ..utils import get_visibility_partition_info, StudioPermissionsService from .access import get_user_role from .session_kv_store import SessionKeyValueStore @@ -198,6 +198,7 @@ def _prepare_runtime_for_preview(request, block): deprecated_anonymous_user_id = anonymous_id_for_user(request.user, None) services = { + "studio_user_permissions": StudioPermissionsService(request.user), "i18n": XBlockI18nService, 'mako': mako_service, "settings": SettingsService(), @@ -310,6 +311,9 @@ def _studio_wrap_xblock(xblock, view, frag, context, display_name_only=False): 'is_reorderable': is_reorderable, 'can_edit': can_edit, 'can_edit_visibility': context.get('can_edit_visibility', is_course), + 'is_loading': context.get('is_loading', False), + 'is_selected': context.get('is_selected', False), + 'selectable': context.get('selectable', False), 'selected_groups_label': selected_groups_label, 'can_add': context.get('can_add', True), 'can_move': context.get('can_move', is_course), diff --git a/cms/djangoapps/contentstore/views/tests/test_block.py b/cms/djangoapps/contentstore/views/tests/test_block.py index 26b3f91a0bd7..63a37df8df02 100644 --- a/cms/djangoapps/contentstore/views/tests/test_block.py +++ b/cms/djangoapps/contentstore/views/tests/test_block.py @@ -211,7 +211,7 @@ def test_get_empty_container_fragment(self): self.assertNotRegex(html, r"wrapper-xblock[^-]+") # Verify that the header and article tags are still added - self.assertIn('
', html) + self.assertIn('
', html) self.assertIn('
', html) def test_get_container_fragment(self): @@ -233,7 +233,7 @@ def test_get_container_fragment(self): # Verify that the Studio nesting wrapper has been added self.assertIn("level-nesting", html) - self.assertIn('
', html) + self.assertIn('
', html) self.assertIn('
', html) # Verify that the Studio element wrapper has been added diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index f7d369630769..f798a2d4c21f 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -178,6 +178,7 @@ def handle_xblock(request, usage_key_string=None): the public studio content API. """ if usage_key_string: + usage_key = usage_key_with_run(usage_key_string) access_check = ( @@ -218,7 +219,9 @@ def handle_xblock(request, usage_key_string=None): _delete_item(usage_key, request.user) return JsonResponse() else: # Since we have a usage_key, we are updating an existing xblock. - return modify_xblock(usage_key, request) + modified_xblock = modify_xblock(usage_key, request) + _post_editor_saved_callback(get_xblock(usage_key, request.user)) + return modified_xblock elif request.method in ("PUT", "POST"): if "duplicate_source_locator" in request.json: @@ -226,7 +229,6 @@ def handle_xblock(request, usage_key_string=None): duplicate_source_usage_key = usage_key_with_run( request.json["duplicate_source_locator"] ) - source_course = duplicate_source_usage_key.course_key dest_course = parent_usage_key.course_key if not has_studio_write_access( @@ -253,6 +255,8 @@ def handle_xblock(request, usage_key_string=None): request.user, request.json.get("display_name"), ) + _post_editor_saved_callback(get_xblock(dest_usage_key, request.user)) + return JsonResponse( { "locator": str(dest_usage_key), @@ -296,7 +300,6 @@ def handle_xblock(request, usage_key_string=None): def modify_xblock(usage_key, request): request_data = request.json - print(f'In modify_xblock with data = {request_data.get("data")}, fields = {request_data.get("fields")}') return _save_xblock( request.user, get_xblock(usage_key, request.user), @@ -372,7 +375,15 @@ def _update_with_callback(xblock, user, old_metadata=None, old_content=None): return modulestore().update_item(xblock, user.id) -def _save_xblock( # lint-amnesty, pylint: disable=too-many-statements +def _post_editor_saved_callback(xblock): + """ + Updates the xblock in the modulestore after saving xblock. + """ + if callable(getattr(xblock, "post_editor_saved", None)): + xblock.post_editor_saved() + + +def _save_xblock( user, xblock, data=None, @@ -387,12 +398,11 @@ def _save_xblock( # lint-amnesty, pylint: disable=too-many-statements publish=None, fields=None, summary_configuration_enabled=None, -): +): # lint-amnesty, pylint: disable=too-many-statements """ Saves xblock w/ its fields. Has special processing for grader_type, publish, and nullout and Nones in metadata. nullout means to truly set the field to None whereas nones in metadata mean to unset them (so they revert to default). - """ store = modulestore() # Perform all xblock changes within a (single-versioned) transaction diff --git a/cms/envs/common.py b/cms/envs/common.py index cbc100bcabe6..2941087e859e 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -1994,8 +1994,6 @@ ] LIBRARY_BLOCK_TYPES = [ - # Per https://github.com/openedx/build-test-release-wg/issues/231 - # we removed the library source content block from defaults until complete. { 'component': 'library_content', 'boilerplate_name': None diff --git a/cms/lib/xblock/tagging/test.py b/cms/lib/xblock/tagging/test.py index f00d1a53c89b..173523452d54 100644 --- a/cms/lib/xblock/tagging/test.py +++ b/cms/lib/xblock/tagging/test.py @@ -148,9 +148,12 @@ def test_preview_html(self): tree = etree.parse(StringIO(problem_html), parser) main_div_nodes = tree.xpath('/html/body/div/section/div') - self.assertEqual(len(main_div_nodes), 1) + self.assertEqual(len(main_div_nodes), 2) - div_node = main_div_nodes[0] + loader_div_node = main_div_nodes[0] + self.assertIn('ui-loading', loader_div_node.get('class')) + + div_node = main_div_nodes[1] self.assertEqual(div_node.get('data-init'), 'StructuredTagsInit') self.assertEqual(div_node.get('data-runtime-class'), 'PreviewRuntime') self.assertEqual(div_node.get('data-block-type'), 'tagging_aside') diff --git a/cms/static/js/views/pages/container.js b/cms/static/js/views/pages/container.js index 83a3c177d13b..9cc41e59f830 100644 --- a/cms/static/js/views/pages/container.js +++ b/cms/static/js/views/pages/container.js @@ -7,11 +7,14 @@ define(['jquery', 'underscore', 'backbone', 'gettext', 'js/views/pages/base_page 'js/views/components/add_xblock', 'js/views/modals/edit_xblock', 'js/views/modals/move_xblock_modal', 'js/models/xblock_info', 'js/views/xblock_string_field_editor', 'js/views/xblock_access_editor', 'js/views/pages/container_subviews', 'js/views/unit_outline', 'js/views/utils/xblock_utils', - 'common/js/components/views/feedback_notification', 'common/js/components/views/feedback_prompt', + 'common/js/components/views/feedback_notification', 'common/js/components/views/feedback_prompt', 'js/utils/module', ], -function($, _, Backbone, gettext, BasePage, ViewUtils, ContainerView, XBlockView, AddXBlockComponent, - EditXBlockModal, MoveXBlockModal, XBlockInfo, XBlockStringFieldEditor, XBlockAccessEditor, - ContainerSubviews, UnitOutlineView, XBlockUtils, NotificationView, PromptView) { +function($, _, Backbone, gettext, BasePage, + ViewUtils, ContainerView, XBlockView, + AddXBlockComponent, EditXBlockModal, MoveXBlockModal, + XBlockInfo, XBlockStringFieldEditor, XBlockAccessEditor, + ContainerSubviews, UnitOutlineView, XBlockUtils, + NotificationView, PromptView, ModuleUtils) { 'use strict'; var XBlockContainerPage = BasePage.extend({ @@ -26,7 +29,10 @@ function($, _, Backbone, gettext, BasePage, ViewUtils, ContainerView, XBlockView 'click .delete-button': 'deleteXBlock', 'click .show-actions-menu-button': 'showXBlockActionsMenu', 'click .new-component-button': 'scrollToNewComponentButtons', + 'click .save-button': 'saveSelectedLibraryComponents', 'click .paste-component-button': 'pasteComponent', + 'change .header-library-checkbox': 'toggleLibraryComponent', + 'click .collapse-button': 'collapseXBlock', }, options: { @@ -101,6 +107,7 @@ function($, _, Backbone, gettext, BasePage, ViewUtils, ContainerView, XBlockView model: this.model }); this.unitOutlineView.render(); + } this.listenTo(Backbone, 'move:onXBlockMoved', this.onXBlockMoved); @@ -500,6 +507,78 @@ function($, _, Backbone, gettext, BasePage, ViewUtils, ContainerView, XBlockView }); }, + duplicateXBlock: function(event) { + event.preventDefault(); + this.duplicateComponent(this.findXBlockElement(event.target)); + }, + + showMoveXBlockModal: function(event) { + var xblockElement = this.findXBlockElement(event.target), + parentXBlockElement = xblockElement.parents('.studio-xblock-wrapper'), + modal = new MoveXBlockModal({ + sourceXBlockInfo: XBlockUtils.findXBlockInfo(xblockElement, this.model), + sourceParentXBlockInfo: XBlockUtils.findXBlockInfo(parentXBlockElement, this.model), + XBlockURLRoot: this.getURLRoot(), + outlineURL: this.options.outlineURL + }); + + event.preventDefault(); + modal.show(); + }, + + deleteXBlock: function(event) { + event.preventDefault(); + this.deleteComponent(this.findXBlockElement(event.target)); + }, + + createPlaceholderElement: function() { + return $('
', {class: 'studio-xblock-wrapper'}); + }, + + createComponent: function(template, target) { + // A placeholder element is created in the correct location for the new xblock + // and then onNewXBlock will replace it with a rendering of the xblock. Note that + // for xblocks that can't be replaced inline, the entire parent will be refreshed. + var parentElement = this.findXBlockElement(target), + parentLocator = parentElement.data('locator'), + buttonPanel = target.closest('.add-xblock-component'), + listPanel = buttonPanel.prev(), + scrollOffset = ViewUtils.getScrollOffset(buttonPanel), + $placeholderEl = $(this.createPlaceholderElement()), + requestData = _.extend(template, { + parent_locator: parentLocator + }), + placeholderElement; + placeholderElement = $placeholderEl.appendTo(listPanel); + return $.postJSON(this.getURLRoot() + '/', requestData, + _.bind(this.onNewXBlock, this, placeholderElement, scrollOffset, false)) + .fail(function() { + // Remove the placeholder if the update failed + placeholderElement.remove(); + }); + }, + + duplicateComponent: function(xblockElement) { + // A placeholder element is created in the correct location for the duplicate xblock + // and then onNewXBlock will replace it with a rendering of the xblock. Note that + // for xblocks that can't be replaced inline, the entire parent will be refreshed. + var self = this, + parentElement = self.findXBlockElement(xblockElement.parent()), + scrollOffset = ViewUtils.getScrollOffset(xblockElement), + $placeholderEl = $(self.createPlaceholderElement()), + placeholderElement; + + placeholderElement = $placeholderEl.insertAfter(xblockElement); + XBlockUtils.duplicateXBlock(xblockElement, parentElement) + .done(function(data) { + self.onNewXBlock(placeholderElement, scrollOffset, true, data); + }) + .fail(function() { + // Remove the placeholder if the update failed + placeholderElement.remove(); + }); + }, + deleteComponent: function(xblockElement) { var self = this, xblockInfo = new XBlockInfo({ @@ -510,6 +589,61 @@ function($, _, Backbone, gettext, BasePage, ViewUtils, ContainerView, XBlockView }); }, + getSelectedLibraryComponents: function() { + var self = this; + var locator = this.$el.find('.studio-xblock-wrapper').data('locator'); + console.log(ModuleUtils); + $.getJSON( + ModuleUtils.getUpdateUrl(locator) + '/handler/get_block_ids', + function(data) { + self.selectedLibraryComponents = Array.from(data.source_block_ids); + self.storedSelectedLibraryComponents = Array.from(data.source_block_ids); + } + ); + }, + + saveSelectedLibraryComponents: function(e) { + var self = this; + var locator = this.$el.find('.studio-xblock-wrapper').data('locator'); + e.preventDefault(); + $.postJSON( + ModuleUtils.getUpdateUrl(locator) + '/handler/submit_studio_edits', + {values: {source_block_ids: self.storedSelectedLibraryComponents}}, + function() { + self.selectedLibraryComponents = Array.from(self.storedSelectedLibraryComponents); + self.toggleSaveButton(); + } + ); + }, + + toggleLibraryComponent: function(event) { + var componentId = $(event.target).closest('.studio-xblock-wrapper').data('locator'); + var storeIndex = this.storedSelectedLibraryComponents.indexOf(componentId); + if (storeIndex > -1) { + this.storedSelectedLibraryComponents.splice(storeIndex, 1); + this.toggleSaveButton(); + } else { + this.storedSelectedLibraryComponents.push(componentId); + this.toggleSaveButton(); + } + }, + + toggleSaveButton: function() { + var $saveButton = $('.nav-actions .save-button'); + if (JSON.stringify(this.selectedLibraryComponents.sort()) === JSON.stringify(this.storedSelectedLibraryComponents.sort())) { + $saveButton.addClass('is-hidden'); + window.removeEventListener('beforeunload', this.onBeforePageUnloadCallback); + } else { + $saveButton.removeClass('is-hidden'); + window.addEventListener('beforeunload', this.onBeforePageUnloadCallback); + } + }, + + onBeforePageUnloadCallback: function (event) { + event.preventDefault(); + event.returnValue = ''; + }, + onDelete: function(xblockElement) { // get the parent so we can remove this component from its parent. var xblockView = this.xblockView, diff --git a/cms/static/sass/elements/_layout.scss b/cms/static/sass/elements/_layout.scss index d4240b450f47..ae1090fe4623 100644 --- a/cms/static/sass/elements/_layout.scss +++ b/cms/static/sass/elements/_layout.scss @@ -223,6 +223,10 @@ box-shadow: none; border: 0; background-color: $white; + + &-fullwidth { + width: flex-grid(12, 12); + } } .content-supplementary { diff --git a/cms/static/sass/elements/_xblocks.scss b/cms/static/sass/elements/_xblocks.scss index 87dcb8c7d7c6..119a14826c63 100644 --- a/cms/static/sass/elements/_xblocks.scss +++ b/cms/static/sass/elements/_xblocks.scss @@ -43,6 +43,19 @@ display: flex; align-items: center; + .header-library-checkbox { + margin-right: 10px; + width: 17px; + height: 17px; + cursor: pointer; + vertical-align: middle; + } + + .header-library-checkbox-label { + vertical-align: middle; + cursor: pointer; + } + .header-details { @extend %cont-truncated; @@ -433,7 +446,17 @@ border-color: $blue; } - .xblock-header { + &.is-collapsed { + .xblock-render { + display: none; + } + + .collapse-button .fa { + transform: scale(1, -1); + } + } + + .xblock-header:not(.is-hidden) { display: block; } diff --git a/cms/templates/container.html b/cms/templates/container.html index 89cb4bbfd62f..750a7b4cdd53 100644 --- a/cms/templates/container.html +++ b/cms/templates/container.html @@ -151,6 +151,14 @@

${_("Page Actions")}

${_("Edit")} + % if is_collapsible: + + % endif % endif @@ -163,8 +171,7 @@

${_("Page Actions")}

- -
+
diff --git a/cms/templates/studio_xblock_wrapper.html b/cms/templates/studio_xblock_wrapper.html index fa48a905cca4..58a2c7cddfac 100644 --- a/cms/templates/studio_xblock_wrapper.html +++ b/cms/templates/studio_xblock_wrapper.html @@ -17,7 +17,6 @@ xblock_url = xblock_studio_url(xblock) show_inline = xblock.has_children and not xblock_url section_class = "level-nesting" if show_inline else "level-element" -collapsible_class = "is-collapsible" if xblock.has_children else "" label = determine_label(xblock.display_name_with_default, xblock.scope_ids.block_type) messages = xblock.validate().to_json() block_is_unit = is_unit(xblock) @@ -48,14 +47,17 @@
% endif -
+
+

${_("Importing components")}

+
% endif -
+