From 744cc87ffb970f3c3ac7332a2808a310f63b0d56 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Mon, 20 Oct 2025 11:20:37 +0530 Subject: [PATCH] feat: follow migrated legacy library content block (#37405) * feat: show item bank ui for migrated legacy library content * feat: migrate legacy content block to item bank block on view in studio * fix: duplicate and copy issues * refactor: migration location and add tests * fix: lint issues * fix: item bank and library content children view add button functionality Newly added blocks from library in children view page of item bank block and migrated library content block were not displayed automatically. * fix: lint issues * fix: lint issues * feat: only migrate if same version of library is migrated * refactor: migrate block on request * fix: component reload on migration * fix: tests * refactor: comments and message wordings * refactor: update alert text * docs: add context * fix: component links not being created on migrating legacy blocks * fix: api docs and types * refactor: use inheritance and specific parent method call * fix: imports * fix: api typing * fix: upstream_version check * refactor: rename variables * refactor: parsing entity keys to usage_keys --- cms/djangoapps/contentstore/tasks.py | 2 +- cms/djangoapps/modulestore_migrator/api.py | 46 ++++- .../modulestore_migrator/tests/test_api.py | 24 +++ cms/lib/xblock/upstream_sync.py | 10 +- cms/static/js/views/pages/container.js | 9 +- .../public/js/library_content_edit.js | 62 +++++- xmodule/item_bank_block.py | 117 +++++------ xmodule/library_content_block.py | 150 ++++++++++++-- xmodule/tests/test_library_content.py | 186 ++++++++++++++---- 9 files changed, 461 insertions(+), 145 deletions(-) diff --git a/cms/djangoapps/contentstore/tasks.py b/cms/djangoapps/contentstore/tasks.py index 91b49b8a37..13c72ff339 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 d084be764a..21ed2d4aa3 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 6f4daee94b..d208ff85e3 100644 --- a/cms/djangoapps/modulestore_migrator/tests/test_api.py +++ b/cms/djangoapps/modulestore_migrator/tests/test_api.py @@ -38,6 +38,9 @@ class TestModulestoreMigratorAPI(LibraryTestCase): ) 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 @@ class TestModulestoreMigratorAPI(LibraryTestCase): 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 78d14d01c2..508fb1379e 100644 --- a/cms/lib/xblock/upstream_sync.py +++ b/cms/lib/xblock/upstream_sync.py @@ -17,17 +17,17 @@ from __future__ import annotations 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 f65f88e281..e58c656003 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 fea66e2661..8dba248ca0 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 b53617e2c8..d09aabcf4b 100644 --- a/xmodule/item_bank_block.py +++ b/xmodule/item_bank_block.py @@ -7,6 +7,7 @@ import json 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.mako_block import MakoTemplateBlockBase 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 @@ class ItemBankMixin( 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 @@ class ItemBankMixin( 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 @@ class ItemBankMixin( """ 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 @@ class ItemBankBlock(ItemBankMixin, XBlock): ) 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 @@ class ItemBankBlock(ItemBankMixin, XBlock): } 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 52e3310802..d110bcf537 100644 --- a/xmodule/library_content_block.py +++ b/xmodule/library_content_block.py @@ -12,7 +12,7 @@ from __future__ import annotations 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 opaque_keys.edx.locator import LibraryLocator 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 @@ class LegacyLibraryContentBlock(ItemBankMixin, XModuleToXBlockMixin, XBlock): """ 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 @@ class LegacyLibraryContentBlock(ItemBankMixin, XModuleToXBlockMixin, XBlock): 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 @@ class LegacyLibraryContentBlock(ItemBankMixin, XModuleToXBlockMixin, XBlock): 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 @@ class LegacyLibraryContentBlock(ItemBankMixin, XModuleToXBlockMixin, XBlock): 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 @@ class LegacyLibraryContentBlock(ItemBankMixin, XModuleToXBlockMixin, XBlock): 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 @@ class LegacyLibraryContentBlock(ItemBankMixin, XModuleToXBlockMixin, XBlock): 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 @@ class LegacyLibraryContentBlock(ItemBankMixin, XModuleToXBlockMixin, XBlock): 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 @@ class LegacyLibraryContentBlock(ItemBankMixin, XModuleToXBlockMixin, XBlock): ) ) 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 @@ class LegacyLibraryContentBlock(ItemBankMixin, XModuleToXBlockMixin, XBlock): """ 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 @@ class LegacyLibraryContentBlock(ItemBankMixin, XModuleToXBlockMixin, XBlock): """ 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 092606142e..a5d37cff42 100644 --- a/xmodule/tests/test_library_content.py +++ b/xmodule/tests/test_library_content.py @@ -7,13 +7,17 @@ import ddt 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.modulestore.tests.utils import MixedSplitTestCase 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 @@ class LegacyLibraryContentTest(MixedSplitTestCase): 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 @@ class LegacyLibraryContentTest(MixedSplitTestCase): 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 @@ class TestLibraryContentExportImport(LegacyLibraryContentTest): 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 @@ class TestLibraryContentExportImport(LegacyLibraryContentTest): 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 @@ class TestLibraryContentExportImport(LegacyLibraryContentTest): """ 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 @@ class LegacyLibraryContentBlockTestMixin: """ 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 @@ class TestLibraryContentAnalytics(LegacyLibraryContentTest): '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'