diff --git a/cms/djangoapps/contentstore/tasks.py b/cms/djangoapps/contentstore/tasks.py index 91b49b8a37c7..13c72ff3391a 100644 --- a/cms/djangoapps/contentstore/tasks.py +++ b/cms/djangoapps/contentstore/tasks.py @@ -1677,7 +1677,7 @@ def handle_update_xblock_upstream_link(usage_key): except (ItemNotFoundError, InvalidKeyError): LOGGER.exception(f'Could not find item for given usage_key: {usage_key}') return - if not xblock.upstream or not xblock.upstream_version: + if not xblock.upstream or xblock.upstream_version is None: return create_or_update_xblock_upstream_link(xblock) diff --git a/cms/djangoapps/modulestore_migrator/api.py b/cms/djangoapps/modulestore_migrator/api.py index d084be764a2e..21ed2d4aa3a4 100644 --- a/cms/djangoapps/modulestore_migrator/api.py +++ b/cms/djangoapps/modulestore_migrator/api.py @@ -2,22 +2,25 @@ API for migration from modulestore to learning core """ from celery.result import AsyncResult -from opaque_keys.edx.keys import CourseKey, LearningContextKey -from opaque_keys.edx.locator import LibraryLocator, LibraryLocatorV2 +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey, LearningContextKey, UsageKey +from opaque_keys.edx.locator import LibraryLocator, LibraryLocatorV2, LibraryUsageLocatorV2 from openedx_learning.api.authoring import get_collection +from openedx_learning.api.authoring_models import Component from user_tasks.models import UserTaskStatus -from openedx.core.djangoapps.content_libraries.api import get_library +from openedx.core.djangoapps.content_libraries.api import get_library, library_component_usage_key from openedx.core.types.user import AuthUser from . import tasks -from .models import ModulestoreSource +from .models import ModulestoreBlockMigration, ModulestoreSource __all__ = ( "start_migration_to_library", "start_bulk_migration_to_library", "is_successfully_migrated", "get_migration_info", + "get_target_block_usage_keys", ) @@ -102,13 +105,17 @@ def start_bulk_migration_to_library( ) -def is_successfully_migrated(source_key: CourseKey | LibraryLocator) -> bool: +def is_successfully_migrated( + source_key: CourseKey | LibraryLocator, + source_version: str | None = None, +) -> bool: """ Check if the source course/library has been migrated successfully. """ - return ModulestoreSource.objects.get_or_create(key=str(source_key))[0].migrations.filter( - task_status__state=UserTaskStatus.SUCCEEDED - ).exists() + filters = {"task_status__state": UserTaskStatus.SUCCEEDED} + if source_version is not None: + filters["source_version"] = source_version + return ModulestoreSource.objects.get_or_create(key=str(source_key))[0].migrations.filter(**filters).exists() def get_migration_info(source_keys: list[CourseKey | LibraryLocator]) -> dict: @@ -129,3 +136,26 @@ def get_migration_info(source_keys: list[CourseKey | LibraryLocator]) -> dict: named=True, ) } + + +def get_target_block_usage_keys(source_key: CourseKey | LibraryLocator) -> dict[UsageKey, LibraryUsageLocatorV2 | None]: + """ + For given source_key, get a map of legacy block key and its new location in migrated v2 library. + """ + query_set = ModulestoreBlockMigration.objects.filter(overall_migration__source__key=source_key).select_related( + 'source', 'target__component__component_type', 'target__learning_package' + ) + + def construct_usage_key(lib_key_str: str, component: Component) -> LibraryUsageLocatorV2 | None: + try: + lib_key = LibraryLocatorV2.from_string(lib_key_str) + except InvalidKeyError: + return None + return library_component_usage_key(lib_key, component) + + # Use LibraryUsageLocatorV2 and construct usage key + return { + obj.source.key: construct_usage_key(obj.target.learning_package.key, obj.target.component) + for obj in query_set + if obj.source.key is not None + } diff --git a/cms/djangoapps/modulestore_migrator/tests/test_api.py b/cms/djangoapps/modulestore_migrator/tests/test_api.py index 6f4daee94bc5..d208ff85e375 100644 --- a/cms/djangoapps/modulestore_migrator/tests/test_api.py +++ b/cms/djangoapps/modulestore_migrator/tests/test_api.py @@ -38,6 +38,9 @@ def setUp(self): ) self.library_v2 = lib_api.ContentLibrary.objects.get(slug=self.lib_key_v2.slug) self.learning_package = self.library_v2.learning_package + self.blocks = [] + for _ in range(3): + self.blocks.append(self._add_simple_content_block().usage_key) def test_start_migration_to_library(self): """ @@ -384,3 +387,24 @@ def test_get_migration_info(self): assert row.migrations__target__title == "Test Library" assert row.migrations__target_collection__key == collection_key assert row.migrations__target_collection__title == "Test Collection" + + def test_get_target_block_usage_keys(self): + """ + Test that the API can get the list of target block usage keys for a given library. + """ + user = UserFactory() + + api.start_migration_to_library( + user=user, + source_key=self.lib_key, + target_library_key=self.library_v2.library_key, + target_collection_slug=None, + composition_level=CompositionLevel.Component.value, + repeat_handling_strategy=RepeatHandlingStrategy.Skip.value, + preserve_url_slugs=True, + forward_source_to_target=True, + ) + with self.assertNumQueries(1): + result = api.get_target_block_usage_keys(self.lib_key) + for key in self.blocks: + assert result.get(key) is not None diff --git a/cms/lib/xblock/upstream_sync.py b/cms/lib/xblock/upstream_sync.py index 78d14d01c246..508fb1379e1a 100644 --- a/cms/lib/xblock/upstream_sync.py +++ b/cms/lib/xblock/upstream_sync.py @@ -17,17 +17,17 @@ import logging import typing as t -from dataclasses import dataclass, asdict +from dataclasses import asdict, dataclass from django.conf import settings from django.utils.translation import gettext_lazy as _ from opaque_keys import InvalidKeyError -from opaque_keys.edx.keys import CourseKey +from opaque_keys.edx.keys import CourseKey, UsageKey from opaque_keys.edx.locator import LibraryContainerLocator, LibraryUsageLocatorV2 -from opaque_keys.edx.keys import UsageKey +from xblock.core import XBlock, XBlockMixin from xblock.exceptions import XBlockNotFoundError -from xblock.fields import Scope, String, Integer, List -from xblock.core import XBlockMixin, XBlock +from xblock.fields import Integer, List, Scope, String + from xmodule.util.keys import BlockKey if t.TYPE_CHECKING: diff --git a/cms/static/js/views/pages/container.js b/cms/static/js/views/pages/container.js index f65f88e2811d..e58c656003ea 100644 --- a/cms/static/js/views/pages/container.js +++ b/cms/static/js/views/pages/container.js @@ -632,13 +632,13 @@ function($, _, Backbone, gettext, BasePage, doneAddingBlock = (addResult) => { const $placeholderEl = $(this.createPlaceholderElement()); const placeholderElement = $placeholderEl.insertBefore($insertSpot); - placeholderElement.data('locator', addResult.locator); - return this.refreshXBlock(placeholderElement, true); + return this.onNewXBlock(placeholderElement, 0, false, addResult); }; doneAddingAllBlocks = () => {}; } // Note: adding all the XBlocks in parallel will cause a race condition 😢 so we have to add // them one at a time: + let lastAdded = $.when(); for (const { usageKey, blockType } of selectedBlocks) { const addData = { @@ -1220,12 +1220,13 @@ function($, _, Backbone, gettext, BasePage, refreshXBlock: function(element, block_added, is_duplicate) { var xblockElement = this.findXBlockElement(element), parentElement = xblockElement.parent(), - rootLocator = this.xblockView.model.id; + rootLocator = this.xblockView.model.id, + parentBlockType = parentElement.data('block-type'); if (xblockElement.length === 0 || xblockElement.data('locator') === rootLocator) { if (block_added) { this.render({refresh: true, block_added: block_added}); } - } else if (parentElement.hasClass('reorderable-container')) { + } else if (parentElement.hasClass('reorderable-container') || ["itembank", "library_content"].includes(parentBlockType) ) { this.refreshChildXBlock(xblockElement, block_added, is_duplicate); } else { this.refreshXBlock(this.findXBlockElement(parentElement)); diff --git a/xmodule/assets/library_content/public/js/library_content_edit.js b/xmodule/assets/library_content/public/js/library_content_edit.js index fea66e266120..8dba248ca0dc 100644 --- a/xmodule/assets/library_content/public/js/library_content_edit.js +++ b/xmodule/assets/library_content/public/js/library_content_edit.js @@ -1,11 +1,39 @@ /* JavaScript for special editing operations that can be done on LibraryContentXBlock */ -window.LibraryContentAuthorView = function(runtime, element) { +window.LibraryContentAuthorView = function(runtime, element, initArgs) { 'use strict'; var $element = $(element); var usage_id = $element.data('usage-id'); // The "Update Now" button is not a child of 'element', as it is in the validation message area // But it is still inside this xblock's wrapper element, which we can easily find: var $wrapper = $element.parents('*[data-locator="' + usage_id + '"]'); + var { is_root: isRoot = false } = initArgs; + + function postMessageToParent(body, callbackFn = null) { + try { + window.parent.postMessage(body, document.referrer); + if (callbackFn) { + callbackFn(); + } + } catch (e) { + console.error('Failed to post message:', e); + } + }; + + function reloadPreviewPage() { + if (window.self !== window.top) { + // We are inside iframe + // Normal location.reload() reloads the iframe but subsequent calls to + // postMessage fails. So we are using postMessage to tell the parent page + // to reload the iframe. + postMessageToParent({ + type: 'refreshIframe', + message: 'Refresh Iframe', + payload: {}, + }) + } else { + location.reload(); + } + } $wrapper.on('click', '.library-update-btn', function(e) { e.preventDefault(); @@ -20,17 +48,33 @@ window.LibraryContentAuthorView = function(runtime, element) { state: 'end', element: element }); - if ($element.closest('.wrapper-xblock').is(':not(.level-page)')) { - // We are on a course unit page. The notify('save') should refresh this block, - // but that is only working on the container page view of this block. - // Why? On the unit page, this XBlock's runtime has no reference to the - // XBlockContainerPage - only the top-level XBlock (a vertical) runtime does. - // But unfortunately there is no way to get a reference to our parent block's - // JS 'runtime' object. So instead we must refresh the whole page: - location.reload(); + if (isRoot) { + // We are inside preview page where all children blocks are listed. + reloadPreviewPage(); } }); }); + + $wrapper.on('click', '.library-block-migrate-btn', function(e) { + e.preventDefault(); + // migrate library content block to item bank block + runtime.notify('save', { + state: 'start', + element: element, + message: gettext('Migrating to Problem Bank') + }); + $.post(runtime.handlerUrl(element, 'upgrade_to_v2_library')).done(function() { + runtime.notify('save', { + state: 'end', + element: element + }); + if (isRoot) { + // We are inside preview page where all children blocks are listed. + reloadPreviewPage(); + } + }); + }); + // Hide loader and show element when update task finished. var $loader = $wrapper.find('.ui-loading'); var $xblockHeader = $wrapper.find('.xblock-header'); diff --git a/xmodule/item_bank_block.py b/xmodule/item_bank_block.py index b53617e2c8e4..d09aabcf4b05 100644 --- a/xmodule/item_bank_block.py +++ b/xmodule/item_bank_block.py @@ -7,6 +7,7 @@ import logging import random from copy import copy + from django.conf import settings from django.utils.functional import classproperty from lxml import etree @@ -24,13 +25,13 @@ from xmodule.studio_editable import StudioEditableBlock from xmodule.util.builtin_assets import add_webpack_js_to_fragment from xmodule.validation import StudioValidation, StudioValidationMessage -from xmodule.xml_block import XmlMixin from xmodule.x_module import ( + STUDENT_VIEW, ResourceTemplates, XModuleMixin, shim_xmodule_js, - STUDENT_VIEW, ) +from xmodule.xml_block import XmlMixin _ = lambda text: text @@ -268,20 +269,6 @@ def selected_children(self): return self.selected - def format_block_keys_for_analytics(self, block_keys: list[tuple[str, str]]) -> list[dict]: - """ - Given a list of (block_type, block_id) pairs, prepare the JSON-ready metadata needed for analytics logging. - - This is [ - {"usage_key": x, "original_usage_key": y, "original_usage_version": z, "descendants": [...]} - ] - where the main list contains all top-level blocks, and descendants contains a *flat* list of all - descendants of the top level blocks, if any. - - Must be implemented in child class. - """ - raise NotImplementedError - @XBlock.handler def reset_selected_children(self, _, __): """ @@ -432,6 +419,40 @@ def definition_to_xml(self, resource_fs): xml_object.set(field_name, str(field.read_from(self))) return xml_object + def author_view(self, context): + """ + Renders the Studio views. + Normal studio view: If block is properly configured, displays library status summary + Studio container view: displays a preview of all possible children. + """ + fragment = Fragment() + root_xblock = context.get('root_xblock') + is_root = root_xblock and root_xblock.usage_key == self.usage_key + if is_root and self.children: + # User has clicked the "View" link. Show a preview of all possible children: + context['can_edit_visibility'] = False + context['can_move'] = False + context['can_collapse'] = True + self.render_children(context, fragment, can_reorder=False, can_add=False) + else: + # We're just on the regular unit page, or we're on the "view" page but no children exist yet. + # Show a summary message and instructions. + summary_html = loader.render_django_template('templates/item_bank/author_view.html', { + # Due to template interpolation limitations, we have to pass some HTML for the link here: + "view_link": f'', + "blocks": [ + {"display_name": display_name_with_default(child)} + for child in self.get_children() + ], + "block_count": len(self.children), + "max_count": self.max_count, + }) + fragment.add_content(summary_html) + # Whether on the main author view or the detailed children view, show a button to add more from the library: + add_html = loader.render_django_template('templates/item_bank/author_view_add.html', {}) + fragment.add_content(add_html) + return fragment + @classmethod def get_selected_event_prefix(cls) -> str: """ @@ -442,21 +463,6 @@ def get_selected_event_prefix(cls) -> str: """ raise NotImplementedError - -class ItemBankBlock(ItemBankMixin, XBlock): - """ - An XBlock which shows a random subset of its children to each learner. - - Unlike LegacyLibraryContentBlock, this block does not need to worry about synchronization, capa_type filtering, etc. - That is all implemented using `upstream` links on each individual child. - """ - display_name = String( - display_name=_("Display Name"), - help=_("The display name for this component."), - default="Problem Bank", - scope=Scope.settings, - ) - def validate(self): """ Validates the state of this ItemBankBlock Instance. @@ -492,40 +498,6 @@ def validate(self): ) return validation - def author_view(self, context): - """ - Renders the Studio views. - Normal studio view: If block is properly configured, displays library status summary - Studio container view: displays a preview of all possible children. - """ - fragment = Fragment() - root_xblock = context.get('root_xblock') - is_root = root_xblock and root_xblock.usage_key == self.usage_key - if is_root and self.children: - # User has clicked the "View" link. Show a preview of all possible children: - context['can_edit_visibility'] = False - context['can_move'] = False - context['can_collapse'] = True - self.render_children(context, fragment, can_reorder=False, can_add=False) - else: - # We're just on the regular unit page, or we're on the "view" page but no children exist yet. - # Show a summary message and instructions. - summary_html = loader.render_django_template('templates/item_bank/author_view.html', { - # Due to template interpolation limitations, we have to pass some HTML for the link here: - "view_link": f'', - "blocks": [ - {"display_name": display_name_with_default(child)} - for child in self.get_children() - ], - "block_count": len(self.children), - "max_count": self.max_count, - }) - fragment.add_content(summary_html) - # Whether on the main author view or the detailed children view, show a button to add more from the library: - add_html = loader.render_django_template('templates/item_bank/author_view_add.html', {}) - fragment.add_content(add_html) - return fragment - def format_block_keys_for_analytics(self, block_keys: list[tuple[str, str]]) -> list[dict]: """ Implement format_block_keys_for_analytics using the `upstream` link system. @@ -541,6 +513,21 @@ def format_block_keys_for_analytics(self, block_keys: list[tuple[str, str]]) -> } for block_key in block_keys ] + +class ItemBankBlock(ItemBankMixin, XBlock): + """ + An XBlock which shows a random subset of its children to each learner. + + Unlike LegacyLibraryContentBlock, this block does not need to worry about synchronization, capa_type filtering, etc. + That is all implemented using `upstream` links on each individual child. + """ + display_name = String( + display_name=_("Display Name"), + help=_("The display name for this component."), + default="Problem Bank", + scope=Scope.settings, + ) + @classmethod def get_selected_event_prefix(cls) -> str: """ diff --git a/xmodule/library_content_block.py b/xmodule/library_content_block.py index 52e33108027c..d110bcf53724 100644 --- a/xmodule/library_content_block.py +++ b/xmodule/library_content_block.py @@ -12,7 +12,7 @@ import json import logging -from gettext import ngettext, gettext +from gettext import gettext, ngettext import nh3 from django.core.exceptions import ObjectDoesNotExist, PermissionDenied @@ -20,11 +20,12 @@ from web_fragments.fragment import Fragment from webob import Response from xblock.core import XBlock -from xblock.fields import Integer, Scope, String +from xblock.fields import Boolean, Scope, String from xmodule.capa.responsetypes import registry -from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.item_bank_block import ItemBankMixin +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.validation import StudioValidation, StudioValidationMessage from xmodule.x_module import XModuleToXBlockMixin @@ -90,12 +91,6 @@ class LegacyLibraryContentBlock(ItemBankMixin, XModuleToXBlockMixin, XBlock): display_name=_("Library Version"), scope=Scope.settings, ) - max_count = Integer( - display_name=_("Count"), - help=_("Enter the number of components to display to each student. Set it to -1 to display all components."), - default=1, - scope=Scope.settings, - ) capa_type = String( display_name=_("Problem Type"), help=_('Choose a problem type to fetch from the library. If "Any Type" is selected no filtering is applied.'), @@ -103,6 +98,17 @@ class LegacyLibraryContentBlock(ItemBankMixin, XModuleToXBlockMixin, XBlock): values=_get_capa_types(), scope=Scope.settings, ) + # This is a hidden field that stores whether child blocks are migrated to v2, i.e., whether they have an upstream. + # We can never completely remove the legacy library_content block; otherwise, we'd lose student data, + # (such as selected fields), which tracks the children selected for each user. + # However, once all legacy libraries are migrated to v2 and removed, this block can be converted into a very thin + # compatibility wrapper around ItemBankBlock. All other aspects of LegacyLibraryContentBlock (the editor, the child + # viewer, the block picker, the legacy syncing mechanism, etc.) can then be removed. + is_migrated_to_v2 = Boolean( + display_name=_("Is Migrated to library v2"), + scope=Scope.settings, + default=False, + ) @property def source_library_key(self): @@ -111,12 +117,35 @@ def source_library_key(self): """ return LibraryLocator.from_string(self.source_library_id) + @property + def is_source_lib_migrated_to_v2(self): + """ + Determines whether the source library has been migrated to v2. + """ + from cms.djangoapps.modulestore_migrator.api import is_successfully_migrated + + return ( + self.source_library_id + and self.source_library_version + and is_successfully_migrated(self.source_library_key, source_version=self.source_library_version) + ) + + @property + def is_ready_to_migrated_to_v2(self): + """ + Returns whether the block can be migrated to v2. + """ + return self.is_source_lib_migrated_to_v2 and not self.is_migrated_to_v2 + def author_view(self, context): """ Renders the Studio views. Normal studio view: If block is properly configured, displays library status summary Studio container view: displays a preview of all possible children. """ + if self.is_migrated_to_v2: + # Show ItemBank UI in this case + return super().author_view(context) fragment = Fragment() root_xblock = context.get('root_xblock') is_root = root_xblock and root_xblock.location == self.location @@ -151,7 +180,7 @@ def author_view(self, context): else: fragment.add_javascript_url(self.runtime.local_resource_url(self, 'public/js/library_content_edit.js')) - fragment.initialize_js('LibraryContentAuthorView') + fragment.initialize_js('LibraryContentAuthorView', {"is_root": is_root}) return fragment @property @@ -159,7 +188,14 @@ def non_editable_metadata_fields(self): non_editable_fields = super().non_editable_metadata_fields non_editable_fields.extend([ LegacyLibraryContentBlock.source_library_version, + LegacyLibraryContentBlock.is_migrated_to_v2, ]) + if self.is_migrated_to_v2: + # If the block is migrated, hide legacy settings to make it similar to the new ItemBankBlock. + non_editable_fields.extend([ + LegacyLibraryContentBlock.capa_type, + LegacyLibraryContentBlock.source_library_id, + ]) return non_editable_fields def get_tools(self, to_read_library_content: bool = False) -> 'LegacyLibraryToolsService': @@ -195,6 +231,12 @@ def upgrade_and_sync(self, request=None, suffix=None): # pylint: disable=unused Returns 400 if libraray tools or user permission services are not available. Returns 403/404 if user lacks read access on source library or write access on this block. """ + if self.is_migrated_to_v2: + # If the block is already migrated to behave like ItemBankBlock + return Response( + _("This block is already migrated to use library v2. You can sync individual blocks now"), + status=400 + ) self._validate_sync_permissions() if not self.source_library_id: return Response(_("Source content library has not been specified."), status=400) @@ -246,6 +288,10 @@ def studio_post_duplicate(self, store, source_block): if hasattr(super(), 'studio_post_duplicate'): super().studio_post_duplicate(store, source_block) + if self.is_migrated_to_v2: + # If the block is already migrated to behave like ItemBankBlock + return False # Children have not been handled + self._validate_sync_permissions() self.get_tools(to_read_library_content=True).trigger_duplication(source_block=source_block, dest_block=self) return True # Children have been handled. @@ -257,14 +303,57 @@ def studio_post_paste(self, store, source_node) -> bool: if hasattr(super(), 'studio_post_paste'): super().studio_post_paste(store, source_node) + if self.is_migrated_to_v2: + # If the block is already migrated to behave like ItemBankBlock + return False # Children have not been handled + self.sync_from_library(upgrade_to_latest=False) return True # Children have been handled + def _v2_update_children_upstream_version(self): + """ + Update the upstream and upstream version fields of all children to point to library v2 version of the legacy + library blocks. This essentially converts this legacy block to new ItemBankBlock. + """ + from cms.djangoapps.modulestore_migrator.api import get_target_block_usage_keys + blocks = get_target_block_usage_keys(self.source_library_key) + store = modulestore() + with store.bulk_operations(self.course_id): + for child in self.get_children(): + source_key, _ = self.runtime.modulestore.get_block_original_usage(child.usage_key) + child.upstream = str(blocks.get(source_key, "")) + # Since after migration, the component in library is in draft state, we want to make sure that sync icon + # appears when it is published + child.upstream_version = 0 + child.save() + # Use `modulestore()` instead of `self.runtime.modulestore` to make sure that the XBLOCK_UPDATED signal + # is triggered + store.update_item(child, None) + self.is_migrated_to_v2 = True + self.save() + store.update_item(self, None) + def _validate_library_version(self, validation, lib_tools, version, library_key): """ Validates library version """ latest_version = lib_tools.get_latest_library_version(library_key) + if self.is_ready_to_migrated_to_v2: + validation.set_summary( + StudioValidationMessage( + StudioValidationMessage.WARNING, + _( + 'This legacy library reference is no longer supported, and' + ' needs to be updated to receive future changes' + ), + # TODO: change this to action_runtime_event='...' once the unit page supports that feature. + # See https://openedx.atlassian.net/browse/TNL-993 + action_class='library-block-migrate-btn', + # Translators: {refresh_icon} placeholder is substituted to "↻" (without double quotes) + action_label=_('{refresh_icon} Update reference').format(refresh_icon='↻'), + ) + ) + return False if latest_version is not None: if version is None or version != latest_version: validation.set_summary( @@ -296,13 +385,33 @@ def _set_validation_error_if_empty(self, validation, summary): if validation.empty: validation.set_summary(summary) + @XBlock.handler + def upgrade_to_v2_library(self, request=None, suffix=None): + """ + Upgrate this legacy block to a mode where it behaves like the new ItemBankBlock which uses library v2 blocks as + children. + """ + if not self.is_source_lib_migrated_to_v2: + return Response(_("The source library has not been migrated to version 2"), status=400) + if self.is_migrated_to_v2: + return Response(_("The block has already been upgraded to version 2"), status=400) + # If the source library is migrated but this block still depends on legacy library + # Migrate the block by setting upstream field to all children blocks + self._v2_update_children_upstream_version() + return Response() + def validate(self): """ - Validates the state of this Library Content Block Instance. This - is the override of the general XBlock method, and it will also ask - its superclass to validate. + Validates the state of this Library Content Block Instance. """ - validation = super().validate() + if self.is_migrated_to_v2: + # If the block is already migrated to v2 i.e. ItemBankBlock + # super() will call ItemBankMixin.validate() as it is first in inheritance order + return super().validate() + + # We cannot use `super()` here because we do not want to invoke `ItemBankMixin.validate()`. + # Instead, we want to use `XBlock.validate`. + validation = XBlock.validate(self) if not isinstance(validation, StudioValidation): validation = StudioValidation.copy(validation) try: @@ -328,7 +437,12 @@ def validate(self): ) ) return validation - self._validate_library_version(validation, lib_tools, self.source_library_version, self.source_library_key) + self._validate_library_version( + validation, + lib_tools, + self.source_library_version, + self.source_library_key + ) # Note: we assume children have been synced # since the last time fields like source_library_id or capa_types were changed. @@ -390,6 +504,9 @@ def post_editor_saved(self, user, old_metadata, old_content): # pylint: disable """ If source library or capa_type have been edited, upgrade library & sync automatically. """ + if self.is_migrated_to_v2: + # If the block is already migrated to v2 i.e. ItemBankBlock, Do nothing + return True source_lib_changed = (self.source_library_id != old_metadata.get("source_library_id", "")) capa_filter_changed = (self.capa_type != old_metadata.get("capa_type", ANY_CAPA_TYPE_VALUE)) if source_lib_changed or capa_filter_changed: @@ -403,6 +520,9 @@ def format_block_keys_for_analytics(self, block_keys: list[tuple[str, str]]) -> """ Implement format_block_keys_for_analytics using the modulestore-specific legacy library original-usage system. """ + if self.is_migrated_to_v2: + return super().format_block_keys_for_analytics(block_keys) + def summarize_block(usage_key): """ Basic information about the given block """ orig_key, orig_version = self.runtime.modulestore.get_block_original_usage(usage_key) diff --git a/xmodule/tests/test_library_content.py b/xmodule/tests/test_library_content.py index 092606142e1d..a5d37cff4288 100644 --- a/xmodule/tests/test_library_content.py +++ b/xmodule/tests/test_library_content.py @@ -7,13 +7,17 @@ from bson.objectid import ObjectId from fs.memoryfs import MemoryFS from lxml import etree -from opaque_keys.edx.locator import LibraryLocator +from opaque_keys.edx.locator import LibraryLocator, LibraryLocatorV2 +from organizations.tests.factories import OrganizationFactory from rest_framework import status from search.search_engine_base import SearchEngine from web_fragments.fragment import Fragment from xblock.runtime import Runtime as VanillaRuntime +from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.content_libraries import api as lib_api from openedx.core.djangolib.testing.utils import skip_unless_cms +from xmodule.capa_block import ProblemBlock from xmodule.library_content_block import ANY_CAPA_TYPE_VALUE, LegacyLibraryContentBlock from xmodule.library_tools import LegacyLibraryToolsService from xmodule.modulestore import ModuleStoreEnum @@ -22,8 +26,6 @@ from xmodule.tests import prepare_block_runtime from xmodule.validation import StudioValidationMessage from xmodule.x_module import AUTHOR_VIEW -from xmodule.capa_block import ProblemBlock -from common.djangoapps.student.tests.factories import UserFactory from .test_course_block import DummySystem as TestImportSystem @@ -42,7 +44,12 @@ def setUp(self): self.tools = LegacyLibraryToolsService(self.store, self.user_id) self.library = LibraryFactory.create(modulestore=self.store) self.lib_blocks = [ - self.make_block("html", self.library, data=f"Hello world from block {i}") + self.make_block( + "html", + self.library, + data=f"Hello world from block {i}", + display_name=f"html {i}" + ) for i in range(1, 5) ] self.course = CourseFactory.create(modulestore=self.store) @@ -84,6 +91,18 @@ def get_block(descriptor): block.runtime.get_block_for_descriptor = get_block + def _verify_xblock_properties(self, imported_lc_block): + """ + Check the new XBlock has the same properties as the old one. + """ + assert imported_lc_block.display_name == self.lc_block.display_name + assert imported_lc_block.source_library_id == self.lc_block.source_library_id + assert imported_lc_block.source_library_version == self.lc_block.source_library_version + assert imported_lc_block.max_count == self.lc_block.max_count + assert imported_lc_block.capa_type == self.lc_block.capa_type + assert len(imported_lc_block.children) == len(self.lc_block.children) + assert imported_lc_block.children == self.lc_block.children + @ddt.ddt class LegacyLibraryContentGeneralTest(LegacyLibraryContentTest): @@ -133,15 +152,14 @@ def setUp(self): self._sync_lc_block_from_library() self.expected_olx = ( - '\n' - ' \n' - ' \n' - ' \n' - ' \n' + f'\n' + f' \n' + f' \n' + f' \n' + f' \n' '\n' - ).format( - block=self.lc_block, ) # Set the virtual FS to export the olx to. @@ -157,27 +175,13 @@ def setUp(self): node = etree.Element("unknown_root") self.lc_block.add_xml_to_node(node) - def _verify_xblock_properties(self, imported_lc_block): - """ - Check the new XBlock has the same properties as the old one. - """ - assert imported_lc_block.display_name == self.lc_block.display_name - assert imported_lc_block.source_library_id == self.lc_block.source_library_id - assert imported_lc_block.source_library_version == self.lc_block.source_library_version - assert imported_lc_block.max_count == self.lc_block.max_count - assert imported_lc_block.capa_type == self.lc_block.capa_type - assert len(imported_lc_block.children) == len(self.lc_block.children) - assert imported_lc_block.children == self.lc_block.children - def test_xml_export_import_cycle(self): """ Test the export-import cycle. """ # Read back the olx. - with self.export_fs.open('{dir}/{file_name}.xml'.format( - dir=self.lc_block.scope_ids.usage_id.block_type, - file_name=self.lc_block.scope_ids.usage_id.block_id - )) as f: + file_path = f'{self.lc_block.scope_ids.usage_id.block_type}/{self.lc_block.scope_ids.usage_id.block_id}.xml' + with self.export_fs.open(file_path) as f: exported_olx = f.read() # And compare. @@ -195,16 +199,15 @@ def test_xml_import_with_comments(self): """ olx_with_comments = ( '\n' - '\n' + f'\n' '\n' - ' \n' - ' \n' - ' \n' - ' \n' + f' \n' + f' \n' + f' \n' + f' \n' '\n' - ).format( - block=self.lc_block, ) # Import the olx. @@ -234,7 +237,7 @@ def _get_capa_problem_type_xml(self, *args): """ Helper function to create empty CAPA problem definition """ problem = "" for problem_type in args: - problem += "<{problem_type}>".format(problem_type=problem_type) + problem += f"<{problem_type}>" problem += "" return problem @@ -500,7 +503,7 @@ class TestLegacyLibraryContentBlockWithSearchIndex(LegacyLibraryContentBlockTest def _get_search_response(self, field_dictionary=None): """ Mocks search response as returned by search engine """ - target_type = field_dictionary.get('problem_types') + target_type = (field_dictionary or {}).get('problem_types') matched_block_locations = [ key for key, problem_types in self.problem_type_lookup.items() if target_type in problem_types @@ -726,3 +729,110 @@ def test_removed_invalid(self): 'original_usage_key': str(keep_block_lib_usage_key), 'original_usage_version': str(keep_block_lib_version), 'descendants': []}] assert event_data['reason'] == 'invalid' + + +@patch( + 'xmodule.modulestore.split_mongo.caching_descriptor_system.CachingDescriptorSystem.render', VanillaRuntime.render +) +@patch('xmodule.html_block.HtmlBlock.author_view', dummy_render, create=True) +@patch('xmodule.x_module.DescriptorSystem.applicable_aside_types', lambda self, block: []) +class TestMigratedLibraryContentRender(LegacyLibraryContentTest): + """ + Rendering unit tests for LegacyLibraryContentBlock + """ + + def setUp(self): + from cms.djangoapps.modulestore_migrator import api + from cms.djangoapps.modulestore_migrator.data import CompositionLevel, RepeatHandlingStrategy + super().setUp() + user = UserFactory() + self._sync_lc_block_from_library() + self.organization = OrganizationFactory() + self.lib_key_v2 = LibraryLocatorV2.from_string( + f"lib:{self.organization.short_name}:test-key" + ) + lib_api.create_library( + org=self.organization, + slug=self.lib_key_v2.slug, + title="Test Library", + ) + self.library_v2 = lib_api.ContentLibrary.objects.get(slug=self.lib_key_v2.slug) + api.start_migration_to_library( + user=user, + source_key=self.library.location.library_key, + target_library_key=self.library_v2.library_key, + target_collection_slug=None, + composition_level=CompositionLevel.Component.value, + repeat_handling_strategy=RepeatHandlingStrategy.Skip.value, + preserve_url_slugs=True, + forward_source_to_target=True, + ) + # Migrate block + self.lc_block.upgrade_to_v2_library(None, None) + + def test_preview_view(self): + """ Test preview view rendering """ + assert len(self.lc_block.children) == len(self.lib_blocks) + self._bind_course_block(self.lc_block) + rendered = self.lc_block.render(AUTHOR_VIEW, {'root_xblock': self.lc_block}) + assert 'Hello world from block 1' in rendered.content + assert 'Hello world from block 2' in rendered.content + assert 'Hello world from block 3' in rendered.content + assert 'Hello world from block 4' in rendered.content + + def test_author_view(self): + """ Test author view rendering """ + assert len(self.lc_block.children) == len(self.lib_blocks) + self._bind_course_block(self.lc_block) + rendered = self.lc_block.render(AUTHOR_VIEW, {}) + # content should be similar to ItemBankBlock + assert 'Learners will see 1 of the 4 selected components' in rendered.content + assert '
  • html 1
  • ' in rendered.content + assert '
  • html 2
  • ' in rendered.content + assert '
  • html 3
  • ' in rendered.content + assert '
  • html 4
  • ' in rendered.content + + def test_xml_export_import_cycle(self): + """ + Test the export-import cycle. + """ + # Render block to migrate it first + self.lc_block.render(AUTHOR_VIEW, {}) + # Set the virtual FS to export the olx to. + export_fs = MemoryFS() + self.lc_block.runtime.export_fs = export_fs # pylint: disable=protected-access + + # Export the olx. + node = etree.Element("unknown_root") + self.lc_block.add_xml_to_node(node) + + # Read back the olx. + file_path = f'{self.lc_block.scope_ids.usage_id.block_type}/{self.lc_block.scope_ids.usage_id.block_id}.xml' + with export_fs.open(file_path) as f: + exported_olx = f.read() + + expected_olx_export = ( + f'\n' + f' \n' + f' \n' + f' \n' + f' \n' + '\n' + ) + # And compare. + assert exported_olx == expected_olx_export + + # Now import it. + runtime = TestImportSystem(load_error_blocks=True, course_id=self.lc_block.location.course_key) + runtime.resources_fs = export_fs + olx_element = etree.fromstring(exported_olx) + imported_lc_block = LegacyLibraryContentBlock.parse_xml(olx_element, runtime, None) + + self._verify_xblock_properties(imported_lc_block) + # Verify migration info in the child + assert imported_lc_block.is_migrated_to_v2 + for child in imported_lc_block.get_children(): + assert child.xml_attributes.get('upstream') is not None + assert str(child.xml_attributes.get('upstream_version')) == '0'